Conversation
crodriguezvega
requested review from
AdityaSripal,
DimitrisJim,
charleenfei,
chatton,
colin-axner,
damiannolan and
srdtrk
as code owners
WalkthroughWalkthroughThe changes primarily focus on refactoring the handling of packet data unmarshaling and denomination traces within the Cosmos IBC Go project. Key modifications include updating the Changes
Sequence Diagram(s) (Beta)Silently ignored as the changes are primarily refactoring and do not introduce new features or modify control flow significantly. Assessment against linked issues
Possibly related issues
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 (
|
modules/apps/transfer/types/denom.go
Outdated
| ) | ||
|
|
||
| // NewDenom creates a new Denom instance given the base denomination and trace. | ||
| func NewDenom(base string, trace ...Trace) Denom { |
There was a problem hiding this comment.
I thought it would be nice to have a constructor function. I can use it other places of the codebase, once the PR is merged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
Line range hint
218-218: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.- * (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version. + * (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version.
Line range hint
268-268: Remove trailing spaces to maintain clean and consistent formatting in the markdown file.- * (apps/transfer) [\#6440](https://github.com/cosmos/ibc-go/pull/6440) Remove `GetPrefixedDenom`. + * (apps/transfer) [\#6440](https://github.com/cosmos/ibc-go/pull/6440) Remove `GetPrefixedDenom`.
Line range hint
203-203: Replace bare URL with a markdown link to enhance the readability and formatting of the document.- * [\#3133](https://github.com/cosmos/ibc-go/pull/3133) Add linter for markdown documents. + * [[\#3133](https://github.com/cosmos/ibc-go/pull/3133)](https://github.com/cosmos/ibc-go/pull/3133) Add linter for markdown documents.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- e2e/tests/wasm/grandpa_test.go (1 hunks)
- modules/apps/callbacks/replay_test.go (4 hunks)
- modules/apps/callbacks/transfer_test.go (2 hunks)
- modules/apps/transfer/keeper/migrations_test.go (1 hunks)
- modules/apps/transfer/keeper/relay_test.go (19 hunks)
- modules/apps/transfer/types/denom.go (1 hunks)
Additional context used
Path-based instructions (7)
modules/apps/transfer/types/denom.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/transfer_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/migrations_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/replay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/wasm/grandpa_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
CHANGELOG.md
218-218: Expected: 0 or 2; Actual: 1
Trailing spaces
268-268: Expected: 0 or 2; Actual: 1
Trailing spaces
203-203: null
Bare URL used
Additional comments not posted (13)
modules/apps/transfer/types/denom.go (1)
14-20: The implementation ofNewDenomis clear and concise. It effectively replaces the removedGetPrefixedDenomfunction.modules/apps/callbacks/transfer_test.go (2)
185-186: The refactoring to useNewDenominExecuteTransferaligns with the removal ofGetPrefixedDenomand is implemented correctly.Also applies to: 213-213
210-211: The use ofNewDenominExecuteTransferTimeoutis consistent with the changes inExecuteTransferand correctly implements the new approach.modules/apps/transfer/keeper/migrations_test.go (1)
210-211: The use ofNewDenominTestMigrateTotalEscrowForDenomcorrectly reflects the intended removal ofGetPrefixedDenomand its replacement withNewDenom.modules/apps/callbacks/replay_test.go (2)
289-290: The refactoring to useNewDenominTestTransferRecvPacketReplayProtectionis correctly implemented and aligns with the PR's objectives.Also applies to: 299-301
322-323: The use ofNewDenominExecuteFailedTransferis consistent with the changes in other test methods and correctly implements the new approach.Also applies to: 350-350
e2e/tests/wasm/grandpa_test.go (1)
209-210: The usage oftransfertypes.NewDenomandtransfertypes.NewTracehere is consistent with the PR's objective to replaceGetPrefixedDenomwith these new functions. This change simplifies the creation of IBC denominations by directly constructing them with base and trace information.modules/apps/transfer/keeper/relay_test.go (5)
667-667: Ensure correct usage of the newDenomstructure in packet acknowledgements.Check that the
Denomstructure is correctly used in the context of packet acknowledgements, particularly ensuring that theIBCDenom()method is utilized.Also applies to: 683-683, 692-692, 707-707, 718-718
Verification successful
The
OnAcknowledgementPacketfunction inmodules/apps/transfer/keeper/relay.godoes not explicitly show the usage of theDenomstructure. To ensure the correct usage of theDenomstructure, particularly theIBCDenom()method, we need to check therefundPacketTokenfunction, as it is called withinOnAcknowledgementPacket.Let's extract the implementation of the
refundPacketTokenfunction to verify the usage of theDenomstructure.
The
refundPacketTokenfunction inmodules/apps/transfer/keeper/relay.gocorrectly uses theDenomstructure, specifically theIBCDenom()method, to handle token refunds. This confirms that theDenomstructure is being used appropriately in the context of packet acknowledgements.
modules/apps/transfer/keeper/relay.go:336: TheIBCDenom()method is used to create a new coin.modules/apps/transfer/keeper/relay.go:343: TheSenderChainIsSourcemethod of theDenomstructure is used to determine the source chain.These usages indicate that the
Denomstructure is integrated correctly within the packet acknowledgement process.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new `Denom` structure is used correctly in packet acknowledgements. # Test: Search for `Denom` usage in packet acknowledgements. Expect: Correct implementation. rg --type go $'OnAcknowledgementPacket'Length of output: 8721
Script:
#!/bin/bash # Extract the OnAcknowledgementPacket function implementation from relay.go ast-grep --lang go --pattern $'func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) error { $$$ }' modules/apps/transfer/keeper/relay.goLength of output: 1412
Script:
#!/bin/bash # Extract the refundPacketToken function implementation from relay.go ast-grep --lang go --pattern $'func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error { $$$ }' modules/apps/transfer/keeper/relay.goLength of output: 3314
402-402: Verify metadata retrieval and comparison.Ensure that the metadata retrieval and comparison logic correctly uses the
IBCDenom()method from the newDenomstructure.Verification successful
To verify the metadata retrieval and comparison logic, we need to examine the context and usage of the
GetDenomMetaDatamethod in the identified files. Specifically, we need to ensure that theIBCDenom()method from the newDenomstructure is correctly used.Let's inspect the relevant lines in the identified files to confirm the correct implementation.
The context and usage of the
GetDenomMetaDatamethod in the identified files show that the metadata retrieval and comparison logic is correctly implemented. TheIBCDenom()method from the newDenomstructure is used appropriately in the tests.
- relay_test.go: The
IBCDenom()method is used to retrieve the denomination metadata, and the results are compared as expected.- migrations_test.go: The metadata is retrieved using the base denomination, and the results are compared correctly.
- genesis_test.go: The
IBCDenom()method is used to retrieve the denomination metadata, and the presence of the metadata is verified.No issues were found with the metadata retrieval and comparison logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that metadata retrieval and comparison are correctly implemented. # Test: Search for metadata retrieval and comparison logic. Expect: Correct implementation. rg --type go $'GetDenomMetaData'Length of output: 673
Script:
#!/bin/bash # Description: Inspect the context and usage of GetDenomMetaData in the identified files. # Extract lines around the usage of GetDenomMetaData in relay_test.go rg --type go --context 5 'GetDenomMetaData' modules/apps/transfer/keeper/relay_test.go # Extract lines around the usage of GetDenomMetaData in migrations_test.go rg --type go --context 5 'GetDenomMetaData' modules/apps/transfer/keeper/migrations_test.go # Extract lines around the usage of GetDenomMetaData in genesis_test.go rg --type go --context 5 'GetDenomMetaData' modules/apps/transfer/keeper/genesis_test.goLength of output: 1734
356-368: Ensure metadata consistency with the newDenomstructure.Check that the
Metadatastructure is correctly populated using the newDenomfields, especially theIBCDenom()andFullPath()methods.
Line range hint
878-979: Review timeout handling for accuracy and compliance with newDenomstructure.Check that the timeout handling logic accurately uses the new
Denomstructure, particularly in the context of refunds and escrow adjustments.Verification successful
The
refundPacketTokenfunction inmodules/apps/transfer/keeper/relay.gocorrectly handles the newDenomstructure. It processes each token in the packet data, checks if the sender chain is the source, and either unescrows the tokens back to the sender or mints vouchers back to the sender. This ensures proper handling of refunds and escrow adjustments.
- The function uses
token.Denom.IBCDenom()to create the coin.- It checks
token.Denom.SenderChainIsSourceto determine the appropriate action (unescrow or mint).This confirms that the timeout handling logic is accurate and complies with the new
Denomstructure.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that timeout handling is accurate and complies with the new `Denom` structure. # Test: Search for timeout handling logic. Expect: Compliance with new `Denom` structure. rg --type go $'OnTimeoutPacket'Length of output: 7862
Script:
#!/bin/bash # Extract the OnTimeoutPacket function implementation from relay.go for detailed review. ast-grep --lang go --pattern $'func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error { $$$ }' modules/apps/transfer/keeper/relay.goLength of output: 499
Script:
#!/bin/bash # Extract the refundPacketToken function implementation for detailed review. ast-grep --lang go --pattern $'func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error { $$$ }' modules/apps/transfer/keeper/relay.goLength of output: 3314
750-761: Verify balance calculations post-acknowledgement.Ensure that the balance calculations after packet acknowledgement are accurate and reflect the expected changes due to the acknowledgement result.
CHANGELOG.md (1)
57-58: The changelog entries for the modifications to theUnmarshalPacketDatainterface and the removal ofGetPrefixedDenomfunction are correctly documented under the "API Breaking" section. This aligns with the changes described in the AI-generated summary and PR objectives, ensuring that the documentation is consistent with the code changes.
DimitrisJim
left a comment
DimitrisJim
left a comment
There was a problem hiding this comment.
definite yes for the constructor!
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/transfer/types/denom.go (1 hunks)
Additional context used
Path-based instructions (1)
modules/apps/transfer/types/denom.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
GitHub Check: tests (03)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
GitHub Check: tests (02)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
GitHub Check: tests (01)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
GitHub Check: tests (00)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
GitHub Check: build (arm64)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
GitHub Check: lint
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
GitHub Check: build (amd64)
modules/apps/transfer/types/denom.go
[failure] 18-18:
undefined: trace
| // NewDenom creates a new Denom instance given the base denomination and a variable number of traces. | ||
| func NewDenom(base string, traces ...Trace) Denom { | ||
| return Denom{ | ||
| Base: base, | ||
| Trace: trace, | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the variable name to match the function parameter.
- Trace: trace,
+ Trace: traces,The function parameter is traces but you used trace which is undefined. This change should resolve the compilation errors reported by static analysis tools.
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.
| // NewDenom creates a new Denom instance given the base denomination and a variable number of traces. | |
| func NewDenom(base string, traces ...Trace) Denom { | |
| return Denom{ | |
| Base: base, | |
| Trace: trace, | |
| } | |
| } | |
| // NewDenom creates a new Denom instance given the base denomination and a variable number of traces. | |
| func NewDenom(base string, traces ...Trace) Denom { | |
| return Denom{ | |
| Base: base, | |
| Trace: traces, | |
| } | |
| } |
Tools
GitHub Check: tests (03)
[failure] 18-18:
undefined: trace
GitHub Check: tests (02)
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
GitHub Check: tests (01)
[failure] 18-18:
undefined: trace
[failure] 18-18:
undefined: trace
GitHub Check: tests (00)
[failure] 18-18:
undefined: trace
GitHub Check: build (arm64)
[failure] 18-18:
undefined: trace
GitHub Check: lint
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
[failure] 18-18:
undefined: trace) (typecheck)
GitHub Check: build (amd64)
[failure] 18-18:
undefined: trace
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- modules/apps/transfer/types/denom.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/denom.go
| // execute | ||
| trace := types.ParseDenomTrace(types.GetPrefixedDenom(path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, sdk.DefaultBondDenom)) | ||
| coin = sdk.NewCoin(trace.IBCDenom(), sdkmath.NewInt(100)) | ||
| voucherDenom := types.NewDenom(sdk.DefaultBondDenom, types.NewTrace(path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID)) |
There was a problem hiding this comment.
last week locally I had these changes to remove ParseDenomTrace, what about just denom instead of voucherDenom?
There was a problem hiding this comment.
Sounds good, I will quickly update.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/callbacks/replay_test.go (4 hunks)
- modules/apps/callbacks/transfer_test.go (2 hunks)
- modules/apps/transfer/keeper/relay_test.go (19 hunks)
Files skipped from review as they are similar to previous changes (3)
- modules/apps/callbacks/replay_test.go
- modules/apps/callbacks/transfer_test.go
- modules/apps/transfer/keeper/relay_test.go
|
3 participants
Description
closes: #6429
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
Refactor
GetPrefixedDenomfunction in the transfer application.NewDenomandNewTrace.Tests
GrandpaTestSuite.replay_test.goandtransfer_test.go.migrations_test.goandrelay_test.go.