Allow supplying final hop memos in XCS contracts #8895
Conversation
|
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." WalkthroughThis pull request introduces a new 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: 1
🔭 Outside diff range comments (2)
cosmwasm/packages/registry/src/registry.rs (1)
Line range hint
519-627: Review memo construction logic for potential issuesThe memo construction within
unwrap_coin_intohas been significantly modified. Ensure that:
- Memos are correctly merged and nested.
- Edge cases where memos might be empty or
Noneare properly handled.- The logic does not inadvertently overwrite or lose memo information.
Recommend thorough testing with various memo configurations to validate correctness.
cosmwasm/contracts/crosschain-registry/src/execute.rs (1)
Line range hint
72-109: Implement enable/disable functionality indenom_alias_operationsThe
denom_alias_operationsfunction now supports enabling and disabling denom aliases. Confirm that:
- The status of denom aliases is accurately reflected in both forward and reverse maps.
- Enabling/disabling operations correctly update the
enabledstatus without affecting the alias data.- There is consistency between the forward map (
DENOM_ALIAS_MAP) and the reverse map (DENOM_ALIAS_REVERSE_MAP).Potential mismatch between the forward and reverse maps when aliases are disabled. Recommend adding tests to validate map synchronization.
🧹 Nitpick comments (6)
cosmwasm/contracts/crosschain-swaps/src/execute.rs (1)
456-468: Enhance validation invalidate_user_provided_memofunctionThe
validate_user_provided_memofunction checks for the presence of theCALLBACK_KEYin user-provided memos. Consider strengthening this validation by:
- Ensuring the memo is a properly formatted JSON object.
- Checking for other disallowed keys or structures that could pose security risks.
- Providing detailed error messages for invalid memos.
cosmwasm/contracts/outpost/src/execute.rs (1)
Line range hint
73-74: Consider making IBC channel configurableThe source channel is hardcoded as "channel-0". Consider:
- Making it configurable through contract state
- Validating channel existence during instantiation
tests/ibc-hooks/ibc_middleware_test.go (4)
2152-2162: Consider using test constants package.The magic numbers for amounts could be moved to test constants for better maintainability. Also, consider adding comments explaining why these specific values were chosen.
+ const ( + // Amounts chosen to ensure: + // transferAmount > defaultPoolAmount > swapAmount + testTransferAmount = 12000000 + testSwapAmount = 1000 + testReceiveAmount = 980 + )
2164-2173: Consider adding error context to balance assertions.The balance checks could benefit from more descriptive error messages to help debug test failures.
- suite.Require().Equal(int64(-transferAmount), (postToken0BalanceOnC.Amount.Sub(preToken0BalanceOnC.Amount)).Int64()) + suite.Require().Equal( + int64(-transferAmount), + (postToken0BalanceOnC.Amount.Sub(preToken0BalanceOnC.Amount)).Int64(), + "Chain C's token0 balance should decrease by transfer amount", + )
2182-2196: Consider extracting message templates to constants.The JSON message templates could be moved to constants to improve readability and maintainability.
+ const ( + forwardMsgTemplate = `{"forward":{"receiver":"%s", "port":"transfer", "channel":%q}}` + swapMsgTemplate = `{"osmosis_swap":{"output_denom":%q,"slippage":{"twap": {"window_seconds": 1, "slippage_percentage":"20"}},"receiver":"chainB/%s", "on_failed_delivery": "do_nothing", "final_memo":%s}}` + )
2205-2228: Consider adding packet relay helper function.The packet relay logic is repeated in multiple tests. Consider extracting it to a helper function.
+ func (suite *HooksTestSuite) relayPacketInOrder(initialRes *abci.ExecTxResult, directions []Direction) *abci.ExecTxResult { + res := initialRes + for _, direction := range directions { + packet, err := ibctesting.ParsePacketFromEvents(res.GetEvents()) + suite.Require().NoError(err) + res = suite.RelayPacketNoAck(packet, direction) + } + return res + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/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 (17)
cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(3 hunks)cosmwasm/packages/registry/src/registry.rs(6 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
✅ Files skipped from review due to trivial changes (2)
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
🔇 Additional comments (36)
cosmwasm/contracts/crosschain-swaps/src/execute.rs (9)
85-85: Confirm final_memo inclusion in OsmosisSwap execute message
The final_memo field is passed to the OsmosisSwap variant of ExecuteMsg. Ensure that this variant has been updated accordingly and that final_memo is handled correctly in downstream processing.
149-149: Validate addition of final_memo parameter in swap_and_forward
The final_memo parameter has been added to the swap_and_forward function signature. Confirm that this change is correctly propagated throughout the codebase and that any necessary documentation is updated.
163-164: Ensure proper validation of next_memo and final_memo
User-provided memos next_memo and final_memo are being validated using validate_user_provided_memo. Verify that the validation function adequately checks for malicious content and invalid structures to prevent security vulnerabilities.
175-176: Pass validated memos to unwrap_coin_into method
The validated memos s_next_memo and s_final_memo are passed to unwrap_coin_into. Ensure the method handles these parameters correctly and that their inclusion doesn't introduce unexpected behavior.
212-212: Include final_memo in SwapMsgReplyState
The final_memo field is now part of the ForwardTo struct within SwapMsgReplyState. Confirm that serialization and deserialization of SwapMsgReplyState correctly handle this new field and that state persistence remains intact.
261-267: Correctly construct the final_memo string
The final_memo is being serialized into a string using serde_json_wasm::to_string. Ensure that the serialization is error-free and that any potential None values are appropriately handled with unwrap_or_default to avoid runtime exceptions.
279-280: Pass memos accurately to unwrap_coin_into
Both next_memo and final_memo are passed to unwrap_coin_into. Verify that the order and types of these arguments match the function's signature and that they are correctly utilized within the method.
43-43: Ensure all calls to unwrap_or_swap_and_forward include final_memo
The function unwrap_or_swap_and_forward now includes a new parameter final_memo: Option<SerializableJson>. Please verify that all calls to this function have been updated to include the final_memo parameter to prevent any compilation errors.
Run the following script to find any calls missing the final_memo parameter:
✅ Verification successful
All calls to unwrap_or_swap_and_forward include the final_memo parameter
Based on the codebase analysis, there is only one call to unwrap_or_swap_and_forward in contract.rs, and it correctly includes the final_memo parameter. No issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `unwrap_or_swap_and_forward` that lack the `final_memo` argument.
# Search for function calls with 8 arguments (excluding `final_memo`).
ast-grep --lang rust 'unwrap_or_swap_and_forward($_, $_, $_, $_, $_, $_, $_)' --exclude 'src/execute.rs'
Length of output: 220
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all calls to unwrap_or_swap_and_forward
rg "unwrap_or_swap_and_forward" -A 3 -B 3
Length of output: 1337
Script:
#!/bin/bash
# Let's look at the actual call to unwrap_or_swap_and_forward in contract.rs to see all parameters
rg -A 10 "execute::unwrap_or_swap_and_forward\(" cosmwasm/contracts/crosschain-swaps/src/contract.rs
Length of output: 434
131-131: Verify all calls to swap_and_forward are updated with final_memo
The swap_and_forward function now accepts final_memo as a parameter. Please ensure all calls to this function include the new parameter to maintain consistency and functionality.
Run the following script to identify any outdated calls:
✅ Verification successful
All calls to swap_and_forward are properly updated with final_memo
Based on the code analysis, the only external call to swap_and_forward is through the unwrap_or_swap_and_forward function in contract.rs, which correctly passes the final_memo parameter. The implementation in execute.rs shows that final_memo is properly defined as a parameter, and all usage is consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `swap_and_forward` missing the `final_memo` parameter.
# Search for function calls with 9 arguments (excluding `final_memo`).
ast-grep --lang rust 'swap_and_forward($_, $_, $_, $_, $_, $_, $_, $_, $_)' --exclude 'src/execute.rs'
Length of output: 218
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all calls to swap_and_forward
rg "swap_and_forward\(" --type rust -A 3 -B 1
Length of output: 1751
Script:
#!/bin/bash
# Let's look at the implementation of swap_and_forward to understand its parameters
rg "fn swap_and_forward" -A 15 --type rust
# And let's also check the actual call site to see how it's being used
rg "swap_and_forward\(" -A 10 --type rust --glob "!execute.rs"
Length of output: 2347
cosmwasm/packages/registry/src/registry.rs (6)
7-7: Add SerializableJson to imports
The SerializableJson type has been imported from crate::msg. This import is necessary for handling flexible memo serialization and deserialization.
119-119: Update ForwardingMemo struct to use SerializableJson
The next field in ForwardingMemo has been updated to Option<Box<SerializableJson>>, enhancing memo flexibility. Ensure that all instances where ForwardingMemo is used accommodate this change.
555-587: Verify correct handling of nested memos and callbacks
In lines 555-587, the memo construction involves conditional logic based on the presence of next and callback. Ensure that:
- The
final_memoandcallbackare correctly incorporated. - There are no unintended side effects when both
next_memoandfinal_memoare present. - The recursion properly nests memos without exceeding stack limits.
596-612: Handle scenarios with no generated memo
When no memo is generated (next is None), the code now includes a default memo with callback and last_transfer_memo. Confirm that this approach correctly ensures memos are not omitted in IBC transfers.
615-627: Ensure first transfer memo is correctly merged
The code merges first_transfer_memo into the existing memo structure. Verify that the merge operation maintains the integrity of existing memo data and that it correctly handles conflicts or duplicate keys.
424-424: Include last_transfer_memo parameter in unwrap_coin_into
A new parameter last_transfer_memo: String is added to the unwrap_coin_into function. Verify that all calls to this function include the new parameter and that it's correctly handled within the function.
Run the following script to find any function calls missing last_transfer_memo:
✅ Verification successful
Based on the code analysis, I can now provide the final response:
last_transfer_memo parameter is correctly handled in all calls
The unwrap_coin_into function is called in 5 locations across the codebase, and all calls are made through the registry instance which automatically handles the new last_transfer_memo parameter with proper defaults. The function signature shows that last_transfer_memo is properly integrated into the memo construction logic, where it's merged with callback information when building the final memo for IBC transfers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `unwrap_coin_into` missing the `last_transfer_memo` parameter.
ast-grep --lang rust 'unwrap_coin_into($_, $_, $_, $_, $_, $_, $_, $_, $_)' --exclude 'src/registry.rs'
Length of output: 219
Script:
#!/bin/bash
# Search for all occurrences of unwrap_coin_into function calls
rg "unwrap_coin_into" -A 3
Length of output: 2052
Script:
#!/bin/bash
# Search for the function definition to understand its parameters
ast-grep -p 'fn unwrap_coin_into($$$)'
Length of output: 40
Script:
#!/bin/bash
# Get the full context of the registry.rs file
cat cosmwasm/packages/registry/src/registry.rs
Length of output: 27076
cosmwasm/contracts/crosschain-registry/src/execute.rs (6)
Line range hint 30-32: Extend FullOperation enum with Enable and Disable variants
The FullOperation enum now includes Enable and Disable. Make sure all pattern matches and logic handling this enum are updated to accommodate the new variants.
Line range hint 34-37: Introduce ConnectionInput struct
A new struct ConnectionInput has been added to manage chain connections. Ensure it's correctly integrated into functions handling connection operations.
Line range hint 39-42: Add ChainToBech32PrefixInput struct
The ChainToBech32PrefixInput struct has been introduced for handling chain-to-prefix mappings. Verify its utilization in relevant operations and that it aligns with the intended data flow.
Line range hint 57-70: Enhance error handling and authorization in contract_alias_operations
The function now includes improved error handling for alias existence checks and enforces authorization. Ensure that unauthorized attempts are properly rejected and that error messages provide clear guidance.
Line range hint 111-148: Update connection_operations to handle new FullOperation variants
The connection_operations function now accommodates Enable and Disable operations. Verify that these operations correctly modify the connection status and that appropriate authorization checks are in place.
Line range hint 150-187: Implement enable/disable logic in chain_to_prefix_operations
Ensure that enabling and disabling chain-to-prefix associations function as intended. Check that:
- The reverse mapping (
CHAIN_TO_BECH32_PREFIX_REVERSE_MAP) is updated whenever an association's status changes. - Disabling a chain's prefix removes it from the reverse map to prevent incorrect resolution.
cosmwasm/contracts/crosschain-swaps/src/state.rs (1)
21-21: Add final_memo field to ForwardTo struct
The final_memo: Option<SerializableJson> field has been added to the ForwardTo struct. Ensure that:
- All serialization and deserialization processes are updated to include this new field.
- Existing persisted data is migrated appropriately if necessary.
- The new field is properly utilized in logic that handles forwarding operations.
cosmwasm/contracts/outpost/src/contract.rs (1)
29-29: LGTM: Improved error handling pattern
The change adopts the modern Rust let Ok(...) else pattern, making the error handling more idiomatic and readable while maintaining the same functionality.
cosmwasm/contracts/crosschain-registry/src/msg.rs (1)
56-57: Consider documenting the final_memo field purpose
While the implementation is correct, adding documentation comments would help users understand:
- The purpose of
final_memo - When it's used in the unwrapping process
- Any formatting requirements or restrictions
Verify empty string as default
The field defaults to an empty string. Verify if this is the intended behavior for cross-chain operations.
✅ Verification successful
Let me gather more context about the memo handling by checking the documentation and implementation.
Let me check the registry package implementation to understand how the memo is used in the unwrap operation.
Let me try a different approach to find the memo handling implementation.
Let me check the registry implementation specifically for memo handling.
Based on the evidence gathered from the codebase:
-
The empty string default for
final_memois consistent with the existing memo handling in the registry package, as seen in the first search results where empty memos are handled gracefully:- Empty memos are converted to
SerializableJson::empty() - Non-empty memos are properly parsed and processed
- The code has explicit checks for
memo.is_empty()
- Empty memos are converted to
-
The implementation follows the pattern where memo fields are optional with empty string defaults, as seen in both the contract and registry package.
Empty string default is appropriate for final_memo
The implementation is consistent with the codebase's memo handling patterns where empty memos are valid and properly handled. No issues found with the default value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for empty string memo handling in IBC transfer code
rg -A 5 'memo.is_empty\(\)'
Length of output: 2487
Script:
#!/bin/bash
# Search for documentation or comments about memo handling
rg -B 2 -A 5 'memo' cosmwasm/contracts/crosschain-registry/src/
# Check if there are any tests related to memo handling
ast-grep --pattern 'fn $test_name() {
$$$
memo
$$$
}'
Length of output: 3275
Script:
#!/bin/bash
# Check the unwrap implementation in registry package
ast-grep --pattern 'fn unwrap_coin_into(
$$$
) -> $$ {
$$$
}'
# Look for memo-related documentation in the registry package
rg -B 2 'final_memo|with_memo' cosmwasm/packages/registry/src/
Length of output: 132
Script:
#!/bin/bash
# Search for unwrap_coin implementation with broader pattern
ast-grep --pattern 'impl Registry {
$$$
unwrap_coin$$$($$$) {
$$$
}
$$$
}'
# Search for memo usage in registry package
rg -l 'memo' cosmwasm/packages/registry/src/
Length of output: 294
Script:
#!/bin/bash
# Check the registry implementation for memo handling
rg -A 10 'unwrap_coin.*memo' cosmwasm/packages/registry/src/registry.rs
# Check for any memo validation or error handling
rg -A 5 'memo.*Error|Error.*memo' cosmwasm/packages/registry/src/error.rs
Length of output: 149
cosmwasm/contracts/outpost/src/execute.rs (1)
55-55: Verify handling of None final_memo in downstream contracts
The final_memo is initialized as None. Please ensure:
- Downstream contracts handle
Nonevalue appropriately - Test coverage includes scenarios with both
SomeandNonevalues
✅ Verification successful
Setting final_memo as None is correct and properly handled
The codebase analysis shows that final_memo is properly handled throughout the contract chain:
- In crosschain-swaps contract, it's defined as
Option<SerializableJson>in both message definitions and state - The value is properly validated via
validate_user_provided_memofunction - It's correctly propagated to IBC transfer construction
- The None case is handled by
.unwrap_or_default()when constructing the final memo string - Test coverage exists in the integration tests (ibc_middleware_test.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for final_memo handling in downstream contracts
ast-grep --pattern 'match $_.final_memo {
$$$
}'
# Search for test coverage
rg -A 10 'final_memo.*None'
rg -A 10 'final_memo.*Some'
Length of output: 816
Script:
#!/bin/bash
# Let's search for final_memo usage in the codebase
rg -A 5 "final_memo"
# Search for IBC transfer message construction
rg -A 10 "IbcTransfer|IbcMsg::Transfer"
# Search for test files that might cover this functionality
fd "test.*\.rs$" --exec rg -l "final_memo|IbcTransfer"
Length of output: 9250
cosmwasm/packages/registry/src/error.rs (1)
71-73: LGTM: Well-structured error variant addition
The new DuplicateKeyError variant is appropriately placed and follows the existing error pattern. The error message is clear and descriptive.
cosmwasm/contracts/crosschain-swaps/src/utils.rs (2)
18-18: LGTM: Improved error message formatting
The error message formatting has been improved for better clarity.
Line range hint 1-70: Consider updating function signature for final_memo support
The build_memo function should be updated to handle the new final_memo parameter to align with the PR objectives.
Let's verify if this function is called with final_memo:
Consider updating the function signature:
pub fn build_memo(
next_memo: Option<SerializableJson>,
+ final_memo: Option<SerializableJson>,
contract_addr: &str,
) -> Result<String, ContractError>cosmwasm/contracts/crosschain-swaps/src/contract.rs (1)
62-62: LGTM: Added final_memo support to OsmosisSwap
The final_memo parameter has been properly added to both the message struct and function call.
Let's verify the implementation of unwrap_or_swap_and_forward:
Also applies to: 71-71
✅ Verification successful
LGTM: final_memo is properly handled in the implementation
The implementation correctly:
- Accepts
final_memoas an optional parameter inunwrap_or_swap_and_forward - Passes it through to the
swap_and_forwardfunction - Both functions have proper documentation and consistent parameter handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for unwrap_or_swap_and_forward implementation
ast-grep --pattern 'fn unwrap_or_swap_and_forward($_) {
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Let's try a different pattern to find the implementation
rg -A 10 "unwrap_or_swap_and_forward" --type rust
Length of output: 1920
Script:
#!/bin/bash
# Let's check the implementation details of unwrap_or_swap_and_forward in execute.rs
rg -B 5 -A 30 "pub fn unwrap_or_swap_and_forward" cosmwasm/contracts/crosschain-swaps/src/execute.rs
Length of output: 1559
Script:
#!/bin/bash
# Let's check where this final_memo is used in the actual swap and forward logic
rg -A 20 "swap_and_forward" cosmwasm/contracts/crosschain-swaps/src/execute.rs
Length of output: 3166
cosmwasm/contracts/crosschain-swaps/src/msg.rs (1)
57-60: LGTM! Well-documented addition of final_memo field.
The addition of the final_memo field is well-implemented with proper documentation and consistent typing with the existing next_memo field. The documentation clearly explains its purpose for the last hop's IBC packet.
cosmwasm/packages/registry/src/msg.rs (3)
114-123: LGTM! Well-implemented empty state handling.
The empty() and is_empty() methods follow Rust conventions and provide a clean way to handle empty JSON objects.
129-161: LGTM! Robust merge implementation with proper error handling.
The merge method is well-implemented with:
- Proper validation of JSON object types
- Prevention of duplicate keys
- Clear error messages
170-177: LGTM! Clean TryFrom implementation.
The TryFrom<Memo> implementation provides a clean way to convert from Memo type to SerializableJson with proper error handling.
cosmwasm/contracts/crosschain-registry/src/contract.rs (1)
Line range hint 85-96: LGTM! Proper integration of final_memo parameter.
The addition of the final_memo parameter to the UnwrapCoin variant and its passing to unwrap_coin_into is implemented correctly.
tests/ibc-hooks/ibc_middleware_test.go (2)
2141-2150: LGTM! Well-structured test setup.
The test function is well-named and has clear initialization of test accounts and contract addresses. The comments effectively explain the test's purpose and flow.
2229-2232: LGTM! Clear final verification.
The final balance check effectively verifies the end state of the cross-chain swap with memo forwarding.
| let serde_cw_value::Value::Map(mut m) = memo.0 else { | ||
| unreachable!() | ||
| }; |
There was a problem hiding this comment.
Replace unreachable!() with proper error handling
Using unreachable!() macro could lead to runtime panics if the JSON structure is unexpected. Consider proper error handling instead.
Apply this diff for safer error handling:
- let serde_cw_value::Value::Map(mut m) = memo.0 else {
- unreachable!()
- };
+ let serde_cw_value::Value::Map(mut m) = memo.0 else {
+ return Err(ContractError::InvalidMemo {
+ error: "expected JSON object".to_string(),
+ memo: format!("{:?}", memo.0),
+ });
+ };📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let serde_cw_value::Value::Map(mut m) = memo.0 else { | |
| unreachable!() | |
| }; | |
| let serde_cw_value::Value::Map(mut m) = memo.0 else { | |
| return Err(ContractError::InvalidMemo { | |
| error: "expected JSON object".to_string(), | |
| memo: format!("{:?}", memo.0), | |
| }); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/ibc-hooks/ibc_middleware_test.go (3)
2141-2248: Consider improving test documentation and constant organizationThe test case is well structured but could benefit from:
- Adding a detailed comment block explaining the test flow and expected behavior
- Moving the constants (
transferAmount,swapAmount,receiveAmount) to package level if they're used across tests- Adding comments explaining the significance of magic numbers like
980inreceiveAmountfunc (suite *HooksTestSuite) TestCrosschainSwapsFinalMemo() { + // Test verifies that final_memo in crosschain swaps is correctly handled: + // 1. Start on chain B with token0 + // 2. Swap on chain A with C/token0 + // 3. Use final_memo to forward swapped tokens to chain C + // 4. Verify native token0 is received on chain C + + const ( + // Amount transferred between chains for pool setup + transferAmount = 12000000 + // Amount used in the actual swap + swapAmount = 1000 + // Expected amount after swap (accounting for slippage) + receiveAmount = 980 + )
2250-2257: Improve function implementation and documentationThe helper function could be enhanced for better readability and maintainability:
+// findFungibleTokenPacketEvent searches for and returns the first fungible_token_packet +// event in the provided events slice. Returns nil if no matching event is found. func findFungibleTokenPacketEvent(events []abci.Event) *abci.Event { - for i := 0; i < len(events); i++ { - if events[i].Type == "fungible_token_packet" { - return &events[i] + for _, event := range events { + if event.Type == "fungible_token_packet" { + return &event } } return nil }
2298-2305: Enhance function documentation and validationThe helper function could be improved with better documentation and validation:
+// assertForwardMemoStructure verifies that a memo map contains the expected forward +// structure with matching channel, port and receiver values. Fails the test if +// the structure is invalid or values don't match. func (suite *HooksTestSuite) assertForwardMemoStructure(memo map[string]any, channel, port, receiver string) { + suite.Require().NotNil(memo, "memo cannot be nil") + forward, ok := memo["forward"].(map[string]any) - suite.Require().True(ok) + suite.Require().True(ok, "memo must contain 'forward' map") - suite.Require().Equal(channel, forward["channel"]) - suite.Require().Equal(port, forward["port"]) - suite.Require().Equal(receiver, forward["receiver"]) + suite.Require().Equal(channel, forward["channel"], "forward channel mismatch") + suite.Require().Equal(port, forward["port"], "forward port mismatch") + suite.Require().Equal(receiver, forward["receiver"], "forward receiver mismatch") }
| func extractFinalMemoFromSwapMsg(memo map[string]any) map[string]any { | ||
| wasm, ok := memo["wasm"].(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| msg, ok := wasm["msg"].(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| osmosisSwap, ok := msg["osmosis_swap"].(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| finalMemo, ok := osmosisSwap["final_memo"].(map[string]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| return finalMemo | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function documentation and error handling
The helper function could be enhanced for better maintainability and error reporting:
+// extractFinalMemoFromSwapMsg navigates through a nested map structure to extract
+// the final_memo from an osmosis_swap message. The expected structure is:
+// {
+// "wasm": {
+// "msg": {
+// "osmosis_swap": {
+// "final_memo": {...}
+// }
+// }
+// }
+// }
+// Returns nil if any part of the path is missing or of wrong type.
func extractFinalMemoFromSwapMsg(memo map[string]any) map[string]any {
+ if memo == nil {
+ return nil
+ }
+
wasm, ok := memo["wasm"].(map[string]any)
if !ok {
return nil
}
-
msg, ok := wasm["msg"].(map[string]any)
if !ok {
return nil
}
-
osmosisSwap, ok := msg["osmosis_swap"].(map[string]any)
if !ok {
return nil
}
-
finalMemo, ok := osmosisSwap["final_memo"].(map[string]any)
if !ok {
return nil
}
-
return finalMemo
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func extractFinalMemoFromSwapMsg(memo map[string]any) map[string]any { | |
| wasm, ok := memo["wasm"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| msg, ok := wasm["msg"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| osmosisSwap, ok := msg["osmosis_swap"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| finalMemo, ok := osmosisSwap["final_memo"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| return finalMemo | |
| } | |
| // extractFinalMemoFromSwapMsg navigates through a nested map structure to extract | |
| // the final_memo from an osmosis_swap message. The expected structure is: | |
| // { | |
| // "wasm": { | |
| // "msg": { | |
| // "osmosis_swap": { | |
| // "final_memo": {...} | |
| // } | |
| // } | |
| // } | |
| // } | |
| // Returns nil if any part of the path is missing or of wrong type. | |
| func extractFinalMemoFromSwapMsg(memo map[string]any) map[string]any { | |
| if memo == nil { | |
| return nil | |
| } | |
| wasm, ok := memo["wasm"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| msg, ok := wasm["msg"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| osmosisSwap, ok := msg["osmosis_swap"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| finalMemo, ok := osmosisSwap["final_memo"].(map[string]any) | |
| if !ok { | |
| return nil | |
| } | |
| return finalMemo | |
| } |
| func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) { | ||
| for i := 0; i < len(event.Attributes); i++ { | ||
| if event.Attributes[i].Key == "memo" { | ||
| var memo map[string]any | ||
| decoder := json.NewDecoder(strings.NewReader(event.Attributes[i].Value)) | ||
| err := decoder.Decode(&memo) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return memo, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("could not find memo in event") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance function implementation, documentation and error handling
The helper function could be improved in several ways:
+// parseFungibleTokenPacketEventMemo extracts and parses the memo attribute from a
+// fungible token packet event. Returns the parsed memo as a map and any error encountered.
+// Returns an error if the memo attribute is not found or if JSON parsing fails.
func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) {
- for i := 0; i < len(event.Attributes); i++ {
- if event.Attributes[i].Key == "memo" {
- var memo map[string]any
- decoder := json.NewDecoder(strings.NewReader(event.Attributes[i].Value))
- err := decoder.Decode(&memo)
+ if event == nil {
+ return nil, fmt.Errorf("event cannot be nil")
+ }
+
+ for _, attr := range event.Attributes {
+ if attr.Key == "memo" {
+ memo := make(map[string]any)
+ err := json.Unmarshal([]byte(attr.Value), &memo)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to parse memo JSON: %w", err)
}
return memo, nil
}
}
- return nil, fmt.Errorf("could not find memo in event")
+ return nil, fmt.Errorf("memo attribute not found in event")
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) { | |
| for i := 0; i < len(event.Attributes); i++ { | |
| if event.Attributes[i].Key == "memo" { | |
| var memo map[string]any | |
| decoder := json.NewDecoder(strings.NewReader(event.Attributes[i].Value)) | |
| err := decoder.Decode(&memo) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return memo, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("could not find memo in event") | |
| } | |
| // parseFungibleTokenPacketEventMemo extracts and parses the memo attribute from a | |
| // fungible token packet event. Returns the parsed memo as a map and any error encountered. | |
| // Returns an error if the memo attribute is not found or if JSON parsing fails. | |
| func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) { | |
| if event == nil { | |
| return nil, fmt.Errorf("event cannot be nil") | |
| } | |
| for _, attr := range event.Attributes { | |
| if attr.Key == "memo" { | |
| memo := make(map[string]any) | |
| err := json.Unmarshal([]byte(attr.Value), &memo) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse memo JSON: %w", err) | |
| } | |
| return memo, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("memo attribute not found in event") | |
| } |
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
2 times, most recently
from
c3c3332 to
14b98f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/ibc-hooks/ibc_middleware_test.go (1)
2260-2272: 🛠️ Refactor suggestionEnhance function implementation and error handling
The
parseFungibleTokenPacketEventMemofunction can be improved by adding a nil check for the event parameter, using a range loop for better readability, and utilizingjson.Unmarshalfor parsing. This enhances error handling and code clarity.Apply the following diff:
+// parseFungibleTokenPacketEventMemo extracts and parses the memo attribute from a +// fungible token packet event. Returns the parsed memo as a map and any error encountered. +// Returns an error if the memo attribute is not found or if JSON parsing fails. func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) { + if event == nil { + return nil, fmt.Errorf("event cannot be nil") + } - for i := 0; i < len(event.Attributes); i++ { - if event.Attributes[i].Key == "memo" { + for _, attr := range event.Attributes { + if attr.Key == "memo" { var memo map[string]any - decoder := json.NewDecoder(strings.NewReader(event.Attributes[i].Value)) - err := decoder.Decode(&memo) + err := json.Unmarshal([]byte(attr.Value), &memo) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse memo JSON: %w", err) } return memo, nil } } - return nil, fmt.Errorf("could not find memo in event") + return nil, fmt.Errorf("memo attribute not found in event") }
🧹 Nitpick comments (4)
tests/ibc-hooks/ibc_middleware_test.go (4)
2141-2249: Simplify balance difference calculations for clarityIn
TestCrosschainSwapsFinalMemoMultipleHops, the balance difference calculations can be made clearer by consistently subtracting the pre-transfer balance from the post-transfer balance. This improves readability and reduces potential confusion.Apply the following diff to standardize the balance calculations:
- suite.Require().Equal(int64(-transferAmount), (postToken0BalanceOnC.Amount.Sub(preToken0BalanceOnC.Amount)).Int64()) + suite.Require().Equal(int64(transferAmount), (preToken0BalanceOnC.Amount.Sub(postToken0BalanceOnC.Amount)).Int64())Similarly, update the balance check on line 2173:
- suite.Require().Equal(int64(-transferAmount), (postToken0BalanceOnB.Amount.Sub(preToken0BalanceOnB.Amount)).Int64()) + suite.Require().Equal(int64(transferAmount), (preToken0BalanceOnB.Amount.Sub(postToken0BalanceOnB.Amount)).Int64())
2274-2301: Add nil checks and error handling in memo extractionIn
extractNextAndFinalMemosFromSwapMsg, consider adding nil checks and proper type assertions to prevent potential nil pointer dereferences or type assertion errors. This enhances the robustness of the code.Apply this diff to improve safety:
func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (map[string]any, map[string]any) { + if memo == nil { + return nil, nil + } wasm, ok := memo["wasm"].(map[string]any) if !ok { return nil, nil } msg, ok := wasm["msg"].(map[string]any) if !ok { return nil, nil } osmosisSwap, ok := msg["osmosis_swap"].(map[string]any) if !ok { return nil, nil } nextMemo, _ := osmosisSwap["next_memo"].(map[string]any) finalMemo, _ := osmosisSwap["final_memo"].(map[string]any) return nextMemo, finalMemo }
2303-2310: Enhance error handling inassertForwardMemoStructureIn the
assertForwardMemoStructurefunction, adding type assertions and checks ensures that the expected keys are present and of the correct type, preventing potential panics during test execution.Apply this diff to improve error handling:
func (suite *HooksTestSuite) assertForwardMemoStructure(memo map[string]any, channel, port, receiver string) { forward, ok := memo["forward"].(map[string]any) suite.Require().True(ok, "forward key missing or incorrect type in memo") + ch, ok := forward["channel"].(string) + suite.Require().True(ok, "channel key missing or incorrect type in forward memo") + suite.Require().Equal(channel, ch) + p, ok := forward["port"].(string) + suite.Require().True(ok, "port key missing or incorrect type in forward memo") + suite.Require().Equal(port, p) + recv, ok := forward["receiver"].(string) + suite.Require().True(ok, "receiver key missing or incorrect type in forward memo") + suite.Require().Equal(receiver, recv) }
2312-2407: Expand test cases forTestCrosschainSwapsFinalMemoOneHopTo improve test coverage, consider adding test cases where
next_memoandfinal_memoare absent, empty, or contain invalid data. This ensures the memo merging logic is robust against various inputs.For example, add scenarios where:
- Both
next_memoandfinal_memoare omitted.next_memois provided, butfinal_memois missing.- Memos contain unexpected data types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/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 (1)
tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/ibc-hooks/ibc_middleware_test.go (1)
2274-2310: Improve error handling and add documentation forextractNextAndFinalMemosFromSwapMsgConsider enhancing the function by adding documentation and improving error handling to provide more informative errors when the memo structure is not as expected.
Apply this diff to enhance the function:
+// extractNextAndFinalMemosFromSwapMsg extracts the `next_memo` and `final_memo` fields +// from an osmosis_swap message within the provided memo map. It returns the extracted memos +// as maps if they are present and correctly formatted, or an error otherwise. +func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (nextMemo map[string]any, finalMemo map[string]any, err error) { var ok bool wasm, ok := memo["wasm"].(map[string]any) if !ok { + return nil, nil, fmt.Errorf("memo missing 'wasm' field or incorrect type") } msg, ok := wasm["msg"].(map[string]any) if !ok { + return nil, nil, fmt.Errorf("wasm missing 'msg' field or incorrect type") } osmosisSwap, ok := msg["osmosis_swap"].(map[string]any) if !ok { + return nil, nil, fmt.Errorf("msg missing 'osmosis_swap' field or incorrect type") } nextMemoAny := osmosisSwap["next_memo"] if nextMemoAny != nil { nextMemo, ok = nextMemoAny.(map[string]any) if !ok { + return nil, nil, fmt.Errorf("'next_memo' field is present but not a map") } } finalMemoAny := osmosisSwap["final_memo"] if finalMemoAny != nil { finalMemo, ok = finalMemoAny.(map[string]any) if !ok { + return nil, nil, fmt.Errorf("'final_memo' field is present but not a map") } } + return nextMemo, finalMemo, nil }Update the calling code accordingly to handle the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
🔇 Additional comments (1)
tests/ibc-hooks/ibc_middleware_test.go (1)
2259-2272: Enhance function implementation and error handling
Consider improving the parseFungibleTokenPacketEventMemo function by adding documentation and enhancing error handling. Specifically, check for nil events, use range-based loops, and simplify the code by using json.Unmarshal.
Apply this diff to enhance the function:
+// parseFungibleTokenPacketEventMemo extracts and parses the memo attribute from a
+// fungible token packet event. Returns the parsed memo as a map and any error encountered.
+// Returns an error if the memo attribute is not found or if JSON parsing fails.
func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) {
+ if event == nil {
+ return nil, fmt.Errorf("event cannot be nil")
+ }
+
+ for _, attr := range event.Attributes {
+ if attr.Key == "memo" {
+ memo := make(map[string]any)
+ err := json.Unmarshal([]byte(attr.Value), &memo)
if err != nil {
+ return nil, fmt.Errorf("failed to parse memo JSON: %w", err)
}
return memo, nil
}
}
+ return nil, fmt.Errorf("memo attribute not found in event")
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/test_helpers.go (1)
142-171: Consider adding error handling for the logger initialization.The setup logic looks good, but consider adding error handling around the logger initialization to prevent potential panics.
if len(t) > 0 { testEnv := t[0] - testEnv.Log("Using test environment logger") - l = log.NewTestLogger(testEnv) + testEnv.Log("Initializing test environment logger") + logger, err := log.NewTestLogger(testEnv) + if err != nil { + testEnv.Fatalf("Failed to initialize logger: %v", err) + } + l = logger } else { l = log.NewNopLogger() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)
🔇 Additional comments (5)
app/test_helpers.go (4)
21-22: LGTM!
The imports are correctly added to support the new debug logging functionality.
123-124: LGTM!
The function signature change to accept testing context is a good improvement for better test logging and debugging capabilities.
127-136: LGTM!
The DebugAppOptions implementation correctly implements the AppOptions interface and provides trace flag support.
138-140: LGTM!
The IsDebugLogEnabled function provides a clean way to check debug logging state via environment variable.
app/apptesting/test_suite.go (1)
106-116: LGTM! Good improvements to test environment setup.
The changes improve the test environment setup by:
- Adding proper logging of setup initialization
- Ensuring cleanup of temporary directories
- Integrating with the debug logging system
crnbarr93
added
the
V:state/compatible/no_backport
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
5be4db8 to
0ad6304
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
cosmwasm/packages/registry/src/registry.rs (3)
119-119: Good architectural choice using SerializableJson!The change from
Option<Box<Memo>>toOption<Box<SerializableJson>>enhances the flexibility of the memo structure. This allows for better handling of user-provided memos and makes the forwarding system more extensible for future memo formats.
555-587: Consider improving error handling in memo construction.While the memo construction logic is correct, consider adding more specific error types for memo-related failures to improve debugging and error handling.
next = Some(Box::new( Memo { forward: Some(ForwardingMemo { - receiver: self.encode_addr_for_chain(&receiver_addr, prev_chain)?, + receiver: self.encode_addr_for_chain(&receiver_addr, prev_chain) + .map_err(|e| RegistryError::MemoConstructionError { + context: "Failed to encode receiver address".into(), + source: e, + })?, // ... rest of the code }), } .try_into()?, ));
596-612: Consider extracting memo merging logic.The memo merging logic is repeated in multiple places. Consider extracting it into a separate function for better maintainability and reusability.
+ fn merge_memo( + base: Option<Box<SerializableJson>>, + additional: &str, + ) -> Result<Option<Box<SerializableJson>>, RegistryError> { + if additional.is_empty() { + return Ok(base); + } + + let additional_memo: SerializableJson = serde_json_wasm::from_str(additional)?; + + if let Some(mut box_base) = base { + use std::ops::DerefMut; + let base_memo = std::mem::replace(box_base.deref_mut(), SerializableJson::empty()); + *box_base.deref_mut() = base_memo.merge(additional_memo)?; + Ok(Some(box_base)) + } else { + Ok(Some(Box::new(additional_memo))) + } + }Also applies to: 615-627
tests/ibc-hooks/ibc_middleware_test.go (3)
2140-2249: Ensure robust error-handling and coverage in multi-hop final memo tests
This test effectively verifies the multi-hop scenario and final memo usage. Consider introducing negative test scenarios (e.g. invalid final memo structure or insufficient transfer amounts) to further strengthen coverage.
2259-2272: Potential improvement in memo parsing error context
The current error message"could not find memo in event"is acceptable. Consider adding more context about which event was scanned if debugging complex logs.
2320-2415: Single-hop final memo test logic is solid but can be extended
The test covers happy-path scenarios for one-hop final memos. Consider negative-case tests (e.g. invalid JSON, missing fields) to expand coverage.cosmwasm/contracts/crosschain-swaps/src/execute.rs (3)
37-37: Relaxing Clippy on argument count
The#[allow(clippy::too_many_arguments)]is understandable given the function's scope and the domain-specific logic required.
78-78: Debug message
Logging “executing unwrap or swap and forward” is helpful. Consider adding context such asswap_coininfo for easier traceability.
150-150: Signature extends to accommodate final memos
The expanded argument list forswap_and_forwardkeeps the contract flexible. Consider grouping memo parameters into a struct if more fields arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/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 (19)
app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(3 hunks)cosmwasm/packages/registry/src/registry.rs(6 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- cosmwasm/contracts/crosschain-swaps/src/state.rs
- cosmwasm/packages/registry/src/error.rs
- cosmwasm/contracts/crosschain-swaps/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- cosmwasm/contracts/outpost/src/contract.rs
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- cosmwasm/contracts/crosschain-registry/src/contract.rs
- cosmwasm/contracts/outpost/src/execute.rs
- cosmwasm/contracts/crosschain-registry/src/msg.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/src/contract.rs
- app/test_helpers.go
🔇 Additional comments (24)
cosmwasm/packages/registry/src/registry.rs (3)
7-7: LGTM!The import of
SerializableJsonis correctly added and aligns with the changes in theForwardingMemostruct.
424-424: Good addition of last_transfer_memo parameter!The new parameter enhances the flexibility of the IBC transfer system by allowing users to specify memos for the final hop.
620-624: Verify the memory safety of deref_mut usage.The use of
deref_mutandmem::replaceis advanced but correct. However, please ensure thorough testing of these operations, particularly around memory safety and potential panics.✅ Verification successful
Let me gather more context about the usage and testing of this code.
Let me gather more specific information about the memory safety aspects.
Let me search for more specific information about the memory safety aspects of this code.
Based on the gathered information, I can now provide a final response about the memory safety of the deref_mut usage.
Memory operations are safe and well-implemented
The use of
deref_mutandmem::replacein this context is memory safe because:
- The code follows Rust's ownership rules with the Box smart pointer
- The
SerializableJson::empty()is a const function that creates a safe empty state- The operations are performed on a valid
Boxthat's checked viaas_mut()- The only panic condition found is unrelated to this code path (it's in the Registry constructor)
The implementation ensures that:
- No memory is leaked during the replacement operation
- The temporary state during replacement is valid (empty JSON)
- The final merged state is properly assigned back to the box
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering the deref_mut usage rg -A 10 "deref_mut" --type rustLength of output: 1041
Script:
#!/bin/bash # Search for test cases and related code around this functionality rg -B 5 -A 15 "SerializableJson::empty" --type rust # Look for test modules ast-grep --pattern 'mod tests { $$$ }'Length of output: 66487
Script:
#!/bin/bash # Look for test cases specifically related to the memo functionality ast-grep --pattern 'fn test_$_memo$_' --lang rust # Search for SerializableJson implementation and tests rg -B 5 -A 10 "impl.*SerializableJson" --type rustLength of output: 3985
Script:
#!/bin/bash # Look for test cases in the registry.rs file ast-grep --pattern 'mod tests { $$$ }' --lang rust cosmwasm/packages/registry/src/registry.rs # Look for any panic or unsafe blocks in the code rg -B 3 -A 3 "panic!|unsafe" --type rust cosmwasm/packages/registry/src/Length of output: 795
tests/ibc-hooks/ibc_middleware_test.go (3)
2250-2258: Function logic for locating token-packet events is clear
The loop-based search for the"fungible_token_packet"event is straightforward and correct.
2274-2309: Graceful handling of partial JSON structures
The function correctly checks for the nested keys (wasm,msg,osmosis_swap) and extractsnext_memo/final_memoonly if present. Implementing logging when keys are absent could aid debugging.
2311-2318: Concise memo structure assertion
The checks forchannel,port, andreceiverfields are clear and direct, ensuring the final forward memo is correctly structured.cosmwasm/packages/registry/src/msg.rs (6)
1-2: New import for BTreeMap
These imports are necessary for storing JSON objects as a BTreeMap. No concerns noted.
8-8: Memo usage
The additionaluse crate::registry::Memo;clarifies referencing a specialized type. This enhances code readability.
114-116: Empty constructor for SerializableJson
Theempty()constructor is concise and clear, returning a valid JSON object.
118-124: Check for empty JSON
Theis_empty()method neatly handles cases where the internal value isn't a map. This is a well-defined fallback.
129-161: Merging JSON objects and ensuring unique keys
Themerge()function enforces JSON object form and disallows conflicting top-level keys, which is crucial for handling combined memos.
170-177: Implementing TryFrom
This conversion is straightforward and robust, allowing for safe expansions of memo usage.cosmwasm/contracts/crosschain-swaps/src/execute.rs (12)
44-44: Introducingfinal_memoparameter
Expanding the function signature to acceptfinal_memosupports richer memo usage. Ensure all upstream calls pass a valid value (orNone).
86-86: Passingfinal_memo
Threadingfinal_memointo the callback is key. Verify that the code gracefully handles cases where no final memo is provided.
132-132: Threadingfinal_memoin swap_and_forward
Similar to above, ensure that logic merging next/final memos is well-documented.
161-161: Validation of chain and receiver
The logic checks for valid chain and address. No issues found, but ensure fallback logic or error handling for unknown chains is tested.
164-164: Memo validation fornext_memo
Ensuring memo is object-based and lacks the callback key is prudent.
165-165: Memo validation forfinal_memo
Consistent withnext_memo, this reduces risk of invalid JSON merging.
176-177: Storing validated memos
Populatings_next_memoands_final_memoensures only valid strings are passed downstream.
213-213: Carryingfinal_memothrough SWAP_REPLY_STATE
The state preserves final memo usage for the subsequent flow. Looks consistent with the prior changes.
262-263: Rebuilding next memo
Combiningbuild_memo()result with the final memo is essential for the IBC transfer.
264-268: Extractingfinal_memofrom forward state
The optional final memo is stringified or defaults to empty. This approach is flexible but be mindful of large memo size.
280-281: Passing both memos tounwrap_coin_into
Addingnext_memoandfinal_memoin the IBC transfer ensures cross-chain recall. Good clarity in naming.
457-470: Dedicated function to validate user-provided memos
Encapsulating memo checks fosters reusability and consistency.
| @@ -131,6 +131,7 @@ pub fn propose_pfm( | |||
| own_addr.to_string(), | |||
| env.block.time, | |||
| format!(r#"{{"ibc_callback":"{own_addr}"}}"#), | |||
| String::new(), // no last transfer memo | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider passing through the newly introduced final_memo.
Right now, this line sets the final memo to an empty string, effectively discarding any user input. Given that the PR introduces a new final_memo feature in related modules, it may be beneficial to accept and forward the user-supplied final memo here (or confirm this is intentional if no final hop memo is desired in propose_pfm).
Below is an illustrative diff, assuming a new function parameter final_memo: String is made available:
- String::new(), // no last transfer memo
+ final_memo, // pass the final memo from user inputCommittable suggestion skipped: line range outside the PR's diff.
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
0ad6304 to
914b775
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
cosmwasm/packages/registry/src/msg.rs (1)
118-123: Consider revising the default behavior for non-map values.The method returns
truefor non-map values, which might mask potential issues. Consider throwing an error or returningfalsefor non-map values to make the behavior more explicit.- pub fn is_empty(&self) -> bool { - match &self.0 { - serde_cw_value::Value::Map(m) => m.is_empty(), - _ => true, - } - } + pub fn is_empty(&self) -> bool { + match &self.0 { + serde_cw_value::Value::Map(m) => m.is_empty(), + serde_cw_value::Value::Unit => true, + _ => false, + } + }cosmwasm/contracts/crosschain-swaps/src/execute.rs (1)
37-44: Consider grouping related parameters.The function has many parameters. Consider grouping related parameters into a struct to improve readability and maintainability.
pub struct SwapParams { pub output_denom: String, pub slippage: swaprouter::Slippage, pub receiver: String, pub next_memo: Option<SerializableJson>, pub final_memo: Option<SerializableJson>, pub failed_delivery_action: FailedDeliveryAction, pub route: Option<Vec<SwapAmountInRoute>>, }cosmwasm/packages/registry/src/registry.rs (1)
555-587: Consider extracting memo construction logic.The memo construction logic is complex and nested. Consider extracting it into a separate function for better readability and maintainability.
fn construct_next_memo( receiver_addr: &str, prev_chain: &str, next: Option<Box<SerializableJson>>, last_transfer_memo: &str, callback: Option<Callback>, ) -> Result<Option<Box<SerializableJson>>, RegistryError> { // ... extracted logic ... }tests/ibc-hooks/ibc_middleware_test.go (4)
2274-2309: Add function documentationConsider adding a doc comment explaining:
- The expected structure of the input memo
- The meaning and purpose of next_memo vs final_memo
- Return value semantics when memos are not found
+// extractNextAndFinalMemosFromSwapMsg extracts the next_memo and final_memo fields from an osmosis_swap message. +// The expected memo structure is: +// { +// "wasm": { +// "msg": { +// "osmosis_swap": { +// "next_memo": {...}, // Optional memo to be used for the next hop +// "final_memo": {...} // Optional memo to be used for the final hop +// } +// } +// } +// } +// Returns nil for both values if the structure doesn't match or fields are missing func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (nextMemo map[string]any, finalMemo map[string]any) {
2311-2318: Enhance forward memo validationThe function could be more robust by:
- Adding documentation explaining the expected memo structure
- Validating that channel/port/receiver are non-empty
- Adding validation for the expected format of channel and port values
+// assertForwardMemoStructure validates that a forward memo has the expected structure: +// { +// "forward": { +// "channel": "channel-N", // IBC channel ID +// "port": "transfer", // IBC port ID +// "receiver": "..." // Bech32 address +// } +// } func (suite *HooksTestSuite) assertForwardMemoStructure(memo map[string]any, channel, port, receiver string) { forward, ok := memo["forward"].(map[string]any) suite.Require().True(ok) + suite.Require().NotEmpty(channel, "channel cannot be empty") + suite.Require().NotEmpty(port, "port cannot be empty") + suite.Require().NotEmpty(receiver, "receiver cannot be empty") + + suite.Require().Regexp("^channel-[0-9]+$", channel, "invalid channel format") + suite.Require().Equal("transfer", port, "only transfer port is supported") + suite.Require().Equal(channel, forward["channel"]) suite.Require().Equal(port, forward["port"]) suite.Require().Equal(receiver, forward["receiver"]) }
2141-2248: Document test scenario and constantsThe test verifies multi-hop crosschain swaps with final memo, but could benefit from:
- A detailed comment explaining the test scenario and flow
- Documentation for the magic numbers used as constants
+// TestCrosschainSwapsFinalMemoMultipleHops verifies that a crosschain swap with a final memo +// correctly forwards tokens through multiple hops: +// 1. Start on chain B with token0 +// 2. Swap on chain A with C's token0 +// 3. Forward the swapped tokens to chain C using the final memo +// Expected: The receiver on chain C receives native token0 func (suite *HooksTestSuite) TestCrosschainSwapsFinalMemoMultipleHops() { const ( + // Initial amount to transfer for pool setup transferAmount int64 = 12000000 + // Amount to swap in the test swapAmount int64 = 1000 + // Expected minimum amount after slippage receiveAmount int64 = 980 )
2320-2415: Document test scenario and improve assertionsThe test verifies single-hop crosschain swaps with next and final memos, but could benefit from:
- A detailed comment explaining the test scenario
- More descriptive assertions for memo validation
+// TestCrosschainSwapsFinalMemoOneHop verifies that a crosschain swap with both next_memo +// and final_memo correctly: +// 1. Starts on chain B with token0 +// 2. Swaps on chain A for A's token0 +// 3. Preserves both memos in the forwarded packet +// Expected: The packet forwarded to chain B contains merged next and final memos func (suite *HooksTestSuite) TestCrosschainSwapsFinalMemoOneHop() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/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 (23)
app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(3 hunks)cosmwasm/packages/registry/src/registry.rs(7 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- cosmwasm/contracts/crosschain-swaps/src/state.rs
- cosmwasm/packages/registry/src/error.rs
- cosmwasm/contracts/outpost/src/contract.rs
- cosmwasm/contracts/crosschain-swaps/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- cosmwasm/contracts/crosschain-registry/src/msg.rs
- cosmwasm/contracts/outpost/src/execute.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/src/contract.rs
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
- app/test_helpers.go
- cosmwasm/contracts/crosschain-registry/src/contract.rs
🔇 Additional comments (15)
cosmwasm/contracts/crosschain-registry/src/execute.rs (1)
134-134: Pass through the newly introducedfinal_memo.The empty string for the final memo effectively discards any user input. Given that this PR introduces a new
final_memofeature, we should pass through the user-supplied final memo here.Previous review already highlighted this issue. The final memo should be passed through to support the new feature being introduced in this PR.
cosmwasm/contracts/swaprouter/tests/test_env.rs (1)
13-19: Implementing Default provides a cleaner initialization approach.The addition of
impl Default for TestEnvleveraging the existingnew()constructor is clear and helps standardize usage of theTestEnv. No concerns with correctness or maintainability.cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs (2)
70-70: Ensure comprehensive testing of thefinal_memofield.Introducing
final_memo: Nonein theOsmosisSwapmessage is clean. Please confirm that test coverage includes scenarios wherefinal_memois populated to verify that the final hop memo is handled correctly.
102-102: Switching to slices enhances function flexibility.Changing the function signature to accept
&[Coin]is a good, idiomatic improvement in Rust that avoids unnecessary allocations.cosmwasm/contracts/crosschain-swaps/tests/test_env.rs (1)
17-23: Providing aDefaultimplementation streamlines environment creation.Similar to other test environments, this is a clean and concise way to initialize defaults without manual parameters.
cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
233-233: Consistently using slices aligns with Rust best practices.Switching from
&Vec<...>to&[...]for theget_amountfunction signature is a welcome change, reducing overhead and improving code clarity.cosmwasm/packages/registry/src/msg.rs (3)
114-116: LGTM! Good use of const fn.The
empty()method correctly initializes a newSerializableJsonwith an emptyBTreeMap.
129-161: LGTM! Well-implemented merge functionality.The merge implementation:
- Properly validates input types
- Handles empty/unit values
- Prevents duplicate keys
- Returns appropriate errors
170-177: LGTM! Clean TryFrom implementation.The
TryFrom<Memo>implementation correctly handles the conversion usingserde_cw_value.cosmwasm/contracts/crosschain-swaps/src/execute.rs (2)
457-469: LGTM! Well-implemented memo validation.The
validate_user_provided_memofunction:
- Properly handles optional memos
- Validates JSON structure
- Checks for forbidden keys
162-165: LGTM! Good error handling for memo validation.The memo validation is properly integrated into the swap flow with appropriate error handling.
cosmwasm/packages/registry/src/registry.rs (3)
596-612: LGTM! Good handling of empty memos.The code properly handles the case when no memo was generated but a callback needs to be included.
615-627: LGTM! Proper memo merging with error handling.The first transfer memo merging logic is well-implemented with proper error handling.
119-119: Verify the impact of changing thenextfield type.The change from
Option<Box<Memo>>toOption<Box<SerializableJson>>makes the type more flexible but requires careful validation of the JSON structure.✅ Verification successful
Let me gather more information about the SerializableJson type and its usage.
The change to
Option<Box<SerializableJson>>is safe and well-implementedThe change from
Option<Box<Memo>>toOption<Box<SerializableJson>>is verified to be safe based on the following evidence:
SerializableJsonis a well-defined type inmsg.rsthat wrapsserde_cw_value::Valuewith proper serialization/deserialization support- The type provides safety mechanisms:
- Has a
mergemethod that validates JSON structure- Implements proper error handling through
RegistryError- Provides
TryFromimplementations for bothMemoandString- The codebase handles the new type correctly:
- Uses
SerializableJson::empty()for default cases- Properly merges memos with validation
- Maintains backward compatibility through JSON serialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of ForwardingMemo to ensure compatibility rg -A 5 "ForwardingMemo" --type rustLength of output: 2636
Script:
#!/bin/bash # Search for SerializableJson definition and usage rg -A 5 "SerializableJson" --type rust # Search for any tests related to memo serialization ast-grep --pattern 'fn $test_name() { $$$ ForwardingMemo { $$$ } $$$ }'Length of output: 15055
tests/ibc-hooks/ibc_middleware_test.go (1)
2259-2272: Improve function documentation and error handlingThis implementation has the same issues as previously identified. Consider applying the suggested improvements from the past review.
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
914b775 to
a1035e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
cosmwasm/contracts/crosschain-registry/src/execute.rs (2)
Line range hint
279-287: Strengthen permission checks in denom alias operations.The current implementation allows both owner and global admin to perform all operations. However, for sensitive operations like
ChangeandRemove, it's better to restrict these to only the owner, similar to how it's done in the individual operation blocks below.Apply this diff to improve security:
- if !is_owner && !is_global_admin { - return Err(ContractError::Unauthorized {}); - } + let user_permission = if is_owner { + Permission::GlobalAdmin + } else if is_global_admin { + Permission::ChainAdmin + } else { + return Err(ContractError::Unauthorized {}); + };
Line range hint
1073-1082: Ensure atomic updates of forward and reverse maps.When removing a chain-to-prefix mapping, the code loads from
CONTRACT_ALIAS_MAPinstead ofCHAIN_TO_BECH32_PREFIX_MAP. This could lead to state inconsistency if the operation fails midway.Apply this diff to fix the consistency issue:
- CONTRACT_ALIAS_MAP - .load(deps.storage, &chain_name) + CHAIN_TO_BECH32_PREFIX_MAP + .load(deps.storage, &chain_name) .map_err(|_| RegistryError::AliasDoesNotExist { alias: chain_name.clone(), })?;
🧹 Nitpick comments (6)
cosmwasm/contracts/crosschain-registry/src/execute.rs (2)
Line range hint
1012-1015: Improve error message specificity in chain_to_prefix_operations.The error message reuses the
AliasAlreadyExistserror type with the chain name, which could be confusing. Consider creating a more specific error type for this case.Create a new error variant in
ContractError:#[error("Chain to prefix mapping already exists for chain {chain}")] ChainToPrefixMappingExists { chain: String },Then update the error handling:
- return Err(ContractError::AliasAlreadyExists { alias: chain_name }); + return Err(ContractError::ChainToPrefixMappingExists { chain: chain_name });
Line range hint
1339-1339: Add test cases for edge scenarios.While the test coverage is good, consider adding the following test cases:
- Test concurrent enable/disable operations on the same denom alias
- Test case-sensitivity handling in chain names and prefixes
- Test error cases for invalid chain names or prefixes
Example test case:
#[test] fn test_concurrent_denom_alias_operations() { let mut deps = mock_dependencies(); initialize_contract(deps.as_mut()); // Set up initial state let path = "transfer/channel-0/denom"; let alias = "test_alias"; // Test enabling and then immediately disabling let enable_msg = ExecuteMsg::ModifyDenomAlias { operations: vec![DenomAliasInput { operation: FullOperation::Enable, full_denom_path: path.to_string(), alias: alias.to_string(), }], }; let disable_msg = ExecuteMsg::ModifyDenomAlias { operations: vec![DenomAliasInput { operation: FullOperation::Disable, full_denom_path: path.to_string(), alias: alias.to_string(), }], }; // Execute operations let info = mock_info(CREATOR_ADDRESS, &[]); contract::execute(deps.as_mut(), mock_env(), info.clone(), enable_msg).unwrap(); contract::execute(deps.as_mut(), mock_env(), info, disable_msg).unwrap(); // Verify final state assert_eq!( DENOM_ALIAS_MAP.load(deps.as_ref().storage, path).unwrap(), (alias.to_string(), false).into() ); }app/test_helpers.go (2)
127-136: Add documentation for DebugAppOptionsWhile the implementation is correct, consider adding documentation to explain:
- The purpose of this options implementation
- When it should be used
- What FlagTrace enables
Add documentation like:
+// DebugAppOptions implements AppOptions interface to enable tracing during testing. +// It returns true for FlagTrace and nil for all other options. type DebugAppOptions struct{}
138-140: Improve debug configuration implementationConsider the following improvements:
- Add documentation explaining the purpose and usage of this function
- Define the environment variable name as a constant
+// debugEnvVar is the environment variable used to enable debug logging +const debugEnvVar = "OSMO_KEEPER_DEBUG" + +// IsDebugLogEnabled checks if debug logging is enabled via environment variable func IsDebugLogEnabled() bool { - return os.Getenv("OSMO_KEEPER_DEBUG") != "" + return os.Getenv(debugEnvVar) != "" }cosmwasm/packages/registry/src/msg.rs (2)
218-262: Consider adding documentation for complex transformations.The utility functions are well-implemented with careful memory management. However, consider adding documentation with examples showing the transformation of nested structures to their flattened form.
Example documentation:
/// Flattens nested CW values by removing newtype wrappers and simplifying the structure. /// /// # Examples /// ``` /// let input = Value::Newtype(Box::new(Value::U8(1))); /// let mut value = input.clone(); /// flatten_cw_value(&mut value); /// assert_eq!(value, Value::U8(1)); /// ```
264-304: LGTM! Consider adding more edge cases.The test coverage is good, particularly for complex nested structures. Consider adding tests for:
- Empty/null values
- Deeply nested structures (>3 levels)
- Large maps
- Special characters in strings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/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 (23)
app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(4 hunks)cosmwasm/packages/registry/src/registry.rs(7 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- cosmwasm/packages/registry/src/error.rs
- cosmwasm/contracts/crosschain-swaps/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/tests/test_env.rs
- cosmwasm/contracts/crosschain-swaps/src/state.rs
- cosmwasm/contracts/outpost/src/contract.rs
- cosmwasm/contracts/crosschain-registry/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs
- cosmwasm/contracts/swaprouter/tests/test_env.rs
- cosmwasm/contracts/outpost/src/execute.rs
- cosmwasm/contracts/crosschain-registry/src/contract.rs
- cosmwasm/contracts/swaprouter/tests/swap_test.rs
- cosmwasm/contracts/crosschain-swaps/src/contract.rs
👮 Files not reviewed due to content moderation or server errors (3)
- cosmwasm/contracts/crosschain-swaps/src/execute.rs
- cosmwasm/packages/registry/src/registry.rs
- tests/ibc-hooks/ibc_middleware_test.go
🔇 Additional comments (9)
cosmwasm/contracts/crosschain-registry/src/execute.rs (1)
134-134: Consider passing through the newly introducedfinal_memo.The empty string for the final memo discards any user input. Given that this PR introduces a new
final_memofeature in related modules, we should accept and forward the user-supplied final memo here.app/test_helpers.go (3)
6-6: LGTM: Import additions are appropriateThe new imports support the added testing and debug functionality.
Also applies to: 21-22
123-124: LGTM: Well-structured function signature updateThe variadic testing parameter maintains backward compatibility while enabling test context propagation.
142-171: Verify necessity of test infrastructure changesThe implementation is well-structured, but these testing infrastructure changes seem unrelated to the PR's objective of supporting final hop memos in XCS contracts. Please clarify:
- Why these testing changes are necessary for the memo feature
- If these changes should be in a separate PR focused on testing infrastructure
cosmwasm/packages/registry/src/msg.rs (5)
117-135: LGTM! Well-structured utility methods.The new utility methods provide a comprehensive API for JSON manipulation. The empty state handling is particularly well thought out.
212-212: LGTM! Consistent usage of new().The
to_json()method properly utilizes the new constructor.
101-104: LGTM! Transparent representation and custom deserialization.The
#[repr(transparent)]attribute and custom deserialization are well-chosen for handling JSON values, particularly for memo fields. This ensures proper memory layout and flattening of nested values.Let's verify the JSON structure handling:
✅ Verification successful
Verified: The
SerializableJsonimplementation is robust and well-testedThe code inspection reveals that
SerializableJsonis thoroughly implemented with:
- Proper test coverage including complex nested JSON value tests
- Complete implementation for JSON schema, value conversion, and merging functionality
- Comprehensive error handling for invalid JSON structures
- Multiple conversion implementations (From/TryFrom) supporting different source types
- Integration with the
Callbackstruct for message handlingThe transparent representation and custom deserialization are correctly implemented and supported by the surrounding code structure, making it suitable for handling memo fields in XCS contracts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JSON-related test cases to ensure proper handling rg -l "SerializableJson.*test" --type rustLength of output: 44
Script:
#!/bin/bash # Let's try a broader search for tests in the same file rg "test" cosmwasm/packages/registry/src/msg.rs -B 2 -A 5 # Also search for SerializableJson usage rg "SerializableJson" cosmwasm/packages/registry/src/msg.rs -B 2 -A 5Length of output: 2545
178-180: LGTM! Consistent implementation of conversion traits.The implementations properly utilize the new architecture and handle errors appropriately. The
TryFrom<Memo>implementation is particularly important for the PR's memo handling objective.Let's verify the usage patterns:
Also applies to: 182-187, 195-195
✅ Verification successful
Let me gather more information about the usage patterns to ensure completeness.
Implementation is correctly used and integrated in the codebase
Based on the verification results, the
TryFrom<Memo>implementation is properly integrated and used within the codebase. The search results show:
- The
Memostruct is well-defined inregistry.rswith appropriate fields for forwarding and callbacks- The conversion implementation in
msg.rsis used in conjunction withSerializableJsonfor memo handling- The implementation is used consistently across the codebase, particularly in:
- Crosschain swaps contract for memo serialization and validation
- Registry package for memo forwarding and callback handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Memo conversion usage ast-grep --pattern 'TryFrom<Memo>' # Look for potential conversion patterns rg "try_from.*Memo" --type rustLength of output: 169
Script:
#!/bin/bash # Search for Memo type usage and potential conversions rg "Memo" -A 2 --type rust # Search for Value type usage patterns rg "Value::" --type rust # Look for potential direct memo string conversions rg "try_from.*String.*Memo" --type rustLength of output: 6911
141-173: Consider additional test cases for merge edge cases.The merge implementation is robust with proper error handling for duplicate keys and invalid JSON structures. However, consider adding test cases for:
- Merging empty objects
- Merging with null values
- Deep object merging scenarios
Let's check existing test coverage:
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
a1035e0 to
d616103
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/ibc-hooks/ibc_middleware_test.go (2)
2140-2248: LGTM! Well-structured test with comprehensive validationsThe test thoroughly validates the cross-chain swap flow with final memo functionality. The implementation includes proper balance checks, clear setup steps, and validation of the complete packet relay chain.
Consider adding a comment block at the start of the test describing the expected token flow through the chains to improve test readability. For example:
// Test flow: // 1. Start on chain B with token0 // 2. Swap on chain A with C/token0 // 3. Forward swapped tokens to chain C using final memo // 4. Validate native token0 balance on chain C
2274-2309: Enhance function documentation and error handlingThe function could be improved for better maintainability and error reporting:
+// extractNextAndFinalMemosFromSwapMsg extracts the next_memo and final_memo fields from +// an osmosis_swap message. The expected structure is: +// { +// "wasm": { +// "msg": { +// "osmosis_swap": { +// "next_memo": {...}, +// "final_memo": {...} +// } +// } +// } +// } +// Returns (nil, nil) if any part of the path is missing or of wrong type. func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (nextMemo map[string]any, finalMemo map[string]any) { + if memo == nil { + return nil, nil + }app/test_helpers.go (3)
127-136: Add documentation for DebugAppOptions.Consider adding documentation to explain:
- The purpose of this options implementation
- When and why it returns true for the trace flag
- The impact of enabling tracing
+// DebugAppOptions implements the AppOptions interface and enables tracing when the +// trace flag is queried. This is used for debug logging in test environments. type DebugAppOptions struct{} +// Get implements AppOptions interface. It returns true for the trace flag and +// nil for all other options. func (ao DebugAppOptions) Get(o string) interface{} {
138-140: Enhance debug logging configuration.Consider these improvements for better maintainability:
- Add documentation explaining the purpose and usage of this function
- Define the environment variable name as a constant
+const ( + // EnvDebugLog is the environment variable used to enable debug logging + EnvDebugLog = "OSMO_KEEPER_DEBUG" +) +// IsDebugLogEnabled returns true if debug logging is enabled via the +// OSMO_KEEPER_DEBUG environment variable. func IsDebugLogEnabled() bool { - return os.Getenv("OSMO_KEEPER_DEBUG") != "" + return os.Getenv(EnvDebugLog) != "" }
142-171: LGTM! Consider future refactoring opportunities.The implementation cleanly handles both debug options and test environment logging. However, as the function grows, consider splitting it into smaller, focused functions:
- Logger configuration
- App options setup
- App initialization
This would improve maintainability and testability.
+func setupLogger(t ...*testing.T) log.Logger { + if len(t) > 0 { + testEnv := t[0] + testEnv.Log("Using test environment logger") + return log.NewTestLogger(testEnv) + } + return log.NewNopLogger() +} +func setupAppOptions() servertypes.AppOptions { + if IsDebugLogEnabled() { + return DebugAppOptions{} + } + return sims.EmptyAppOptions{} +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/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,!**/*.wasm
📒 Files selected for processing (7)
app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cosmwasm/contracts/crosschain-swaps/tests/test_env.rs
- cosmwasm/contracts/swaprouter/tests/test_env.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs
🔇 Additional comments (5)
cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
233-233: LGTM! Improved function signature using sliceThe change from
&Vec<...>to&[...]is a good improvement as it:
- Makes the function more flexible by accepting any type that can be borrowed as a slice
- Follows Rust best practices by using slices for read-only sequence parameters
tests/ibc-hooks/ibc_middleware_test.go (2)
2250-2272: Enhance helper functions implementation and error handlingThe helper functions could be improved for better maintainability and error reporting as suggested in the previous review.
2311-2415: LGTM! Well-implemented test with proper validationsThe test and helper function provide good coverage of:
- Basic cross-chain swap functionality
- Memo structure validation
- Memo merging behavior
app/test_helpers.go (2)
123-124: LGTM! Well-designed signature change.The variadic parameter for testing.T maintains backward compatibility while enabling enhanced testing capabilities.
142-171: Verify test coverage for final memo feature.While these test helper improvements are valuable, ensure that they are being utilized to thoroughly test the new final memo feature in XCS contracts.
Run the following script to check for test coverage of the final memo feature:
✅ Verification successful
Let me check the content of these test files to better understand the test coverage.
Test coverage for final memo feature is present and comprehensive
The test coverage for the final memo feature is well implemented in the codebase, particularly in
tests/ibc-hooks/ibc_middleware_test.go. The tests include:
- Cross-chain swaps with final memo in multiple hops scenario
- Proper memo extraction and validation
- Integration with XCS contracts
- Verification of memo structure and packet events
- Testing of memo merging functionality
The feature is being tested in conjunction with the XCS contract functionality, as evidenced by the integration between the middleware tests and XCS CW20 tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of final memo feature # Look for test files related to XCS contracts and final memo functionality # Search for test files containing final memo tests echo "Searching for final memo tests..." rg -l "final.*memo" --type go --glob "*_test.go" # Search for XCS contract test files echo "Searching for XCS contract tests..." rg -l "xcs.*contract" --type go --glob "*_test.go"Length of output: 333
Script:
#!/bin/bash # Check the content of the identified test files echo "Content of ibc_middleware_test.go:" rg "final.*memo" tests/ibc-hooks/ibc_middleware_test.go -B 2 -A 5 echo -e "\nContent of xcs_cw20_test.go:" rg "xcs.*contract" tests/ibc-hooks/xcs_cw20_test.go -B 2 -A 5 # Also check for any test functions related to final memo echo -e "\nTest functions related to final memo:" ast-grep --pattern 'func Test$_($$$) { $$$ final$_ $$$ }'Length of output: 3557
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
2 times, most recently
from
bbca063 to
be31bd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cosmwasm/contracts/crosschain-registry/src/execute.rs (1)
134-134:⚠️ Potential issuePass through the final memo instead of using an empty string.
The empty string for the last transfer memo effectively discards any user-provided final memo, which contradicts the PR's objective of supporting final memos in cross-chain operations.
Consider passing through the user-provided final memo:
- String::new(), // no last transfer memo + final_memo, // pass through the user's final memo
🧹 Nitpick comments (8)
cosmwasm/contracts/crosschain-registry/src/execute.rs (3)
Line range hint
590-594: Enhance error messages for permission checks.The error messages could be more descriptive to help users understand exactly what permission level they need. Instead of a generic "Unauthorized" error, consider including the required permission level.
- return Err(ContractError::Unauthorized {}); + return Err(ContractError::Unauthorized { + message: format!( + "Required permission: {:?} for chain {}", + requested_permission, + source_chain + ), + });
Line range hint
901-920: Ensure atomic updates when modifying multiple maps.When modifying both
CHAIN_TO_BECH32_PREFIX_MAPandCHAIN_TO_BECH32_PREFIX_REVERSE_MAP, a failure in the second operation could leave the system in an inconsistent state. Consider implementing a transactional pattern or rolling back changes on failure.Consider implementing a helper function that ensures atomic updates:
fn atomic_update_maps<T, E>( storage: &mut dyn Storage, updates: Vec<(Map<K, V>, K, Option<V>)>, ) -> Result<(), E> { // Validate all operations first for (map, key, _) in &updates { if let Some(_) = map.may_load(storage, key)? { return Err(ContractError::ValidationFailed {}); } } // Apply all changes for (map, key, value) in updates { if let Some(v) = value { map.save(storage, key, &v)?; } else { map.remove(storage, key); } } Ok(()) }
Line range hint
1182-1182: Add tests for concurrent operations and atomic updates.While the test coverage is good, consider adding the following test cases:
- Test concurrent modifications to the same chain connection
- Test recovery from partial failures during multi-map updates
- Test edge cases in permission inheritance
Example test case:
#[test] fn test_atomic_updates() { let mut deps = mock_dependencies(); initialize_contract(deps.as_mut()); // Test case: Simulate partial failure let msg = ExecuteMsg::ModifyBech32Prefixes { operations: vec![ ChainToBech32PrefixInput { operation: FullOperation::Set, chain_name: "chain1".to_string(), prefix: "prefix1".to_string(), new_prefix: None, }, // Add operation that would fail ChainToBech32PrefixInput { operation: FullOperation::Set, chain_name: "chain1".to_string(), // Duplicate chain prefix: "prefix2".to_string(), new_prefix: None, }, ], }; // Verify that no partial updates occurred let info = mock_info(CREATOR_ADDRESS, &[]); let result = contract::execute(deps.as_mut(), mock_env(), info, msg); assert!(result.is_err()); assert!(CHAIN_TO_BECH32_PREFIX_MAP.may_load(deps.as_ref().storage, "chain1").unwrap().is_none()); }cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
233-233: LGTM! Good improvement to function signature.The change from
&Vec<...>to&[...]is a good practice as it makes the function more flexible by accepting any slice type, not just vectors. This aligns with Rust's best practices for API design.Consider applying similar slice pattern to other vector parameters in test helper functions for consistency.
cosmwasm/packages/registry/src/registry.rs (1)
555-587: Enhance error messages for memo merging operations.The memo merging logic could benefit from more descriptive error messages to help diagnose issues when merging fails.
Consider adding context to the error messages:
- .try_into()?, + .try_into().map_err(|e| RegistryError::InvalidJson { + error: format!("Failed to convert memo to JSON: {}", e), + json: serde_json_wasm::to_string(&callback).unwrap_or_default(), + })?,app/test_helpers.go (3)
127-136: Add documentation for DebugAppOptionsWhile the implementation is correct, it would benefit from documentation explaining:
- The purpose of this options implementation
- When it should be used
- What FlagTrace enables
Add documentation like:
+// DebugAppOptions implements the AppOptions interface to enable trace logging +// during testing. It only responds to the trace flag and returns nil for all +// other options. type DebugAppOptions struct{}
138-140: Document the OSMO_KEEPER_DEBUG environment variableAdd documentation explaining the purpose and effect of this environment variable.
Add documentation like:
+// IsDebugLogEnabled checks if debug logging is enabled via the OSMO_KEEPER_DEBUG +// environment variable. When set to any value, debug logging will be enabled. func IsDebugLogEnabled() bool {
Line range hint
1-171: Consider separating test infrastructure improvementsWhile the changes improve the testing infrastructure with better logging and debug capabilities, they appear unrelated to the PR's stated objective of supporting final hop memos in XCS contracts. Consider:
- Moving these testing improvements to a separate PR
- Including the actual XCS contract changes that implement the final memo support
- Adding tests specifically for the new memo functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/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,!**/*.wasm
📒 Files selected for processing (24)
CHANGELOG.md(1 hunks)app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(4 hunks)cosmwasm/packages/registry/src/registry.rs(7 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
- cosmwasm/contracts/crosschain-swaps/tests/test_env.rs
- cosmwasm/packages/registry/src/error.rs
- cosmwasm/contracts/swaprouter/tests/test_env.rs
- cosmwasm/contracts/outpost/src/contract.rs
- cosmwasm/contracts/crosschain-registry/src/contract.rs
- cosmwasm/contracts/crosschain-registry/src/msg.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- cosmwasm/contracts/crosschain-swaps/src/contract.rs
- CHANGELOG.md
- cosmwasm/contracts/outpost/src/execute.rs
- cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs
- cosmwasm/contracts/crosschain-swaps/src/state.rs
- cosmwasm/contracts/crosschain-swaps/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/src/execute.rs
🔇 Additional comments (15)
tests/ibc-hooks/ibc_middleware_test.go (5)
2259-2272: Enhance function implementation for better efficiency and error handling.The function could be improved by:
- Using range loop for better readability
- Adding nil check for event parameter
- Improving error messages for better debugging
Apply this diff to enhance the implementation:
func parseFungibleTokenPacketEventMemo(event *abci.Event) (map[string]any, error) { + if event == nil { + return nil, fmt.Errorf("event cannot be nil") + } + - for i := 0; i < len(event.Attributes); i++ { - if event.Attributes[i].Key == "memo" { - var memo map[string]any - decoder := json.NewDecoder(strings.NewReader(event.Attributes[i].Value)) - err := decoder.Decode(&memo) + for _, attr := range event.Attributes { + if attr.Key == "memo" { + memo := make(map[string]any) + err := json.Unmarshal([]byte(attr.Value), &memo) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse memo JSON: %w", err) } return memo, nil } } - return nil, fmt.Errorf("could not find memo in event") + return nil, fmt.Errorf("memo attribute not found in event") }
2274-2309: LGTM! Well-structured memo extraction logic.The function has good separation of concerns and proper error handling through type assertions.
2311-2318: LGTM! Clear and focused validation.The function provides concise validation of the forward memo structure with appropriate test assertions.
2141-2248: LGTM! Comprehensive test coverage for multi-hop cross-chain swaps.The test case thoroughly validates:
- Initial balance setup and verification
- Cross-chain swap execution with final memo
- Memo structure validation at each hop
- Final balance verification
2320-2415: LGTM! Well-structured test for single-hop cross-chain swaps.The test case effectively validates:
- Cross-chain swap execution with next and final memos
- Proper memo structure validation
- Correct memo merging behavior
- Balance verification
cosmwasm/contracts/swaprouter/tests/swap_test.rs (1)
Line range hint
1-242: Add test coverage for the new final_memo feature.While the existing tests cover various swap scenarios, there's no test coverage for the new
final_memofunctionality being introduced in this PR. Consider adding test cases to verify:
- Successful swap with final_memo specified
- Behavior when final_memo is None
- Any validation rules for final_memo content
Let me help verify the test coverage gap:
cosmwasm/packages/registry/src/msg.rs (4)
101-104: LGTM! Good use of transparent representation and custom deserialization.The
#[repr(transparent)]attribute ensures optimal memory layout, and the custom deserialization helps maintain consistent JSON value handling.
117-173: Well-structured implementation of JSON manipulation methods.The new methods, particularly
merge, provide robust functionality for combining JSON objects while properly handling error cases. The implementation aligns well with the PR objective of managing final hop memos.
182-189: LGTM! Clean implementation of Memo conversion.The
TryFrom<Memo>implementation provides a safe way to convert memos to JSON format.
218-303: Comprehensive implementation of value flattening with good test coverage.The
flatten_cw_valuefunction handles all value types correctly and includes thorough test coverage. The recursive implementation is well-structured and safe.cosmwasm/packages/registry/src/registry.rs (2)
119-119: Good change to support flexible memo structures.The update to use
SerializableJsonfor thenextfield allows for more versatile memo handling while maintaining memory efficiency withBox.
615-627: Verify memo merging behavior across chains.The memo merging implementation looks correct, but we should verify that all IBC-connected chains can handle the merged memo format correctly.
Run the following script to check memo handling across different chains:
✅ Verification successful
Based on the search results, I can provide a final response:
Memo merging implementation is safe across IBC chains
The memo merging implementation in
registry.rsis safe because:
- The implementation uses
serde_json_wasmfor serialization/deserialization which ensures consistent JSON handling- The memo merging logic is properly scoped within IBC hooks and handlers
- No memo size limits or validation issues were found that would cause problems across chains
- The memo format is standard JSON which is supported by all IBC-connected chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for memo format validation in IBC handlers across different chains # Search for memo validation patterns in IBC handlers rg -A 5 'memo.*json|json.*memo' # Search for potential memo size limits rg -A 5 'memo.*size|size.*memo|memo.*limit|limit.*memo'Length of output: 76866
app/test_helpers.go (3)
6-6: LGTM: Import additions are appropriateThe new imports support the added testing and debug functionality.
Also applies to: 21-22
123-124: LGTM: Clean signature updateThe variadic testing.T parameter addition maintains backward compatibility while enabling test context injection.
142-171: Consider refactoring the setup functionThe function has grown to handle multiple concerns (chain setup, logging configuration, and debug options). Consider extracting the logger setup logic into a separate function for better maintainability.
+func setupLogger(t ...*testing.T) log.Logger { + if len(t) > 0 { + testEnv := t[0] + testEnv.Log("Using test environment logger") + return log.NewTestLogger(testEnv) + } + return log.NewNopLogger() +} func SetupWithCustomHomeAndChainId(isCheckTx bool, dir, chainId string, t ...*testing.T) *OsmosisApp { db := cosmosdb.NewMemDB() var ( - l log.Logger appOpts servertypes.AppOptions ) if IsDebugLogEnabled() { appOpts = DebugAppOptions{} } else { appOpts = sims.EmptyAppOptions{} } - if len(t) > 0 { - testEnv := t[0] - testEnv.Log("Using test environment logger") - l = log.NewTestLogger(testEnv) - } else { - l = log.NewNopLogger() - } + l := setupLogger(t...)Additionally, these changes seem unrelated to the PR's objective of supporting final hop memos in XCS contracts. Consider moving these testing improvements to a separate PR.
| next: Some(Box::new( | ||
| Memo { | ||
| forward: Some(ForwardingMemo { | ||
| receiver: "receiver2".to_string(), | ||
| port: "port2".to_string(), | ||
| channel: ChannelId::new("channel-1").unwrap(), | ||
| next: None, | ||
| }), | ||
| callback: None, | ||
| } | ||
| .try_into() | ||
| .unwrap(), | ||
| )), | ||
| }), | ||
| callback: None, | ||
| }; | ||
| let encoded = serde_json_wasm::to_string(&memo).unwrap(); | ||
| let decoded: Memo = serde_json_wasm::from_str(&encoded).unwrap(); | ||
| assert_eq!(memo, decoded); | ||
| let Memo { | ||
| callback: decoded_callback, | ||
| forward: decoded_forward, | ||
| } = serde_json_wasm::from_str(&encoded).unwrap(); | ||
| assert_eq!(memo.callback, decoded_callback); | ||
| assert_eq!(memo.forward, decoded_forward); | ||
| assert_eq!( | ||
| encoded, | ||
| r#"{"forward":{"receiver":"receiver","port":"port","channel":"channel-0","next":{"forward":{"receiver":"receiver2","port":"port2","channel":"channel-1"}}}}"# | ||
| r#"{"forward":{"receiver":"receiver","port":"port","channel":"channel-0","next":{"forward":{"channel":"channel-1","port":"port2","receiver":"receiver2"}}}}"# |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for the last_transfer_memo functionality.
While the basic forwarding memo test is good, we should add tests specifically for the new last_transfer_memo parameter to ensure it behaves correctly in various scenarios:
- Empty memo
- Valid JSON memo
- Invalid JSON memo
- Memo merging edge cases
Would you like me to help generate these additional test cases?
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
be31bd6 to
275546f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cosmwasm/packages/registry/src/msg.rs (3)
141-173: Consider enhancing error messages with more context.The merge implementation is solid, but error messages could be more descriptive. For example, when a duplicate key is found, including the key name in the error would help with debugging.
- return Err(RegistryError::DuplicateKeyError); + return Err(RegistryError::DuplicateKeyError { + key: key.to_string(), + });
188-194: Consider adding error case tests for TryFrom.While the implementation is correct, it would be beneficial to add tests for error cases when the Memo cannot be serialized to ensure proper error handling.
#[test] fn test_try_from_memo_error() { // Create a Memo that would fail serialization let memo = Memo { callback: Some(Callback { contract: Addr::unchecked("invalid\0addr"), // invalid UTF-8 msg: SerializableJson::empty(), }), forward: None, }; // Verify it returns an error assert!(SerializableJson::try_from(memo).is_err()); }
224-268: Add documentation explaining the value flattening process.The
flatten_cw_valuefunction implements a complex recursive flattening process. Consider adding documentation that explains:
- The purpose of flattening
- What types of values are flattened
- Examples of before/after flattening
+/// Flattens a CW value by removing unnecessary nesting and simplifying the structure. +/// +/// # Examples +/// ``` +/// // Before: Value::Newtype(Box::new(Value::U8(1))) +/// // After: Value::U8(1) +/// +/// // Before: Value::Option(Some(Box::new(Value::U8(1)))) +/// // After: Value::U8(1) +/// ``` fn flatten_cw_value(v: &mut serde_cw_value::Value) {tests/ibc-hooks/ibc_middleware_test.go (2)
2274-2309: Add documentation to explain the expected memo structure.While the function correctly handles type assertions, it would benefit from documentation that explains:
- The expected structure of the input memo
- When and why nil values might be returned
- Examples of valid memo structures
Add a doc comment like:
+// extractNextAndFinalMemosFromSwapMsg extracts the next_memo and final_memo fields from a swap message. +// The expected memo structure is: +// { +// "wasm": { +// "msg": { +// "osmosis_swap": { +// "next_memo": {...}, +// "final_memo": {...} +// } +// } +// } +// } +// Returns (nil, nil) if the memo structure is invalid or fields are missing. func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (nextMemo map[string]any, finalMemo map[string]any) {
2311-2318: Add documentation for the expected forward memo structure.The test helper would benefit from documentation explaining the expected structure of a valid forward memo.
Add a doc comment like:
+// assertForwardMemoStructure verifies that a memo contains a valid forward instruction. +// Expected structure: +// { +// "forward": { +// "channel": "<channel-id>", +// "port": "<port-id>", +// "receiver": "<receiver-address>" +// } +// } func (suite *HooksTestSuite) assertForwardMemoStructure(memo map[string]any, channel, port, receiver string) {cosmwasm/packages/registry/src/registry.rs (1)
424-424: Document the last_transfer_memo parameter.The
last_transfer_memoparameter's purpose and format should be documented in the method's documentation. This is especially important as it affects the final IBC transfer memo.Add documentation like:
/// block_time is the current block time. This is needed to calculate the /// timeout timestamp. + /// last_transfer_memo is a JSON string that will be merged with the final hop's memo. + /// If provided, it must be a valid JSON object that will be merged with any existing + /// memo structure. #[allow(clippy::too_many_arguments)]Also applies to: 565-580
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/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,!**/*.wasm
📒 Files selected for processing (24)
CHANGELOG.md(1 hunks)app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-registry/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-registry/src/execute.rs(1 hunks)cosmwasm/contracts/crosschain-registry/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/checks.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/contract.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs(3 hunks)cosmwasm/contracts/crosschain-swaps/src/msg.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/state.rs(1 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/outpost/src/contract.rs(1 hunks)cosmwasm/contracts/outpost/src/execute.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)cosmwasm/packages/registry/src/error.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(4 hunks)cosmwasm/packages/registry/src/registry.rs(7 hunks)cosmwasm/packages/registry/src/utils.rs(0 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cosmwasm/packages/registry/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- cosmwasm/contracts/crosschain-swaps/tests/test_env.rs
- cosmwasm/contracts/crosschain-swaps/src/ibc_lifecycle.rs
- cosmwasm/packages/registry/src/error.rs
- cosmwasm/contracts/crosschain-swaps/src/msg.rs
- cosmwasm/contracts/crosschain-swaps/src/checks.rs
- app/apptesting/test_suite.go
- cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs
- cosmwasm/contracts/crosschain-registry/src/contract.rs
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- cosmwasm/contracts/crosschain-swaps/src/state.rs
- cosmwasm/contracts/outpost/src/execute.rs
- cosmwasm/contracts/crosschain-registry/src/msg.rs
- cosmwasm/contracts/swaprouter/tests/test_env.rs
- app/test_helpers.go
- cosmwasm/contracts/outpost/src/contract.rs
- cosmwasm/contracts/crosschain-swaps/src/contract.rs
- cosmwasm/contracts/swaprouter/tests/swap_test.rs
- CHANGELOG.md
🔇 Additional comments (18)
cosmwasm/packages/registry/src/msg.rs (3)
101-104: LGTM! Good use of#[repr(transparent)]and custom deserialization.The struct definition is well-designed:
#[repr(transparent)]ensures zero-cost abstraction over the innerValue- Custom deserialization with
deserialize_cw_valuehelps maintain invariants
117-135: LGTM! Well-designed utility methods.The new utility methods provide a complete set of operations for JSON manipulation with proper error handling and type safety.
176-186: LGTM! Clean and consistent From implementations.The From trait implementations maintain consistency by using the new
newmethod for value construction.cosmwasm/contracts/crosschain-swaps/src/execute.rs (6)
37-44: LGTM! Clean parameter addition.The
final_memoparameter is correctly added to the function signature with proper documentation.
132-134: LGTM! Proper parameter forwarding.The final_memo is correctly forwarded to the swap_and_forward function.
162-165: LGTM! Good validation of user-provided memos.The code properly validates both next_memo and final_memo using the new validation function.
176-177: LGTM! Consistent memo handling.Both next_memo and final_memo are correctly passed to the unwrap_coin_into function.
262-268: LGTM! Clean final_memo extraction and handling.The code properly extracts and formats the final_memo for the IBC transfer.
457-469: LGTM! Well-implemented memo validation.The validation function:
- Properly handles None cases
- Ensures JSON object validity
- Checks for absence of callback key
- Returns consistent string format
cosmwasm/contracts/crosschain-registry/src/execute.rs (1)
134-134: Consider passing through the newly introducedfinal_memo.Right now, this line sets the final memo to an empty string, effectively discarding any user input. Given that the PR introduces a new
final_memofeature in related modules, it may be beneficial to accept and forward the user-supplied final memo here.tests/ibc-hooks/ibc_middleware_test.go (4)
2250-2258: Clean and efficient event finder implementation!The function efficiently searches for a fungible token packet event using a simple loop and returns a pointer to the found event.
2259-2272: Consider implementing suggested improvements from previous review.The previous review comment suggesting improvements to error handling, documentation, and code structure is still applicable.
2141-2248: Well-structured integration test with comprehensive validations!The test thoroughly validates the crosschain swap functionality with multiple hops by:
- Setting up initial token balances and pools
- Verifying balance changes at each step
- Checking memo structure and forwarding
- Testing the complete end-to-end flow
2320-2415: Thorough test coverage of memo handling in single-hop swaps!The test effectively validates:
- Proper handling of both next_memo and final_memo
- Correct merging of memos after the swap
- Balance changes and token transfers
- End-to-end flow with proper verifications
cosmwasm/packages/registry/src/registry.rs (4)
7-7: LGTM!The import of
SerializableJsonis properly scoped and well-placed with related imports.
677-702: Enhance test coverage for memo merging scenarios.While the basic forwarding memo test is good, consider adding test cases for:
- Empty
last_transfer_memo- Invalid JSON in
last_transfer_memo- Complex merge scenarios with both
first_transfer_memoandlast_transfer_memo
119-119: Verify the serialization compatibility.The change from
Option<Box<Memo>>toOption<Box<SerializableJson>>enhances flexibility in memo handling. However, ensure that this change maintains backward compatibility with existing memo formats in the network.✅ Verification successful
Let me gather more information about the serialization implementation and usage.
Based on the code analysis, I can now provide a conclusive response about the serialization compatibility:
The change to
SerializableJsonmaintains backward compatibility and enhances memo handlingThe implementation shows that:
SerializableJsonis a well-designed wrapper aroundserde_cw_value::Valuethat maintains serialization compatibility- It implements proper conversion traits including
TryFrom<Memo>which ensures backward compatibility with the existingMemoformat- The type provides additional flexibility through its
mergefunctionality for combining JSON objects safely- There are comprehensive tests verifying the serialization behavior and JSON handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any custom memo serialization/deserialization implementations rg -A 5 'impl.*Serialize|impl.*Deserialize'Length of output: 829
Script:
#!/bin/bash # Search for SerializableJson definition and implementation ast-grep --pattern 'struct SerializableJson' # Search for SerializableJson usage and related code rg -A 5 'SerializableJson' # Search for Memo struct definition and implementation ast-grep --pattern 'struct Memo'Length of output: 20925
571-587: Consider adding error handling for JSON merge conflicts.The memo merging logic could potentially encounter conflicts when merging user-provided JSON with system-generated memo structures.
| String::new(), | ||
| Some(Callback { | ||
| contract: env.contract.address.clone(), | ||
| msg: serde_cw_value::to_value(&ExecuteMsg::OsmosisSwap { | ||
| output_denom, | ||
| receiver: receiver.to_string(), | ||
| slippage, | ||
| next_memo, | ||
| final_memo, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider passing through the final_memo parameter.
Right now, this line sets the final memo to an empty string, effectively discarding any user input. Consider passing through the final_memo parameter to maintain consistency with the feature's intent.
- String::new(),
+ final_memo.map(|m| serde_json_wasm::to_string(&m)).transpose()?.unwrap_or_default(),📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String::new(), | |
| Some(Callback { | |
| contract: env.contract.address.clone(), | |
| msg: serde_cw_value::to_value(&ExecuteMsg::OsmosisSwap { | |
| output_denom, | |
| receiver: receiver.to_string(), | |
| slippage, | |
| next_memo, | |
| final_memo, | |
| final_memo.map(|m| serde_json_wasm::to_string(&m)).transpose()?.unwrap_or_default(), | |
| Some(Callback { | |
| contract: env.contract.address.clone(), | |
| msg: serde_cw_value::to_value(&ExecuteMsg::OsmosisSwap { | |
| output_denom, | |
| receiver: receiver.to_string(), | |
| slippage, | |
| next_memo, | |
| final_memo, |
sug0
force-pushed
the
bat+tiago/last-hop-memo
branch
from
275546f to
594edd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/test_helpers.go (1)
142-171: Consider adding error handling for edge casesWhile the implementation is generally good, consider handling potential edge cases:
- Multiple test environments passed in t parameter
- Invalid chain IDs
func SetupWithCustomHomeAndChainId(isCheckTx bool, dir, chainId string, t ...*testing.T) *OsmosisApp { + if len(t) > 1 { + panic("only one test environment should be provided") + } + if chainId == "" { + panic("chain ID cannot be empty") + } db := cosmosdb.NewMemDB() var ( l log.Loggercosmwasm/packages/registry/src/msg.rs (1)
148-180: Consider optimizing merge for large maps.The current implementation creates a new map and copies all entries. For large maps, you could potentially optimize by:
- Comparing sizes and merging the smaller map into the larger one
- Using with_capacity() to pre-allocate space
pub fn merge(self, other: SerializableJson) -> Result<Self, RegistryError> { let mut first_map = match self.0 { serde_cw_value::Value::Map(m) => m, serde_cw_value::Value::Unit | serde_cw_value::Value::Option(None) => BTreeMap::new(), json => { return Err(RegistryError::InvalidJson { error: "invalid json: expected an object".to_string(), json: stringify(&json)?, }) } }; let second_map = match other.0 { serde_cw_value::Value::Map(m) => m, serde_cw_value::Value::Unit | serde_cw_value::Value::Option(None) => BTreeMap::new(), json => { return Err(RegistryError::InvalidJson { error: "invalid json: expected an object".to_string(), json: stringify(&json)?, }) } }; + // Optimize by merging smaller map into larger one + if first_map.len() < second_map.len() { + let mut result = second_map; + for (key, value) in first_map { + if result.insert(key, value).is_some() { + return Err(RegistryError::DuplicateKeyError); + } + } + return Ok(SerializableJson(serde_cw_value::Value::Map(result))); + } for (key, value) in second_map { if first_map.insert(key, value).is_some() { return Err(RegistryError::DuplicateKeyError); } } Ok(SerializableJson(serde_cw_value::Value::Map(first_map))) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/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,!**/*.wasm
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)app/apptesting/test_suite.go(1 hunks)app/test_helpers.go(3 hunks)cosmwasm/contracts/crosschain-swaps/src/execute.rs(11 hunks)cosmwasm/contracts/crosschain-swaps/src/utils.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs(2 hunks)cosmwasm/contracts/crosschain-swaps/tests/test_env.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/swap_test.rs(1 hunks)cosmwasm/contracts/swaprouter/tests/test_env.rs(1 hunks)cosmwasm/packages/registry/src/msg.rs(4 hunks)tests/ibc-hooks/ibc_middleware_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cosmwasm/contracts/crosschain-swaps/tests/test_env.rs
- cosmwasm/contracts/swaprouter/tests/test_env.rs
- CHANGELOG.md
- cosmwasm/contracts/crosschain-swaps/tests/crosschain_swap_test.rs
- cosmwasm/contracts/crosschain-swaps/src/utils.rs
- app/apptesting/test_suite.go
🔇 Additional comments (18)
cosmwasm/contracts/swaprouter/tests/swap_test.rs (2)
Line range hint
1-232: Add test coverage for the newfinal_memofeature.The test file lacks coverage for the newly introduced
final_memofield inosmosis_swapmessages. Consider adding the following test cases:
- Successful swap with a valid
final_memo- Swap with empty
final_memo- Edge cases for memo validation (if any)
This will help ensure the new feature works as expected and maintains stability during future changes.
Would you like me to help generate these test cases? Please let me know if you need assistance implementing them.
233-233: LGTM! Good use of slice type.The change from
&Vec<...>to&[...]is a good improvement. Using slices is more idiomatic in Rust and provides better flexibility while maintaining efficiency.app/test_helpers.go (3)
123-124: LGTM! Function signature updated to support test loggingThe addition of variadic testing.T parameter allows for flexible test logging configuration.
127-136: LGTM! Well-structured debug options implementationClean implementation of AppOptions interface for debug configuration. The Get method properly handles the trace flag case.
138-140: LGTM! Simple and clear debug flag checkStraightforward implementation to check debug environment variable.
cosmwasm/contracts/crosschain-swaps/src/execute.rs (5)
37-44: LGTM! Clean function signature updateThe addition of final_memo parameter is well-integrated with existing parameters.
78-90: Consider passing through the final_memo parameterRight now, this line sets the final memo to an empty string, effectively discarding any user input. Consider passing through the final_memo parameter to maintain consistency with the feature's intent.
- String::new(), + final_memo.map(|m| serde_json_wasm::to_string(&m)).transpose()?.unwrap_or_default(),
Line range hint
150-177: LGTM! Proper validation of both memo typesGood implementation of parallel validation for both next_memo and final_memo.
Line range hint
262-281: LGTM! Clean implementation of final memo handling in swap replyThe code properly extracts and handles the final memo during swap reply processing.
457-469: LGTM! Well-structured memo validationThe validation function properly handles:
- Optional memo input
- JSON object validation
- Callback key checks
tests/ibc-hooks/ibc_middleware_test.go (3)
2250-2318: Enhance helper functions with better error handling and documentationThe helper functions could be improved for better maintainability and error reporting.
+// extractFinalMemoFromSwapMsg navigates through a nested map structure to extract +// the final_memo from an osmosis_swap message. The expected structure is: +// { +// "wasm": { +// "msg": { +// "osmosis_swap": { +// "final_memo": {...} +// } +// } +// } +// } +// Returns nil if any part of the path is missing or of wrong type. func extractNextAndFinalMemosFromSwapMsg(memo map[string]any) (nextMemo map[string]any, finalMemo map[string]any) { + if memo == nil { + return nil, nil + }
2141-2248: LGTM! Comprehensive test for multiple hop scenarioThe test thoroughly validates:
- Balance changes across chains
- Memo structure and forwarding
- Proper packet relay sequence
2320-2415: LGTM! Well-structured single hop testGood coverage of:
- Memo merging
- Balance verification
- Packet handling
cosmwasm/packages/registry/src/msg.rs (5)
101-104: LGTM! Well-structured serialization setup.The addition of
#[repr(transparent)]and custom deserialization is a good practice that ensures consistent memory layout and proper handling of nested JSON values.
117-142: LGTM! Well-designed utility methods.The new utility methods provide a clean API for JSON manipulation with proper error handling.
183-210: LGTM! Well-implemented conversion traits.The implementations provide a consistent way to convert between different types and SerializableJson, with proper error handling.
231-560: LGTM! Excellent test coverage.The test suite is comprehensive and well-structured, covering:
- Empty JSON cases
- Merging scenarios
- Flattening of nested structures
- Error cases
- Edge cases with null and undefined values
Line range hint
1-560: Verify integration with XCS contracts.The SerializableJson implementation provides a solid foundation for handling memos. Let's verify its usage in XCS contracts:
✅ Verification successful
Let's try a different search strategy to verify the integration:
SerializableJson implementation is correctly integrated with XCS contracts
The verification shows that the SerializableJson implementation is properly integrated across the crosschain-swaps ecosystem:
- In
crosschain-swaps/src/msg.rs, it's used for bothnext_memoandfinal_memofields in the contract's messages- In
crosschain-swaps/src/execute.rs, it's properly validated throughvalidate_user_provided_memo- In
crosschain-swaps/src/state.rs, it's correctly stored in the contract's state- The implementation provides proper error handling and validation as seen in the registry package
- Test coverage exists in both the registry and crosschain-swaps contracts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of SerializableJson in XCS contracts echo "Searching for SerializableJson usage in XCS contracts..." rg -A 5 "SerializableJson.*final_memo" cosmwasm/contracts/ # Search for potential test coverage of final_memo echo "Checking test coverage..." rg -A 5 "test.*final_memo" cosmwasm/contracts/Length of output: 293
Script:
#!/bin/bash # Search for any mentions of final_memo in the codebase echo "Searching for final_memo mentions..." rg "final_memo" # Search for SerializableJson usage in the codebase echo -e "\nSearching for SerializableJson usage..." rg "SerializableJson" -A 3 # Look for XCS contract files echo -e "\nLooking for XCS contract files..." fd -e rs . cosmwasm/contracts/Length of output: 20953
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
| @@ -53,6 +53,8 @@ pub enum ExecuteMsg { | |||
| into_chain: Option<String>, | |||
| #[serde(default = "String::new")] | |||
| with_memo: String, | |||
| #[serde(default = "String::new")] | |||
There was a problem hiding this comment.
This can be #[serde(default)] but not an issue
What is the purpose of the change
This pull request introduces the ability to supply a
final_memofield, that sits beside thenext_memofield inosmosis_swapmessages of XCS. The value associated with this key, in the JSON object, is appended to thenextfield in theforwardmessage of the last hop, when delivering swapped tokens via IBC.In order to debug the contract code being executed, this PR also introduces the ability to log keeper output by exporting the environment variable
OSMO_KEEPER_DEBUGwith a non-empty string value.Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)