DRAFT: IBC Lite Implementation #6299
Conversation
AdityaSripal
requested review from
colin-axner,
damiannolan and
chatton
as code owners
|
Important Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce and enhance functionalities related to Inter-Blockchain Communication (IBC) within the project. Key modifications include the implementation of packet handling methods in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
modules/core/04-channel/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (5)
- modules/core/keeper/msg_server.go (1 hunks)
- modules/core/lite/keeper/msg_server.go (1 hunks)
- modules/core/lite/types/client.go (1 hunks)
- modules/core/lite/types/expected_keepers.go (1 hunks)
- proto/ibc/core/channel/v1/tx.proto (2 hunks)
Files skipped from review due to trivial changes (1)
- modules/core/keeper/msg_server.go
Additional Context Used
GitHub Check Runs (1)
tests (03) failure (3)
modules/core/lite/types/expected_keepers.go: [failure] 27-27:
undefined: port.AppModule
Path-based Instructions (3)
modules/core/lite/types/client.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/lite/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/lite/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (7)
modules/core/lite/types/client.go (2)
9-10: MethodGetCounterpartyis well-defined and adheres to good practices in Go.
11-29: MethodsVerifyMembershipandVerifyNonMembershipare crucial for blockchain integrity and are implemented with necessary parameters for thorough verification.modules/core/lite/types/expected_keepers.go (2)
10-12: MethodRouteinClientRouterinterface is well-defined and adheres to good practices in Go.
15-24: Methods inChannelKeeperinterface are crucial for managing channel states and are implemented correctly.modules/core/lite/keeper/msg_server.go (2)
13-22:Keeperstruct is well-defined and correctly implements thePacketMsgServerinterface, ensuring proper handling of IBC operations.
38-103: MethodRecvPacketis well-implemented with sophisticated caching strategies. However, please review the caching strategy to ensure it does not lead to state inconsistencies.Verification successful
The caching strategy in the
RecvPacketmethod appears to be well-implemented, with mechanisms to handle both successful and unsuccessful packet acknowledgements effectively. The use ofcacheCtxandwriteFn()to manage state changes conditionally based on the success of the acknowledgement process seems to mitigate the risk of state inconsistencies. Therefore, the initial concern about potential state inconsistencies due to the caching strategy does not seem warranted based on the current implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the caching strategy in `RecvPacket` method to ensure it does not lead to state inconsistencies. # Test: Review the caching implementation and test for potential state inconsistencies. grep -r "CacheContext" modules/core/lite/keeper/Length of output: 128
Script:
#!/bin/bash # Description: Examine the use of cache context and write function in the `RecvPacket` method to understand how state changes are managed. # Extract the relevant code section around the caching implementation to see how it's used in different scenarios. rg --context 10 "CacheContext" modules/core/lite/keeper/msg_server.goLength of output: 979
proto/ibc/core/channel/v1/tx.proto (1)
67-79: Creation of thePacketMsgservice enhances modularity and clarity by segregating packet-related operations into a dedicated service.
| type AppRouter interface { | ||
| Route(portID string) port.AppModule | ||
| } |
There was a problem hiding this comment.
Method Route in AppRouter interface references an undefined AppModule. Please ensure that AppModule is correctly imported or defined.
- Route(portID string) port.AppModule
+ // Ensure AppModule is defined or imported correctly
+ Route(portID string) port.AppModuleCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| type AppRouter interface { | |
| Route(portID string) port.AppModule | |
| } | |
| type AppRouter interface { | |
| // Ensure AppModule is defined or imported correctly | |
| Route(portID string) port.AppModule | |
| } |
| // SendPacket implements the MsgServer interface. It creates a new packet | ||
| // with the given source port and source channel and sends it over the channel | ||
| // end with the given destination port and channel identifiers. | ||
| func (k Keeper) SendPacket(context.Context, *channeltypes.MsgSendPacket) (*channeltypes.MsgSendPacketResponse, error) { | ||
|
|
||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Method SendPacket is currently a stub and does not implement any logic. Please ensure to complete the implementation or provide documentation if this is intentional.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
modules/core/04-channel/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (5)
- modules/core/04-channel/types/packet.go (1 hunks)
- modules/core/05-port/types/module.go (1 hunks)
- modules/core/lite/keeper/msg_server.go (1 hunks)
- modules/core/lite/types/expected_keepers.go (1 hunks)
- proto/ibc/core/channel/v1/tx.proto (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/core/lite/types/expected_keepers.go
- proto/ibc/core/channel/v1/tx.proto
Additional Context Used
Path-based Instructions (3)
modules/core/04-channel/types/packet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/05-port/types/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/core/lite/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
modules/core/04-channel/types/packet.go (1)
38-57: The implementation ofCommitLitePacketcorrectly computes the packet commitment by including the destination port and channel, which enhances the specificity of the commitment. This change aligns with the PR's goal to improve packet handling.modules/core/05-port/types/module.go (1)
200-238: The addition of theLitePacketHandlerinterface is a significant enhancement for handling IBC Lite specific packet operations. This interface segregates the Lite functionality from the standard IBC operations, which is a good practice in terms of modularity and maintainability.
| // SendPacket implements the MsgServer interface. It creates a new packet | ||
| // with the given source port and source channel and sends it over the channel | ||
| // end with the given destination port and channel identifiers. | ||
| func (k Keeper) SendPacket(goCtx context.Context, msg *channeltypes.MsgSendPacket) (*channeltypes.MsgSendPacketResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
|
|
||
| // Get LightClientModule associated with the destination channel | ||
| // Note: This can be implemented by the current clientRouter | ||
| lightClientModule := k.clientRouter.Route(msg.SourceChannel) | ||
| if lightClientModule == nil { | ||
| return nil, errorsmod.Wrapf(channeltypes.ErrInvalidChannel, "source channel %s not associated with a light client module", msg.SourceChannel) | ||
| } | ||
|
|
||
| // Lookup counterparty associated with our channel and ensure that it was packet was indeed | ||
| // sent by our counterparty. | ||
| // Note: This can be implemented by the current channelKeeper | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| _, counterpartyChannelId, found := k.channelKeeper.GetCounterparty(ctx, "", msg.SourceChannel) | ||
| if !found { | ||
| return nil, channeltypes.ErrChannelNotFound | ||
| } | ||
|
|
||
| if counterpartyChannelId != msg.DestChannel { | ||
| return nil, channeltypes.ErrInvalidChannelIdentifier | ||
| } | ||
|
|
||
| sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, msg.SourcePort, msg.SourceChannel) | ||
| if !found { | ||
| sequence = 1 | ||
| } | ||
|
|
||
| // construct packet from given fields and channel state | ||
| packet := channeltypes.NewPacket(msg.Data, sequence, msg.SourcePort, msg.SourceChannel, | ||
| msg.DestPort, msg.DestChannel, *msg.TimeoutHeight, msg.TimeoutTimestamp) | ||
|
|
||
| if err := packet.ValidateBasic(); err != nil { | ||
| return nil, errorsmod.Wrap(err, "constructed packet failed basic validation") | ||
| } | ||
|
|
||
| latestHeight := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height) | ||
| if latestHeight.IsZero() { | ||
| return nil, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", msg.SourceChannel) | ||
| } | ||
|
|
||
| latestTimestamp, err := lightClientModule.TimestampAtHeight(ctx, msg.SourceChannel, latestHeight) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // check if packet is timed out on the receiving chain | ||
| timeout := channeltypes.NewTimeout(packet.GetTimeoutHeight().(clienttypes.Height), packet.GetTimeoutTimestamp()) | ||
| if timeout.Elapsed(latestHeight, latestTimestamp) { | ||
| return nil, errorsmod.Wrap(timeout.ErrTimeoutElapsed(latestHeight, latestTimestamp), "invalid packet timeout") | ||
| } | ||
|
|
||
| commitment := channeltypes.CommitLitePacket(k.cdc, packet) | ||
|
|
||
| k.channelKeeper.SetNextSequenceSend(ctx, msg.SourcePort, msg.SourceChannel, sequence+1) | ||
| k.channelKeeper.SetPacketCommitment(ctx, msg.SourcePort, msg.SourceChannel, packet.GetSequence(), commitment) | ||
|
|
||
| // IBC Lite routes to the application to do specific sendpacket logic rather than assuming the caller is the application module. | ||
| // IMPORTANT: This changes the ordering of core and application execution for SendPacket | ||
| // TODO: Add SendPacket callback to IBCModule interface | ||
|
|
||
| return &channeltypes.MsgSendPacketResponse{Sequence: packet.GetSequence()}, nil | ||
| } |
There was a problem hiding this comment.
The SendPacket method correctly implements the packet sending logic for IBC Lite, including validation and commitment handling. However, there are several TODO comments indicating incomplete features, such as adding a SendPacket callback to the IBCModule interface. It's important to address these TODOs before finalizing the PR.
Would you like me to help implement the missing features or open a GitHub issue to track these tasks?
| // ReceivePacket implements the MsgServer interface. It receives an incoming | ||
| // packet, which was sent over a channel end with the given port and channel | ||
| // identifiers, performs all necessary application logic, and then | ||
| // acknowledges the packet. | ||
| func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| packet := msg.Packet | ||
|
|
||
| // Lookup counterparty associated with our channel and ensure that it was packet was indeed | ||
| // sent by our counterparty. | ||
| // Note: This can be implemented by the current channelKeeper | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| _, counterpartyChannelId, found := k.channelKeeper.GetCounterparty(ctx, "", packet.DestinationChannel) | ||
| if !found { | ||
| return nil, channeltypes.ErrChannelNotFound | ||
| } | ||
|
|
||
| if counterpartyChannelId != packet.SourceChannel { | ||
| return nil, channeltypes.ErrInvalidChannelIdentifier | ||
| } | ||
|
|
||
| // create key/value pair for proof verification | ||
| key := host.PacketCommitmentPath(packet.SourcePort, packet.SourceChannel, packet.Sequence) | ||
| commitment := channeltypes.CommitLitePacket(k.cdc, packet) | ||
|
|
||
| // Get LightClientModule associated with the destination channel | ||
| // Note: This can be implemented by the current clientRouter | ||
| lightClientModule := k.clientRouter.Route(packet.DestinationChannel) | ||
|
|
||
| // TODO: Use context instead of sdk.Context eventually | ||
| if err := lightClientModule.VerifyMembership( | ||
| ctx, | ||
| packet.DestinationChannel, | ||
| msg.ProofHeight, | ||
| 0, 0, | ||
| msg.ProofCommitment, | ||
| commitmenttypes.NewMerklePath(key), | ||
| commitment, | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Port should directly correspond to the application module route | ||
| // No need for capabilities and mapping from portID to ModuleName | ||
| appModule, ok := k.appRouter.GetRoute(packet.DestinationPort) | ||
| if !ok { | ||
| return nil, porttypes.ErrInvalidPort | ||
| } | ||
|
|
||
| // TODO: Figure out how to do caching generically without using SDK | ||
| // Perform application logic callback | ||
| // | ||
| // Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful. | ||
| cacheCtx, writeFn := ctx.CacheContext() | ||
| // TODO: Use signer as string rather than sdk.AccAddress | ||
| relayer, err := sdk.AccAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| ctx.Logger().Error("acknowledgement failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer")) | ||
| return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") | ||
| } | ||
| ack := appModule.OnRecvPacket(cacheCtx, packet, relayer) | ||
| if ack == nil || ack.Success() { | ||
| // write application state changes for asynchronous and successful acknowledgements | ||
| writeFn() | ||
| } else { | ||
| // Modify events in cached context to reflect unsuccessful acknowledgement | ||
| // TODO: How do we create interface for this that isn't too SDK specific? | ||
| // ctx.EventManager().EmitEvents(convertToErrorEvents(cacheCtx.EventManager().Events())) | ||
| } | ||
|
|
||
| // Write acknowledgement to store | ||
| if ack != nil { | ||
| // Can be implemented by current channelKeeper with change in sdk.Context to context.Context | ||
| k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement()) | ||
| } | ||
|
|
||
| return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil | ||
| } |
There was a problem hiding this comment.
The RecvPacket method handles the receipt of packets appropriately, including proof verification and application logic execution. The use of caching to manage state changes is a good practice. However, the numerous TODO comments suggest that the implementation is not yet complete, particularly in areas like error handling and event emission.
Should I assist in completing these implementations or would you prefer to track them via a GitHub issue?
| // Acknowledgement implements the MsgServer interface. It processes the acknowledgement | ||
| // of a packet previously sent by the calling chain once the packet has been received and acknowledged | ||
| // by the counterparty chain. | ||
| func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAcknowledgement) (*channeltypes.MsgAcknowledgementResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| packet := msg.Packet | ||
|
|
||
| // Lookup counterparty associated with our channel and ensure that it was packet was indeed | ||
| // sent by our counterparty. | ||
| // Note: This can be implemented by the current channelKeeper | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| _, counterpartyChannelId, found := k.channelKeeper.GetCounterparty(ctx, "", packet.SourceChannel) | ||
| if !found { | ||
| return nil, channeltypes.ErrChannelNotFound | ||
| } | ||
|
|
||
| if counterpartyChannelId != packet.DestinationChannel { | ||
| return nil, channeltypes.ErrInvalidChannelIdentifier | ||
| } | ||
|
|
||
| commitment := k.channelKeeper.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | ||
| if len(commitment) == 0 { | ||
| // TODO: events | ||
| // emitAcknowledgePacketEvent(ctx, packet, channel) | ||
|
|
||
| // This error indicates that the acknowledgement has already been relayed | ||
| // or there is a misconfigured relayer attempting to prove an acknowledgement | ||
| // for a packet never sent. Core IBC will treat this error as a no-op in order to | ||
| // prevent an entire relay transaction from failing and consuming unnecessary fees. | ||
| return nil, channeltypes.ErrNoOpMsg | ||
| } | ||
|
|
||
| packetCommitment := channeltypes.CommitLitePacket(k.cdc, packet) | ||
|
|
||
| // verify we sent the packet and haven't cleared it out yet | ||
| if !bytes.Equal(commitment, packetCommitment) { | ||
| return nil, errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "commitment bytes are not equal: got (%v), expected (%v)", packetCommitment, commitment) | ||
| } | ||
|
|
||
| proofPath := commitmenttypes.NewMerklePath(host.PacketAcknowledgementPath(packet.DestinationPort, packet.DestinationChannel, packet.Sequence)) | ||
|
|
||
| // Get LightClientModule associated with the destination channel | ||
| // Note: This can be implemented by the current clientRouter | ||
| lightClientModule := k.clientRouter.Route(packet.SourceChannel) | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| if err := lightClientModule.VerifyMembership( | ||
| ctx, | ||
| packet.SourceChannel, | ||
| msg.ProofHeight, | ||
| 0, 0, | ||
| msg.ProofAcked, | ||
| proofPath, | ||
| channeltypes.CommitAcknowledgement(msg.Acknowledgement), | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| k.channelKeeper.DeletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | ||
|
|
||
| // TODO: emit events | ||
| // emitAcknowledgePacketEvent(ctx, packet, channel) | ||
|
|
||
| // Port should directly correspond to the application module route | ||
| // No need for capabilities and mapping from portID to ModuleName | ||
| appModule, ok := k.appRouter.GetRoute(packet.SourcePort) | ||
| if !ok { | ||
| return nil, porttypes.ErrInvalidPort | ||
| } | ||
|
|
||
| relayer, err := sdk.AccAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| ctx.Logger().Error("acknowledgement failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer")) | ||
| return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") | ||
| } | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| err = appModule.OnAcknowledgementPacket(ctx, packet, msg.Acknowledgement, relayer) | ||
| if err != nil { | ||
| ctx.Logger().Error("acknowledgement failed", "port-id", packet.SourcePort, "channel-id", packet.SourceChannel, "error", errorsmod.Wrap(err, "acknowledge packet callback failed")) | ||
| return nil, errorsmod.Wrap(err, "acknowledge packet callback failed") | ||
| } | ||
|
|
||
| return &channeltypes.MsgAcknowledgementResponse{Result: channeltypes.SUCCESS}, nil | ||
| } |
There was a problem hiding this comment.
The Acknowledgement method processes packet acknowledgements correctly, ensuring that the packet commitment matches before proceeding. The method also handles errors appropriately. However, the presence of TODO comments related to event emission and context usage indicates areas that need further development.
I can help finalize these sections or create a tracking issue if that would be helpful.
| // Timeout implements the MsgServer interface. It processes a timeout | ||
| // for a packet previously sent by the calling chain. | ||
| func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*channeltypes.MsgTimeoutResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| packet := msg.Packet | ||
|
|
||
| // Lookup counterparty associated with our channel and ensure that it was packet was indeed | ||
| // sent by our counterparty. | ||
| // Note: This can be implemented by the current channelKeeper | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| _, counterpartyChannelId, found := k.channelKeeper.GetCounterparty(ctx, "", packet.SourceChannel) | ||
| if !found { | ||
| return nil, channeltypes.ErrChannelNotFound | ||
| } | ||
|
|
||
| if counterpartyChannelId != packet.DestinationChannel { | ||
| return nil, channeltypes.ErrInvalidChannelIdentifier | ||
| } | ||
|
|
||
| // TODO: Use context instead of sdk.Context eventually | ||
| commitment := k.channelKeeper.GetPacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) | ||
| if len(commitment) == 0 { | ||
| // TODO: events | ||
| // emitTimeoutPacketEvent(ctx, packet, channel) | ||
|
|
||
| // This error indicates that the timeout has already been relayed | ||
| // or there is a misconfigured relayer attempting to prove a timeout | ||
| // for a packet never sent. Core IBC will treat this error as a no-op in order to | ||
| // prevent an entire relay transaction from failing and consuming unnecessary fees. | ||
| return nil, channeltypes.ErrNoOpMsg | ||
| } | ||
|
|
||
| packetCommitment := channeltypes.CommitLitePacket(k.cdc, packet) | ||
|
|
||
| // verify we sent the packet and haven't cleared it out yet | ||
| if !bytes.Equal(commitment, packetCommitment) { | ||
| return nil, errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "packet commitment bytes are not equal: got (%v), expected (%v)", commitment, packetCommitment) | ||
| } | ||
|
|
||
| proofPath := commitmenttypes.NewMerklePath(host.PacketReceiptPath(packet.DestinationPort, packet.DestinationChannel, packet.Sequence)) | ||
|
|
||
| // Get LightClientModule associated with the destination channel | ||
| // Note: This can be implemented by the current clientRouter | ||
| lightClientModule := k.clientRouter.Route(packet.SourceChannel) | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| if err := lightClientModule.VerifyNonMembership( | ||
| ctx, | ||
| packet.DestinationChannel, | ||
| msg.ProofHeight, | ||
| 0, 0, | ||
| msg.ProofUnreceived, | ||
| proofPath, | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // TODO: Use context instead of sdk.Context eventually | ||
| k.channelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) | ||
|
|
||
| // TODO: emit an event marking that we have processed the timeout | ||
| // emitTimeoutPacketEvent(ctx, packet, channel) | ||
|
|
||
| // Port should directly correspond to the application module route | ||
| // No need for capabilities and mapping from portID to ModuleName | ||
| appModule, ok := k.appRouter.GetRoute(packet.SourcePort) | ||
| if !ok { | ||
| return nil, porttypes.ErrInvalidPort | ||
| } | ||
| relayer, err := sdk.AccAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| ctx.Logger().Error("acknowledgement failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer")) | ||
| return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") | ||
| } | ||
| // Perform application logic callback | ||
| // TODO: Use context instead of sdk.Context eventually | ||
| err = appModule.OnTimeoutPacket(ctx, packet, relayer) | ||
| if err != nil { | ||
| ctx.Logger().Error("timeout failed", "port-id", packet.SourcePort, "channel-id", packet.SourceChannel, "error", errorsmod.Wrap(err, "timeout packet callback failed")) | ||
| return nil, errorsmod.Wrap(err, "timeout packet callback failed") | ||
| } | ||
|
|
||
| return &channeltypes.MsgTimeoutResponse{Result: channeltypes.SUCCESS}, nil | ||
| } |
There was a problem hiding this comment.
The Timeout method implements the timeout logic for packets, including verification of non-membership. The method's structure is consistent with the other methods in this file, but like the others, it includes several TODO comments that need to be addressed to complete the implementation.
Would you like assistance in addressing these TODOs, or should they be tracked via a GitHub issue?
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/core/lite/keeper/msg_server.go (1 hunks)
Additional Context Used
GitHub Check Runs (1)
lint failure (9)
modules/core/lite/keeper/msg_server.go: [failure] 174-174:
Error return value ofk.channelKeeper.WriteAcknowledgementis not checked (errcheck)
modules/core/lite/keeper/msg_server.go: [failure] 73-73:
unchecked-type-assertion: type cast result is unchecked in lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height) - type assertion will panic if not matched (revive)
modules/core/lite/keeper/msg_server.go: [failure] 165-165:
empty-block: this block is empty, you can remove it (revive)
modules/core/lite/keeper/msg_server.go: [failure] 51-51:
var-naming: var counterpartyChannelId should be counterpartyChannelID (revive)
modules/core/lite/keeper/msg_server.go: [failure] 113-113:
var-naming: var counterpartyChannelId should be counterpartyChannelID (revive)
modules/core/lite/keeper/msg_server.go: [failure] 191-191:
var-naming: var counterpartyChannelId should be counterpartyChannelID (revive)
modules/core/lite/keeper/msg_server.go: [failure] 274-274:
var-naming: var counterpartyChannelId should be counterpartyChannelID (revive)
Path-based Instructions (1)
modules/core/lite/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
| return nil, errorsmod.Wrap(err, "constructed packet failed basic validation") | ||
| } | ||
|
|
||
| latestHeight := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height) |
There was a problem hiding this comment.
Unchecked type assertion in lightClientModule.LatestHeight.
- latestHeight := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height)
+ latestHeight, ok := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height)
+ if !ok {
+ return nil, fmt.Errorf("type assertion to clienttypes.Height failed")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| latestHeight := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height) | |
| latestHeight, ok := lightClientModule.LatestHeight(ctx, msg.SourceChannel).(clienttypes.Height) | |
| if !ok { | |
| return nil, fmt.Errorf("type assertion to clienttypes.Height failed") | |
| } |
|
|
||
| // IBC Lite routes to the application to do specific sendpacket logic rather than assuming the caller is the application module. | ||
| // IMPORTANT: This changes the ordering of core and application execution for SendPacket | ||
| // TODO: Add SendPacket callback to IBCModule interface |
There was a problem hiding this comment.
Reminder: TODO to add SendPacket callback to IBCModule interface.
Would you like me to help implement this feature or should I open a GitHub issue to track this task?
| // Write acknowledgement to store | ||
| if ack != nil { | ||
| // Can be implemented by current channelKeeper with change in sdk.Context to context.Context | ||
| k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement()) |
There was a problem hiding this comment.
Error return value of k.channelKeeper.WriteAcknowledgement is not checked.
- k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement())
+ if err := k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement()); err != nil {
+ return nil, fmt.Errorf("failed to write acknowledgement: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement()) | |
| if err := k.channelKeeper.WriteAcknowledgement(ctx, packet.DestinationPort, packet.DestinationChannel, ack.Acknowledgement()); err != nil { | |
| return nil, fmt.Errorf("failed to write acknowledgement: %w", err) | |
| } |
| // Note: This can be implemented by the current clientRouter | ||
| lightClientModule := k.clientRouter.Route(packet.DestinationChannel) | ||
|
|
||
| // TODO: Use context instead of sdk.Context eventually |
There was a problem hiding this comment.
Reminder: TODO to use context instead of sdk.Context.
Would you like me to help refactor this to use the standard context.Context?
|
|
||
| k.channelKeeper.DeletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) | ||
|
|
||
| // TODO: emit events |
There was a problem hiding this comment.
Reminder: TODO to emit events.
Would you like me to help implement event emission or should I open a GitHub issue to track this task?
| // TODO: Use context instead of sdk.Context eventually | ||
| k.channelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) | ||
|
|
||
| // TODO: emit an event marking that we have processed the timeout |
There was a problem hiding this comment.
Reminder: TODO to emit an event marking that we have processed the timeout.
Would you like me to help implement this event emission or should I open a GitHub issue to track this task?
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/core/lite/keeper/msg_server.go (1 hunks)
- modules/core/lite/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/core/lite/keeper/msg_server.go
- modules/core/lite/types/expected_keepers.go
* imp: added counterparty client store * imp: added provide counterparty to proto * imp: ran 'make proto-all' * imp: added logic to counterparty client * imp: fix proto * imp: fix proto * imp: ran 'make proto-all' * feat: finished counterparty client logic
* imp: added default prefix * docs: added godoc todos * style: ran linter
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
LiteClientModuleinterface for counterparty information retrieval and membership verification.LitePacketHandlerinterface for handling packet events.Refactor
RecvPacket,Acknowledgement, andTimeoutmethods to a newPacketMsgservice in the IBC channel protocol.Enhancements
Packetstruct properties.