-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(transfer): move denom trace to internal folder. #6508
Conversation
WalkthroughThe recent changes in the Changes
Sequence Diagram(s) (Beta)No sequence diagrams are needed for these changes as they mainly involve refactoring and type updates. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
|
||
// TODO: https://github.com/cosmos/ibc-go/issues/6421#issuecomment-2137516426 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to move out as these are the types interchaintest still requires (left comment on slack on the current pain point on this)
oh note: need changelog on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
scripts/protocgen.sh (1)
Line range hint
8-8
: Address the unassigned variable usage.The variable
$file
is used but not defined in the script. Ensure that it is assigned appropriately or removed if not needed.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
modules/apps/transfer/internal/types/denomtrace.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
modules/apps/transfer/types/transfer.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
Files selected for processing (12)
- CHANGELOG.md (1 hunks)
- modules/apps/transfer/internal/types/legacy_denomtrace.go (2 hunks)
- modules/apps/transfer/internal/types/legacy_denomtrace_test.go (1 hunks)
- modules/apps/transfer/keeper/export_test.go (1 hunks)
- modules/apps/transfer/keeper/migrations.go (5 hunks)
- modules/apps/transfer/keeper/migrations_test.go (15 hunks)
- modules/apps/transfer/simulation/decoder.go (2 hunks)
- modules/apps/transfer/simulation/decoder_test.go (1 hunks)
- modules/apps/transfer/types/legacy_denomtrace_responses.go (1 hunks)
- proto/ibc/applications/transfer/v1/denomtrace.proto (1 hunks)
- proto/ibc/applications/transfer/v1/transfer.proto (1 hunks)
- scripts/protocgen.sh (1 hunks)
Files skipped from review due to trivial changes (3)
- modules/apps/transfer/internal/types/legacy_denomtrace_test.go
- modules/apps/transfer/types/legacy_denomtrace_responses.go
- proto/ibc/applications/transfer/v1/transfer.proto
Additional context used
Path-based instructions (7)
modules/apps/transfer/simulation/decoder.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/internal/types/legacy_denomtrace.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/simulation/decoder_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/export_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.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"CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Shellcheck
scripts/protocgen.sh
[warning] 8-8: file is referenced but not assigned (for output from commands, use "$(file ...)" ). (SC2154)
[info] 8-8: Double quote to prevent globbing and word splitting. (SC2086)
Markdownlint
CHANGELOG.md
222-222: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
272-272: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
207-207: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (12)
proto/ibc/applications/transfer/v1/denomtrace.proto (1)
9-16
: MarkingDenomTrace
as deprecated is well-handled.The addition of the
deprecated
option to theDenomTrace
message is a clear and effective way to signal the future removal of this feature to developers.modules/apps/transfer/simulation/decoder.go (1)
22-22
: Ensure proper error handling in decoding operations.Consider adding error handling for the
MustUnmarshal
calls to prevent runtime panics in case of malformed data:- types.ModuleCdc.MustUnmarshal(kvA.Value, &denomTraceA) - types.ModuleCdc.MustUnmarshal(kvB.Value, &denomTraceB) + err := types.ModuleCdc.Unmarshal(kvA.Value, &denomTraceA) + if err != nil { + return fmt.Sprintf("Error unmarshalling DenomTrace A: %v", err) + } + err = types.ModuleCdc.Unmarshal(kvB.Value, &denomTraceB) + if err != nil { + return fmt.Sprintf("Error unmarshalling DenomTrace B: %v", err) + }modules/apps/transfer/internal/types/legacy_denomtrace.go (1)
29-29
: Correctly utilize thetypes.DenomPrefix
for formatting the IBC denomination.The use of
types.DenomPrefix
in theIBCDenom
method ensures consistency with the expected format across different modules.modules/apps/transfer/simulation/decoder_test.go (1)
19-19
: Proper instantiation and usage ofinternaltypes.DenomTrace
in unit tests.The test setup correctly initializes
DenomTrace
instances with appropriate values, ensuring that the decoder functionality is thoroughly tested.modules/apps/transfer/keeper/export_test.go (1)
11-11
: Ensure the internal API changes are reflected in the test helpers.The modifications in the test helper functions to use
internaltypes.DenomTrace
instead oftypes.DenomTrace
align with the internalization of theDenomTrace
type, maintaining encapsulation and modularity.Also applies to: 16-16, 21-23
modules/apps/transfer/keeper/migrations.go (2)
13-13
: Ensure the new import aliasinternaltypes
is consistently used throughout the file to avoid confusion with similar types.
116-116
: The functionssetDenomTrace
,deleteDenomTrace
,iterateDenomTraces
, andsetDenomMetadataWithDenomTrace
are correctly updated to use the internal types. This aligns with the PR's goal of makingDenomTrace
internal.Also applies to: 124-124, 131-131, 147-147
modules/apps/transfer/keeper/migrations_test.go (5)
12-12
: The import aliasinternaltransfertypes
is correctly used to reflect the internal scope ofDenomTrace
. This change is consistent with the modifications in the main codebase.
63-63
: The test cases inTestMigratorMigrateDenomTraceToDenom
are well-structured to cover various scenarios including no trace, single trace, multiple traces, and complex paths. This ensures comprehensive testing of the migration logic.Also applies to: 76-76, 89-89, 102-102, 107-107, 112-112, 117-117, 134-134, 147-147, 160-160
186-186
: The test for ensuring no leftoverDenomTrace
keys exist after migration is a good practice to ensure data integrity and cleanliness post-migration.
198-198
: The corruption detection tests inTestMigratorMigrateDenomTraceToDenomCorruptionDetection
are crucial for ensuring the robustness of the migration logic against invalid or corrupted data.Also applies to: 202-202, 209-209
309-309
: The tests inTestMigratorMigrateMetadata
are comprehensive, covering various scenarios to ensure that metadata migration handles different denom trace configurations correctly. This is essential for maintaining accurate and consistent metadata across token denominations.Also applies to: 320-320, 347-347, 374-374, 418-418
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
CHANGELOG.md
222-222: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
272-272: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
207-207: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
60-60
: Add a changelog entry for the removal ofDenomTrace
.The changelog correctly documents the removal of the
DenomTrace
type as part of the PR changes. This aligns with the PR's objective to removeDenomTrace
from the public API, ensuring that the documentation is consistent with the codebase changes.
Quality Gate passed for 'ibc-go'Issues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Moves the proto gen go file to internal and moves relevant denom trace files there too.
closes: #6454
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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.Summary by CodeRabbit
Refactor
DenomTrace
type, replacing custom types with standardized types from theibc-go
repository.Chores
DenomTrace
message from protobuf definitions.DenomTrace
type in theapps/transfer
module.