Cw20 kernel routing #723
Cw20 kernel routing #723
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive changes to the Andromeda ecosystem, focusing on enhancing CW20 token support and AMP protocol message handling. The modifications span multiple contracts and packages, including the kernel, splitter, and set-amount splitter contracts. Key improvements include adding CW20 message routing capabilities, refining fund distribution logic, and updating message generation methods to support more flexible token interactions. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 5
🔭 Outside diff range comments (2)
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
270-308: Validate consistency of CW20 token denominations inamp_fundsIn the
execute_send_cw20function, when accumulatingamp_funds, ensure that all coins have the samedenom, as the CW20 token transfers should be homogeneous. Mixing different CW20 tokens in a single transfer can lead to errors.Add a validation step to confirm that all coins in
amp_fundsshare the samedenom.let first_denom = amp_funds.first().map(|c| &c.denom); if !amp_funds.iter().all(|c| &c.denom == first_denom.unwrap()) { return Err(ContractError::InvalidFunds { msg: "All funds must be of the same CW20 token".to_string(), }); }contracts/os/andromeda-kernel/Cargo.toml (1)
Line range hint
3-3: Version number needs to be updatedAccording to the PR objectives, the kernel version should be updated from
1.1.1to1.1.1-b.1to reflect the beta status of the CW20 routing feature.-version = "1.1.1" +version = "1.1.1-b.1"
🧹 Nitpick comments (6)
contracts/finance/andromeda-splitter/src/contract.rs (1)
306-308: Avoid unnecessary cloning ofamp_fundsThe cloning of
amp_fundswhen callingto_sub_msg_cw20can be avoided to improve performance.Modify the code to borrow
amp_fundsinstead of cloning:- let distro_msg = pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?; + let distro_msg = pkt.to_sub_msg_cw20(kernel_address, &_funds, 1)?;Ensure that the
to_sub_msg_cw20method is updated to accept a reference toamp_funds.packages/andromeda-data-storage/src/string_storage.rs (1)
Line range hint
100-105: Consider expanding test coverageWhile the change from vector to array is reasonable for a single test case, consider adding more test cases to thoroughly validate the string storage functionality, especially in the context of CW20 message handling.
Examples of additional test cases could include:
- Unicode strings
- Very long strings
- Strings with special characters
packages/std/src/os/kernel.rs (1)
85-89: Consider adding documentation for CW20HookMsgWhile the implementation is correct, adding documentation would help explain the purpose and usage of this enum for CW20 token handling.
#[cw_serde] +/// Message type for CW20 token hook handling +/// Used to process CW20 tokens through the AMP system pub enum Cw20HookMsg { + /// Receives an AMP packet for processing with CW20 tokens AmpReceive(AMPPkt), }packages/std/src/ado_base/rates.rs (1)
229-235: Consider fixing indentation for better readabilityThe indentation in the CW20 message generation block could be improved to match the surrounding code style.
- self.recipient.generate_msg_cw20( - &deps, - Cw20Coin { - amount: fee.amount, - address: fee.denom.to_string(), - }, - )? + self.recipient.generate_msg_cw20( + &deps, + Cw20Coin { + amount: fee.amount, + address: fee.denom.to_string(), + }, + )?contracts/os/andromeda-adodb/src/tests.rs (2)
721-723: LGTM! Consider adding pagination test cases.The array initialization is more concise and efficient for the fixed set of test data. However, since
QueryMsg::AllADOTypessupports pagination throughstart_afterandlimitparameters, consider adding test cases to verify this functionality.Add test cases for pagination by extending the test data and verifying the results with different
start_afterandlimitcombinations:#[test] fn test_all_ado_types() { // ... existing setup ... let ados = [ADOVersion::from_string("[email protected]".to_string()), ADOVersion::from_string("[email protected]".to_string()), - ADOVersion::from_string("[email protected]".to_string())]; + ADOVersion::from_string("[email protected]".to_string()), + ADOVersion::from_string("[email protected]".to_string()), + ADOVersion::from_string("[email protected]".to_string())]; // ... existing publishing logic ... + // Test pagination + let query_msg = QueryMsg::AllADOTypes { + start_after: Some("[email protected]".to_string()), + limit: Some(2), + }; + let res = query(deps.as_ref(), env.clone(), query_msg).unwrap(); + let value: Vec<String> = from_json(res).unwrap(); + let expected = vec![ + "[email protected]".to_string(), + "[email protected]".to_string(), + ]; + assert_eq!(value, expected);
721-723: Consider adding test cases for version ordering and semantic versioning validation.While the current test coverage is good, consider adding the following test cases to improve robustness:
- Version ordering tests to verify that the latest version is correctly determined when publishing multiple versions in non-sequential order.
- Semantic versioning validation tests to ensure invalid version strings are rejected.
Here's an example of additional test cases:
#[test] fn test_version_ordering() { // ... setup code ... let versions = [ "1.0.0", "0.9.0", "1.1.0-alpha", "1.0.1", "2.0.0-rc.1", "2.0.0", "1.9.9" ]; // Publish versions in this order and verify that // LATEST_VERSION always points to the highest stable version } #[test] fn test_invalid_versions() { // ... setup code ... let invalid_versions = [ "1.0", "1", "v1.0.0", "1.0.0.0", "latest", "1.0.0-", "-1.0.0" ]; // Verify that publishing with these versions fails }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
contracts/finance/andromeda-set-amount-splitter/src/contract.rs(3 hunks)contracts/finance/andromeda-splitter/src/contract.rs(3 hunks)contracts/math/andromeda-matrix/src/testing/tests.rs(1 hunks)contracts/os/andromeda-adodb/src/tests.rs(1 hunks)contracts/os/andromeda-kernel/Cargo.toml(1 hunks)contracts/os/andromeda-kernel/src/contract.rs(2 hunks)contracts/os/andromeda-kernel/src/execute.rs(6 hunks)packages/andromeda-data-storage/src/string_storage.rs(1 hunks)packages/std/Cargo.toml(1 hunks)packages/std/src/ado_base/rates.rs(1 hunks)packages/std/src/amp/messages.rs(2 hunks)packages/std/src/amp/recipient.rs(1 hunks)packages/std/src/os/kernel.rs(3 hunks)tests-integration/tests/rates_orch.rs(1 hunks)
🔇 Additional comments (12)
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
195-200: Ensure correct handling of CW20 tokens when generating AMP messages
The code correctly generates AMP messages for each recipient with their respective CW20 funds. However, ensure that the generate_amp_msg function properly handles CW20 tokens and that all required validations are in place to prevent any potential security issues.
Run the following script to confirm that generate_amp_msg correctly handles CW20 tokens:
✅ Verification successful
Let me gather more specific information about the generate_amp_msg implementation.
Let me try a different approach to get the implementation.
Let me check how CW20 tokens are handled in the AMP message generation process.
Based on the code review, I can now provide a final response:
CW20 tokens are properly handled in AMP message generation
The implementation correctly handles CW20 tokens through dedicated methods and proper message generation:
- The
generate_msg_cw20method properly handles CW20 tokens by creating either aSendorTransfermessage based on whether additional instructions are included - CW20 token transfers are properly wrapped in
Cw20ExecuteMsg::TransferorCw20ExecuteMsg::Send - The implementation includes proper validation of addresses and amounts
- The code includes test coverage verifying the CW20 message generation functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `generate_amp_msg` supports CW20 tokens
# Search for the `generate_amp_msg` implementation
fd -t f .rs | xargs rg 'fn\s+generate_amp_msg\(' -A 5
# Check for CW20 token handling in the implementation
rg 'Cw20' $(fd -t f .rs)
Length of output: 94685
Script:
#!/bin/bash
# Get the implementation of generate_amp_msg
ast-grep --pattern 'fn generate_amp_msg($$$)' -f packages/std/src/amp/recipient.rs -A 50
Length of output: 268
Script:
#!/bin/bash
# Get the implementation of generate_amp_msg
rg -A 50 "fn\s+generate_amp_msg" packages/std/src/amp/recipient.rs
Length of output: 1780
Script:
#!/bin/bash
# Check how CW20 tokens are handled in AMP messages
rg -A 10 "Cw20" packages/std/src/amp/messages.rs packages/std/src/amp/recipient.rs
Length of output: 7314
contracts/os/andromeda-kernel/src/execute.rs (1)
170-195: Verify proper handling of received CW20 tokens
The handle_receive_cw20 function correctly processes incoming CW20 tokens, but ensure that the validation of the sender and the amount is thorough to prevent misuse.
Run the following script to check that only authorized contracts can send CW20 tokens and that the amount is validated:
✅ Verification successful
Let me gather more information about the CW20 token handling and validation.
CW20 token handling is properly secured with multiple validations
The implementation has robust security measures in place:
- Validates non-zero amount using
ensure!(!amount_sent.is_zero()) - Enforces
nonpayablecheck to prevent native token transfers - Verifies sender address using
query::verify_addressinamp_receive_cw20 - Only allows verified ADOs to access the functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that only allowed CW20 contracts can call `handle_receive_cw20`
# Search for validation of `info.sender`
rg -A3 'fn handle_receive_cw20' contracts/os/andromeda-kernel/src/execute.rs
# Check for authorized sender validation
rg 'query::verify_address' contracts/os/andromeda-kernel/src/execute.rs
Length of output: 455
Script:
#!/bin/bash
# Search for CW20-related validations and the amp_receive_cw20 implementation
rg -A 10 'amp_receive_cw20' contracts/os/andromeda-kernel/src/execute.rs
# Check for any other CW20-related validations
rg -l 'Cw20' contracts/os/andromeda-kernel/src/
# Look for contract address validation patterns
ast-grep --pattern 'nonpayable($info)'
Length of output: 822
contracts/os/andromeda-kernel/Cargo.toml (1)
28-29: LGTM: Dependencies align with CW20 support
The addition of cw20 and cw-utils as workspace dependencies is appropriate for implementing CW20 token handling in the kernel.
packages/std/Cargo.toml (1)
26-26: LGTM: Consistent workspace dependency
The change to use workspace-level cw20 dependency aligns with the project's dependency management strategy.
packages/std/src/os/kernel.rs (2)
11-11: LGTM: CW20 support integration
The import of Cw20ReceiveMsg correctly enables CW20 token handling capabilities.
44-45: LGTM: CW20 receive handler implementation
The Receive(Cw20ReceiveMsg) variant correctly implements the CW20 specification for token reception.
contracts/os/andromeda-kernel/src/contract.rs (2)
13-13: LGTM: CW20 handler import
The import of handle_receive_cw20 is correctly placed and follows the module organization pattern.
100-100: Verify CW20 message handling implementation
The routing to handle_receive_cw20 is correct, but we should verify the implementation of the handler itself.
✅ Verification successful
CW20 message handling implementation verified
The handle_receive_cw20 implementation in the kernel is properly implemented with the following security measures:
- Validates non-zero amount transfers
- Performs nonpayable check
- Handles CW20 tokens through the
AmpReceivehook - Properly tracks the sender and amount information
- Converts received CW20 tokens into a standardized coin representation for internal processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of handle_receive_cw20
ast-grep --pattern 'fn handle_receive_cw20($ctx: ExecuteContext, $msg: Cw20ReceiveMsg) -> Result<Response, ContractError> {
$$$
}'
Length of output: 134
Script:
#!/bin/bash
# Let's try a different pattern to locate the handle_receive_cw20 function
ast-grep --pattern 'fn handle_receive_cw20'
# Also search for the function using ripgrep for better context
rg "handle_receive_cw20" -A 10
Length of output: 13222
Script:
#!/bin/bash
# Let's check the implementation of handle_receive_cw20 in the kernel specifically
rg -A 20 "pub fn handle_receive_cw20" contracts/os/andromeda-kernel/src/execute.rs
Length of output: 692
Script:
#!/bin/bash
# Let's see the complete implementation including the match arms
rg -A 40 "pub fn handle_receive_cw20" contracts/os/andromeda-kernel/src/execute.rs
# Also check what Cw20HookMsg looks like
rg "Cw20HookMsg" contracts/os/andromeda-kernel/src/
Length of output: 1675
contracts/math/andromeda-matrix/src/testing/tests.rs (1)
141-141: LGTM: Simplified string reference
The change from &UNAUTHORIZED_OPERATOR to UNAUTHORIZED_OPERATOR is a valid simplification since String implements AsRef<str>.
tests-integration/tests/rates_orch.rs (1)
129-129: LGTM: Simplified instantiation call
The change simplifies the instantiation by passing rates_init_msg directly instead of by reference, which is a valid improvement as the message is moved into the function call.
packages/std/src/amp/recipient.rs (1)
63-63: LGTM: Improved code clarity with matches! macro
The change from a match statement to the matches! macro is a good improvement that makes the code more idiomatic and concise while maintaining the same functionality.
packages/std/src/ado_base/rates.rs (1)
225-228: LGTM: Improved message generation logic
The conditional structure for message generation has been simplified and properly handles native token routing through direct messages.
contracts/finance/andromeda-set-amount-splitter/src/contract.rs
Outdated
Show resolved
Hide resolved
| pub fn amp_receive_cw20( | ||
| deps: &mut DepsMut, | ||
| info: MessageInfo, | ||
| env: Env, | ||
| packet: AMPPkt, | ||
| received_funds: Vec<Coin>, | ||
| ) -> Result<Response, ContractError> { | ||
| // Only verified ADOs can access this function | ||
| ensure!( | ||
| query::verify_address(deps.as_ref(), info.sender.to_string(),)?.verify_address, | ||
| ContractError::Unauthorized {} | ||
| ); | ||
| ensure!( | ||
| packet.ctx.id == 0, | ||
| ContractError::InvalidPacket { | ||
| error: Some("Packet ID cannot be provided from outside the Kernel".into()) | ||
| } | ||
| ); | ||
|
|
||
| let mut res = Response::default(); | ||
| ensure!( | ||
| !packet.messages.is_empty(), | ||
| ContractError::InvalidPacket { | ||
| error: Some("No messages supplied".to_string()) | ||
| } | ||
| ); | ||
|
|
||
| for (idx, message) in packet.messages.iter().enumerate() { | ||
| let mut handler = MsgHandler::new(message.clone()); | ||
| let msg_res = handler.handle_cw20( | ||
| deps.branch(), | ||
| info.clone(), | ||
| env.clone(), | ||
| Some(packet.clone()), | ||
| idx as u64, | ||
| )?; | ||
| res.messages.extend_from_slice(&msg_res.messages); | ||
| res.attributes.extend_from_slice(&msg_res.attributes); | ||
| res.events.extend_from_slice(&msg_res.events); | ||
| } | ||
|
|
||
| let message_funds = packet | ||
| .messages | ||
| .iter() | ||
| .flat_map(|m| m.funds.clone()) | ||
| .collect::<Vec<Coin>>(); | ||
| ensure!( | ||
| has_coins_merged(received_funds.as_slice(), message_funds.as_slice()), | ||
| ContractError::InsufficientFunds {} | ||
| ); | ||
|
|
||
| Ok(res.add_attribute("action", "handle_amp_packet")) | ||
| } |
There was a problem hiding this comment.
Ensure correct fund matching for AMP CW20 reception
In amp_receive_cw20, confirm that the received funds match the expected amount and denominations specified in the AMP messages to prevent fund mismanagement.
Consider implementing a rigorous funds matching mechanism to compare received_funds with the sum of message_funds.
| config, | ||
| .. | ||
| } = self.message(); | ||
| let recipient_addr = recipient.get_raw_address(&deps.as_ref())?; | ||
| let adodb_addr = KERNEL_ADDRESSES.load(deps.storage, ADO_DB_KEY)?; | ||
| // A default message is a bank message | ||
| if Binary::default() == message.clone() { | ||
| ensure!( | ||
| !funds.is_empty(), | ||
| ContractError::InvalidPacket { | ||
| error: Some("No message or funds supplied".to_string()) | ||
| } | ||
| ); | ||
|
|
||
| let sub_msg = SubMsg::reply_on_error( | ||
| WasmMsg::Execute { | ||
| contract_addr: funds[0].denom.clone(), | ||
| msg: encode_binary(&Cw20ExecuteMsg::Transfer { | ||
| recipient: recipient_addr.to_string(), | ||
| amount: funds[0].amount, | ||
| })?, | ||
| funds: vec![], | ||
| }, | ||
| ReplyId::AMPMsg.repr(), | ||
| ); | ||
|
|
||
| let mut attrs = vec![]; | ||
| for (idx, fund) in funds.iter().enumerate() { | ||
| attrs.push(attr(format!("funds:{sequence}:{idx}"), fund.to_string())); | ||
| } | ||
| attrs.push(attr(format!("recipient:{sequence}"), recipient_addr)); | ||
| res = res.add_submessage(sub_msg).add_attributes(attrs); | ||
| } else { | ||
| let origin = if let Some(amp_ctx) = ctx { | ||
| amp_ctx.get_origin() | ||
| } else { | ||
| info.sender.to_string() | ||
| }; | ||
| let previous_sender = info.sender.to_string(); | ||
| // Ensure recipient is a smart contract | ||
| let ContractInfoResponse { | ||
| code_id: recipient_code_id, | ||
| .. | ||
| } = deps | ||
| .querier | ||
| .query_wasm_contract_info(recipient_addr.clone()) | ||
| .ok() | ||
| .ok_or(ContractError::InvalidPacket { | ||
| error: Some("Recipient is not a contract".to_string()), | ||
| })?; | ||
|
|
||
| let sub_msg = if config.direct | ||
| || AOSQuerier::ado_type_getter(&deps.querier, &adodb_addr, recipient_code_id)? | ||
| .is_none() | ||
| { | ||
| // Message is direct (no AMP Ctx) | ||
| // self.message() | ||
| // .generate_sub_msg_direct(recipient_addr.clone(), ReplyId::AMPMsg.repr()) | ||
| SubMsg { | ||
| id: ReplyId::AMPMsg.repr(), | ||
| reply_on: self.message().config.reply_on.clone(), | ||
| gas_limit: self.message().config.gas_limit, | ||
| msg: CosmosMsg::Wasm(WasmMsg::Execute { | ||
| contract_addr: funds[0].denom.clone(), | ||
| msg: encode_binary(&Cw20ExecuteMsg::Send { | ||
| contract: recipient_addr.to_string(), | ||
| amount: funds[0].amount, | ||
| msg: message.clone(), | ||
| })?, | ||
| funds: vec![], | ||
| }), | ||
| } | ||
| } else { | ||
| let amp_msg = | ||
| AMPMsg::new(recipient_addr.clone(), message.clone(), Some(funds.clone())); | ||
|
|
||
| let new_packet = AMPPkt::new(origin, previous_sender, vec![amp_msg]); | ||
|
|
||
| SubMsg { | ||
| id: ReplyId::AMPMsg.repr(), | ||
| reply_on: self.message().config.reply_on.clone(), | ||
| gas_limit: self.message().config.gas_limit, | ||
| msg: CosmosMsg::Wasm(WasmMsg::Execute { | ||
| contract_addr: funds[0].denom.clone(), | ||
| msg: encode_binary(&Cw20ExecuteMsg::Send { | ||
| contract: recipient_addr.to_string(), | ||
| amount: funds[0].amount, | ||
| msg: encode_binary(&Cw20HookMsg::AmpReceive(new_packet))?, | ||
| })?, | ||
| funds: vec![], | ||
| }), | ||
| } | ||
| }; | ||
|
|
||
| res = res | ||
| .add_submessage(sub_msg) | ||
| .add_attributes(vec![attr(format!("recipient:{sequence}"), recipient_addr)]); | ||
| } | ||
| Ok(res) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor handle_local_cw20 to support multiple funds and improve clarity
The function currently handles only the first coin in funds. To support multiple CW20 tokens or improve clarity, consider refactoring the code.
Example refactor:
- Loop over
fundsto handle multiple CW20 token transfers. - Validate that
fundscontains only one coin if multiple tokens are not supported. - Provide clear error messages for unsupported cases.
if funds.len() != 1 {
return Err(ContractError::InvalidFunds {
msg: "Exactly one CW20 token must be provided".to_string(),
});
}
let coin = &funds[0];
// Proceed with handling `coin` as before…unds, check denoms
…tocol/andromeda-core into cw20-kernel-routing
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
239-244: Optimize Nested Loops to Prevent Potential DoS AttacksThe nested loops in
execute_sendcan be optimized to improve performance and prevent potential Denial-of-Service vulnerabilities:for recipient_addr in splitter_recipients { for (i, coin) in info.funds.clone().iter().enumerate() { // ... } }Consider restructuring the logic or using more efficient data structures, such as hash maps, to reduce the time complexity.
♻️ Duplicate comments (2)
contracts/os/andromeda-kernel/src/execute.rs (1)
738-850:⚠️ Potential issueAvoid Potential Index Out-of-Bounds Panic When Accessing
funds[0]In
handle_local_cw20, the code accessesfunds[0]without prior checks:contract_addr: funds[0].denom.clone(),Ensure that
fundsis not empty before accessing elements to prevent runtime panics.Add the following check before accessing
funds[0]:ensure!( !funds.is_empty(), ContractError::InvalidPacket { error: Some("No funds supplied for CW20 transfer".to_string()) } );packages/std/src/amp/messages.rs (1)
368-374:⚠️ Potential issueAdd bounds check for funds vector.
The code assumes
fundsis non-empty when accessingfunds[0]. Add a length check to prevent panic.+ if funds.is_empty() { + return Err(ContractError::InvalidFunds { + msg: "No CW20 funds provided".to_string(), + }); + } let contract_addr = funds[0].denom.clone();The validation of same denomination for all funds was previously suggested and is correctly implemented here.
🧹 Nitpick comments (6)
contracts/os/andromeda-kernel/src/execute.rs (3)
174-188: Use a More Specific Error Message When Amount Is ZeroThe current check for a zero
amount_sentuses a genericInvalidFundserror:ensure!( !amount_sent.is_zero(), ContractError::InvalidFunds { msg: "Cannot send a 0 amount".to_string() } );Consider using a more specific error variant or message to clearly indicate that zero transfers are not allowed, improving debuggability and user feedback.
257-261: Optimize Verification of Sender AddressIn
amp_receive_cw20, the verification of the sender's address involves querying:ensure!( query::verify_address(deps.as_ref(), info.sender.to_string(),)?.verify_address, ContractError::Unauthorized {} );If
info.senderis already validated earlier or is guaranteed to be a contract address, consider caching the result or optimizing the query to reduce computational overhead.
631-638: Handle Unsupported Protocols GracefullyWhen matching protocols in
handle_cw20, the error message for unsupported protocols is the same for all cases:Some("ibc") => Err(ContractError::NotImplemented { msg: Some("CW20 over IBC not supported".to_string()), }), _ => Err(ContractError::NotImplemented { msg: Some("CW20 over IBC not supported".to_string()), }),Consider distinguishing between different unsupported protocols to provide more informative error messages.
Apply this diff to improve error messages:
_ => Err(ContractError::UnsupportedProtocol { - msg: Some("CW20 over IBC not supported".to_string()), + protocol: protocol.unwrap_or_default(), }),CHANGELOG.md (1)
19-19: Fix Typographical Error in 'Added' SectionThere's a typo in the word "suppport" on line 19:
- Added CW20 suppport in Splitter contracts [(#703)](...)It should be corrected to "support":
- Added CW20 suppport in Splitter contracts [(#703)](...) + Added CW20 support in Splitter contracts [(#703)](...)contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
Line range hint
174-223: Simplify Cloning ofdistro_msgIn
execute_send_cw20, the cloneddistro_msgis immediately pushed into themsgsvector:let distro_msg = pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?; msgs.push(distro_msg.clone());Cloning may be unnecessary here. Instead, push the
distro_msgdirectly without cloning to optimize performance.Apply this diff:
-let distro_msg = pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?; -msgs.push(distro_msg.clone()); +msgs.push(pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?);contracts/finance/andromeda-splitter/src/contract.rs (1)
304-308: Avoid Unnecessary Cloning ofdistro_msgSimilar to the previous file, in
execute_send_cw20, thedistro_msgis cloned before pushing:let distro_msg = pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?; msgs.push(distro_msg.clone());Eliminate the clone to optimize performance.
Apply this diff:
-let distro_msg = pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?; -msgs.push(distro_msg.clone()); +msgs.push(pkt.to_sub_msg_cw20(kernel_address, amp_funds.clone(), 1)?);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)contracts/finance/andromeda-set-amount-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-set-amount-splitter/src/contract.rs(3 hunks)contracts/finance/andromeda-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-splitter/src/contract.rs(3 hunks)contracts/os/andromeda-kernel/Cargo.toml(2 hunks)contracts/os/andromeda-kernel/src/execute.rs(6 hunks)packages/std/src/amp/messages.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/finance/andromeda-splitter/Cargo.toml
- contracts/finance/andromeda-set-amount-splitter/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/os/andromeda-kernel/Cargo.toml
🔇 Additional comments (5)
contracts/os/andromeda-kernel/src/execute.rs (1)
291-299: Correctly Verify Received CW20 Funds Against Expected Funds
The check to verify that received funds match the expected message funds uses has_coins_merged:
ensure!(
has_coins_merged(received_funds.as_slice(), message_funds.as_slice()),
ContractError::InsufficientFunds {}
);Ensure that has_coins_merged correctly handles CW20 tokens and that the comparison is accurate. Since CW20 tokens are represented differently than native coins, consider using a specialized comparison function for CW20 tokens.
Run the following script to confirm that has_coins_merged properly verifies CW20 funds:
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
195-200: Check for Empty amp_funds Before Generating AMP Messages
While amp_funds is being populated, ensure that it's not empty before attempting to create AMP messages. This prevents unnecessary processing when there's no data.
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint 270-308: Clarify Logic for Remaining Funds in CW20 Send
In execute_send_cw20, there is a comment questioning who receives the remaining funds:
// From tests, it looks like owner of smart contract (Andromeda) will receive the rest of funds.
// If so, should be documentedEnsure that the logic for the remaining funds is intentional and properly documented. If the default recipient is intended to receive the remainder, clarify this behavior in the code comments and documentation.
Run the following script to verify who the default recipient is:
packages/std/src/amp/messages.rs (2)
5-5: LGTM! Import changes are appropriate.
The addition of Cw20HookMsg and Uint128 imports aligns with the PR objective of implementing CW20 token handling.
Also applies to: 9-9
362-390: LGTM! CW20 message generation is well-implemented.
The implementation correctly:
- Calculates total amount from multiple coins
- Uses
Cw20ExecuteMsg::Sendfor token transfer - Encodes the AMP message as payload
- Sets empty
fundsvector as CW20 transfers don't use native funds
Let's verify the CW20 message handling implementation across the codebase:
✅ Verification successful
CW20 message handling is consistently implemented across the codebase
The verification confirms:
to_sub_msg_cw20is used correctly in splitter contracts for distributing CW20 tokens- All CW20 hooks consistently use
Cw20HookMsg::AmpReceivepattern for message passing - The kernel properly handles incoming CW20 messages via the same hook type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CW20 message handling implementation
# Check for consistent CW20 message handling patterns
# Look for other CW20 Send implementations
ast-grep --pattern 'Cw20ExecuteMsg::Send { $$$'
# Check for potential places where to_sub_msg_cw20 is called
rg 'to_sub_msg_cw20'
# Look for other CW20 hooks
ast-grep --pattern 'Cw20HookMsg::$_'
Length of output: 805
| let received_funds = vec![Coin::new(amount_sent.u128(), asset_sent)]; | ||
|
|
||
| match from_json(&receive_msg.msg)? { | ||
| Cw20HookMsg::AmpReceive(packet) => { | ||
| amp_receive_cw20(&mut ctx.deps, ctx.info, ctx.env, packet, received_funds) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure Proper Handling of Unsupported Cw20HookMsg Variants
Currently, the match statement only handles Cw20HookMsg::AmpReceive(packet):
match from_json(&receive_msg.msg)? {
Cw20HookMsg::AmpReceive(packet) => {
amp_receive_cw20(&mut ctx.deps, ctx.info, ctx.env, packet, received_funds)
}
}To prevent panics due to unhandled variants, consider adding a wildcard arm or explicitly handling other variants with appropriate error messages.
Apply this diff to handle unsupported variants:
match from_json(&receive_msg.msg)? {
Cw20HookMsg::AmpReceive(packet) => {
amp_receive_cw20(&mut ctx.deps, ctx.info, ctx.env, packet, received_funds)
}
+ _ => Err(ContractError::UnsupportedCw20HookMsg {}),
}| #[inline] | ||
| pub fn handle_cw20( | ||
| &mut self, | ||
| deps: DepsMut, | ||
| info: MessageInfo, | ||
| env: Env, | ||
| ctx: Option<AMPPkt>, | ||
| sequence: u64, | ||
| ) -> Result<Response, ContractError> { | ||
| let resolved_recipient = if self.message().recipient.is_vfs_path() { | ||
| let vfs_address = KERNEL_ADDRESSES.load(deps.storage, VFS_KEY)?; | ||
| vfs_resolve_symlink( | ||
| self.message().recipient.clone(), | ||
| vfs_address.to_string(), | ||
| &deps.querier, | ||
| )? | ||
| } else { | ||
| self.message().recipient.clone() | ||
| }; | ||
| self.update_recipient(resolved_recipient); | ||
| let protocol = self.message().recipient.get_protocol(); | ||
|
|
||
| match protocol { | ||
| None => self.handle_local_cw20(deps, info, env, ctx.map(|ctx| ctx.ctx), sequence), | ||
| Some("ibc") => Err(ContractError::NotImplemented { | ||
| msg: Some("CW20 over IBC not supported".to_string()), | ||
| }), | ||
| _ => Err(ContractError::NotImplemented { | ||
| msg: Some("CW20 over IBC not supported".to_string()), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Add Check for Empty funds Before Accessing Index Zero
In handle_cw20, when handling local CW20 messages, the code accesses funds[0] without checking if funds is non-empty:
contract_addr: funds[0].denom.clone(),To prevent potential panics due to an out-of-bounds access, ensure that funds is not empty before accessing its elements.
Apply this diff to add the necessary check:
+ensure!(
+ !funds.is_empty(),
+ ContractError::InvalidPacket {
+ error: Some("No funds supplied for CW20 transfer".to_string())
+ }
+);
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/os/andromeda-kernel/src/contract.rs (1)
Line range hint
82-133: Consider refactoring the execute functionThe execute function is growing large with many match arms. Consider splitting it into smaller, focused functions grouped by functionality (e.g.,
handle_ownership_messages,handle_token_messages, etc.) to improve maintainability.Example refactor:
fn execute( deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg, ) -> Result<Response, ContractError> { let execute_env = ExecuteContext { deps, env, info, amp_ctx: None, }; match msg { // Token-related messages ExecuteMsg::Receive(msg) => handle_token_messages(execute_env, TokenMsg::Cw20(msg)), ExecuteMsg::Send { message } => handle_token_messages(execute_env, TokenMsg::Send(message)), // Channel-related messages ExecuteMsg::AssignChannels { .. } | ExecuteMsg::TriggerRelay { .. } => { handle_channel_messages(execute_env, msg) }, // ... other message groups // Base messages ExecuteMsg::Ownership(ownership_message) => handle_ownership_messages(execute_env, ownership_message), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)contracts/os/andromeda-kernel/Cargo.toml(1 hunks)contracts/os/andromeda-kernel/src/contract.rs(2 hunks)contracts/os/andromeda-kernel/src/execute.rs(6 hunks)packages/std/Cargo.toml(1 hunks)packages/std/src/amp/messages.rs(2 hunks)packages/std/src/os/kernel.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/std/Cargo.toml
- contracts/os/andromeda-kernel/src/execute.rs
- CHANGELOG.md
- contracts/os/andromeda-kernel/Cargo.toml
- packages/std/src/os/kernel.rs
🔇 Additional comments (3)
packages/std/src/amp/messages.rs (2)
5-5: LGTM: Import change aligns with CW20 support
The addition of Cw20HookMsg to the kernel imports is consistent with the PR objectives for CW20 support.
360-388:
Add validation for empty funds vector and zero amounts
The to_sub_msg_cw20 implementation needs additional validation:
- The function assumes
fundsis non-empty when accessingfunds[0]which could panic - Consider validating that amounts are non-zero to prevent unnecessary transactions
Apply this diff to add the missing validations:
pub fn to_sub_msg_cw20(
&self,
address: impl Into<String>,
funds: Vec<Coin>,
id: u64,
) -> Result<SubMsg, ContractError> {
+ if funds.is_empty() {
+ return Err(ContractError::InvalidFunds {
+ msg: "No CW20 tokens provided".to_string(),
+ });
+ }
let contract_addr = funds[0].denom.clone();
// Verify that all funds are of the same CW20 token by checking if each fund's denom matches the contract address
if !funds.iter().all(|c| c.denom == contract_addr) {
return Err(ContractError::InvalidFunds {
msg: "All funds must be of the same CW20 token".to_string(),
});
}
+ // Verify no zero amounts
+ if funds.iter().any(|c| c.amount.is_zero()) {
+ return Err(ContractError::InvalidFunds {
+ msg: "Zero amounts are not allowed".to_string(),
+ });
+ }
// Collect total amount of funds
let total_amount = funds.iter().map(|c| c.amount).sum::<Uint128>();Let's verify the usage of this function across the codebase to ensure these validations won't break existing code:
✅ Verification successful
Let me check how the amp_funds are constructed in the calling contracts to ensure the validations won't break existing code.
Let me check how the amounts are validated before being added to amp_funds.
Add validation for empty funds vector
The proposed validation for empty funds vector is necessary, but the zero amount validation is redundant since both splitter contracts already validate for non-zero amounts before constructing amp_funds:
- They check incoming funds with
!coin.amount.is_zero() - They verify CW20 transfers with
!amount_sent.is_zero() - They only add coins to
amp_fundsafter checking!amount_owed.is_zero()
Apply this simplified diff that only adds the empty funds check:
pub fn to_sub_msg_cw20(
&self,
address: impl Into<String>,
funds: Vec<Coin>,
id: u64,
) -> Result<SubMsg, ContractError> {
+ if funds.is_empty() {
+ return Err(ContractError::InvalidFunds {
+ msg: "No CW20 tokens provided".to_string(),
+ });
+ }
let contract_addr = funds[0].denom.clone();
// Verify that all funds are of the same CW20 token by checking if each fund's denom matches the contract address
if !funds.iter().all(|c| c.denom == contract_addr) {🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to to_sub_msg_cw20 to verify impact of new validations
rg "to_sub_msg_cw20" -A 3
Length of output: 1020
Script:
#!/bin/bash
# Check the context around amp_funds construction in splitter contracts
rg "amp_funds" -B 5 -A 2 contracts/finance/andromeda-splitter/src/contract.rs contracts/finance/andromeda-set-amount-splitter/src/contract.rs
Length of output: 9170
Script:
#!/bin/bash
# Check amount validation in the splitter contracts
rg "amount" -B 3 -A 3 contracts/finance/andromeda-splitter/src/contract.rs contracts/finance/andromeda-set-amount-splitter/src/contract.rs | rg -B 3 -A 3 "is_zero|checked_sub|checked_mul"
Length of output: 6922
contracts/os/andromeda-kernel/src/contract.rs (1)
13-13: LGTM: Import for CW20 handling
The import is correctly placed and follows the module's organization pattern.
| @@ -96,6 +97,7 @@ pub fn execute( | |||
| execute_env.env, | |||
| packet, | |||
| ), | |||
| ExecuteMsg::Receive(msg) => handle_receive_cw20(execute_env, msg), | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis from previous results, I can now provide the final response:
CW20 token validation needs improvement
The current implementation of handle_receive_cw20 lacks explicit validation of the CW20 token contract address. While it uses the sender address (info.sender) as the asset identifier, there's no whitelist or validation mechanism to ensure only trusted CW20 tokens are processed. Additionally:
- The handler performs basic checks using
nonpayablebut lacks token-specific validation - CW20 over IBC is explicitly rejected with
NotImplementederror - The handler delegates to
amp_receive_cw20which focuses on message handling rather than token validation
Suggested improvements:
- Add a whitelist or registry of allowed CW20 token contracts
- Implement validation of the token contract address before processing
- Consider adding token-specific restrictions or limits
🔗 Analysis chain
Verify CW20 token validation
The new match arm correctly delegates CW20 reception to the dedicated handler. Please ensure that:
- The handler validates the CW20 token contract address
- Proper error handling is in place for failed transfers
Let's verify the CW20 token validation:
Let me gather more information about the CW20 receive handler implementation:
Let me analyze the CW20 token validation one more time by looking at the message types and error handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CW20 token validation in the handler implementation
ast-grep --pattern 'fn handle_receive_cw20($_, $msg) {
$$$
}'
# Look for error handling patterns
rg -A 5 "ContractError::" contracts/os/andromeda-kernel/src/
Length of output: 26678
Script:
#!/bin/bash
# Search for handle_receive_cw20 implementation
rg -A 10 "fn handle_receive_cw20" contracts/os/andromeda-kernel/src/
# Look for CW20 token validation patterns
rg -A 5 "cw20|Cw20" contracts/os/andromeda-kernel/src/
Length of output: 8441
Script:
#!/bin/bash
# Look for CW20 message types and error definitions
rg -A 5 "pub enum (ExecuteMsg|ContractError)" contracts/os/andromeda-kernel/src/
# Look for token validation patterns in tests
rg -A 10 "mock.*cw20" contracts/os/andromeda-kernel/src/testing/
Length of output: 149
Motivation
Closes #704
Implementation
Created Cw20Receive entry point in kernel, made similar functions that handle funds specifically for cw20 funds instead of native funds.
Re-introduced amp_msgs in splitter and set-amount-splitter for cw20, just like it's handled in native send.
Testing
No tests were adjusted and they still pass.
Version Changes
set-amount-splitter:
1.2.0-b.1->1.2.0-b.2splitter:
2.3.0-b.1->2.3.0-b.2kernel:
1.1.1->1.2.0Notes
Some function may be unified to reduce repeated code, will double-check.
Future work
Handle ibc scenarios
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores