feat: Cross Chain Rates Recipient #671
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request encompasses significant updates to the Andromeda protocol, including the introduction of multiple ADOs (Curve, IBC Registry, MultiSig) and enhancements such as Denom Validation and Kernel ICS20 Transfer. Key changes involve the restructuring of recipient handling from vectors to single instances across various components, including rate settings and testing functions. Additionally, the version of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (18)
tests-integration/tests/cw20_app.rs (1)
102-102: Clean up outdated test setup and commentsThe test has been updated to use a single recipient, but there are some cleanup items needed:
- Remove the unused
recipient_twowallet setup- Update the comment "Rates are 10% , so we expect a balance of one for recipients one and two..." to reflect single recipient logic
- ("recipient_two", vec![]), // Remove this line from wallet setup- // Rates are 10% , so we expect a balance of one for recipients one and two and the leftover 8 will be sent to buyer two + // Rate is 10%, so we expect a balance of one for recipient and the leftover 9 will be sent to buyer twocontracts/modules/andromeda-rates/src/contract.rs (1)
168-179: Consider enhancing event attributes for cross-chain trackingWhile the event includes basic payment information, for cross-chain transfers it would be beneficial to add additional attributes such as:
- Source chain identifier
- Destination chain identifier
- Transfer status
event = event.add_attribute( "payment", PaymentAttribute { receiver: local_rate .recipient .address .get_raw_address(&deps)? .to_string(), amount: fee.clone(), } .to_string(), ); +event = event + .add_attribute("source_chain", local_rate.recipient.get_source_chain()?) + .add_attribute("destination_chain", local_rate.recipient.get_destination_chain()?);contracts/modules/andromeda-rates/src/testing/tests.rs (1)
Line range hint
156-183: Remove commented out codeThere are several blocks of commented out code from the previous implementation with multiple recipients. Since we're moving to a single recipient model, these comments should be removed to improve code maintainability and readability.
contracts/fungible-tokens/andromeda-cw20/src/testing/tests.rs (1)
Line range hint
1-300: Consider restructuring cross-chain tests.To improve maintainability and coverage of cross-chain functionality, consider:
- Creating a dedicated test module for cross-chain scenarios
- Extracting common test utilities and fixtures
- Adding integration tests that verify the complete flow from rate collection to IBC message execution
Suggested structure:
mod cross_chain_tests { mod fixtures { // Common test setup, mock IBC channels, etc. } mod transfer_tests { // Cross-chain transfer specific tests } mod rates_tests { // Cross-chain rates specific tests } mod recovery_tests { // Recovery address handling tests } }contracts/data-storage/andromeda-string-storage/src/testing/tests.rs (2)
65-69: LGTM! Consider adding a descriptive test name.The test case correctly validates that a rate with an empty recipient address is rejected. However, consider renaming the test section or adding a comment to explicitly indicate that this tests the empty recipient scenario.
- // Set percent rates + // Test: Rate with empty recipient address should be rejected
Line range hint
1-324: Consider future-proofing tests for multiple rates support.The PR objectives mention future plans for implementing multiple rates. While the current changes correctly implement and test single recipient functionality, consider:
- Adding a TODO comment about future test cases for multiple rates
- Structuring the test file to easily accommodate additional test cases when multiple rates are implemented
packages/std/src/amp/recipient.rs (2)
61-67: Consider using a constant for the IBC protocol identifier.The implementation correctly identifies cross-chain recipients by checking the "ibc" protocol. Consider these improvements:
- Define a constant for the IBC protocol identifier
- Simplify the match expression to a direct comparison
+const IBC_PROTOCOL: &str = "ibc"; pub fn is_cross_chain(&self) -> bool { - let protocol = self.address.get_protocol(); - match protocol { - Some("ibc") => true, - _ => false, - } + self.address.get_protocol() == Some(IBC_PROTOCOL) }
61-67: Add documentation for cross-chain functionality.The
is_cross_chainmethod is part of the cross-chain rates system but lacks documentation explaining its purpose and usage. Consider:
- Adding doc comments explaining when and why to use this method
- Including examples of cross-chain scenarios
- Documenting the relationship with
ibc_recovery_address+/// Returns true if the recipient's address uses the IBC protocol, indicating +/// it's a cross-chain recipient. This is used by the rates system to handle +/// cross-chain payments appropriately. +/// +/// # Examples +/// +/// ``` +/// use crate::Recipient; +/// +/// let recipient = Recipient::from_string("ibc/..."); +/// assert!(recipient.is_cross_chain()); +/// +/// let recipient = Recipient::from_string("local_address"); +/// assert!(!recipient.is_cross_chain()); +/// ``` +/// +/// When dealing with cross-chain recipients, consider setting an +/// `ibc_recovery_address` using `with_ibc_recovery()` to handle failed transfers. pub fn is_cross_chain(&self) -> bool {contracts/data-storage/andromeda-point/src/testing/tests.rs (1)
Line range hint
1-1: Enhance test organization for cross-chain functionality.Consider adding a dedicated test module for cross-chain scenarios to improve test organization and coverage:
- Group cross-chain tests in a new module (e.g.,
mod cross_chain_tests)- Include test cases from the PR objectives:
- Auction scenario with rates and cross-chain recipient
- Kernel IBC funds handling
- Cross-chain user handling in rates system
contracts/data-storage/andromeda-primitive/src/testing/tests.rs (3)
75-79: LGTM! Consider using a constant for the default addressThe empty recipient setup correctly tests the invalid rate scenario. Consider extracting the default address to a constant for better readability.
+ const DEFAULT_ADDRESS: &str = ""; recipient: Recipient { - address: AndrAddr::from_string(String::default()), + address: AndrAddr::from_string(DEFAULT_ADDRESS.to_string()), msg: None, ibc_recovery_address: None, },
100-104: Update outdated commentThe comment "Make sure sender is set as recipient when the recipients vector is empty" is outdated since we've moved to a single recipient model.
- // Make sure sender is set as recipient when the recipients vector is empty + // Verify rate setup with creator as recipient
122-122: Use Recipient::new consistentlyFor consistency and cleaner code, consider using
Recipient::newhelper method throughout the test file instead of mixing it with direct struct initialization.- recipient: Recipient { - address: AndrAddr::from_string("creator".to_string()), - msg: None, - ibc_recovery_address: None, - }, + recipient: Recipient::new(AndrAddr::from_string("creator"), None),contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs (1)
Line range hint
94-145: Consider architectural implications of simplified message handlingThe removal of action handling and response aggregation represents a significant architectural change. While this might streamline cross-chain operations, consider:
- Document the new message flow architecture, especially for cross-chain scenarios
- Update integration tests to verify the new behavior
- Consider adding feature flags to gradually roll out these changes
- Ensure backward compatibility for existing integrations
This change could benefit from a more detailed architectural discussion.
tests-integration/tests/marketplace_app.rs (1)
Line range hint
921-928: Remove unused rate configuration with hardcoded address.This LocalRate instance is unused (prefixed with underscore) and contains a hardcoded address. Consider removing it or utilizing it in the test if needed.
- let _local_rate2 = LocalRate { - rate_type: LocalRateType::Additive, - recipient: Recipient::from_string(rates_receiver.to_string()), - value: LocalRateValue::Flat(coin( - 100, - "andr1ywhkkafy0jgr3etypp40v6ct9ffmvakrsruwvp595pd9juv5tafqqzph5h", - )), - description: None, - };contracts/non-fungible-tokens/andromeda-auction/src/testing/tests.rs (1)
1296-1300: Consider refactoring duplicatedRecipientinitialization codeThe initialization of the
Recipientstruct is repeated across multiple test functions (execute_claim_with_tax,execute_claim_with_royalty,execute_claim_cw20_with_tax). Consider creating a helper function or utilizing a common setup method to reduce code duplication and improve maintainability.Also applies to: 1473-1477, 1635-1639
tests-integration/tests/kernel_orch.rs (3)
832-835: Consider removing debug print statementsThe
println!statement used to print the osmosis kernel address may not be necessary in the final test code. If it's for debugging purposes, consider removing it or replacing it with proper logging.
832-835: Consider removing debug print statementsThe
println!statement for logging the osmosis kernel address may clutter test outputs if left in production code. If the output is not essential, it is advisable to remove it.
Line range hint
593-1121: Consider refactoring common test setup codeThere is noticeable duplication in test setups across different test functions, particularly in initializing chains, uploading contracts, and setting up channels. Refactoring this code into reusable helper functions or setup modules would enhance maintainability and reduce redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
CHANGELOG.md(1 hunks)contracts/data-storage/andromeda-boolean/src/testing/tests.rs(2 hunks)contracts/data-storage/andromeda-point/src/testing/tests.rs(2 hunks)contracts/data-storage/andromeda-primitive/src/testing/tests.rs(3 hunks)contracts/data-storage/andromeda-string-storage/src/testing/tests.rs(2 hunks)contracts/fungible-tokens/andromeda-cw20/src/testing/tests.rs(2 hunks)contracts/modules/andromeda-rates/src/contract.rs(3 hunks)contracts/modules/andromeda-rates/src/testing/tests.rs(4 hunks)contracts/non-fungible-tokens/andromeda-auction/src/contract.rs(2 hunks)contracts/non-fungible-tokens/andromeda-auction/src/testing/tests.rs(3 hunks)contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs(2 hunks)contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs(3 hunks)contracts/os/andromeda-kernel/src/execute.rs(0 hunks)packages/std/Cargo.toml(1 hunks)packages/std/src/ado_base/rates.rs(3 hunks)packages/std/src/ado_contract/rates.rs(2 hunks)packages/std/src/amp/recipient.rs(1 hunks)tests-integration/tests/auction_app.rs(5 hunks)tests-integration/tests/cw20_app.rs(2 hunks)tests-integration/tests/kernel_orch.rs(9 hunks)tests-integration/tests/marketplace_app.rs(4 hunks)tests-integration/tests/primitive.rs(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/os/andromeda-kernel/src/execute.rs
🔇 Additional comments (46)
packages/std/Cargo.toml (1)
Line range hint 29-53: Verify dependency versions for security vulnerabilities
Some dependencies have specific version pins. Let's ensure they are up-to-date and free from known vulnerabilities.
Run the following script to check for potential security issues:
tests-integration/tests/cw20_app.rs (2)
136-138: LGTM: Balance calculation is correct
The updated balance calculation correctly reflects the new single-recipient logic:
- From 10 tokens transferred, 1 token (10%) goes to the recipient
- Remaining 9 tokens correctly added to buyer_two's balance
Line range hint 1-140: Verify consistent test updates across the codebase
Since this represents a fundamental change in how rates work (moving from multiple recipients to single recipient), we should verify that all related tests have been updated consistently.
✅ Verification successful
All test files have been updated correctly to use single recipient
After reviewing the LocalRate usage across all test files, I can confirm that all test implementations are consistent with the single-recipient model:
- auction_app.rs: Uses single recipient with splitter
- primitive.rs: Uses single recipient (recipient_one)
- marketplace_app.rs: Uses single recipient (rates_receiver)
- kernel_orch.rs: Uses single recipient with IBC address
- cw20_app.rs: Uses single recipient (recipient_one)
While recipient_two appears in some test files, it's only used for separate test cases and not as part of multiple recipients in a single rate configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar updates
echo "Searching for other test files using rates..."
rg -l "LocalRate" "tests-integration/tests"
echo "Checking for potentially outdated recipient vectors in tests..."
rg "recipients: vec!\[Recipient::new" "tests-integration/tests"
echo "Checking for other test files that might reference recipient_two..."
rg "recipient_two" "tests-integration/tests"
Length of output: 1920
Script:
#!/bin/bash
# Let's check the LocalRate usage in auction_app.rs to see if it needs updates
echo "Checking LocalRate usage in auction_app.rs..."
rg -A 10 "LocalRate" "tests-integration/tests/auction_app.rs"
# Let's also check the other test files for their LocalRate implementations
echo "Checking LocalRate usage in primitive.rs..."
rg -A 10 "LocalRate" "tests-integration/tests/primitive.rs"
echo "Checking LocalRate usage in marketplace_app.rs..."
rg -A 10 "LocalRate" "tests-integration/tests/marketplace_app.rs"
echo "Checking LocalRate usage in kernel_orch.rs..."
rg -A 10 "LocalRate" "tests-integration/tests/kernel_orch.rs"
Length of output: 6460
tests-integration/tests/primitive.rs (3)
87-87: LGTM: Recipient field updated correctly
The change from a vector of recipients to a single recipient field aligns with the new data model while maintaining the same test behavior.
107-107: LGTM: Flat rate recipient field updated correctly
The recipient field change is properly integrated with the flat rate test case, and the subsequent balance checks verify the correct rate application.
Line range hint 1-1: Add test coverage for cross-chain rate scenarios
The PR objectives mention cross-chain rates recipient functionality, including Send messages to the kernel and TriggerRelay mechanism, but these scenarios are not covered in the current test suite. Consider adding test cases that verify:
- Cross-chain recipient handling
- Send message dispatch to kernel with associated fees
- TriggerRelay mechanism functionality
Let's check if cross-chain tests exist elsewhere:
Would you like help creating comprehensive test cases for the cross-chain functionality?
contracts/modules/andromeda-rates/src/contract.rs (3)
33-33: LGTM: Simplified rate initialization
The simplified rate initialization aligns well with the transition to a single recipient model.
82-82: LGTM: Appropriate removal of mutability
The removal of the mut keyword is correct as the rate parameter is not modified within the function scope.
171-175: Verify cross-chain recipient address handling
The code handles recipient address resolution, but we should verify that cross-chain addresses are properly handled through the kernel.
✅ Verification successful
Let me gather more information about the address validation and cross-chain message handling.
Let me search for the recipient address handling implementation in the rates contract to complete the verification.
Based on the gathered information, I can now provide the final response:
Cross-chain recipient address handling is properly implemented
The code correctly handles cross-chain recipient addresses through:
- Proper validation using
get_raw_addressfrom the Andromeda standard library - Integration with the kernel for address resolution
- Support for both native and CW20 token transfers
- Appropriate IBC recovery address handling in the Recipient struct
The implementation follows the expected pattern for cross-chain message handling in the Andromeda ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cross-chain address validation patterns
ast-grep --pattern 'get_raw_address($_)'
# Search for kernel transfer handling
rg -A 5 'TriggerRelay|IbcTransfer'
Length of output: 3974
Script:
#!/bin/bash
# Search for get_raw_address implementation and usage
rg -A 5 "fn get_raw_address"
# Search for cross-chain message handling in the rates contract
rg -A 10 "cross|ibc|kernel" contracts/modules/andromeda-rates/src/
Length of output: 10867
Script:
#!/bin/bash
# Search for recipient handling in the rates contract
rg -A 10 "recipient" contracts/modules/andromeda-rates/src/
# Look for the LocalRate struct definition
ast-grep --pattern 'struct LocalRate'
Length of output: 13653
packages/std/src/ado_contract/rates.rs (2)
41-41: LGTM: Improved parameter passing semantics
The change to pass rate by value instead of mutable reference is a good improvement. It better reflects the ownership semantics since the rate is moved into storage without intermediate mutations.
198-202: LGTM: Updated recipient structure
The change from a vector of recipients to a single recipient with cross-chain support (ibc_recovery_address) aligns well with the PR objectives.
contracts/modules/andromeda-rates/src/testing/tests.rs (2)
30-34: 🛠️ Refactor suggestion
Add test coverage for cross-chain recipient scenarios
Given that this PR implements cross-chain rates recipient functionality, this test should include scenarios with ibc_recovery_address set and verify the cross-chain behavior.
Consider adding test cases for:
- Cross-chain recipient with valid IBC recovery address
- Verification of TriggerRelay message dispatch
- Error cases for invalid IBC addresses
Line range hint 1-1: Verify existence of cross-chain integration tests
The PR objectives mention implementing cross-chain rates recipient functionality, but the current test file lacks comprehensive cross-chain testing.
Let's check for cross-chain tests in other files:
contracts/fungible-tokens/andromeda-cw20/src/testing/tests.rs (1)
92-96: Add test coverage for cross-chain recipient scenarios.
While the ibc_recovery_address field has been added to support cross-chain functionality, the test only covers local chain scenarios. Consider adding test cases that verify:
- Transfer behavior with cross-chain recipients
- Recovery address handling
- IBC message generation for cross-chain transfers
Let's check if there are any other cross-chain related tests:
contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs (2)
141-145:
Verify impact of simplified response handling
The removal of submessages, attributes, and events aggregation from the action response could affect:
- Event monitoring systems
- Cross-chain message delivery (critical for the cross-chain rates feature)
- Indexers or UIs relying on these attributes
Please ensure this simplification aligns with the cross-chain rates recipient implementation goals.
94-100: Verify impact of removing action handling
The removal of call_action could affect cross-chain message handling and permission checks. Please verify:
- All necessary permission checks are still enforced
- Cross-chain functionality (mentioned in PR objectives) works as expected
- Other contracts depending on action responses are updated accordingly
Also, remove the unused call_action import if this change is permanent.
✅ Verification successful
Removal of call_action in CW721 contract requires further investigation
The search results show that call_action is widely used across many Andromeda contracts, including other NFT-related contracts like marketplace and crowdfund. Let's verify if this specific CW721 contract has a unique reason to remove action handling.
Commenting out call_action appears to be an intentional architectural change
The search results reveal that:
- The code still tracks the action via
let action = msg.as_ref().to_string()but processes it differently - The response handling is modified to remove action response aggregation
- The contract now directly processes messages through
execute_cw721orcontract.executebased on message type - Other contracts still use
call_action, but this CW721 implementation seems to be intentionally diverging
This appears to be a deliberate architectural change specific to the CW721 contract, likely to optimize or simplify its execution flow. The change maintains message handling capabilities while removing the generic action processing layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other contracts that might expect action responses
rg "call_action.*response" --type rust
# Check for any remaining action handling patterns
ast-grep --pattern 'call_action($$$)'
Length of output: 14733
Script:
#!/bin/bash
# Check if there are any special imports or configurations in the CW721 contract
rg -A 5 "use (andromeda|common).*action" contracts/non-fungible-tokens/andromeda-cw721/src/
# Look for any alternative action handling in CW721
rg "execute.*msg" contracts/non-fungible-tokens/andromeda-cw721/src/contract.rs -A 10
# Check if there are any related changes in test files
rg "call_action" contracts/non-fungible-tokens/andromeda-cw721/src/tests/ -A 5
Length of output: 1551
tests-integration/tests/marketplace_app.rs (3)
Line range hint 570-574: LGTM! CW20 rate handling is properly implemented.
The LocalRate correctly configures an additive fee using CW20 tokens, and the test validates proper fee collection through CW20 balance checks.
Line range hint 799-805: LGTM! Percentage-based rate is correctly configured.
The LocalRate properly implements a 20% fee using PercentRate, and the test validates the correct fee calculation through balance checks.
71-74: LGTM! Rate configuration looks correct.
The LocalRate structure properly handles the single recipient pattern with a deductive flat fee. The test correctly validates the fee collection through balance checks.
Let's verify the rate handling implementation:
✅ Verification successful
Based on the search results, I can now verify the rate configuration. The codebase shows consistent usage of the LocalRate struct with a single recipient field across all implementations and tests. The struct definition in packages/std/src/ado_base/rates.rs confirms this:
pub struct LocalRate {
pub rate_type: LocalRateType,
pub recipient: Recipient,
pub value: LocalRateValue,
pub description: Option<String>,
}The rate configuration in the review comment matches this structure and is used correctly:
LocalRate {
rate_type: LocalRateType::Deductive,
recipient: Recipient::from_string(rates_receiver.to_string()),
value: LocalRateValue::Flat(coin(100, "uandr")),
description: None,
};LGTM! Rate configuration is correctly structured.
The implementation follows the standard pattern used throughout the codebase for handling deductive flat fees with a single recipient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check rate handling implementation across the codebase
# Verify the LocalRate struct usage has been consistently updated
ast-grep --pattern 'struct LocalRate {
$$$
recipient: $_,
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Let's try a different approach to verify rate handling
# First, let's find all files that might contain LocalRate definitions
fd -e rs | xargs rg "LocalRate" -B 2 -A 5
# Also search for specific rate-related patterns
rg "struct LocalRate" -A 10
Length of output: 77749
contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs (1)
839-843: Cross-chain recipient test coverage needed here as well.
Similar to the native token test case above, this CW20 test case should also verify cross-chain recipient scenarios.
packages/std/src/ado_base/rates.rs (3)
3-6: Imports updated correctly for AMP message handling
The newly added imports for AMPMsg, AMPMsgConfig, AndrAddr, and Recipient are appropriate and necessary for the cross-chain recipient functionality.
133-133: Updated LocalRate struct to use a single recipient
Changing from recipients: Vec<Recipient> to recipient: Recipient simplifies the rate handling and aligns with the move towards a single recipient model as described in the PR objectives.
157-214: generate_response method correctly handles single recipient and cross-chain logic
The updates to the generate_response method appropriately handle the logic for a single recipient, including constructing the correct messages for cross-chain recipients using AMPMsg. This ensures that both local and cross-chain payments are processed correctly.
tests-integration/tests/auction_app.rs (7)
14-14: Import statements for splitter module are correctly added.
The additional imports from andromeda_splitter::mock are necessary for the splitter functionality in the tests.
82-102: Splitter initialization is accurate and consistent.
The splitter_init_msg is correctly constructed with two recipients:
recipient_onewith 50%recipient_twowith 50%
The percentages sum up to 100%, ensuring proper fund distribution. The splitter_component is appropriately created with the initialization message.
104-108: Splitter component correctly added to app components.
The splitter_component is included in the app_components vector, ensuring it is instantiated within the application.
137-139: Splitter component is properly retrieved for testing.
The splitter instance is correctly queried using app.query_ado_by_component_name, allowing interaction with the splitter in the test cases.
148-151: Recipient for the rate is correctly set to the splitter.
The rate's recipient is set to the splitter's address with an associated message:
- Address:
splitter.addr() - Message:
mock_splitter_send_msg()serialized withto_json_binary
Using unwrap() is acceptable here since the context is a test environment, and serialization is expected to succeed.
153-153: Rate percentage is accurately specified.
The rate value is set to 50%, which aligns with the intended deduction percentage in the auction rate setup.
438-438: No issues found with the auction claim execution.
The auction claim is executed correctly, ensuring the winner can claim the auctioned item as expected.
contracts/non-fungible-tokens/andromeda-auction/src/contract.rs (1)
98-104:
Potential unintended disabling of call_action functionality
Commenting out the call_action function in lines 98-104 may disable important action processing in the contract. Ensure that this change is intentional and that all dependent functionalities are adjusted accordingly.
Run the following script to verify if call_action is used elsewhere in the codebase and assess potential impacts:
tests-integration/tests/kernel_orch.rs (15)
2-2: Imports for auction functionalities added
The necessary imports for mock_start_auction and AuctionContract have been correctly added to support auction operations in the tests.
12-12: Import for CW721Contract added
The import for CW721Contract has been correctly added to enable CW721 token functionalities.
14-14: Import for TokenExtension added
The TokenExtension import is appropriately added to extend token capabilities in CW721 contracts.
17-17: Imports for rate-related types added
The necessary rate-related types from ado_base::rates are correctly imported to handle rate functionalities.
22-22: Imports for Asset, Expiry, and Milliseconds added
The imports for Asset, Expiry, and Milliseconds are correctly added to manage assets and time-related operations.
30-31: Standard library imports extended
Additional imports from cosmwasm_std are added to support IBC operations and numeric computations, which are appropriate for the expanded test scenarios.
Line range hint 593-607: Buyer setup in the test environment
The buyer address is correctly initialized, and the balance is set appropriately for testing auction scenarios involving bids from this address.
611-614: Initialization of additional contracts on Juno
The contracts adodb_juno, economics_juno, auction_juno, and cw721_juno are properly instantiated to extend the test capabilities for auctions and token management.
623-626: Uploading new contracts to Juno
The newly initialized contracts are correctly uploaded to the Juno chain, ensuring they are available for deployment in the test environment.
721-730: Instantiation of adodb_juno contract
The adodb_juno contract is instantiated with the correct kernel address and owner settings, aligning with the test requirements.
732-741: Instantiation of economics_juno contract
The economics_juno contract is correctly instantiated, setting up the economic module for the test scenarios.
743-751: Upsert of 'economics' key in kernel_juno
The 'economics' key is correctly upserted into kernel_juno with the appropriate contract address, ensuring that economic functionalities are accessible.
890-890: Upsert of 'trigger_key' in kernel_juno
The 'trigger_key' is correctly upserted with the sender's address, facilitating the relay trigger mechanism in the test.
946-1071: Implementation of cross-chain rates recipient and auction logic
The addition of the cross-chain rates recipient setup and auction functionalities is well-implemented. The steps include:
- Instantiating the
auction_junocontract with correct parameters. - Instantiating the
cw721_junocontract for NFT management. - Setting a rate with a 50% deduction to a cross-chain recipient on Osmosis.
- Minting an NFT and starting an auction.
- Placing a bid from the
buyeraddress. - Claiming the NFT and verifying the balances reflect the rate deduction.
1065-1065: Verify balance assertion after rate deduction
The assertion assert_eq!(balances[0].amount.u128(), 25); correctly checks that the recipient on Osmosis receives 25 units after a 50% rate deduction from the 50-unit bid. This confirms the rate logic functions as intended.
packages/std/Cargo.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "andromeda-std" | |||
| version = "1.3.4" | |||
| version = "1.4.0" | |||
There was a problem hiding this comment.
💡 Codebase verification
Version update required to match PR objectives
The version in packages/std/Cargo.toml is currently set to "1.4.0", but according to the PR objectives, it should be updated to "1.5.0". All other packages in the workspace are using workspace-level dependency resolution for andromeda-std, which means they will automatically use the version specified in the workspace configuration.
- Update the version in
packages/std/Cargo.tomlfrom "1.4.0" to "1.5.0" to align with the PR objectives.
🔗 Analysis chain
Version number inconsistency with PR objectives
The PR objectives mention updating andromeda-std from version "1.4.0" to "1.5.0", but the implementation shows version "1.4.0". Please clarify if this is the intended version or if it needs to be updated to "1.5.0".
Run the following script to check version references across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version references of andromeda-std
# Look for any references to andromeda-std versions in Cargo.toml files
rg -g 'Cargo.toml' 'andromeda-std.*version.*1\.[0-9]+\.[0-9]+'
Length of output: 62
Script:
#!/bin/bash
# Let's check the version in packages/std/Cargo.toml and also look for any dependencies on this package
cat packages/std/Cargo.toml
# Also check for any workspace dependencies in the root Cargo.toml
cat Cargo.toml
# Search for any references to andromeda-std in all Cargo.toml files
fd Cargo.toml --exec grep -l "andromeda-std"
Length of output: 6245
Script:
#!/bin/bash
# Let's check the dependencies in the files that reference andromeda-std to see version constraints
for file in $(fd Cargo.toml --exec grep -l andromeda-std); do
echo "=== $file ==="
grep -A 1 "andromeda-std.*=" "$file" || true
done
Length of output: 17705
CHANGELOG.md
Outdated
| @@ -75,6 +75,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - ADODB now supports pre-release tagging [(#560)](https://github.com/andromedaprotocol/andromeda-core/pull/560) | |||
| - Validator Staking: Updated according to audit [(#565)](https://github.com/andromedaprotocol/andromeda-core/pull/565) | |||
| - Conditional Splitter: Change lock_time's type from MillisecondsDuration to Expiry [(#567)](https://github.com/andromedaprotocol/andromeda-core/pull/567) | |||
| - Rates: Limit rates recipient to only one address [(#669)](https://github.com/andromedaprotocol/andromeda-core/pull/669) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing changelog entries for cross-chain functionality.
While the current entry correctly documents the architectural change to single recipient, please add entries for:
- The new cross-chain rates recipient functionality with TriggerRelay mechanism
- The andromeda-std version update from 1.4.0 to 1.5.0
These are significant changes mentioned in the PR description that should be documented.
Add these entries under the appropriate sections:
### Changed
- Rates: Limit rates recipient to only one address [(#669)](https://github.com/andromedaprotocol/andromeda-core/pull/669)
+- Rates: Added cross-chain recipient support with TriggerRelay mechanism [(#671)](https://github.com/andromedaprotocol/andromeda-core/pull/671)
+- Updated andromeda-std from 1.4.0 to 1.5.0 [(#671)](https://github.com/andromedaprotocol/andromeda-core/pull/671)Committable suggestion skipped: line range outside the PR's diff.
| recipient: Recipient { | ||
| address: AndrAddr::from_string("owner".to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for cross-chain scenarios
While the basic recipient structure is tested, there's no coverage for cross-chain scenarios using ibc_recovery_address. Consider adding test cases that verify:
- Setting a cross-chain recipient with recovery address
- Handling of IBC recovery scenarios
Example test case structure:
#[test]
fn test_cross_chain_recipient() {
let mut deps = mock_dependencies();
let contract = ADOContract::default();
let cross_chain_rate = Rate::Local(LocalRate {
rate_type: LocalRateType::Additive,
recipient: Recipient {
address: AndrAddr::from_string("recipient".to_string()),
msg: None,
ibc_recovery_address: Some("recovery_addr".to_string()),
},
value: LocalRateValue::Flat(coin(100_u128, "uandr")),
description: None,
});
// Test setting and retrieving cross-chain rate
// Test IBC recovery scenarios
}| recipient: Recipient { | ||
| address: AndrAddr::from_string("recipient1".to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for cross-chain native token deductions
The test should be expanded to verify:
- Native token deductions with cross-chain recipients
- Proper handling of IBC messages for cross-chain transfers
- Integration with the kernel's Send message for cross-chain scenarios
| recipient: Recipient { | ||
| address: AndrAddr::from_string("recipient1".to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for cross-chain CW20 token handling
The test should include scenarios for:
- CW20 token transfers to cross-chain recipients
- Proper message construction for cross-chain CW20 transfers
- Integration with IBC transfer mechanisms
| recipient: Recipient { | ||
| address: AndrAddr::from_string(tax_recipient.to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
Add test coverage for cross-chain recipient scenarios
Given that this PR implements cross-chain rates recipient functionality, this test should include scenarios where:
- The recipient is on a different chain (using
ibc_recovery_address) - The tax collection triggers IBC transfers
- The
TriggerRelaymechanism is properly invoked
This ensures the new cross-chain functionality is properly tested.
Would you like me to help generate additional test cases covering these cross-chain scenarios?
| recipient: Recipient { | ||
| address: AndrAddr::from_string("tax_recipient".to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Cross-chain recipient functionality needs test coverage
The codebase analysis reveals that while the Recipient struct includes an ibc_recovery_address field, there are currently no test cases that verify cross-chain functionality:
- All test instances set
ibc_recovery_address: None - No test functions exist for IBC recovery scenarios
- No implementation of IBC recovery handlers was found
Please add test coverage for:
- Setting and validating IBC recovery addresses
- Cross-chain recipient message handling
- Recovery flows when IBC transfers fail
🔗 Analysis chain
Add test coverage for cross-chain recipient scenarios.
Given that this PR implements cross-chain rates recipient functionality, consider adding test cases that verify:
- Setting an IBC recovery address
- Cross-chain recipient message handling
- Recovery scenarios when IBC transfers fail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any cross-chain recipient tests
rg -l "ibc_recovery_address:\s+Some\("
Length of output: 40
Script:
#!/bin/bash
# Let's check for any tests related to IBC recovery or cross-chain functionality
rg -A 5 "ibc_recovery_address" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
# Also check the main contract code for IBC recovery implementation
rg -A 5 "ibc_recovery_address" contracts/non-fungible-tokens/andromeda-marketplace/src/
# Check if there are any test cases with "ibc" in their name or description
rg "fn test.*ibc" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
Length of output: 2726
Script:
#!/bin/bash
# Let's check the contract's types and structs to understand the Recipient implementation
rg -A 10 "struct Recipient" contracts/non-fungible-tokens/andromeda-marketplace/src/
# Check if there are any messages or handlers related to IBC recovery
rg -A 5 "fn.*ibc.*recovery" contracts/non-fungible-tokens/andromeda-marketplace/src/
# Look for any documentation or implementation details about cross-chain functionality
rg -l "cross.?chain|ibc" contracts/non-fungible-tokens/andromeda-marketplace/src/
Length of output: 330
| recipient: Recipient { | ||
| address: AndrAddr::from_string("tax_recipient".to_string()), | ||
| msg: None, | ||
| ibc_recovery_address: None, | ||
| }], | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered evidence, I can now provide the final response:
Test name is misleading and cross-chain scenarios need coverage
-
The test name
test_execute_buy_with_tax_and_royalty_worksis misleading as the implementation only handles tax. This is evident from:- The TODO comment stating both tax and royalty is unsupported
- The commented-out royalty recipient code in the test
- Only tax-related SubMsg being sent in the test assertions
-
No tests cover the
ibc_recovery_addressscenario despite the struct supporting it.
Recommendations:
- Rename the test to
test_execute_buy_with_tax_worksto accurately reflect its functionality - Add test coverage for cross-chain scenarios using
ibc_recovery_address - Either implement royalty support or remove related code and tests
🔗 Analysis chain
Address TODO comment and add cross-chain success path test.
Two issues to address:
- The test name suggests support for both tax and royalty, but there's a TODO comment (line 907) indicating this is unsupported. Consider either implementing the support or renaming the test to reflect current capabilities.
- Add test coverage for successful cross-chain recipient scenarios.
Let me gather more information about the tax and royalty implementation to verify both concerns.
Let me check the implementation of tax and royalty handling to understand if they are actually supported or not.
Let me check one final aspect to understand the tax and royalty implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any cross-chain related tests
rg "test.*cross.*chain"
# Check for any IBC-related message handling
ast-grep --pattern 'fn test_$_($$$) {
$$$
msg: Some($$$)
ibc_recovery_address: Some($$$)
$$$
}'
Length of output: 644
Script:
#!/bin/bash
# Check for the TODO comment and surrounding context
rg -B 5 -A 5 "TODO.*tax.*royalty" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
# Check for any other tax and royalty related tests
rg "test.*tax.*royalty|test.*royalty.*tax" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
# Check if there are any other tests using ibc_recovery_address
rg "ibc_recovery_address: Some" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
Length of output: 949
Script:
#!/bin/bash
# Check for tax and royalty handling in the implementation
rg -B 3 -A 3 "fn (handle|execute)_buy" contracts/non-fungible-tokens/andromeda-marketplace/src/contract.rs
# Check for any tax or royalty related structs/types
ast-grep --pattern 'struct $_ {
$$$
tax: $_,
royalty: $_,
$$$
}'
# Check for tax and royalty calculations
rg -A 5 "calculate.*tax|calculate.*royalty" contracts/non-fungible-tokens/andromeda-marketplace/src/
Length of output: 501
Script:
#!/bin/bash
# Check for tax and royalty related state or configuration
rg -B 2 -A 2 "tax|royalty" contracts/non-fungible-tokens/andromeda-marketplace/src/state.rs
# Look for any tax or royalty related messages or errors
rg -B 2 -A 2 "tax|royalty" contracts/non-fungible-tokens/andromeda-marketplace/src/error.rs contracts/non-fungible-tokens/andromeda-marketplace/src/msg.rs
# Check the test file for tax and royalty setup
rg -B 5 -A 5 "tax.*recipient|royalty.*recipient" contracts/non-fungible-tokens/andromeda-marketplace/src/testing/tests.rs
Length of output: 2803
| .get_raw_address(&deps) | ||
| .unwrap_or(Addr::unchecked(self.recipient.address.to_string())) | ||
| .to_string(), |
There was a problem hiding this comment.
Handle potential errors when retrieving recipient's raw address
Using unwrap_or with Addr::unchecked may mask errors from get_raw_address, potentially leading to invalid addresses being used without proper validation. It's important to handle the Result returned by get_raw_address explicitly to avoid unexpected behavior.
Apply this diff to handle the error properly:
- .get_raw_address(&deps)
- .unwrap_or(Addr::unchecked(self.recipient.address.to_string()))
- .to_string(),
+ .get_raw_address(&deps)?
+ .to_string(),This change will propagate any errors from get_raw_address, ensuring that invalid addresses are not silently accepted.
📝 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.
| .get_raw_address(&deps) | |
| .unwrap_or(Addr::unchecked(self.recipient.address.to_string())) | |
| .to_string(), | |
| .get_raw_address(&deps)? | |
| .to_string(), |
contracts/non-fungible-tokens/andromeda-auction/src/contract.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests-integration/tests/kernel_orch.rs (1)
854-857: Remove unnecessaryprintln!statementConsider removing the debug
println!statement used to print the Osmosis kernel address to clean up the test output.Apply this diff to remove the
println!statement:- println!( - "osmosis kernel address: {}", - kernel_osmosis.address().unwrap() - );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/std/Cargo.toml(1 hunks)tests-integration/tests/kernel_orch.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/std/Cargo.toml
🔇 Additional comments (20)
tests-integration/tests/kernel_orch.rs (20)
593-593: Initialize the buyer address
The initialization of the buyer address is correct and follows the standard pattern.
606-607: Set balance for the buyer on Juno chain
Assigning the balance to the buyer on the Juno chain is implemented correctly.
611-614: Instantiate new contracts on the Juno chain
The creation of adodb_juno, economics_juno, auction_juno, and cw721_juno contracts is appropriate for the test setup.
624-627: Upload contracts to the Juno chain
Uploading the newly instantiated contracts is necessary and correctly implemented.
634-634: Upload the economics_osmosis contract
Uploading economics_osmosis to the Osmosis chain is correctly performed.
722-731: Instantiate adodb_juno contract
The instantiation of the adodb_juno contract with the correct kernel address is properly done.
733-742: Instantiate economics_juno contract
The instantiation of economics_juno with the appropriate parameters is correctly implemented.
744-753: Instantiate economics_osmosis contract
The instantiation of economics_osmosis is properly set up with the correct kernel address.
755-763: Upsert economics key in Juno kernel
Updating the Juno kernel's key address map with the economics_juno address is correctly executed.
765-773: Upsert economics key in Osmosis kernel
Updating the Osmosis kernel's key address map with the economics_osmosis address is correctly implemented.
912-912: Confirm the use of sender for trigger_key
Using sender.clone() as the value for trigger_key is acceptable if the sender is intended to handle trigger relay actions. Please ensure this aligns with the intended access control.
970-981: Instantiate auction_juno contract
The instantiation of auction_juno is correctly set up with the necessary parameters.
983-995: Instantiate cw721_juno contract
The cw721_juno contract is properly instantiated with the correct minter and kernel address.
997-1015: Verify the cross-chain recipient address format
Ensure that the cross-chain recipient address in the rate setting is accurately formatted and valid. The address should correctly represent the IBC address format for cross-chain transactions.
1017-1027: Mint NFT with cw721_juno contract
Minting the NFT using the cw721_juno contract is correctly implemented.
1029-1039: Set up the auction start time correctly
Calculating the auction start time using the current block time and setting the expiry appropriately ensures the auction timing functions as expected.
1052-1061: Place a bid in the auction
The PlaceBid execution with the buyer address and bid amount is correctly simulated.
1065-1092: Validate the funds distribution after claim
After the Claim action, the test correctly checks that the kernel on Osmosis chain receives the expected amount (half of the bid due to the 50% deductive rate). The assertions verify that the funds are accurately transferred.
1094-1143: Confirm the trigger relay mechanism
The trigger relay is correctly executed, and the test verifies that the recipient on Osmosis receives the correct amount after the relay. This confirms the cross-chain transfer and rate deduction mechanisms are functioning properly.
1144-1144: Overall test coverage is comprehensive
The added test cases enhance the integration tests by covering cross-chain transactions involving auctions, rates, and NFT interactions, providing valuable validation for the implemented features.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests-integration/tests/kernel_orch.rs (2)
982-1028: Consider enhancing error handling and validationWhile the cross-chain rates recipient setup is functional, consider these improvements:
- Add validation for the rate percentage (currently hardcoded to 50%)
- Consider adding checks for authorized token addresses
- Add error handling for failed contract instantiations
auction_juno .instantiate( &andromeda_non_fungible_tokens::auction::InstantiateMsg { - authorized_token_addresses: None, + authorized_token_addresses: Some(vec![cw721_juno.address().unwrap().into_string()]), authorized_cw20_addresses: None, kernel_address: kernel_juno.address().unwrap().into_string(), owner: None, }, None, None, - ) - .unwrap(); + ) + .map_err(|e| panic!("Failed to instantiate auction contract: {}", e)) + .unwrap();
Line range hint
1-1156: Add test coverage for additional scenariosConsider adding test cases for:
- Failed cross-chain rate applications
- Multiple concurrent auctions
- Edge cases in IBC packet handling
- Recovery scenarios for failed transfers
Would you like me to help create additional test scenarios for these cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests-integration/tests/kernel_orch.rs(8 hunks)
🔇 Additional comments (1)
tests-integration/tests/kernel_orch.rs (1)
Line range hint 1-31: LGTM: Imports are well-organized and appropriate
The new imports for auction (AuctionContract), NFT (CW721Contract), and cross-chain functionality are properly structured and align with the PR objectives.
| let start_time = Milliseconds::from_nanos(juno.block_info().unwrap().time.nanos()); | ||
| let receive_msg = mock_start_auction( | ||
| None, | ||
| Expiry::AtTime(start_time.plus_milliseconds(Milliseconds(10000))), | ||
| None, | ||
| Asset::NativeToken("juno".to_string()), | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| ); | ||
| cw721_juno | ||
| .execute( | ||
| &andromeda_non_fungible_tokens::cw721::ExecuteMsg::SendNft { | ||
| contract: AndrAddr::from_string(auction_juno.address().unwrap()), | ||
| token_id: "1".to_string(), | ||
| msg: to_json_binary(&receive_msg).unwrap(), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
| juno.wait_seconds(1).unwrap(); | ||
|
|
||
| auction_juno.set_sender(&Addr::unchecked(buyer.clone())); | ||
| auction_juno | ||
| .execute( | ||
| &andromeda_non_fungible_tokens::auction::ExecuteMsg::PlaceBid { | ||
| token_id: "1".to_string(), | ||
| token_address: cw721_juno.address().unwrap().into_string(), | ||
| }, | ||
| Some(&[coin(50, "juno")]), | ||
| ) | ||
| .unwrap(); | ||
| juno.next_block().unwrap(); | ||
| juno.next_block().unwrap(); | ||
|
|
||
| // Claim | ||
| let claim_request = auction_juno | ||
| .execute( | ||
| &andromeda_non_fungible_tokens::auction::ExecuteMsg::Claim { | ||
| token_id: "1".to_string(), | ||
| token_address: cw721_juno.address().unwrap().into_string(), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for auction scenarios
The auction test scenario should include additional test cases:
- Verify auction state after bid placement
- Test edge cases like invalid bid amounts
- Validate token ownership transfer
Consider adding these assertions:
auction_juno
.execute(
&andromeda_non_fungible_tokens::auction::ExecuteMsg::PlaceBid {
token_id: "1".to_string(),
token_address: cw721_juno.address().unwrap().into_string(),
},
Some(&[coin(50, "juno")]),
)
.unwrap();
+
+// Add auction state verification
+let auction_info = auction_juno
+ .query(&andromeda_non_fungible_tokens::auction::QueryMsg::GetAuction {
+ token_id: "1".to_string(),
+ token_address: cw721_juno.address().unwrap().into_string(),
+ })
+ .unwrap();
+assert_eq!(auction_info.high_bidder.unwrap(), buyer);Committable suggestion skipped: line range outside the PR's diff.
| // Check recipient balance after trigger execute msg | ||
| let balances = osmosis | ||
| .query_all_balances(kernel_osmosis.address().unwrap()) | ||
| .unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, ibc_denom); | ||
| assert_eq!(balances[0].amount.u128(), 25); | ||
| } else { | ||
| panic!("packet timed out"); | ||
| // There was a decode error or the packet timed out | ||
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| }; | ||
|
|
||
| // Construct an Execute msg from the kernel on juno inteded for the splitter on osmosis | ||
| let kernel_juno_trigger_request = kernel_juno | ||
| .execute( | ||
| &ExecuteMsg::TriggerRelay { | ||
| packet_sequence: "2".to_string(), | ||
| packet_ack_msg: IbcPacketAckMsg::new( | ||
| IbcAcknowledgement::new( | ||
| to_json_binary(&AcknowledgementMsg::<SendMessageWithFundsResponse>::Ok( | ||
| SendMessageWithFundsResponse {}, | ||
| )) | ||
| .unwrap(), | ||
| ), | ||
| IbcPacket::new( | ||
| Binary::default(), | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| IbcEndpoint { | ||
| port_id: "port_id".to_string(), | ||
| channel_id: "channel_id".to_string(), | ||
| }, | ||
| 1, | ||
| IbcTimeout::with_timestamp(Timestamp::from_seconds(1)), | ||
| ), | ||
| Addr::unchecked("relayer"), | ||
| ), | ||
| }, | ||
| None, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let packet_lifetime = interchain | ||
| .await_packets("juno", kernel_juno_trigger_request) | ||
| .unwrap(); | ||
|
|
||
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | ||
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | ||
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | ||
|
|
||
| // Check recipient balance after trigger execute msg | ||
| let balances = osmosis.query_all_balances(recipient).unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, ibc_denom); | ||
| assert_eq!(balances[0].amount.u128(), 100 + 25); | ||
| } else { | ||
| panic!("packet timed out"); | ||
| // There was a decode error or the packet timed out | ||
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen balance verification and error handling
The balance verification and IBC packet handling could be improved:
- Add timeout handling for IBC packets
- Verify balances on both chains
- Add more detailed error messages
if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome {
+ // Verify balances on source chain
+ let juno_balances = juno
+ .query_all_balances(kernel_juno.address().unwrap())
+ .unwrap();
+ assert_eq!(juno_balances.len(), 0, "Kernel should have no balance on source chain");
+
// Check recipient balance after trigger execute msg
let balances = osmosis
.query_all_balances(kernel_osmosis.address().unwrap())
.unwrap();
assert_eq!(balances.len(), 1);
assert_eq!(balances[0].denom, ibc_denom);
assert_eq!(balances[0].amount.u128(), 25);
} else {
- panic!("packet timed out");
+ panic!("IBC packet failed: timeout or execution error");
}📝 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.
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis | |
| .query_all_balances(kernel_osmosis.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!(balances[0].amount.u128(), 25); | |
| } else { | |
| panic!("packet timed out"); | |
| // There was a decode error or the packet timed out | |
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | |
| }; | |
| // Construct an Execute msg from the kernel on juno inteded for the splitter on osmosis | |
| let kernel_juno_trigger_request = kernel_juno | |
| .execute( | |
| &ExecuteMsg::TriggerRelay { | |
| packet_sequence: "2".to_string(), | |
| packet_ack_msg: IbcPacketAckMsg::new( | |
| IbcAcknowledgement::new( | |
| to_json_binary(&AcknowledgementMsg::<SendMessageWithFundsResponse>::Ok( | |
| SendMessageWithFundsResponse {}, | |
| )) | |
| .unwrap(), | |
| ), | |
| IbcPacket::new( | |
| Binary::default(), | |
| IbcEndpoint { | |
| port_id: "port_id".to_string(), | |
| channel_id: "channel_id".to_string(), | |
| }, | |
| IbcEndpoint { | |
| port_id: "port_id".to_string(), | |
| channel_id: "channel_id".to_string(), | |
| }, | |
| 1, | |
| IbcTimeout::with_timestamp(Timestamp::from_seconds(1)), | |
| ), | |
| Addr::unchecked("relayer"), | |
| ), | |
| }, | |
| None, | |
| ) | |
| .unwrap(); | |
| let packet_lifetime = interchain | |
| .await_packets("juno", kernel_juno_trigger_request) | |
| .unwrap(); | |
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | |
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | |
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | |
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis.query_all_balances(recipient).unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!(balances[0].amount.u128(), 100 + 25); | |
| } else { | |
| panic!("packet timed out"); | |
| // There was a decode error or the packet timed out | |
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | |
| }; | |
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis | |
| .query_all_balances(kernel_osmosis.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!(balances[0].amount.u128(), 25); | |
| } else { | |
| panic!("packet timed out"); | |
| // There was a decode error or the packet timed out | |
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | |
| }; | |
| // Construct an Execute msg from the kernel on juno inteded for the splitter on osmosis | |
| let kernel_juno_trigger_request = kernel_juno | |
| .execute( | |
| &ExecuteMsg::TriggerRelay { | |
| packet_sequence: "2".to_string(), | |
| packet_ack_msg: IbcPacketAckMsg::new( | |
| IbcAcknowledgement::new( | |
| to_json_binary(&AcknowledgementMsg::<SendMessageWithFundsResponse>::Ok( | |
| SendMessageWithFundsResponse {}, | |
| )) | |
| .unwrap(), | |
| ), | |
| IbcPacket::new( | |
| Binary::default(), | |
| IbcEndpoint { | |
| port_id: "port_id".to_string(), | |
| channel_id: "channel_id".to_string(), | |
| }, | |
| IbcEndpoint { | |
| port_id: "port_id".to_string(), | |
| channel_id: "channel_id".to_string(), | |
| }, | |
| 1, | |
| IbcTimeout::with_timestamp(Timestamp::from_seconds(1)), | |
| ), | |
| Addr::unchecked("relayer"), | |
| ), | |
| }, | |
| None, | |
| ) | |
| .unwrap(); | |
| let packet_lifetime = interchain | |
| .await_packets("juno", kernel_juno_trigger_request) | |
| .unwrap(); | |
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | |
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | |
| // Verify balances on source chain | |
| let juno_balances = juno | |
| .query_all_balances(kernel_juno.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!(juno_balances.len(), 0, "Kernel should have no balance on source chain"); | |
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis.query_all_balances(recipient).unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!(balances[0].amount.u128(), 100 + 25); | |
| } else { | |
| panic!("IBC packet failed: timeout or execution error"); | |
| // There was a decode error or the packet timed out | |
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests-integration/tests/kernel_orch.rs (1)
983-1157: Enhance test coverage for cross-chain rates scenarioWhile the test successfully validates the basic flow, consider adding these assertions:
- Verify auction state after bid placement
- Confirm NFT ownership transfer after successful claim
- Validate the rates calculations (50% split)
Add these assertions after the bid placement:
auction_juno .execute( &andromeda_non_fungible_tokens::auction::ExecuteMsg::PlaceBid { token_id: "1".to_string(), token_address: cw721_juno.address().unwrap().into_string(), }, Some(&[coin(50, "juno")]), ) .unwrap(); + +// Verify auction state +let auction_info: AuctionResponse = auction_juno + .query(&QueryMsg::GetAuction { + token_id: "1".to_string(), + token_address: cw721_juno.address().unwrap().into_string(), + }) + .unwrap(); +assert_eq!(auction_info.high_bidder.unwrap(), buyer); +assert_eq!(auction_info.high_bid.unwrap().amount.u128(), 50); + +// Verify NFT ownership hasn't changed yet +let owner: OwnerOfResponse = cw721_juno + .query(&Cw721QueryMsg::OwnerOf { + token_id: "1".to_string(), + include_expired: None, + }) + .unwrap(); +assert_eq!(owner.owner, auction_juno.address().unwrap());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests-integration/tests/kernel_orch.rs(11 hunks)
🔇 Additional comments (2)
tests-integration/tests/kernel_orch.rs (2)
2-2: LGTM: New imports align with auction functionality
The added imports for auction, CW721, and token extension types are appropriate for implementing the cross-chain rates recipient test scenarios.
Also applies to: 13-13, 15-15
Line range hint 593-765: LGTM: Well-structured auction test setup
The initialization sequence properly sets up all required components for cross-chain auction testing:
- Buyer account with sufficient balance
- Contract deployments (Auction, CW721, Economics)
- Kernel address configurations
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | ||
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | ||
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | ||
|
|
||
| // Check recipient balance after trigger execute msg | ||
| let balances = osmosis | ||
| .query_all_balances(kernel_osmosis.address().unwrap()) | ||
| .unwrap(); | ||
| assert_eq!(balances.len(), 1); | ||
| assert_eq!(balances[0].denom, ibc_denom); | ||
| assert_eq!(balances[0].amount.u128(), 25); | ||
| } else { | ||
| panic!("packet timed out"); | ||
| // There was a decode error or the packet timed out | ||
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen balance verification for cross-chain transfers
The balance checks could be more comprehensive to ensure the cross-chain rate distribution works correctly.
Add these verifications:
if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome {
+ // Verify balances on source chain
+ let juno_balances = juno
+ .query_all_balances(auction_juno.address().unwrap())
+ .unwrap();
+ assert_eq!(
+ juno_balances.len(),
+ 0,
+ "Auction contract should have transferred all funds"
+ );
+
// Check recipient balance after trigger execute msg
let balances = osmosis
.query_all_balances(kernel_osmosis.address().unwrap())
.unwrap();
assert_eq!(balances.len(), 1);
assert_eq!(balances[0].denom, ibc_denom);
- assert_eq!(balances[0].amount.u128(), 25);
+ assert_eq!(
+ balances[0].amount.u128(),
+ 25,
+ "Should receive 50% of bid amount (50 * 0.5 = 25)"
+ );
} else {
- panic!("packet timed out");
+ panic!("IBC packet failed: timeout or execution error");
}📝 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.
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | |
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | |
| // Packet has been successfully acknowledged and decoded, the transaction has gone through correctly | |
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis | |
| .query_all_balances(kernel_osmosis.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!(balances[0].amount.u128(), 25); | |
| } else { | |
| panic!("packet timed out"); | |
| // There was a decode error or the packet timed out | |
| // Else the packet timed-out, you may have a relayer error or something is wrong in your application | |
| }; | |
| // For testing a successful outcome of the first packet sent out in the tx, you can use: | |
| if let IbcPacketOutcome::Success { .. } = &packet_lifetime.packets[0].outcome { | |
| // Verify balances on source chain | |
| let juno_balances = juno | |
| .query_all_balances(auction_juno.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!( | |
| juno_balances.len(), | |
| 0, | |
| "Auction contract should have transferred all funds" | |
| ); | |
| // Check recipient balance after trigger execute msg | |
| let balances = osmosis | |
| .query_all_balances(kernel_osmosis.address().unwrap()) | |
| .unwrap(); | |
| assert_eq!(balances.len(), 1); | |
| assert_eq!(balances[0].denom, ibc_denom); | |
| assert_eq!( | |
| balances[0].amount.u128(), | |
| 25, | |
| "Should receive 50% of bid amount (50 * 0.5 = 25)" | |
| ); | |
| } else { | |
| panic!("IBC packet failed: timeout or execution error"); | |
| }; |
crnbarr93
changed the base branch from
development
to
rates-single-recipient
| Cw20Coin { | ||
| amount: fee.amount, | ||
| address: fee.denom.to_string(), | ||
| let msg = if self.recipient.is_cross_chain() { |
There was a problem hiding this comment.
We should ensure cw20 isn't used for cross-chain here, maybe at the point of creating the rates recipient we validate?
There was a problem hiding this comment.
That would work when the LocalRateValue is flat, but when it's a percentage we'll have to handle it while processing the rate right?
| .recipient | ||
| .address | ||
| .get_raw_address(&deps) | ||
| .unwrap_or(Addr::unchecked(self.recipient.address.to_string())) |
There was a problem hiding this comment.
Is there any reason to generate unchecked Addr in case get_raw_address failed?
There was a problem hiding this comment.
I'm assuming if it fails then it's a cross-chain address. Maybe we should account for that possibility in get_raw_address itself? Feels like a topic worth discussing.
@crnbarr93
Motivation
Handle cross-chain users in rates.
Implementation
If the recipient is cross-chain, a
Sendmessage is sent to the kernel alongside the fee.TriggerRelayis then needed to move the funds from the kernel on the target chain to the actual recipient.Testing
Added to
test_kernel_ibc_funds_onlyan auction scenario with rates and a cross-chain recipient.Version Changes
andromeda-std:1.4.0->1.5.0Notes
This is branched from
rates-single-recipientFuture work
Allow multiple rates.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation