test: add test ValidateBasic for FungibleTokenPacketDataV2#6397
test: add test ValidateBasic for FungibleTokenPacketDataV2#6397hastur199 wants to merge 62 commits intocosmos:mainfrom
Conversation
* chore: adding proto files for ics20-v2 * chore: add newline
* add CurrentVersion, EscrowVersion, update tests * update hardcoded transfer channel version from interchaintest
…6116) --------- Co-authored-by: Charly <charly@interchain.berlin>
…shalling/conversion (cosmos#6226) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * add CurrentVersion, EscrowVersion, update tests * pr review * fix e2e tests * pr review * update e2e test version * linter * update hardcoded transfer channel version from interchaintest * update hardcoded transfer channel version from interchaintest * wip packet unmarshaller * wip * wip testing * update test * linter * rm unnecessary version changes * rm unnecessary artifacts * update callbacks test * update comment * pr review * rename getMultiDenomFungibleTokenPacketData to unmarshalPacketDataBytesToICS20V2 --------- Co-authored-by: chatton <github.qpeyb@simplelogin.fr> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…allbacks (cosmos#6175) * add SupportedVersions array for different ics20 versions, add version checking on channel handshake application callbacks * add tests * update pr review * pr review * last few pr review nits * linter * add version counter proposing * fix missing app versino * update code + tests to return our proposed version if counterparty version is invalid * remove if statement * address review comments: return ics20-2 if counterparty version is not supported --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…transfers (cosmos#6252) * add transfer authz code + tests * linter * address review comments --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…ks (cosmos#6189) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * pr review * linter * rm unused function: linter * wip pr feedback * fix test * pr review * lintttttt * wip: backwards compatibility for transfer rpc * implement changes for ics20-v2 packet data for onRecvPacket, onAcknowledgePacket and onTimeoutPacket * fix callbacks tests * lint --------- Co-authored-by: chatton <github.qpeyb@simplelogin.fr> Co-authored-by: Charly <charly@interchain.berlin>
* check length of tokens array against maximum allowed * chore: add note on arbitrary selection --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
…#6341) * api(port)!: Allow passing of context, port and channel identifier to unmarshal packet data interface as disussed. This allows us to grab the app version in transfer and unmarshal the packet based on that instead of a hacky unmarshal v2 then v1 and whatever happens. * lint: as we do * callbacks: fix signature of UnmarshalPacketData as per changes, make refactors to hopefully simplify signatures. * chore: lint and remove some todos. * review: address feedback.
* chore: specifically unmarshal v1 or v2 without brute force * chore: fix TestPacketDataUnmarshalerInterface test in transfer module * Pass dest values OnRecv, refactor GetExpectedEvents * chore: fixing TestGetCallbackData test * chore: fixed remaining tests in callbacks module * test: simplify callbacks test, revert back to previous behaviour * chore: fix test case name * chore: addressing PR feedback * chore: added docstring for unmarshalPacketDataBytesToICS20V2 --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
Sync Feature Branch With Main
* refactor: address various self review comments * revert: unnecessary change * lint
* refactor: apply review suggestions * imp: additional updates * refactor: make ValidateIBCDenom private * Update modules/apps/transfer/types/msgs.go Co-authored-by: Cian Hatton <cian@interchain.io> * apply review suggestions --------- Co-authored-by: Cian Hatton <cian@interchain.io>
* chore: move functions from internal/denom to trace.go * lint * lint: the comeback
* imp: self review items * apply jim's suggestion * Update modules/apps/transfer/keeper/msg_server_test.go * Update modules/apps/transfer/ibc_module.go * Update modules/apps/transfer/ibc_module.go
# Conflicts: # modules/apps/transfer/keeper/msg_server.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/types/keys.go # modules/apps/transfer/types/msgs_test.go # modules/apps/transfer/types/v3/packet.go # modules/apps/transfer/types/v3/packet.pb.go # modules/apps/transfer/types/v3/packet_test.go # modules/apps/transfer/types/v3/token_test.go # proto/ibc/applications/transfer/v3/packet.proto
# Conflicts: # e2e/tests/transfer/authz_test.go # e2e/tests/transfer/incentivized_test.go # e2e/tests/transfer/upgrades_test.go # e2e/tests/upgrades/upgrade_test.go # e2e/testsuite/tx.go # modules/apps/27-interchain-accounts/host/keeper/relay_test.go # modules/apps/29-fee/transfer_test.go # modules/apps/callbacks/ibc_middleware_test.go # modules/apps/callbacks/types/types_test.go # modules/apps/transfer/client/cli/tx.go # modules/apps/transfer/internal/convert/convert.go # modules/apps/transfer/internal/convert/convert_test.go # modules/apps/transfer/keeper/mbt_relay_test.go # modules/apps/transfer/keeper/msg_server.go # modules/apps/transfer/keeper/msg_server_test.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/transfer_test.go # modules/apps/transfer/types/keys.go # modules/apps/transfer/types/msgs.go # modules/apps/transfer/types/msgs_test.go # modules/apps/transfer/types/packet.go # modules/apps/transfer/types/packet.pb.go # modules/apps/transfer/types/packet_test.go # modules/apps/transfer/types/token_test.go # modules/apps/transfer/types/transfer_authorization_test.go # modules/apps/transfer/types/tx.pb.go # proto/ibc/applications/transfer/v1/tx.proto # proto/ibc/applications/transfer/v2/packet.proto # testing/solomachine.go
hastur199
requested review from
AdityaSripal,
DimitrisJim,
charleenfei,
chatton,
colin-axner,
crodriguezvega,
damiannolan and
srdtrk
as code owners
WalkthroughWalkthroughThe changes primarily involve adding a Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CLI
participant TransferModule
participant Keeper
participant Channel
User->>CLI: Initiate Transfer
CLI->>TransferModule: Create NewMsgTransfer with forwardingPath
TransferModule->>Keeper: sendTransfer with forwardingPath
Keeper->>Channel: OnRecvPacket with forwardingPath
Channel->>Keeper: Acknowledgment
Keeper->>TransferModule: Handle Acknowledgment
TransferModule->>User: Transfer Complete
Assessment against linked 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 (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (9)
modules/apps/transfer/types/msgs.go (1)
[!TIP]
Codebase VerificationThe script output indicates that several instances of
NewMsgTransferdo not include theforwardingPathparameter. This confirms that not all calls to the function have been updated to handle the new parameter.
- Files with missing
forwardingPathparameter:
modules/apps/transfer/types/msgs_test.gomodules/apps/transfer/keeper/relay_test.gomodules/apps/29-fee/transfer_test.goe2e/tests/upgrades/upgrade_test.goe2e/tests/transfer/incentivized_test.goPlease update these instances to include the
forwardingPathparameter to ensure consistency with the new function signature.Analysis chain
Line range hint
52-63: The updatedNewMsgTransferfunction now accepts aforwardingPathparameter, which is crucial for the new multihop transfer feature. This change is well-implemented and aligns with the modifications in other parts of the codebase. Ensure that all calls to this function across the codebase are updated to handle this new parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewMsgTransfer` include the new `forwardingPath` parameter. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'NewMsgTransfer'Length of output: 10850
Script:
#!/bin/bash # Description: Verify all function calls to `NewMsgTransfer` include the new `forwardingPath` parameter. # List of files to check files=( "testing/solomachine.go" "modules/apps/callbacks/replay_test.go" "modules/apps/callbacks/ibc_middleware_test.go" "modules/apps/transfer/transfer_test.go" "modules/apps/callbacks/transfer_test.go" "modules/apps/transfer/types/transfer_authorization_test.go" "modules/apps/transfer/types/msgs_test.go" "modules/apps/transfer/client/cli/tx.go" "modules/apps/transfer/keeper/msg_server_test.go" "modules/apps/transfer/keeper/relay_test.go" "modules/apps/transfer/keeper/mbt_relay_test.go" "modules/apps/transfer/keeper/invariants_test.go" "modules/apps/27-interchain-accounts/host/keeper/relay_test.go" "modules/apps/29-fee/transfer_test.go" "modules/apps/29-fee/keeper/events_test.go" "e2e/testsuite/tx.go" "e2e/tests/upgrades/upgrade_test.go" "e2e/tests/transfer/incentivized_test.go" "e2e/tests/transfer/upgrades_test.go" "e2e/tests/transfer/authz_test.go" ) # Check each file for the correct usage of NewMsgTransfer for file in "${files[@]}"; do echo "Checking $file" rg 'NewMsgTransfer\([^,]+,[^,]+,[^,]+,[^,]+,[^,]+,[^,]+,[^,]+,[^,]+,[^,]+,[^,]+\)' "$file" doneLength of output: 8239
e2e/tests/transfer/incentivized_test.go (2)
201-201: Ensure the test case description is updated to reflect the scenario being tested.Consider adding a more descriptive comment before the test case to explain that it specifically tests the scenario where an invalid receiver address is used, and the expected behavior in such cases.
326-326: Clarify the test scenario in the comment.It would be beneficial to add a comment here explaining that this test case is simulating a scenario where multiple messages (a transfer and a fee payment) are sent in a single transaction by a single sender. This helps in understanding the purpose and expected outcome of the test.
modules/apps/27-interchain-accounts/host/keeper/relay_test.go (1)
914-914: Clarify the reason for test failure in the comment or improve error handling to provide more specific feedback.Consider enhancing the comment to explain why the test might fail here, or adjust the test setup to handle errors more gracefully.
modules/apps/transfer/keeper/relay_test.go (5)
Line range hint
132-150: Consider adding a test case for the newForwardingPathparameter.It seems that the
ForwardingPathparameter is not being tested in theTestSendTransferfunction. Would you like me to help by adding a test case to ensure that the forwarding functionality behaves as expected?
Line range hint
392-431: Refactor to simplify theTestOnRecvPacketfunction.The
TestOnRecvPacketfunction is quite complex and handles many scenarios. Consider breaking it down into smaller, more focused test functions to improve readability and maintainability.
454-506: Add validation forForwardingPathinTestPathForwarding.The
TestPathForwardingfunction could benefit from additional validation checks for theForwardingPathto ensure that it is set up and utilized correctly throughout the test.
Line range hint
693-1226: Consider using table-driven tests inTestOnAcknowledgementPacket.The
TestOnAcknowledgementPacketfunction could be refactored to use table-driven tests, which would make it easier to add new test cases and maintain existing ones.
1228-1326: Improve documentation forTestSimplifiedHappyPathForwarding.The
TestSimplifiedHappyPathForwardingfunction could benefit from more detailed documentation explaining the test setup and expected outcomes, which would help future maintainers understand the test's purpose and design.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (3)
modules/apps/transfer/types/packet.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodules/apps/transfer/types/transfer.pb.gois excluded by!**/*.pb.go,!**/*.pb.gomodules/apps/transfer/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (34)
- e2e/tests/transfer/authz_test.go (4 hunks)
- e2e/tests/transfer/incentivized_test.go (2 hunks)
- e2e/tests/transfer/upgrades_test.go (1 hunks)
- e2e/tests/upgrades/upgrade_test.go (1 hunks)
- e2e/testsuite/tx.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/relay_test.go (4 hunks)
- modules/apps/29-fee/keeper/events_test.go (1 hunks)
- modules/apps/29-fee/transfer_test.go (2 hunks)
- modules/apps/callbacks/ibc_middleware_test.go (6 hunks)
- modules/apps/callbacks/replay_test.go (1 hunks)
- modules/apps/callbacks/transfer_test.go (2 hunks)
- modules/apps/transfer/client/cli/tx.go (1 hunks)
- modules/apps/transfer/ibc_module.go (1 hunks)
- modules/apps/transfer/internal/convert/convert.go (1 hunks)
- modules/apps/transfer/internal/convert/convert_test.go (7 hunks)
- modules/apps/transfer/keeper/invariants_test.go (1 hunks)
- modules/apps/transfer/keeper/keeper.go (2 hunks)
- modules/apps/transfer/keeper/mbt_relay_test.go (3 hunks)
- modules/apps/transfer/keeper/msg_server.go (1 hunks)
- modules/apps/transfer/keeper/msg_server_test.go (4 hunks)
- modules/apps/transfer/keeper/relay.go (10 hunks)
- modules/apps/transfer/keeper/relay_test.go (16 hunks)
- modules/apps/transfer/transfer_test.go (3 hunks)
- modules/apps/transfer/types/keys.go (3 hunks)
- modules/apps/transfer/types/msgs.go (2 hunks)
- modules/apps/transfer/types/msgs_test.go (2 hunks)
- modules/apps/transfer/types/packet.go (2 hunks)
- modules/apps/transfer/types/packet_test.go (22 hunks)
- modules/apps/transfer/types/token_test.go (2 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (2 hunks)
- proto/ibc/applications/transfer/v1/transfer.proto (1 hunks)
- proto/ibc/applications/transfer/v1/tx.proto (2 hunks)
- proto/ibc/applications/transfer/v2/packet.proto (2 hunks)
- testing/solomachine.go (1 hunks)
Files skipped from review due to trivial changes (2)
- modules/apps/29-fee/keeper/events_test.go
- modules/apps/transfer/types/transfer_authorization_test.go
Additional Context Used
Path-based Instructions (29)
modules/apps/transfer/internal/convert/convert.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/invariants_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/msg_server.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/keys.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/types/msgs.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/29-fee/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/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/types/msgs_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/msg_server_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/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/tests/transfer/authz_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/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/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"modules/apps/transfer/ibc_module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/testsuite/tx.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"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"e2e/tests/transfer/upgrades_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.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testing/solomachine.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/tests/transfer/incentivized_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/27-interchain-accounts/host/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"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"e2e/tests/upgrades/upgrade_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"
Additional comments not posted (60)
proto/ibc/applications/transfer/v1/transfer.proto (1)
30-36: The addition ofForwardingInfoandHopmessages aligns well with the requirements for supporting multihop transfer scenarios. The structure and naming conventions are consistent with existing protobuf definitions.Also applies to: 38-43
modules/apps/transfer/internal/convert/convert.go (1)
24-27: The handling of theForwardingPathby setting it tonilduring the V1 to V2 conversion is appropriate given the context. This ensures that the new field does not carry over inappropriate data.proto/ibc/applications/transfer/v2/packet.proto (1)
38-39: The addition of theForwardingInfofield to theFungibleTokenPacketDataV2message is well-implemented and supports the new multihop transfer functionality as intended.modules/apps/transfer/keeper/invariants_test.go (1)
54-54: Correctly passing anilvalue for theForwardingPathin the test message aligns with the expected behavior in scenarios where no forwarding is involved.modules/apps/transfer/keeper/msg_server.go (1)
51-51: The integration of theForwardingPathinto thesendTransferfunction call is correctly implemented, supporting the new forwarding functionality.proto/ibc/applications/transfer/v1/tx.proto (1)
54-55: The addition of theForwardingInfofield to theMsgTransfermessage is well-implemented and supports the new packet forwarding functionality as intended.modules/apps/transfer/types/keys.go (1)
32-33: The addition of new keys for handling escrow and forwarding addresses is consistent with the enhancements to support multihop transfers. The implementation ofGetForwardAddressfollows the established pattern used inGetEscrowAddress, which is good for consistency. However, ensure that the newforwardAddressVersionis well-documented and understood across different modules that might interact with these addresses.Also applies to: 37-37, 54-54, 84-95
modules/apps/transfer/types/packet.go (1)
Line range hint
105-146: The implementation ofNewFungibleTokenPacketDataV2and itsValidateBasicmethod inFungibleTokenPacketDataV2correctly handles the newForwardingPathfield. The validation logic that ensures theMemoandForwardingPathare not both set is crucial and aligns with the PR objectives. This is a key enhancement for handling packet forwarding in multihop scenarios.modules/apps/transfer/types/msgs_test.go (2)
99-99: TheGetMsgV1Signersfunction test is well-implemented. It correctly tests the retrieval of signers from aMsgTransferinstance.
59-79: The test cases forMsgTransfervalidation are comprehensive and cover a wide range of scenarios. However, it's important to ensure that theForwardingPathparameter is considered in these tests if it affects the validation logic, given the changes in the function signature.Verification successful
The
ForwardingPathparameter does not affect the validation logic inMsgTransferas it is not referenced in theValidateBasicmethod. Therefore, the current test cases are comprehensive and do not need to include scenarios forForwardingPath.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `ForwardingPath` parameter affects the validation logic in MsgTransfer. # Test: Search for validation logic handling `ForwardingPath`. Expect: Handling logic if it affects validation. rg --type go $'ValidateBasic' -- 'modules/apps/transfer/types/msgs.go'Length of output: 364
Script:
#!/bin/bash # Extract the implementation of the ValidateBasic method for MsgTransfer and search for any references to ForwardingPath. # Extract the ValidateBasic method for MsgTransfer ast-grep --lang go --pattern $'func (msg MsgTransfer) ValidateBasic() error { $$$ }' modules/apps/transfer/types/msgs.go # Search for references to ForwardingPath within the ValidateBasic method rg 'ForwardingPath' modules/apps/transfer/types/msgs.goLength of output: 3462
modules/apps/transfer/keeper/keeper.go (1)
315-326: Good use of error handling for packet retrieval.e2e/tests/transfer/authz_test.go (4)
190-190: Similar to the previous comment, theforwardingPathis set tonil. This should be verified against the test's requirements to ensure it's appropriate for the scenario being tested.Verification successful
Setting
forwardingPathtonilin the test scenario appears to be intentional and appropriate, as evidenced by multiple instances in the codebase where this is done in similar test contexts.
- modules/apps/29-fee/transfer_test.go
- modules/apps/transfer/transfer_test.go
- modules/apps/transfer/keeper/relay_test.go
- modules/apps/transfer/types/msgs_test.go
- e2e/testsuite/tx.go
- e2e/tests/upgrades/upgrade_test.go
- e2e/tests/transfer/incentivized_test.go
- e2e/tests/transfer/upgrades_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
333-333: Once more,forwardingPathis set tonil. This repetition across different tests suggests a pattern that should be explicitly justified in the context of the PR's objectives.
130-130: TheforwardingPathparameter is set tonilin theNewMsgTransfercall. Ensure this is intentional and aligns with the test's objectives, especially since the PR focuses on testing theValidateBasicfunction under conditions where bothMemoandForwardingPathare set.Verification successful
The
forwardingPathparameter being set tonilin theNewMsgTransfercall appears to be intentional and aligns with the test's objectives. This is evidenced by multiple instances in the codebase whereNewMsgTransferis called withnilas theforwardingPathparameter, particularly in test files.
- modules/apps/29-fee/transfer_test.go
- modules/apps/transfer/transfer_test.go
- modules/apps/transfer/keeper/relay_test.go
- modules/apps/transfer/types/msgs_test.go
- e2e/testsuite/tx.go
- e2e/tests/transfer/upgrades_test.go
- e2e/tests/transfer/incentivized_test.go
- e2e/tests/upgrades/upgrade_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
273-273: Again,forwardingPathis set tonilin theNewMsgTransfercall. Given the focus onforwardingPathin the PR, it's crucial to ensure this is consistent with the intended test scenarios.Verification successful
The search results show that the
NewMsgTransferfunction is frequently called withnilas theforwardingPathargument across various test files. This pattern suggests that settingforwardingPathtonilis intentional and consistent with the test scenarios.
- modules/apps/transfer/transfer_test.go: Multiple instances where
NewMsgTransferis called withnilforforwardingPath.- modules/apps/transfer/types/msgs_test.go: Various test cases use
nilforforwardingPath.- modules/apps/transfer/keeper/relay_test.go: Instances of
NewMsgTransferwithnilforforwardingPath.- modules/apps/29-fee/transfer_test.go:
NewMsgTransfercalls withnilforforwardingPath.- e2e/testsuite/tx.go:
NewMsgTransferwithnilforforwardingPath.- e2e/tests/upgrades/upgrade_test.go:
NewMsgTransferwithnilforforwardingPath.- e2e/tests/transfer/upgrades_test.go:
NewMsgTransferwithnilforforwardingPath.- e2e/tests/transfer/incentivized_test.go:
NewMsgTransferwithnilforforwardingPath.Given the consistent use of
nilforforwardingPathacross these tests, it appears to be an intentional design choice for these scenarios.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
Script:
#!/bin/bash # Extract lines around the NewMsgTransfer calls to understand the context and check for any comments or documentation. rg --type go --context 5 'NewMsgTransfer.*nil'Length of output: 19487
modules/apps/transfer/keeper/mbt_relay_test.go (2)
164-164: TheforwardingPathis set tonilhere. If this is part of a test setup, ensure it aligns with the test's objectives, especially given the focus onforwardingPathin other parts of the codebase.Verification successful
The
forwardingPathbeing set tonilin the test setup appears to be intentional and aligns with the test's objectives, as evidenced by multiple instances across various test files whereNewMsgTransferis called withnilfor theforwardingPath.
- modules/apps/transfer/transfer_test.go
- modules/apps/transfer/keeper/relay_test.go
- modules/apps/transfer/types/msgs_test.go
- modules/apps/29-fee/transfer_test.go
- e2e/tests/transfer/upgrades_test.go
- e2e/tests/transfer/incentivized_test.go
- e2e/testsuite/tx.go
- e2e/tests/upgrades/upgrade_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
366-366: Similar to previous comments,forwardingPathis set tonil. Consistency in howforwardingPathis handled across tests is crucial, especially in the context of new features related to packet forwarding.Verification successful
The search results indicate that setting
forwardingPathtonilis a common and intentional practice across various test files and scenarios. This suggests that the usage is consistent and aligns with the intended design.
- Instances found in
modules/apps/transfer/transfer_test.go- Instances found in
modules/apps/transfer/types/msgs_test.go- Instances found in
modules/apps/transfer/keeper/relay_test.go- Instances found in
modules/apps/29-fee/transfer_test.go- Instances found in
e2e/tests/upgrades/upgrade_test.go- Instances found in
e2e/tests/transfer/upgrades_test.go- Instances found in
e2e/tests/transfer/incentivized_test.go- Instances found in
e2e/testsuite/tx.goScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
modules/apps/callbacks/replay_test.go (1)
333-333: TheforwardingPathparameter is set tonilin theNewMsgTransfercall. This should be checked to ensure it aligns with the intended behavior, especially given the focus on forwarding features in the PR.Verification successful
The
forwardingPathparameter being set tonilin theNewMsgTransfercall is intentional and aligns with the intended behavior. TheForwardingInfotype is used for forwarding-related features, and its usage in test cases withnilindicates that it is optional and not always required.
- Instances of
NewMsgTransferwithnilforforwardingPathare primarily in test scenarios.- The
ForwardingInfotype is defined with fieldsHopsandMemo, indicating its role in forwarding features.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the forwardingPath is intentionally set to nil in this context. # Test: Search for the function usage with nil forwardingPath. Expect: Only occurances where it's intentional. rg --type go $'NewMsgTransfer.*nil'Length of output: 8047
Script:
#!/bin/bash # Description: Gather more context around the NewMsgTransfer function definition and its usage, focusing on the forwardingPath parameter. # Test: Search for the NewMsgTransfer function definition and its usage in the codebase. rg --type go 'func NewMsgTransfer' -A 10Length of output: 779
Script:
#!/bin/bash # Description: Gather more context around the ForwardingInfo type and its usage in the codebase. # Test: Search for the ForwardingInfo type definition and its usage in the codebase. rg --type go 'type ForwardingInfo' -A 10Length of output: 997
modules/apps/transfer/ibc_module.go (1)
233-245: Consider revising the return ofnilin asynchronous handling.The use of
nilhere might be intended to indicate a 'no operation' or 'success', but it's unconventional in Go, where typically an expliciterroror similar indicator is used to denote success or failure. Could you clarify if this is intended, and ensure it aligns with the overall error handling strategy of the module?modules/apps/transfer/types/packet_test.go (1)
Line range hint
185-416: Comprehensive test coverage forFungibleTokenPacketDataV2.The test cases added for
FungibleTokenPacketDataV2are comprehensive and effectively cover both positive and negative scenarios, including the handling ofForwardingInfo. This aligns well with the PR's objectives to ensure robust validation logic.e2e/tests/transfer/upgrades_test.go (1)
201-201: Ensure theNewMsgTransferfunction call includes all required parameters.Verification successful
The
NewMsgTransferfunction definition includes the following parameters:
sourcePortsourceChanneltokenssenderreceivertimeoutHeighttimeoutTimestampmemoforwardingPathThe usage in
e2e/tests/transfer/upgrades_test.goincludes all these parameters:msgTransfer := transfertypes.NewMsgTransfer( channelA.PortID, channelA.ChannelID, sdk.NewCoins(transferAmount), chainAWallet.FormattedAddress(), chainBWallet.FormattedAddress(), s.GetTimeoutHeight(ctx, chainB), 0, "", nil )This confirms that all required parameters are included in the function call.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `NewMsgTransfer` function call includes all required parameters. # Test: Search for the function usage. Expect: All parameters are included. rg --type go $'NewMsgTransfer'Length of output: 10850
Script:
#!/bin/bash # Description: Retrieve the definition of the `NewMsgTransfer` function to verify required parameters. # Test: Search for the function definition. Expect: Retrieve the function signature. ast-grep --lang go --pattern 'func NewMsgTransfer($_, $_, $_, $_, $_, $_, $_, $_, $_) $_'Length of output: 91
Script:
#!/bin/bash # Description: Retrieve the definition of the `NewMsgTransfer` function to verify required parameters. # Test: Search for the function definition with a more flexible pattern. Expect: Retrieve the function signature. ast-grep --lang go --pattern 'func NewMsgTransfer($$$) $_'Length of output: 1378
modules/apps/transfer/keeper/relay.go (5)
68-68: Ensure theforwardingPathparameter is properly documented and validated.
394-394: Ensure theOnAcknowledgementPacketfunction handles packet acknowledgements correctly, especially in multihop scenarios.Also applies to: 396-396, 399-399, 401-401, 403-403, 406-406, 408-408, 410-410, 412-412, 416-416, 418-418
Verification successful
The
OnAcknowledgementPacketfunction inmodules/apps/transfer/keeper/relay.goappears to handle packet acknowledgements correctly, including multihop scenarios. It checks for the existence of a previous packet and processes the acknowledgement accordingly, either writing a success acknowledgement or reverting changes and writing an error acknowledgement.
- The function retrieves the channel capability and checks if it exists.
- It then checks if a previous packet was forwarded.
- If found, it processes the acknowledgement based on its type (result or error).
- If not found, it processes the current packet's acknowledgement directly.
This logic ensures that the function handles both direct and multihop packet acknowledgements appropriately.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure the `OnAcknowledgementPacket` function handles packet acknowledgements correctly. # Test: Search for the function usage and logic. Expect: Correct handling of packet acknowledgements. rg --type go $'OnAcknowledgementPacket'Length of output: 8727
Script:
#!/bin/bash # Extract the implementation of the `OnAcknowledgementPacket` function from `modules/apps/transfer/keeper/relay.go` for detailed analysis. 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: 4311
149-149: Check the creation ofFungibleTokenPacketDataV2for correct data encapsulation.
183-183: Review the logic inOnRecvPacketfor handling forwarding paths and token transfers, ensuring it aligns with IBC specifications.Also applies to: 186-186, 190-190, 201-201, 204-207, 222-222, 251-251, 256-256, 312-312, 319-319, 343-381
Verification successful
The implementation of
OnRecvPacketinmodules/apps/transfer/keeper/relay.goand the related test cases inmodules/apps/transfer/keeper/relay_test.gohave been extracted. Here's a summary of the findings:Implementation of
OnRecvPacket
- The function validates the packet data and checks if receiving is enabled.
- It handles the receiver address and forwarding path.
- It processes the received tokens, either unescrowing them or minting new tokens based on the source chain.
- It includes telemetry and event emission for monitoring.
- It adds forwarding logic if there is a forwarding path specified.
Test Cases
- The test cases cover various scenarios, including successful and failure cases for receiving tokens.
- They validate the correct handling of forwarding paths, receiver addresses, and token amounts.
- They ensure the total escrow amount is correctly updated.
Alignment with IBC Specifications
- The function appears to follow the IBC specifications for handling token transfers and forwarding paths.
- The test cases provide comprehensive coverage to ensure the function behaves as expected under different conditions.
Points of Attention
- Ensure the forwarding logic correctly handles all edge cases.
- Verify that the telemetry and event emission provide accurate monitoring data.
Locations in the Codebase
modules/apps/transfer/keeper/relay.go(Implementation ofOnRecvPacket)modules/apps/transfer/keeper/relay_test.go(Test cases forOnRecvPacket)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Review the logic in `OnRecvPacket` for handling forwarding paths and token transfers. # Test: Search for the function usage and logic. Expect: Alignment with IBC specifications. rg --type go $'OnRecvPacket'Length of output: 10781
Script:
#!/bin/bash # Extract the implementation of OnRecvPacket from relay.go ast-grep --lang go --pattern $'func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) (bool, error) { $$$ }' modules/apps/transfer/keeper/relay.go # Extract the relevant test cases for OnRecvPacket from relay_test.go ast-grep --lang go --pattern $'func (suite *KeeperTestSuite) TestOnRecvPacket() { $$$ }' modules/apps/transfer/keeper/relay_test.go ast-grep --lang go --pattern $'func (suite *KeeperTestSuite) TestOnRecvPacketSetsTotalEscrowAmountForSourceIBCToken() { $$$ }' modules/apps/transfer/keeper/relay_test.goLength of output: 40874
439-439: Review theOnTimeoutPacketfunction to ensure it properly handles timeouts and state reversions.Also applies to: 441-441, 444-444, 446-446, 448-448, 450-450, 452-452
testing/solomachine.go (1)
381-381: Thenilparameter inSendTransferfunction seems to be a placeholder for future functionality or an oversight. Please clarify its purpose or consider removing it if it's not needed.Verification successful
The
nilparameter in theSendTransferfunction is used to indicate that no additional functions are provided to modify theMsgTransfer. This is a valid use case for variadic parameters in Go.
testing/solomachine.go: TheSendTransferfunction definition and its usage in test files confirm thatnilis used to indicate the absence of additional functions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `nil` parameter in the `SendTransfer` function across the codebase. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'SendTransfer'Length of output: 1302
Script:
#!/bin/bash # Description: Verify the definition and usage of the `SendTransfer` function to understand the purpose of the `nil` parameter. # Test: Extract the definition of the `SendTransfer` function. ast-grep --lang go --pattern $'func (solo *Solomachine) SendTransfer($_, $_, $_ string, fns ...func(*transfertypes.MsgTransfer)) $_' # Test: Extract the usage of the `SendTransfer` function in the test files. rg --type go 'SendTransfer' --context 5Length of output: 8117
e2e/tests/transfer/incentivized_test.go (1)
Line range hint
1-1: Review the overall test coverage and structure.The test cases are well-structured and seem to cover the necessary scenarios effectively. Good use of the testify suite to structure tests and assertions.
modules/apps/27-interchain-accounts/host/keeper/relay_test.go (1)
354-354: Ensure that theforwardingPathparameter is tested for non-nil scenarios.modules/apps/callbacks/ibc_middleware_test.go (10)
Line range hint
14-14: The test cases inTestNewIBCMiddlewareare well-structured and cover the necessary edge cases for middleware instantiation.
Line range hint
14-14: TheTestWithICS4Wrapperfunction correctly implements checks for the ICS4 wrapper, ensuring it is properly integrated and typed within the middleware.
Line range hint
14-14: TheTestSendPacketfunction is comprehensive and effectively tests theSendPacketfunctionality under various conditions, including edge cases and failure scenarios.
Line range hint
14-14: TheTestOnAcknowledgementPacketfunction effectively tests various scenarios of acknowledgement handling, including edge cases and error handling.
Line range hint
14-14: TheTestOnTimeoutPacketfunction is well-structured and thoroughly tests the timeout packet handling, covering necessary scenarios and edge cases.
Line range hint
14-14: TheTestOnRecvPacketfunction provides comprehensive testing for the receive packet functionality, covering a wide range of scenarios and edge cases.
Line range hint
14-14: TheTestWriteAcknowledgementfunction effectively tests the acknowledgement writing functionality, covering various scenarios including error handling and edge cases.
Line range hint
14-14: TheTestProcessCallbackfunction is comprehensive and effectively tests the callback processing functionality under various conditions, including error scenarios.
Line range hint
14-14: TheTestUnmarshalPacketDataV1function correctly tests the unmarshalling of packet data from version 1 to version 2, ensuring compatibility and correctness.
Line range hint
14-14: TheTestUnmarshalPacketDataV2function effectively tests the unmarshalling process for version 2 packet data, ensuring data integrity and correctness.e2e/tests/upgrades/upgrade_test.go (19)
Line range hint
1-1: Ensure the build tag is correctly specified for the e2e tests.
Line range hint
11-11: Ensure all necessary packages are imported and used appropriately.
Line range hint
14-14: Verify the constant values are appropriate and used correctly throughout the tests.
Line range hint
16-16: Ensure theTestUpgradeTestSuitefunction correctly initializes the test suite and handles configuration errors properly.
Line range hint
18-18: Check theUpgradeTestSuitestruct for any missing fields or methods that might be necessary for the tests.
Line range hint
21-21: Review theUpgradeChainmethod for correct implementation of the upgrade process, error handling, and use of context timeouts.
Line range hint
95-95: Ensure theTestIBCChainUpgradefunction correctly sets up the test environment, performs IBC token transfers, and validates the upgrade process.
Line range hint
95-95: Check for proper synchronization and error handling in theTestIBCChainUpgradefunction.
Line range hint
95-95: Review the use of relayers and query utilities in theTestIBCChainUpgradefunction to ensure they are correctly integrated and functioning.
Line range hint
95-95: Validate the logic for handling different scenarios in theTestIBCChainUpgradefunction, such as token escrow and packet relaying.
Line range hint
95-95: Ensure the upgrade process in theTestIBCChainUpgradefunction is correctly implemented and tested.
Line range hint
95-95: Check the final assertions in theTestIBCChainUpgradefunction to ensure they correctly verify the outcomes of the tests.
Line range hint
95-95: Review theTestChainUpgradefunction for correct setup, execution, and validation of chain upgrade tests.
Line range hint
95-95: Ensure theTestV6ToV7ChainUpgradefunction correctly tests the upgrade from v6 to v7 of the ibc-go binary, including client and consensus state migrations.
Line range hint
95-95: Review theTestV7ToV7_1ChainUpgradefunction for correct implementation of the upgrade process and validation of post-upgrade state.
Line range hint
95-95: Ensure theTestV7ToV8ChainUpgradefunction correctly handles the upgrade process, parameter updates, and packet flow validation.
Line range hint
95-95: Review theTestV8ToV8_1ChainUpgradefunction for correct implementation of the upgrade process, fee middleware integration, and packet fee handling.
Line range hint
95-95: Ensure theTestV8ToV8_1ChainUpgrade_ChannelUpgradesfunction correctly tests channel upgrades, relayer interactions, and fee payments.
Line range hint
95-95: Check theClientStatehelper function for correct implementation and error handling.modules/apps/transfer/keeper/relay_test.go (2)
Line range hint
564-599: Check for potential data races inTestOnRecvPacketSetsTotalEscrowAmountForSourceIBCToken.
Line range hint
217-238: Ensure proper handling ofForwardingPathinTestSendTransferSetsTotalEscrowAmountForSourceIBCToken.
| "tokens": [{ "denom": "stake", "amount": "100" }], | ||
| "sender": "` + icaAddress + `", | ||
| "receiver": "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk", | ||
| "timeout_height": { "revision_number": 1, "revision_height": 100 }, | ||
| "timeout_timestamp": 0 | ||
| "timeout_timestamp": 0, | ||
| "memo": "", | ||
| "forwarding_path": { "hops": [], "memo": "" } |
There was a problem hiding this comment.
The test case for MsgTransfer should include scenarios where forwardingPath has actual hops to test multi-hop functionality.
- "forwarding_path": { "hops": [], "memo": "" }
+ "forwarding_path": { "hops": ["path1", "path2"], "memo": "Test multi-hop" }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.
| "tokens": [{ "denom": "stake", "amount": "100" }], | |
| "sender": "` + icaAddress + `", | |
| "receiver": "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk", | |
| "timeout_height": { "revision_number": 1, "revision_height": 100 }, | |
| "timeout_timestamp": 0 | |
| "timeout_timestamp": 0, | |
| "memo": "", | |
| "forwarding_path": { "hops": [], "memo": "" } | |
| "tokens": [{ "denom": "stake", "amount": "100" }], | |
| "sender": "` + icaAddress + `", | |
| "receiver": "cosmos15ulrf36d4wdtrtqzkgaan9ylwuhs7k7qz753uk", | |
| "timeout_height": { "revision_number": 1, "revision_height": 100 }, | |
| "timeout_timestamp": 0, | |
| "memo": "", | |
| "forwarding_path": { "hops": ["path1", "path2"], "memo": "Test multi-hop" } |
| func (k Keeper) SetForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64, packet channeltypes.Packet) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| bz := k.cdc.MustMarshal(&packet) | ||
| store.Set(types.PacketForwardKey(portID, channelID, sequence), bz) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling for packet marshaling in SetForwardedPacket. The current implementation in relay.go does not handle potential panics from MustMarshal.
modules/apps/transfer/keeper/relay.go: Add error handling around theSetForwardedPacketcall to manage potential panics fromMustMarshal.
Analysis chain
Ensure proper error handling for packet marshaling.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that marshaling errors are handled properly in the calling code.
# Test: Search for the function usage. Expect: Proper error handling around this function call.
rg --type go $'SetForwardedPacket'
Length of output: 457
Script:
#!/bin/bash
# Extract lines around the usage of SetForwardedPacket in relay.go to inspect error handling.
rg --type go --context 5 'SetForwardedPacket' modules/apps/transfer/keeper/relay.go
Length of output: 351
| suite.chainB.GetTimeoutHeight(), | ||
| 0, | ||
| "", | ||
| nil, |
There was a problem hiding this comment.
Tip
Codebase Verification
Consider adding a test case to verify behavior when forwardingPath is non-nil along with an empty receiver.
The search did not reveal any existing test cases that specifically cover the scenario where forwardingPath is non-nil and the receiver is empty. It would be beneficial to add such a test case to ensure this edge case is properly handled.
- Location to add test case:
modules/apps/transfer/keeper/relay_test.go
Analysis chain
Consider adding a test case to verify behavior when forwardingPath is non-nil along with an empty receiver.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that tests for non-nil `forwardingPath` with an empty receiver are present.
# Test: Search for such test cases. Expect: At least one occurrence.
rg --type go 'forwardingPath: &.*receiver: ""'
Length of output: 48
Script:
#!/bin/bash
# Description: Broaden the search for test cases with non-nil `forwardingPath` and empty receiver.
# Test: Search for test cases with non-nil `forwardingPath` and empty receiver, considering variations.
rg --type go 'forwardingPath: &.*receiver: ""' -A 5
Length of output: 53
Script:
#!/bin/bash
# Description: Perform a general search for `forwardingPath` to manually inspect the context.
# Test: Search for occurrences of `forwardingPath` in Go test files.
rg --type go 'forwardingPath' -A 10
Length of output: 15412
|
|
||
| // send from chainB to chainC | ||
| msg = types.NewMsgTransfer(pathBtoC.EndpointA.ChannelConfig.PortID, pathBtoC.EndpointA.ChannelID, sdk.NewCoins(coinSentFromAToB), suite.chainB.SenderAccount.GetAddress().String(), suite.chainC.SenderAccount.GetAddress().String(), timeoutHeight, 0, "") | ||
| msg = types.NewMsgTransfer(pathBtoC.EndpointA.ChannelConfig.PortID, pathBtoC.EndpointA.ChannelID, sdk.NewCoins(coinSentFromAToB), suite.chainB.SenderAccount.GetAddress().String(), suite.chainC.SenderAccount.GetAddress().String(), timeoutHeight, 0, "", nil) |
There was a problem hiding this comment.
Similar to the previous comment, the NewMsgTransfer function call here also omits the ForwardingPath parameter. This should be included to match the updated function signature and to test the new functionality effectively.
|
|
||
| // send from chainC back to chainB | ||
| msg = types.NewMsgTransfer(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID, sdk.NewCoins(coinSentFromBToC), suite.chainC.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0, "") | ||
| msg = types.NewMsgTransfer(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID, sdk.NewCoins(coinSentFromBToC), suite.chainC.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0, "", nil) |
There was a problem hiding this comment.
Again, the NewMsgTransfer function call is missing the ForwardingPath parameter. Consistency in implementing the updated function signature across all test cases is crucial for proper validation.
|
|
||
| msg := types.NewMsgTransfer( | ||
| srcPort, srcChannel, coins, sender, receiver, timeoutHeight, timeoutTimestamp, memo, | ||
| srcPort, srcChannel, coins, sender, receiver, timeoutHeight, timeoutTimestamp, memo, nil, |
There was a problem hiding this comment.
The NewMsgTransfer function call in NewTransferTxCmd correctly integrates the new memo parameter. However, it seems that the forwardingPath parameter is still set to nil. If the forwarding functionality is intended to be used from the CLI, this needs to be implemented.
Consider adding CLI flags to specify the forwarding path and integrate it into the NewMsgTransfer call.
| portID, channelID string, token sdk.Coin, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string, | ||
| ) sdk.TxResponse { | ||
| msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo) | ||
| msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo, nil) |
There was a problem hiding this comment.
Include ForwardingInfo in MsgTransfer creation.
The MsgTransfer now supports a ForwardingInfo parameter, which is missing in this function call. Consider updating this to include ForwardingInfo as needed to align with the new function signature.
- msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo, nil)
+ msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo, forwardingInfo)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.
| msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo, nil) | |
| msg := transfertypes.NewMsgTransfer(portID, channelID, sdk.NewCoins(token), sender, receiver, timeoutHeight, timeoutTimestamp, memo, forwardingInfo) |
| suite.chainB.SenderAccount.GetAddress().String(), | ||
| suite.chainB.GetTimeoutHeight(), 0, // only use timeout height | ||
| "memo", | ||
| nil, |
There was a problem hiding this comment.
The NewMsgTransfer function calls in these test cases are missing the ForwardingPath parameter. This parameter should be included in all relevant test cases to ensure the functionality is correctly tested against the updated API.
Also applies to: 135-135, 174-174, 205-205
| sender, | ||
| receiver, | ||
| "", | ||
| nil, |
There was a problem hiding this comment.
The test cases in TestGetFullDenomPath are correctly implemented to check the full denomination path based on the trace. It would be beneficial to add more diverse test cases, especially testing edge cases like malformed traces or extremely long traces to ensure robustness.
Would you like assistance in adding these test cases?
Also applies to: 59-59
| Denom: denom, | ||
| Amount: packet.Data.Amount, | ||
| Trace: trace, | ||
| Amount: packet.Data.Amount, |
There was a problem hiding this comment.
The Amount field is directly set from the packet.Data.Amount, which is a string. This could lead to runtime errors if the string is not a valid integer. Consider parsing the string to an integer with error handling before assigning it to Amount.
- Amount: packet.Data.Amount,
+ amount, err := strconv.ParseInt(packet.Data.Amount, 10, 64)
+ if err != nil {
+ return err
+ }
+ Amount: amount,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.
| Amount: packet.Data.Amount, | |
| amount, err := strconv.ParseInt(packet.Data.Amount, 10, 64) | |
| if err != nil { | |
| return err | |
| } | |
| Amount: amount, |
7 participants
Description
closes: #6386
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
MsgTransferto include forwarding information.Bug Fixes
Tests