fix: XCS incorrect channel #8876
Conversation
crnbarr93
added
the
V:state/compatible/no_backport
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Outside diff range and nitpick comments (1)
cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
77-77: Avoid hardcoding byte-specific error messages in testsUsing specific key bytes in the expected error message can make the test brittle and prone to failure if internal implementations change. Consider matching on a more general error message or pattern that indicates the route was not found.
Apply this change to make the test less sensitive to internal changes:
test_swap!( non_existant_route should failed_with -"type: alloc::vec::Vec<osmosis_std::types::osmosis::poolmanager::v1beta1::SwapAmountInRoute>; key: [00, 0D, 72, 6F, 75, 74, 69, 6E, 67, 5F, 74, 61, 62, 6C, 65, 00, 04, 75, 69, 6F, 6E, 75, 6F, 73, 6D, 6F] not found: execute wasm contract failed", +"Route not found: execute wasm contract failed", msg = ExecuteMsg::Swap { input_coin: Coin::new(1000, "uion"), output_denom: "uosmo".to_string(), slippage: Slippage::MinOutputAmount(1000000000000000000000000u128.into()), route: None, }, funds: [ Coin::new(1000, "uion") ] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
.github/workflows/contracts.ymlis excluded by!**/*.ymlcosmwasm/Cargo.lockis excluded by!**/*.lock,!**/*.lockcosmwasm/Cargo.tomlis excluded by!**/*.tomltests/ibc-hooks/bytecode/crosschain_registry.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/crosschain_swaps.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/outpost.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/swaprouter.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/packages/registry/src/registry.rs(2 hunks)tests/ibc-hooks/ibc_middleware_test.go(20 hunks)
🔇 Additional comments (12)
tests/ibc-hooks/ibc_middleware_test.go (10)
11-11: Importing appparams package is appropriate
The addition of the appparams import is necessary for accessing application parameters used in the tests.
101-102: Initializing transfer path between chainB and chainC
Establishing suite.pathBC and setting it up ensures that tests involving communication between chainB and chainC are properly configured.
837-837: Ensuring correct channel IDs in transfer messages
The transfer message correctly uses channel-1 for the path between chainC and chainB. This aligns with the setup of suite.pathBC and ensures accurate testing of cross-chain transfers.
887-889: Updating chain channel links in registry
The modify_chain_channel_links function appropriately sets channel IDs for the chain pairs, ensuring the correct channels are used for IBC communication.
942-943: Handling removal of chain channel links
Removing the chain channel links between chainB and chainC tests the registry's ability to handle deletions correctly. The implementation appears sound.
1008-1008: Validating error handling for missing channel links
The assertion checking for "not found" in the error message ensures proper error handling when a chain channel link does not exist.
1573-1573: Constructing accurate IBC denom paths
The ACBPath is correctly constructed using the channels from suite.pathBC and suite.pathAC, ensuring that the denom trace paths reflect the correct chain of transfers.
1647-1651: Properly constructing denom prefixes for multi-hop transfers
The computation of prevPrefix and newPrefix accurately handles the denom path construction for tokens transferred across multiple chains, preventing issues with incorrect denom hashing.
1675-1675: Initializing account addresses for chainC
The retrieval of accountC ensures that tests involving chainC have the correct sender account setup.
1686-1690: Defining ChainActorDefinition for test cases
The actorChainB and actorChainC structures are properly defined to represent actors in the test scenarios, facilitating clearer and more maintainable test cases.
cosmwasm/packages/registry/src/registry.rs (1)
354-357: Ensure compatibility with Rust version for let...else syntax
The use of the let Some(...) = ... else { ... } pattern requires Rust version 1.65 or newer. Verify that the project's minimum supported Rust version is updated accordingly to prevent compilation issues.
CHANGELOG.md (1)
47-47: Changelog entry is correctly formatted and placed
The addition of the changelog entry for pull request #8876 under the "State Compatible" section appropriately documents the fix for the XCS incorrect channel issue.
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/ibc-hooks/ibc_middleware_test.go (1)
1689-1690: Correct indentation foractorChainCinitializationThere is inconsistent indentation in the initialization of
actorChainC. Adjust the indentation for better readability.Apply this diff to fix the indentation:
actorChainC := ChainActorDefinition{ - Chain: ChainC, + Chain: ChainC, name: "chainC", address: accountC, }cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
77-77: Simplify error message innon_existant_routetestThe error message includes raw byte arrays, which can be confusing. Consider simplifying the error message to make it more readable and user-friendly.
Apply this diff to revise the error message:
"dispatch: submessages: uion token is lesser than min amount: calculated amount is lesser than min amount", test_swap!( non_existant_route should failed_with - "type: alloc::vec::Vec<osmosis_std::types::osmosis::poolmanager::v1beta1::SwapAmountInRoute>; key: [00, 0D, 72, 6F, 75, 74, 69, 6E, 67, 5F, 74, 61, 62, 6C, 65, 00, 04, 75, 69, 6F, 6E, 75, 6F,73, 6D, 6F] not found: execute wasm contract failed", + "Swap route not found: execute wasm contract failed", msg = ExecuteMsg::Swap { input_coin: Coin::new(1000, "uion"),cosmwasm/packages/registry/src/registry.rs (1)
354-357: Avoid unnecessary cloning ofpathIn line 355,
path.clone()is used in constructing the error. If possible, avoid cloning by passing a reference topath, improving performance and reducing memory usage.Consider modifying the code to use a reference instead:
return Err(RegistryError::InvalidDenomTracePath { - path: path.clone(), + path: path.to_string(), denom: denom.into(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
.github/workflows/contracts.ymlis excluded by!**/*.ymlcosmwasm/Cargo.lockis excluded by!**/*.lock,!**/*.lockcosmwasm/Cargo.tomlis excluded by!**/*.tomltests/ibc-hooks/bytecode/crosschain_registry.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/crosschain_swaps.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/outpost.wasmis excluded by!**/*.wasm,!**/*.wasmtests/ibc-hooks/bytecode/swaprouter.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/packages/registry/src/registry.rs(2 hunks)tests/ibc-hooks/ibc_middleware_test.go(20 hunks)
🔇 Additional comments (6)
tests/ibc-hooks/ibc_middleware_test.go (5)
101-102: Correctly setting up the path between chain B and chain C
The initialization of suite.pathBC and its setup is appropriate and necessary for the new test cases involving chains B and C.
1008-1008: Verify error message content
The assertion checking that the error message contains "not found" is appropriate and ensures the test correctly detects missing channel links.
942-943:
Fix inconsistent chain naming in modifyChainChannelLinks function
In line 942, the chain name "ChainC" is capitalized, whereas elsewhere it is referred to as "chainC". This inconsistency may lead to errors when modifying chain channel links.
Apply this diff to correct the chain name:
{"operation": "set","source_chain": "chainD","destination_chain": "ChainC","channel_id": "channel-1"},
- {"operation": "remove","source_chain": "chainC","destination_chain": "chainB","channel_id": "channel-1"},
+ {"operation": "set","source_chain": "chainD","destination_chain": "chainC","channel_id": "channel-1"},
{"operation": "remove","source_chain": "chainC","destination_chain": "chainB","channel_id": "channel-1"},Likely invalid or redundant comment.
837-839: Verify the correctness of the channel ID in SetupCrosschainRegistry
Ensure that the channel ID "channel-1" used in the transfer from chain C to chain B matches the channel ID established during the setup of suite.pathBC. Inconsistencies may lead to incorrect test behaviors.
Run the following script to verify the channel IDs between chain B and chain C:
✅ Verification successful
Let me gather more information to verify the channel setup and usage.
Based on the gathered information, I can now generate the final response:
The channel ID "channel-1" is correctly used in the transfer from chain C to chain B
The test setup and channel configuration show that "channel-1" is indeed the correct channel ID for transfers between chain C and chain B. This is evidenced by multiple consistent references in the codebase, particularly in the crosschain registry configuration where it explicitly maps:
"source_chain": "chainB", "destination_chain": "chainC", "channel_id": "channel-1""source_chain": "chainC", "destination_chain": "chainB", "channel_id": "channel-1"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the channel IDs between chain B and chain C are correctly configured.
# Search for the channel setup between chain B and chain C
rg 'suite\.pathBC = NewTransferPath\(suite\.chainB, suite\.chainC\)' -A 5 tests/ibc-hooks/ibc_middleware_test.go
# Check the channel IDs assigned to pathBC Endpoints
rg 'suite\.pathBC\.Endpoint[A|B]\.ChannelID' -A 2 tests/ibc-hooks/ibc_middleware_test.go
# Expected: The channel IDs should match the ones used in the transfer message.
Length of output: 1846
Script:
#!/bin/bash
# Let's check the test setup and channel initialization
rg "func \(suite \*HooksTestSuite\) SetupTest\(\)" -A 30 tests/ibc-hooks/ibc_middleware_test.go
# Also check for any channel constants or setup
rg "channel-[0-9]" tests/ibc-hooks/ibc_middleware_test.go
Length of output: 8315
887-889: Ensure consistent channel IDs in chain channel links
Verify that the channel IDs specified when modifying chain channel links match the actual channel IDs established during setup. Inconsistencies can cause routing issues in the tests.
Run the following script to confirm the channel IDs:
✅ Verification successful
Let me gather more information about the test setup to understand the channel configuration better.
Let me search for the path initialization code to understand how the channels are set up.
Let me search for the actual channel setup code in the test framework to understand how channel IDs are assigned.
Based on the gathered information, I can now provide the final response:
Channel IDs in chain channel links are correctly configured
The channel IDs in the test setup match the established paths:
- Channel-0 is used for Osmosis connections (chainA/chainB to Osmosis)
- Channel-1 is used for chainB-chainC connections
- Channel-2 is used for chainB-cw20 specific paths
The channel IDs specified in the chain channel links are consistent with the test setup and routing configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the channel IDs in `setChainChannelLinks` match the actual channels.
# Extract channel IDs used in `setChainChannelLinks`
rg '"channel_id": "channel-1"' -A 2 tests/ibc-hooks/ibc_middleware_test.go
# Compare with the actual channel IDs from the test setup
rg 'suite\.path.*\.Endpoint[A|B]\.ChannelID' -A 2 tests/ibc-hooks/ibc_middleware_test.go
Length of output: 3558
Script:
#!/bin/bash
# Search for test setup code that establishes the paths and channels
rg 'func.*Setup' -A 10 tests/ibc-hooks/ibc_middleware_test.go
# Also look for path setup helper functions
rg 'setup.*Path' -A 5 tests/ibc-hooks/ibc_middleware_test.go
Length of output: 3641
Script:
#!/bin/bash
# Search for path initialization and channel setup
rg 'path[A-Z]{2,3}\s*=' tests/ibc-hooks/ibc_middleware_test.go -A 5
# Also look for coordinator setup that might show channel creation
rg 'coordinator.*Setup.*Path' tests/ibc-hooks/ibc_middleware_test.go -A 10
Length of output: 595
Script:
#!/bin/bash
# Look for the NewTransferPath implementation and channel setup
rg 'func NewTransferPath' -A 15 tests/ibc-hooks/ibc_middleware_test.go
# Also search for any channel ID assignments in the test framework
rg 'channel-[0-9]' tests/ibc-hooks/ibc_middleware_test.go
Length of output: 7547
CHANGELOG.md (1)
47-47: Changelog entry follows the guidelines
The new entry for PR #8876 is appropriately added under the "State Compatible" section and adheres to the changelog format.
| ACBPath := fmt.Sprintf("transfer/%s/transfer/%s", suite.pathBC.EndpointB.ChannelID, suite.pathAC.EndpointB.ChannelID) | ||
| denomTrace0ACB := transfertypes.DenomTrace{Path: ACBPath, BaseDenom: "token0"} | ||
| token0ACB := denomTrace0ACB.IBCDenom() |
There was a problem hiding this comment.
Fix potential incorrect endpoint usage in denom trace path
In line 1573, ACBPath uses suite.pathAC.EndpointB.ChannelID, but it should likely use suite.pathAC.EndpointA.ChannelID to accurately represent the denom trace path. This ensures the denom trace correctly reflects the channels used from chain C to chain A via chain B.
Apply this diff to correct the endpoint:
ACBPath := fmt.Sprintf("transfer/%s/transfer/%s", suite.pathBC.EndpointB.ChannelID, suite.pathAC.EndpointB.ChannelID)
- denomTrace0ACB := transfertypes.DenomTrace{Path: ACBPath, BaseDenom: "token0"}
+ ACBPath := fmt.Sprintf("transfer/%s/transfer/%s", suite.pathBC.EndpointB.ChannelID, suite.pathAC.EndpointA.ChannelID)
+ denomTrace0ACB := transfertypes.DenomTrace{Path: ACBPath, BaseDenom: "token0"}Committable suggestion skipped: line range outside the PR's diff.
What is the purpose of the change
This change fixes an issue with the cross-chain swaps (XCS) contract using the incorrect channel ID when creating a transfer message.
Testing and Verifying
Testing for the XCS contract was adjusted as the current implementation had an issue where the channels between the primary chain (chain A) and the company chains (chain B/C) had mirrored channel IDs. The adjustment was to reorder the creation of the channels so that the channel between chain A and chain C was
channel-1on chain A andchannel-0on chain C. This caused the previous XCS contract to error for the new test and pass for the updated contract code.NOTE: The version of
cosmwasm-stdwas fixed to1.4.4to satisfy linting, however this should be updated in a separate PRDocumentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)