refactor: use typed Trace #6432
Conversation
| @@ -27,9 +27,18 @@ func TestFungibleTokenPacketDataValidateBasic(t *testing.T) { | |||
| expPass bool | |||
| }{ | |||
| {"valid packet", types.NewFungibleTokenPacketData(denom, amount, sender, receiver, ""), true}, | |||
| {"valid packet, base denom with leading slash", types.NewFungibleTokenPacketData("transfer/channel-1/uatom/", amount, sender, receiver, ""), true}, | |||
There was a problem hiding this comment.
pulled from TestValidatePrefixedDenom
|
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 28 minutes and 44 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 across multiple files in the IBC transfer module involve replacing string slices with typed Changes
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 (
|
| path := "" | ||
| if !denom.IsNative() { | ||
| path = denom.FullPath() | ||
| path = strings.TrimSuffix(path, "/"+denom.Base) | ||
| } |
There was a problem hiding this comment.
quick hack, function will be removed in next pr
colin-axner
requested review from
AdityaSripal,
damiannolan,
srdtrk and
chatton
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: 3
Outside diff range and nitpick comments (1)
modules/apps/transfer/types/token_test.go (1)
Line range hint
19-19: The variablessenderandreceiverare declared but not used in any test cases. Consider removing these if they are not planned to be used, to clean up the code.- var sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - var receiver = sdk.AccAddress("testaddr2").String()Also applies to: 20-20
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
modules/apps/transfer/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodules/apps/transfer/types/token.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (19)
- modules/apps/callbacks/ibc_middleware_test.go (4 hunks)
- modules/apps/transfer/ibc_module_test.go (3 hunks)
- modules/apps/transfer/internal/convert/convert_test.go (6 hunks)
- modules/apps/transfer/keeper/genesis_test.go (1 hunks)
- modules/apps/transfer/keeper/grpc_query_test.go (7 hunks)
- modules/apps/transfer/keeper/mbt_relay_test.go (1 hunks)
- modules/apps/transfer/keeper/relay.go (2 hunks)
- modules/apps/transfer/keeper/relay_test.go (5 hunks)
- modules/apps/transfer/transfer_test.go (3 hunks)
- modules/apps/transfer/types/denom.go (3 hunks)
- modules/apps/transfer/types/denom_test.go (2 hunks)
- modules/apps/transfer/types/packet.go (1 hunks)
- modules/apps/transfer/types/packet_test.go (22 hunks)
- modules/apps/transfer/types/token.go (2 hunks)
- modules/apps/transfer/types/token_test.go (11 hunks)
- modules/apps/transfer/types/trace.go (5 hunks)
- modules/apps/transfer/types/trace_test.go (1 hunks)
- proto/ibc/applications/transfer/v1/query.proto (1 hunks)
- proto/ibc/applications/transfer/v2/token.proto (1 hunks)
Files skipped from review due to trivial changes (2)
- modules/apps/transfer/types/trace_test.go
- proto/ibc/applications/transfer/v1/query.proto
Additional context used
Path-based instructions (16)
modules/apps/transfer/types/token.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/genesis_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_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.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/internal/convert/convert_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/token_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/packet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/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/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_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/mbt_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"modules/apps/transfer/keeper/relay.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/packet_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/ibc_module_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/ibc_middleware_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/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"
GitHub Check: lint
modules/apps/transfer/types/token_test.go
[failure] 19-19:
varsenderis unused (unused)
[failure] 20-20:
varreceiveris unused (unused)
Additional comments not posted (31)
proto/ibc/applications/transfer/v2/token.proto (2)
22-22: The use ofgogoproto.nullable = falseensures that thetracefield in theDenommessage will not be nullable, which is a good practice for ensuring data integrity and avoiding null checks in the code.
29-31: Theoption (gogoproto.goproto_stringer) = false;disables the automatic generation of theString()method for theTracemessage. Ensure that this is intentional and that a customString()method is implemented if needed.Verification successful
The custom
String()method for theTracestruct is indeed implemented in themodules/apps/transfer/types/trace.gofile.
modules/apps/transfer/types/trace.go: func (t Trace) String() string {Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if a custom String() method is implemented for the Trace struct. # Test: Search for the String method implementation for Trace. Expect: At least one occurrence. rg --type go 'func \(t Trace\) String\(\) string' modules/apps/transfer/typesLength of output: 149
modules/apps/transfer/types/token.go (2)
10-10: TheValidatemethod now includes validation for both the token denomination and amount. This is a good practice as it ensures that both aspects of a token are correct before proceeding with operations that involve the token.
12-12: The error wrapping provides more context on the failure, which is helpful for debugging. However, ensure that the error messages are clear and provide enough information to understand the issue without ambiguity.Also applies to: 13-13
Verification successful
The error messages in the
modules/apps/transferdirectory are consistently clear and provide sufficient context about the issues. They include specific details that help in understanding the nature of the errors, such as invalid addresses, invalid coins, and other specific conditions.
modules/apps/transfer/types/msgs.go: Error messages specify issues like invalid addresses, invalid coins, and other specific conditions.modules/apps/transfer/types/trace.go: Error messages include details about invalid port IDs, channel IDs, and denomination formats.modules/apps/transfer/types/token.go: Error messages specify issues with token denominations and transfer amounts.modules/apps/transfer/types/packet.go: Error messages detail issues with addresses, amounts, and memo lengths.modules/apps/transfer/types/denom.go: Error messages specify issues with base denominations and trace validations.modules/apps/transfer/types/transfer_authorization.go: Error messages include details about type mismatches, insufficient funds, and invalid authorizations.modules/apps/transfer/ibc_module.go: Error messages specify issues with channel sequences, port IDs, and version mismatches.modules/apps/transfer/keeper/relay.go: Error messages detail issues with channel capabilities, receiver addresses, and token amounts.modules/apps/transfer/keeper/msg_server.go: Error messages specify issues with application versions, multiple coin transfers, and unauthorized actions.modules/apps/transfer/internal/convert/convert.go: Error messages include details about invalid packet data.The error messages are clear and provide enough information to understand the issues without ambiguity.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in error message clarity throughout the project. # Test: Search for error wrapping patterns and review their clarity. Expect: Consistently clear error messages. rg --type go 'errorsmod.Wrap' modules/apps/transferLength of output: 12506
modules/apps/transfer/keeper/genesis_test.go (2)
14-15: ThegetTracefunction is a good utility for generating trace objects in tests. It uses dynamic data for the channel, which can help in testing various scenarios effectively.
22-29: The test setup using a slice oftypes.Traceis comprehensive and covers multiple scenarios. This is crucial for ensuring that the system behaves correctly under different conditions.modules/apps/transfer/types/denom_test.go (2)
23-23: The tests forDenoms.Validateare well-structured and cover various scenarios including valid traces, duplicated denominations, and empty base denominations. This thorough testing is essential for maintaining robustness in the validation logic.Also applies to: 30-31, 37-37
84-84: TheFullPathmethod tests are comprehensive, covering scenarios from no hops to multiple hops with complex base denominations. This ensures that the method behaves correctly across a wide range of inputs.Also applies to: 92-92, 100-100, 108-108
modules/apps/transfer/types/denom.go (3)
19-19: TheValidatemethod in theDenomstruct now includes trace validation, which is a critical addition for ensuring the integrity of the trace data. The use of error wrapping here adds clarity to the error messages, which is beneficial for troubleshooting.Also applies to: 22-24
59-60: TheFullPathmethod concatenates trace elements and the base denomination to form the full path. This method is crucial for generating the correct denomination string in a multi-chain environment.
74-74: The logic inSenderChainIsSourceandReceiverChainIsSourcemethods is crucial for determining the origin of the token in IBC transfers. This logic is sensitive and must be thoroughly tested to prevent issues in token tracking across chains.Also applies to: 81-88
modules/apps/transfer/internal/convert/convert_test.go (1)
32-35: The conversion tests fromFungibleTokenPacketDataV1 to V2 are comprehensive, covering various scenarios including different trace lengths and base denominations with special characters. This ensures that the conversion logic handles all edge cases correctly.Also applies to: 64-67, 81-84, 98-102, 116-121, 135-140
modules/apps/transfer/types/token_test.go (3)
33-37: The use ofNewTracein test cases is consistent with the PR's objective to use typedTraceinstead of strings. This should enhance type safety and reduce errors related to string manipulation.Also applies to: 47-50, 60-65, 83-87, 97-101, 111-115, 125-129, 140-140
133-133: The error messages in the test cases are well-formed and descriptive, providing clear feedback on the nature of the error, which is good for debugging and maintenance.Also applies to: 144-144
Line range hint
178-198: TheTestTokens_Stringmethod correctly tests the string representation of tokens with and without trace information. This ensures that the serialization of token data is consistent with expectations, which is crucial for interoperability and debugging.Also applies to: 205-253
modules/apps/transfer/types/packet.go (1)
52-53: The refactoring to useExtractDenomFromFullPathand validate theDenominValidateBasicaligns with the PR's goal to minimize string parsing by using typedTrace. This should improve the efficiency and maintainability of the code.modules/apps/transfer/transfer_test.go (1)
73-76: The tests correctly use the newTracetype for creatingDenominstances, which is consistent with the changes made in the PR. This ensures that the new functionality is covered by tests, which is crucial for maintaining robustness.Also applies to: 101-101
modules/apps/transfer/types/trace.go (3)
20-47: The implementation ofNewTrace,Validate, andStringmethods for theTracetype is well-done. These methods provide clear and concise functionality that enhances the readability and maintainability of the code.
61-67: TheParseDenomTracefunction has been modified to work with the newTracetype instead of strings. This change is in line with the PR's objectives and should help in reducing the complexity and potential errors associated with string parsing.
Line range hint
116-132: TheExtractDenomFromFullPathfunction's logic to handle the extraction ofDenomfrom a full path is sound. It correctly handles various cases and appends newTraceinstances as needed, which is crucial for maintaining the integrity of token tracing.modules/apps/transfer/keeper/grpc_query_test.go (1)
30-34: The test cases ingrpc_query_test.goeffectively cover the functionality related to querying denoms with the newTracetype. This ensures that the query handlers are correctly interpreting and handling the new data structure.Also applies to: 47-51, 74-78, 131-132, 179-183
modules/apps/transfer/keeper/relay.go (2)
144-144: LGTM! The telemetry label now correctly uses theFullPathmethod from theDenomstruct, which is consistent with the newTraceimplementation.
242-242: The addition ofTraceto the token's denomination when the sender chain is the source is correctly implemented. This aligns with the PR's objectives to use typedTracefor better clarity and less error-prone code.modules/apps/transfer/types/packet_test.go (2)
30-41: The updated test cases correctly reflect the newTracetype usage inFungibleTokenPacketData. These tests ensure that the packet data validation logic handles the new structure properly.
Line range hint
188-378: The test cases forFungibleTokenPacketDataV2are well-formed and cover a wide range of scenarios, including valid and invalid traces, amounts, and memo fields. This thorough testing is crucial for ensuring the robustness of the IBC token transfer functionality.modules/apps/transfer/ibc_module_test.go (1)
559-562: Refactor to use typedTracein token denomination.The change from a string slice to a typed
Traceslice for theTracefield in theDenomstruct aligns with the PR's objective to minimize string parsing and improve maintainability. This is a positive change that enhances type safety and clarity in the codebase.modules/apps/callbacks/ibc_middleware_test.go (4)
172-172: Update to use typedTraceinstead of string slice for better type safety and reduced string parsing.
316-316: Conversion to typedTracein token denomination enhances clarity and type safety.
651-651: The use of typedTracein packet data aligns with the PR's goal to minimize string parsing.
785-785: Continued use of typedTracein theWriteAcknowledgementfunction supports the overall refactor goal.modules/apps/transfer/keeper/relay_test.go (1)
59-62: Refactor to use typedTracein test cases.The changes correctly implement the use of the typed
Tracestruct instead of strings for trace information in theDenomstruct across various test cases. This aligns with the PR's objective to minimize string parsing and improve maintainability.Also applies to: 73-76, 109-112, 122-125, 385-385
| @@ -341,8 +341,7 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() { | |||
| panic(errors.New("MBT failed to convert sender address")) | |||
| } | |||
| registerDenomFn() | |||
| denomTrace := types.ParseDenomTrace(tc.packet.Data.Tokens[0].GetFullDenomPath()) | |||
| denom := denomTrace.IBCDenom() | |||
| denom := tc.packet.Data.Tokens[0].Denom.IBCDenom() | |||
There was a problem hiding this comment.
Refactor to use the new Trace type in the Denom field.
The change at line 344 where denom := tc.packet.Data.Tokens[0].Denom.IBCDenom() is used, correctly reflects the transition from using strings to using the Trace type for denomination tracing. This is in line with the PR's objective to minimize string parsing by using a typed Trace. Ensure that all related functions and methods that interact with this line are updated to handle the Trace type effectively.
modules/apps/transfer/types/trace.go
Outdated
| func (t Trace) String() string { | ||
| var sb strings.Builder | ||
| sb.WriteString(t.PortId) // nolint:revive // no error returned by WriteString | ||
| sb.WriteByte('/') //nolint:revive // no error returned by WriteByte | ||
| sb.WriteString(t.ChannelId) // nolint:revive | ||
| return sb.String() | ||
| } |
There was a problem hiding this comment.
when there's no logic involved, should we just do a direct return fmt.Sprintf("%s/%s", t.PortId, t.ChannelId)?
There was a problem hiding this comment.
that sounds good to me 👍
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
| @@ -605,7 +605,7 @@ func (suite *TransferTestSuite) TestPacketDataUnmarshalerInterface() { | |||
| { | |||
| Denom: types.Denom{ | |||
| Base: ibctesting.TestCoin.Denom, | |||
| Trace: []types.Trace{types.Trace{}}, | |||
| Trace: []types.Trace{{}}, | |||
There was a problem hiding this comment.
I find this to be an odd linting standard
|
Description
closes: #6362
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
Bug Fixes
Refactor
New Features
Documentation
Queryservice for better readability.