imp: add migration handler for DenomTrace to Denom #6453
Conversation
There was a problem hiding this comment.
copied test from trace.go
|
Warning Rate limit exceeded@colin-axner has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 51 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve migrating the internal storage and handling of IBC token denomination traces from a single string format to an array format. This includes replacing the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CLI
participant Keeper
participant Migrator
participant Denom
User->>CLI: Send IBC Token
CLI->>Keeper: Extract Denom From Full Path
Keeper->>Denom: Validate and Hash Denom
Denom-->>Keeper: Return Validated and Hashed Denom
Keeper->>Migrator: Migrate DenomTrace to Denom
Migrator-->>Keeper: Migration Complete
Keeper-->>CLI: Return Processed Token
CLI-->>User: Transaction Complete
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 (
|
There was a problem hiding this comment.
copied from trace.go
| // - "portidone/channel-0/gamm/pool/1" => DenomTrace{Path: "portidone/channel-0", BaseDenom: "gamm/pool/1"} | ||
| // - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"} | ||
| // - "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"} | ||
| func ParseDenomTrace(rawDenom string) DenomTrace { |
| @@ -140,40 +84,6 @@ func ExtractDenomFromFullPath(fullPath string) Denom { | |||
| } | |||
| } | |||
|
|
|||
| // validateTraceIdentifiers validates the correctness of the trace associated with a particular base denom. | |||
| func validateTraceIdentifiers(identifiers []string) error { | |||
| } | ||
|
|
||
| // Validate performs a basic validation of the DenomTrace fields. | ||
| func (dt DenomTrace) Validate() error { |
colin-axner
requested review from
AdityaSripal,
damiannolan,
chatton,
crodriguezvega and
DimitrisJim
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (10)
- e2e/testsuite/testsuite.go (1 hunks)
- modules/apps/transfer/client/cli/tx.go (1 hunks)
- modules/apps/transfer/keeper/grpc_query.go (1 hunks)
- modules/apps/transfer/keeper/migrations.go (3 hunks)
- modules/apps/transfer/keeper/migrations_test.go (3 hunks)
- modules/apps/transfer/module.go (3 hunks)
- modules/apps/transfer/types/denom_trace.go (1 hunks)
- modules/apps/transfer/types/denom_trace_test.go (1 hunks)
- modules/apps/transfer/types/trace.go (3 hunks)
- modules/apps/transfer/types/trace_test.go (3 hunks)
Additional context used
Path-based instructions (10)
modules/apps/transfer/types/denom_trace_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/types/denom_trace.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/trace_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/types/trace.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/client/cli/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/migrations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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"e2e/testsuite/testsuite.go (2)
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"
golangci-lint
modules/apps/transfer/keeper/migrations.go
18-18: undefined: Keeper
22-22: undefined: Keeper
116-116: undefined: Keeper
124-124: undefined: Keeper
131-131: undefined: Keeper
147-147: undefined: Keeper
Additional comments not posted (16)
modules/apps/transfer/types/denom_trace_test.go (1)
10-26: The test caseTestDenomTrace_IBCDenomeffectively checks theIBCDenommethod for both base and trace scenarios. It's good to see that the expected outcomes are clearly defined and tested against the actual results.modules/apps/transfer/types/denom_trace.go (4)
10-16: TheHashmethod correctly computes the SHA256 hash of the full denomination path. This is a crucial function for ensuring the uniqueness and integrity of the denomination trace.
18-21: TheGetPrefixmethod is straightforward and correctly constructs the prefix from the trace path. This method is essential for maintaining consistency in how denominations are represented and processed.
23-30: TheIBCDenommethod is well-implemented, providing a clear and correct format for the IBC denomination based on the trace information. This method is vital for the correct functioning of token transfers in the IBC protocol.
32-40: TheGetFullDenomPathmethod accurately constructs the full denomination path from the trace and base denomination. This method is essential for the correct parsing and handling of IBC tokens.modules/apps/transfer/types/trace_test.go (1)
Line range hint
47-65: The test cases inTestExtractDenomFromFullPathcover a wide range of scenarios, from simple base denominations to complex IBC paths with multiple hops. This thorough testing ensures that theExtractDenomFromFullPathfunction behaves as expected across various inputs.modules/apps/transfer/types/trace.go (1)
1-1: The filetrace.gocontains essential functions for handling IBC token traces, including validation and parsing. It's important to ensure that these functions are robust and error-free given their critical role in the IBC token transfer process.modules/apps/transfer/keeper/grpc_query.go (1)
99-105: TheDenomHashmethod ingrpc_query.gocorrectly extracts and validates the denomination from the provided trace, and then checks for its existence in the store. This method is crucial for the integrity and correctness of denomination queries in the IBC module.modules/apps/transfer/client/cli/tx.go (1)
63-64: The changes inNewTransferTxCmdto useExtractDenomFromFullPathandIBCDenomfor handling coin denominations are correctly implemented. These changes are crucial for ensuring that the CLI handles IBC token transfers correctly with the updated denomination handling logic.modules/apps/transfer/keeper/migrations.go (2)
74-109: ### Review ofMigrateDenomTraceToDenomfunction
The functionMigrateDenomTraceToDenomis designed to migrate storage from usingDenomTracetoDenom. The function iterates over allDenomTraceentries, converts each to aDenom, validates it, and then updates the storage to useDenomwhile deleting the oldDenomTrace. The use ofpanicfor error handling is aggressive but appropriate for a blockchain setting where data integrity is critical.However, there are a few points to consider:
- Error Handling: The use of
panicensures that any error during migration leads to a halt in the process, which is suitable for critical migrations in a blockchain context. However, it might be beneficial to log the error for better traceability before panicking.- Migration Logic: The logic correctly ensures that the IBC denom does not change during the migration, which is crucial for maintaining consistency across the network.
- Performance: The function handles potentially large data sets by iterating over traces and applying changes one by one. This approach is necessary given the constraints of blockchain state management but should be monitored for performance in a production environment.
Overall, the function appears well-constructed for its purpose, with thorough error handling and appropriate safety checks.
123-127: ### Review ofdeleteDenomTracefunction
ThedeleteDenomTracefunction is straightforward and correctly implements the deletion of aDenomTracefrom the store. It uses theDenomTracehash as the key, which is a standard approach for ensuring that deletions are correctly targeted.Suggestion: It might be beneficial to check if the deletion was successful or if the key did not exist in the first place. This could help in debugging issues during migration where expected keys are not found.
Tools
golangci-lint
124-124: undefined: Keeper
modules/apps/transfer/module.go (2)
136-138: ### Review of Migration Registration inRegisterServices
The registration of theMigrateDenomTraceToDenommigration function is correctly implemented. It specifies that this migration should occur when upgrading from version 5 to 6 of the module. The use ofpanicin error handling is appropriate here to prevent the module from running in an incorrect state.Consideration: Ensure that all dependent modules and functionalities are compatible with this version change to prevent issues in multi-module setups.
157-157: ### Consensus Version Update
The update of the consensus version to 6 is a critical change that signifies backward-incompatible changes in the module. This change is well-documented in the code and correctly implemented.Action: It is crucial to ensure that all network participants are aware of this change and that it is coordinated with network upgrades to prevent consensus errors.
modules/apps/transfer/keeper/migrations_test.go (2)
51-127: ### Review ofTestMigratorMigrateDenomTraceToDenom
This test function covers various scenarios for the migration fromDenomTracetoDenom, including cases with no traces, single trace, multiple traces, and complex trace paths. The test is well-structured and covers a broad range of possible real-world scenarios.Improvement: Consider adding assertions for the state of the store before and after the migration to ensure that no unintended changes occur during the process.
195-227: ### Review ofTestMigratorMigrateDenomTraceToDenomCorruptionDetection
This test function checks the robustness of the migration process against corrupted or invalidDenomTraceentries. It is crucial for ensuring that the migration process can handle unexpected data gracefully.Suggestion: Expand the test cases to cover more types of corruption, such as invalid characters in the denom or excessively long paths, to further ensure the migration's resilience.
e2e/testsuite/testsuite.go (1)
521-522: The update to usetransfertypes.ExtractDenomFromFullPathaligns with the PR's objectives to simplify and optimize the handling of token denominations. This change appears correctly implemented.
| panic(err) | ||
| } | ||
|
|
||
| if dt.IBCDenom() != denom.IBCDenom() { |
There was a problem hiding this comment.
should in theory be unreachable or am I missing something? It should all hinge on how much we trust our conversion funcs
There was a problem hiding this comment.
also, on this, do we want to exercise these panic paths, open issue etc.
There was a problem hiding this comment.
in theory unreachable, but provides a nice defense mechanism in case we miss a step. We absolutely do not want to be generating different sdk coin denoms after this migration, so this is just for peace of mind
There was a problem hiding this comment.
sgtm, wouldn't mind a comment signaling defense in depth
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/transfer/keeper/migrations.go (3 hunks)
- modules/apps/transfer/keeper/migrations_test.go (3 hunks)
- modules/apps/transfer/types/denom_trace_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/apps/transfer/types/denom_trace_test.go
Additional context used
Path-based instructions (2)
modules/apps/transfer/keeper/migrations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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"
Additional comments not posted (3)
modules/apps/transfer/keeper/migrations.go (1)
119-123: ThedeleteDenomTracefunction correctly handles the deletion of denom traces using their hash as the key. This is a crucial part of the migration process to ensure that old traces are properly cleaned up.modules/apps/transfer/keeper/migrations_test.go (2)
Line range hint
51-189: The test functionTestMigratorMigrateDenomTraceToDenomis well-structured and covers a comprehensive range of scenarios from simple to complex trace paths. This thorough testing ensures that the migration logic is robust across different cases.
194-226: The functionTestMigratorMigrateDenomTraceToDenomCorruptionDetectioneffectively tests the migration logic's response to corrupted and invalid denom traces, ensuring that such cases lead to a panic as expected. This is crucial for maintaining the integrity of the migration process.
[APROVED]
|
Description
closes: #6221
closes: #6414
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
Bug Fixes
Tests
Refactor