feat: add cw20 support for splitter #703
Conversation
…ighted distribution splitter
WalkthroughThe pull request introduces significant updates across multiple components of the project, primarily focusing on enhancing the Splitter contracts with CW20 token support. Key changes include the addition of new features, such as the ability to handle CW20 tokens, the introduction of new ADOs, and various bug fixes. The changelog has been updated to reflect these modifications, including a reformatting of existing entries for consistency. Additionally, several files have been refactored to streamline message handling and improve code readability. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
| for recipient_addr in splitter_recipients { | ||
| let recipient_percent = recipient_addr.percent; | ||
| let mut vec_coin: Vec<Coin> = Vec::new(); | ||
| let coin = coin(amount.u128(), asset.clone()); | ||
| let amount_owed = coin.amount.mul_floor(recipient_percent); | ||
|
|
||
| if !amount_owed.is_zero() { | ||
| let mut recip_coin: Coin = coin.clone(); | ||
| recip_coin.amount = amount_owed; | ||
| remainder_funds.amount = remainder_funds.amount.checked_sub(recip_coin.amount)?; | ||
| vec_coin.push(recip_coin.clone()); | ||
| amp_funds.push(recip_coin.clone()); | ||
| let amp_msg = recipient_addr.recipient.generate_msg_cw20( | ||
| &deps.as_ref(), | ||
| Cw20Coin { | ||
| address: recip_coin.denom.clone(), | ||
| amount: recip_coin.amount, | ||
| }, | ||
| )?; | ||
| msgs.push(amp_msg); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is a fine temporary solution but we should consider adding CW20 support to Kernel routing in the near future.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
contracts/finance/andromeda-splitter/src/contract.rs (2)
128-143: Remove or clarify the commented-out CW20 validation codeLines 128 to 143 contain commented-out code related to CW20 token validation. If this code is no longer necessary, consider removing it to improve code cleanliness. If it's intended for future use or pending implementation, add a comment explaining its purpose and expected activation timeline.
255-316: Refactor duplicated logic inexecute_sendandexecute_send_cw20functionsThe functions
execute_sendandexecute_send_cw20share similar logic for distributing amounts to recipients based on percentages and handling remainders. Consider abstracting the common functionality into a helper function to enhance code maintainability and reduce duplication.contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs (1)
Line range hint
337-363: Remove commented-out code.This block of commented-out code should be removed as it's no longer needed and adds unnecessary noise to the codebase.
📜 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 (9)
CHANGELOG.md(1 hunks)contracts/finance/andromeda-set-amount-splitter/src/contract.rs(2 hunks)contracts/finance/andromeda-splitter/Cargo.toml(2 hunks)contracts/finance/andromeda-splitter/src/contract.rs(6 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs(2 hunks)contracts/fungible-tokens/andromeda-cw20/src/contract.rs(0 hunks)packages/andromeda-finance/Cargo.toml(1 hunks)packages/andromeda-finance/src/splitter.rs(2 hunks)tests-integration/tests/splitter.rs(2 hunks)
💤 Files with no reviewable changes (1)
- contracts/fungible-tokens/andromeda-cw20/src/contract.rs
🔇 Additional comments (13)
tests-integration/tests/splitter.rs (2)
102-200: New test test_splitter_cw20 effectively validates CW20 token splitting
The added test function test_splitter_cw20 thoroughly checks the distribution of CW20 tokens among recipients based on specified percentages. It correctly sets up the CW20 environment, executes the splitting logic, and asserts the resulting balances.
203-305: Additional test test_splitter_cw20_with_remainder properly handles remainder distribution
The new test function test_splitter_cw20_with_remainder extends coverage by testing the splitter's behavior when there's a remainder after distribution. It ensures that the remainder is correctly allocated to the default recipient as intended.
contracts/finance/andromeda-splitter/Cargo.toml (2)
3-3: Version bump to 3.0.0-beta.1 appropriately reflects significant feature addition
Updating the package version to 3.0.0-beta.1 aligns with semantic versioning practices, indicating a backward-incompatible change due to the new CW20 support.
23-23: Addition of cw20 dependency is necessary for CW20 token integration
Including cw20 in the dependencies enables the contract to handle CW20 tokens, which is essential for the new functionality introduced in the splitter contract.
packages/andromeda-finance/Cargo.toml (1)
26-26: Including cw20 dependency supports new CW20 message handling
Adding cw20 to the dependencies allows the package to define and process CW20-specific messages and types, which is required for the updated splitter functionality.
packages/andromeda-finance/src/splitter.rs (2)
52-56: Addition of Cw20HookMsg enum enhances CW20 token handling capabilities
The new Cw20HookMsg enum with the Send variant allows the splitter contract to interpret and process incoming CW20 token messages, facilitating CW20 support.
72-72: Extending ExecuteMsg with Receive(Cw20ReceiveMsg) enables CW20 message execution
Including the Receive variant in the ExecuteMsg enum allows the contract to handle CW20 Receive messages, which is essential for processing CW20 token transfers sent to the splitter.
CHANGELOG.md (1)
12-13: LGTM! Changelog entries are well-formatted and properly documented.
The entries correctly document the new features and follow the Keep a Changelog format with proper PR links.
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (2)
19-20: LGTM! Import cleanup aligns with the code changes.
The imports have been streamlined by removing unused imports, making the code cleaner.
263-265: LGTM! Improved message generation for remainder funds.
Using generate_direct_msg instead of direct message construction provides better abstraction and consistency in handling different token types (native and CW20).
contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs (3)
20-21: LGTM! Import cleanup aligns with the code changes.
The imports have been streamlined by removing unused imports, making the code cleaner.
331-333: LGTM! Improved message generation for remainder funds.
Using generate_direct_msg instead of direct message construction provides better abstraction and consistency in handling different token types (native and CW20).
Line range hint 1-1: Verify CW20 message handling implementation.
The changes prepare for CW20 support by refactoring message generation, but let's verify the complete implementation.
✅ Verification successful
Let me try a different search strategy to verify the CW20 implementation.
Let me try one more search focusing specifically on the splitter contract's implementation:
Let me check the ExecuteMsg definition in the package to verify CW20 message handling:
Let me check one more thing to verify the CW20 message handling implementation:
Based on the verification results, I can now provide the final response:
CW20 support is properly implemented in the splitter contract.
The implementation includes:
Receive(Cw20ReceiveMsg)in ExecuteMsg enum for handling CW20 token receiptsCw20HookMsgenum withSendvariant for CW20 token distribution- Proper imports of CW20 types (
use cw20::Cw20ReceiveMsg)
The changes align with the PR objectives to add CW20 support, with the message handling properly structured to support both native and CW20 token distribution through the splitter contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CW20 message handling implementation
# Search for CW20 related message handling
echo "Searching for CW20 message handlers..."
ast-grep --pattern 'fn execute_send_cw20($$$) {
$$$
}'
# Search for CW20 receive implementation
echo "Searching for CW20 receive implementation..."
ast-grep --pattern 'fn execute($$$) {
$$$
ExecuteMsg::Receive($$$) => {
$$$
}
$$$
}'
Length of output: 343
Script:
#!/bin/bash
# Search for CW20 related code more broadly
# Look for CW20 imports and types
echo "Searching for CW20 related imports and types..."
rg "cw20" -i
# Look for Receive message in ExecuteMsg enum
echo "Searching for Receive message definition..."
ast-grep --pattern 'enum ExecuteMsg {
$$$
Receive($$$)
$$$
}'
# Look for CW20 in message handling code
echo "Looking for CW20 message handling code..."
rg "cw20" -i -A 5 "execute"
Length of output: 66116
Script:
#!/bin/bash
# Search for CW20 implementation in splitter contract
# Look for ExecuteMsg definition
echo "Searching for ExecuteMsg definition in splitter contract..."
rg -A 10 "ExecuteMsg" contracts/finance/andromeda-weighted-distribution-splitter/src/
# Look for execute function implementation
echo "Searching for execute function implementation..."
rg -A 20 "fn execute" contracts/finance/andromeda-weighted-distribution-splitter/src/
# Look for CW20 receive implementation
echo "Searching for CW20 receive implementation..."
rg -A 10 "Receive|cw20" contracts/finance/andromeda-weighted-distribution-splitter/src/
Length of output: 76048
Script:
#!/bin/bash
# Look for ExecuteMsg definition in the package
echo "Searching for ExecuteMsg definition in andromeda-finance package..."
rg -A 10 "pub enum ExecuteMsg" packages/andromeda-finance/src/splitter.rs
echo "Searching for CW20 related imports in splitter..."
rg "use.*cw20" packages/andromeda-finance/src/splitter.rs
Length of output: 880
Script:
#!/bin/bash
# Search for Receive message and CW20 hook implementation
echo "Searching for Receive message in ExecuteMsg..."
rg -B 5 -A 10 "Receive.*Cw20ReceiveMsg" packages/andromeda-finance/src/splitter.rs
echo "Searching for Cw20HookMsg definition..."
rg -A 5 "pub enum Cw20HookMsg" packages/andromeda-finance/src/splitter.rs
Length of output: 889
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
contracts/finance/andromeda-set-amount-splitter/Cargo.toml (1)
Version inconsistency detected across splitter contracts
Based on the search results, I can see that there are different versions across the splitter-related contracts:
- andromeda-splitter: 3.0.0-b.1
- andromeda-set-amount-splitter: 1.1.0-beta
- andromeda-weighted-distribution-splitter: 2.1.0-beta
- andromeda-conditional-splitter: 1.2.3-beta
The version update to 3.0.0 has been applied to the main splitter contract but not to the set-amount-splitter, which suggests this package also needs to be updated to maintain version consistency across the splitter family of contracts.
🔗 Analysis chain
Line range hint
2-3: Version update needed according to PR objectivesThe PR objectives mention updating the splitter version to 3.0.0, but the package version is still at 1.1.0-beta. This appears to be inconsistent with the stated objectives.
Let's verify the version changes across related packages:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check version specifications across splitter-related packages # Expected: Find references to version updates to verify the intended version change # Search for version specifications in related Cargo.toml files echo "Searching for version specifications in splitter packages:" fd -e toml Cargo.toml -x grep -l "version" {} \; -x grep "version" {} # Search for version-related changes in commit messages echo -e "\nSearching for version-related changes in recent commits:" git log --oneline -n 10 | grep -i "version"Length of output: 9788
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
354-356: Update the error handling for native token refunds.Ensure that the
remainder_recipient.generate_direct_msghandles the possibility of zero amounts. Ifremainder_fundsis zero, consider skipping the refund message to avoid unnecessary transactions.Apply this change:
if !remainder_funds.is_zero() { let remainder_recipient = splitter .default_recipient .clone() .unwrap_or(Recipient::new(info.sender.to_string(), None)); let native_msg = remainder_recipient .generate_direct_msg(&deps.as_ref(), coins(remainder_funds.u128(), denom))?; msgs.push(native_msg); +} else { + // No refund necessary as there are no remaining funds }packages/andromeda-finance/src/set_amount_splitter.rs (1)
62-78: UpdateExecuteMsgto include CW20 receive handling and improve readability.
- The addition of the
Receive(Cw20ReceiveMsg)variant enables the contract to handle CW20Receivemessages, which is necessary for CW20 token interactions.- Reformatting the parameters in
UpdateRecipients,UpdateDefaultRecipient, andSendvariants improves code readability.Consider adding documentation comments for the new
Receivevariant to maintain consistency and clarity.Apply this diff to add documentation:
/// Update the recipients list. Only executable by the contract owner when the contract is not locked. UpdateRecipients { recipients: Vec<AddressAmount>, }, /// Used to lock/unlock the contract allowing the config to be updated. UpdateLock { // Milliseconds from current time lock_time: Expiry, }, /// Update the default recipient. Only executable by the contract owner when the contract is not locked. UpdateDefaultRecipient { recipient: Option<Recipient>, }, +/// Handles receiving CW20 tokens. Receive(Cw20ReceiveMsg), /// Divides any attached funds to the message amongst the recipients list. Send { config: Option<Vec<AddressAmount>>, },
📜 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 (6)
contracts/finance/andromeda-set-amount-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-set-amount-splitter/src/contract.rs(5 hunks)contracts/finance/andromeda-splitter/Cargo.toml(2 hunks)contracts/modules/andromeda-distance/Cargo.toml(1 hunks)contracts/os/andromeda-adodb/src/tests.rs(1 hunks)packages/andromeda-finance/src/set_amount_splitter.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/modules/andromeda-distance/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/finance/andromeda-splitter/Cargo.toml
🔇 Additional comments (5)
contracts/finance/andromeda-set-amount-splitter/Cargo.toml (1)
23-23: LGTM: CW20 dependency addition is appropriate
The addition of the cw20 dependency with workspace-level version management aligns well with the PR objective of adding CW20 support to the splitter functionality.
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
133-133: Ensure proper handling of the new Receive variant.
The addition of ExecuteMsg::Receive in the match statement enhances the contract to handle CW20 token receptions. Verify that the handle_receive_cw20 function correctly processes CW20 receive messages and integrates seamlessly with the existing execution flow.
packages/andromeda-finance/src/set_amount_splitter.rs (2)
11-11: Import Cw20ReceiveMsg for CW20 token support.
The addition of use cw20::Cw20ReceiveMsg; is appropriate for handling CW20 receive messages in the contract.
53-57: Define Cw20HookMsg for CW20 token handling.
The introduction of the Cw20HookMsg enum with the Send variant allows the contract to process incoming CW20 tokens effectively. Ensure that the Send variant aligns with the expected structure of CW20 hook messages.
contracts/os/andromeda-adodb/src/tests.rs (1)
155-155: Ensure consistent version formatting in test cases.
The change from "0.1.0-beta.1" to "0.1.0-b.1" in the ado_version variable may affect version parsing. Verify that this new version format is compliant with semantic versioning and that all parts of the system correctly handle pre-release versions.
Run the following script to check for compliance with semantic versioning:
Additionally, ensure that other parts of the codebase and any external tools used for versioning and deployment can handle this version format.
✅ Verification successful
Let me search for version-related code to understand how versions are handled in the codebase.
Based on the code analysis, I can now provide a final response:
Version format change is consistent with codebase usage
The change from "0.1.0-beta.1" to "0.1.0-b.1" is valid as:
- The
ADOVersionimplementation validates versions usingVersion::from_str()from the semver crate - Both formats are used in the codebase (e.g.,
"0.1.0-beta"and"0.1.0-b.1"in Cargo.lock) - The version string is part of a test case and doesn't affect production code
- The version validation in
ADOVersion::validate()only checks if the version can be parsed by the semver crate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the version string "0.1.0-b.1" complies with semantic versioning.
# Test: Use a regex to match semantic versioning with pre-release identifiers.
version="0.1.0-b.1"
if [[ $version =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z\-\.]+)?$ ]]; then
echo "Version string is valid."
else
echo "Version string is invalid."
fi
Length of output: 146
Script:
#!/bin/bash
# Search for ADOVersion implementation and version parsing logic
ast-grep --pattern 'impl ADOVersion'
# Search for version-related tests
rg "ADOVersion.*version" -A 5
# Search for other occurrences of version strings
rg "0\.1\.0-[a-z]"
Length of output: 17481
contracts/finance/andromeda-set-amount-splitter/src/contract.rs
Outdated
Show resolved
Hide resolved
contracts/finance/andromeda-set-amount-splitter/src/contract.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
contracts/finance/andromeda-set-amount-splitter/src/mock.rs (2)
88-92: Consider adding CW20-specific mock utilities.While the implementation is correct, consider adding mock utilities for CW20 testing scenarios to support the new CW20 functionality mentioned in the PR objectives. This could include:
- Mock CW20 token contract
- Helper functions for CW20 Receive messages
- Mock execution methods for CW20 transfers
Example addition:
pub fn mock_set_amount_splitter_receive_cw20_msg( sender: String, amount: Uint128, msg: Binary, ) -> ExecuteMsg { ExecuteMsg::Receive(Cw20ReceiveMsg { sender, amount, msg, }) }
Mock implementation needs CW20 support added
The mock contract is missing CW20-related functionality that exists in the main contract:
- The main contract implements
ExecuteMsg::Receiveandhandle_receive_cw20- CW20 hook messages are supported via
Cw20HookMsg::Send- The mock should be extended to support testing CW20 scenarios
Required additions to the mock:
- Add
execute_receive_cw20method toMockSetAmountSplitter- Include CW20 message handling in the mock implementation
- Add helper functions for CW20-specific test scenarios
🔗 Analysis chain
Line range hint
1-92: Verify alignment with CW20 implementation.Let's verify that the mock contract changes align with the CW20 implementation in the main contract.
Let me gather more information about the CW20 implementation to ensure complete verification.
Let me check the actual CW20 handling implementation and test scenarios to make a final determination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the mock implementation matches the main contract's CW20 functionality # Check for CW20 message handling in the main contract echo "Checking main contract for CW20 message handling..." rg -A 5 "ExecuteMsg::Receive" contracts/finance/andromeda-set-amount-splitter/src/ # Check for CW20 tests using this mock echo "Checking for CW20 tests using this mock..." rg -A 5 "MockSetAmountSplitter" tests-integration/tests/Length of output: 2927
Script:
#!/bin/bash # Check for CW20 related messages in the contract's message definitions echo "Checking ExecuteMsg definition for CW20..." rg -p "ExecuteMsg" contracts/finance/andromeda-set-amount-splitter/src/ # Check for CW20 handling functions echo "Checking CW20 handling functions..." rg -p "handle_receive_cw20" contracts/finance/andromeda-set-amount-splitter/src/ # Check mock contract's test coverage echo "Checking test coverage for CW20 scenarios..." rg -p "test.*cw20" tests-integration/tests/set_amount_splitter.rsLength of output: 2793
Script:
#!/bin/bash # Check CW20 handling implementation echo "Checking CW20 handling implementation..." rg -A 10 "handle_receive_cw20" contracts/finance/andromeda-set-amount-splitter/src/contract.rs # Check CW20 test scenarios echo "Checking CW20 test scenarios..." rg -A 10 "fn test.*" tests-integration/tests/set_amount_splitter.rs # Check CW20HookMsg implementation echo "Checking CW20HookMsg implementation..." rg -A 5 "Cw20HookMsg" contracts/finance/andromeda-set-amount-splitter/src/Length of output: 3035
tests-integration/tests/set_amount_splitter.rs (2)
30-47: Consider expanding wallet fixtures for edge casesWhile the current fixtures are well-structured, consider adding test cases for:
- Wallets with multiple token types
- Wallets with zero balances for specific tokens
- Maximum balance scenarios
184-255: Remove commented out test codeInstead of keeping the old test commented out:
- Extract the valuable assertions and validation logic to the new test implementation
- Remove the commented code to maintain codebase cleanliness
Key validations to preserve:
assert_eq!(balance_1.amount, Uint128::from(100u128)); assert_eq!(balance_2.amount, Uint128::from(50u128)); assert_eq!(balance_owner.amount, Uint128::from(850u128));
📜 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)
contracts/finance/andromeda-set-amount-splitter/src/mock.rs(2 hunks)tests-integration/tests/set_amount_splitter.rs(1 hunks)
🔇 Additional comments (2)
contracts/finance/andromeda-set-amount-splitter/src/mock.rs (1)
50-60: LGTM! Well-structured implementation.
The new execute_update_recipients method follows the established patterns in the mock contract and maintains consistency with other execute methods.
tests-integration/tests/set_amount_splitter.rs (1)
1-28: Well-structured test organization!
The introduction of the TestCase struct and the organized imports provide a clean foundation for testing both native and CW20 token scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests-integration/tests/set_amount_splitter.rs (4)
23-159: LGTM! Consider adding documentation for TestCase struct.The test setup is well-structured with proper separation of concerns using fixtures. The setup handles both native and CW20 tokens comprehensively.
Consider adding documentation for the
TestCasestruct to explain its purpose and fields:/// TestCase encapsulates the test environment components /// /// # Fields /// * `router` - Mock blockchain router /// * `andr` - Mock Andromeda environment /// * `splitter` - Mock Set Amount Splitter contract /// * `cw20` - Mock CW20 token contract
161-192: Add test cases for error scenarios.While the happy path is well tested, consider adding test cases for:
- Insufficient funds
- Zero amount transfers
- Invalid recipient configurations
Example test case:
#[rstest] fn test_insufficient_funds_native(setup: TestCase) { let TestCase { mut router, andr, splitter, .. } = setup; let owner = andr.get_wallet("owner"); // Send less than required amount let result = splitter .execute_send(&mut router, owner.clone(), &[coin(50, "uandr")], None); assert!(matches!(result, Err(ContractError::InsufficientFunds {}))); }
194-222: Add event validation to CW20 tests.The test verifies balances correctly but should also validate events and attributes to ensure proper tracking.
Add assertions for response attributes:
let response = cw20.execute_send( &mut router, owner.clone(), splitter.addr(), Uint128::new(1000), &hook_msg, )?; assert_eq!( response.attributes, vec![ attr("action", "cw20_send"), attr("sender", owner.to_string()), ] );
254-325: Remove commented out test code.The old test implementation has been replaced by new test cases. Keeping commented out code reduces maintainability.
Remove the commented out test to improve code cleanliness.
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
1-319: Consider extracting common CW20 handling logic.Both splitter contracts share similar CW20 handling code. Consider extracting this into a common module or trait to reduce duplication and improve maintainability.
Create a common trait or module:
mod cw20_handler { pub trait Cw20Handler { fn handle_receive_cw20(&self, ctx: ExecuteContext, msg: Cw20ReceiveMsg) -> Result<Response, ContractError>; fn execute_send_cw20(&self, ctx: ExecuteContext, ...) -> Result<Response, ContractError>; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
contracts/finance/andromeda-set-amount-splitter/src/contract.rs(5 hunks)contracts/finance/andromeda-splitter/src/contract.rs(6 hunks)tests-integration/tests/set_amount_splitter.rs(1 hunks)
🔇 Additional comments (4)
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (2)
361-363: LGTM! Proper handling of native token remainder.
The changes correctly use generate_direct_msg for handling native token remainders.
143-166:
Add CW20 token contract validation.
The function should validate that the CW20 token contract is trusted before processing the transfer.
Add token validation to prevent processing transfers from untrusted CW20 contracts:
pub fn handle_receive_cw20(
ctx: ExecuteContext,
receive_msg: Cw20ReceiveMsg,
) -> Result<Response, ContractError> {
let ExecuteContext { ref info, .. } = ctx;
nonpayable(info)?;
+ // Validate CW20 token contract
+ ensure!(
+ is_trusted_token(deps.as_ref(), info.sender.as_str())?,
+ ContractError::UntrustedToken {
+ address: info.sender.to_string(),
+ }
+ );
+
let asset_sent = info.sender.clone().into_string();
let amount_sent = receive_msg.amount;Likely invalid or redundant comment.
contracts/finance/andromeda-splitter/src/contract.rs (2)
240-242: LGTM! Proper handling of native token remainder.
The changes correctly use generate_direct_msg for handling native token remainders.
257-319: 🛠️ Refactor suggestion
Refactor CW20 token handling to avoid using Coin type.
Using the native Coin type for CW20 tokens is confusing and error-prone.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests-integration/tests/set_amount_splitter.rs (2)
48-159: Consider extracting CW20-specific setup into a separate function.The setup function handles both native and CW20 token scenarios. Consider splitting it for better maintainability.
+fn setup_cw20_components( + router: &mut MockApp, + andr: &MockAndromeda, + owner: Addr, +) -> (AppComponent, MockCW20) { + let initial_balances = vec![Cw20Coin { + address: owner.to_string(), + amount: Uint128::from(1_000_000u128), + }]; + + let cw20_init_msg = mock_cw20_instantiate_msg( + None, + "Test Tokens".to_string(), + "TTT".to_string(), + 6, + initial_balances, + Some(mock_minter(owner.to_string(), Some(Uint128::from(1000000u128)))), + andr.kernel.addr().to_string(), + ); + + let cw20_component = AppComponent::new( + "cw20".to_string(), + "cw20".to_string(), + to_json_binary(&cw20_init_msg).unwrap(), + ); + + (cw20_component, cw20) +}
205-214: Consider adding error cases for CW20 operations.While the happy path is well tested, consider adding test cases for:
- Insufficient CW20 balance
- Invalid hook message
- Unauthorized sender
Example test case:
#[rstest] fn test_insufficient_cw20_balance(#[with(false)] setup: TestCase) { let TestCase { mut router, andr, splitter, cw20, } = setup; let owner = andr.get_wallet("owner"); let hook_msg = Cw20HookMsg::Send { config: None }; // Try to send more than available balance let result = cw20.execute_send( &mut router, owner.clone(), splitter.addr(), Uint128::new(2_000_000), // More than initial balance &hook_msg, ); assert!(result.is_err()); }Also applies to: 235-244
tests-integration/tests/splitter.rs (2)
67-73: Consider using decimal string literals for better readabilityThe percentage calculations using
Decimal::from_ratiocould be simplified usingDecimal::from_str.-percent: Decimal::from_ratio(Uint128::from(2u128), Uint128::from(10u128)), +percent: Decimal::from_str("0.2").unwrap(), -percent: Decimal::from_ratio(Uint128::from(8u128), Uint128::from(10u128)), +percent: Decimal::from_str("0.8").unwrap(),
240-241: Document the purpose of empty config in hook messageConsider adding a comment explaining why the config is set to None in the CW20 hook message.
+// Using default configuration for token distribution let hook_msg = Cw20HookMsg::Send { config: None };Also applies to: 285-286
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
contracts/finance/andromeda-splitter/src/mock.rs(2 hunks)tests-integration/tests/set_amount_splitter.rs(1 hunks)tests-integration/tests/splitter.rs(1 hunks)
🔇 Additional comments (10)
contracts/finance/andromeda-splitter/src/mock.rs (2)
50-60: LGTM! The execute_update_recipients implementation follows the established pattern.
The method correctly handles the update recipients functionality by:
- Taking appropriate parameters (app, sender, funds, recipients)
- Using the mock message constructor
- Delegating to the base execute method
88-90: LGTM! The mock message constructor is properly implemented.
The function correctly creates an ExecuteMsg::UpdateRecipients variant with the provided recipients.
tests-integration/tests/set_amount_splitter.rs (4)
23-28: LGTM! Well-structured test case infrastructure.
The TestCase struct properly encapsulates all necessary components for testing:
- MockApp for routing
- MockAndromeda for core functionality
- MockSetAmountSplitter for specific contract testing
- MockCW20 for token operations
30-46: LGTM! Clean fixture implementation for test dependencies.
The fixtures provide:
- Wallet setup with appropriate initial balances
- Contract setup including CW20 and splitter contracts
161-192: LGTM! Comprehensive native token test case.
The test properly verifies:
- Successful token transfer
- Correct recipient balances
194-222: LGTM! Well-structured CW20 test cases covering key scenarios.
The tests effectively verify:
- CW20 token splitting with remainder
- CW20 token splitting without remainder
- Proper balance updates for all parties
Both test cases follow best practices:
- Clear setup and execution
- Comprehensive balance verification
- Edge case coverage (with/without remainder)
Also applies to: 224-251
tests-integration/tests/splitter.rs (4)
22-138: Well-structured test setup using fixtures!
The test setup is well-organized using rstest fixtures, properly initializing both native and CW20 tokens, and following testing best practices.
140-227: Consider parameterizing similar test cases
The native token test cases share similar structure and could be parameterized using rstest's parameter feature to reduce code duplication.
Example refactor:
#[rstest]
#[case::without_remainder(
vec![
AddressPercent {
recipient: "recipient1",
percent: Decimal::from_str("0.2").unwrap(),
},
AddressPercent {
recipient: "recipient2",
percent: Decimal::from_str("0.8").unwrap(),
},
],
1000,
vec![(200, "recipient1"), (800, "recipient2")]
)]
#[case::with_remainder(
vec![
AddressPercent {
recipient: "recipient1",
percent: Decimal::from_str("0.1").unwrap(),
},
AddressPercent {
recipient: "recipient2",
percent: Decimal::from_str("0.1").unwrap(),
},
],
1000,
vec![(100, "recipient1"), (100, "recipient2")]
)]
fn test_set_amount_splitter_native(
setup: TestCase,
#[case] recipients: Vec<AddressPercent>,
#[case] amount: u128,
#[case] expected_balances: Vec<(u128, &str)>
) {
// Test implementation
}229-301: Comprehensive CW20 integration tests!
The CW20 test cases properly verify token distribution and cover both scenarios with and without remainder. The implementation follows the same pattern as native token tests, ensuring consistent behavior across token types.
1-301: Tests align well with PR objectives
The integration tests thoroughly cover the CW20 support requirements from issue #681:
- Successfully tests CW20 token splitting functionality
- Verifies correct token distribution
- Maintains parity with native token functionality
Let's verify the CW20 message handling implementation:
✅ Verification successful
CW20 integration is properly implemented and tested
The integration tests accurately reflect the contract's implementation:
- The contract implements proper CW20 receive handlers in both splitter contracts
- The
Cw20HookMsgis correctly defined in the finance package - Tests cover both the sending and receiving of CW20 tokens with correct distribution logic
- Implementation maintains consistency between native and CW20 token handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CW20 message handling implementation in the splitter contract
# Check for Cw20HookMsg implementation
ast-grep --pattern 'impl Cw20HookMsg {
$$$
}'
# Check for CW20 receive handler
ast-grep --pattern 'fn handle_receive_cw20($$$) {
$$$
}'
# Check for CW20 send implementation
ast-grep --pattern 'fn execute_send_cw20($$$) {
$$$
}'
Length of output: 169
Script:
#!/bin/bash
# Let's try a broader search for CW20-related implementations
# Search for CW20 receive implementations
rg -l "receive_cw20" --type rust
# Search for Cw20HookMsg definitions and implementations
rg -l "Cw20HookMsg" --type rust
# Search for CW20 execute functions
rg "fn.*cw20.*execute" --type rust -A 2
Length of output: 3233
Script:
#!/bin/bash
# Let's examine the splitter contract implementation specifically
# Check the splitter contract implementation
rg -A 10 "receive_cw20" contracts/finance/andromeda-splitter/src/contract.rs
# Check the set amount splitter contract implementation
rg -A 10 "receive_cw20" contracts/finance/andromeda-set-amount-splitter/src/contract.rs
# Check the Cw20HookMsg definition in the finance package
rg -A 5 "Cw20HookMsg" packages/andromeda-finance/src/splitter.rs
Length of output: 1700
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
13-13: Fix typo in "support"There's a typo in the word "suppport" (extra 'p').
-Added CW20 suppport in Splitter contracts [(#703)](https://github.com/andromedaprotocol/andromeda-core/pull/703) +Added CW20 support in Splitter contracts [(#703)](https://github.com/andromedaprotocol/andromeda-core/pull/703)
📜 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)
CHANGELOG.md(1 hunks)contracts/finance/andromeda-set-amount-splitter/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/finance/andromeda-set-amount-splitter/Cargo.toml
Motivation
Closes #681
Implementation
Created new ExecuteMsg:
which after a non-zero check calls this:
execute_send_cw20is almost likeexecute_sendbut doesn't send any amp packets, I was having trouble with them so I removed them.Testing
test_splitter_cw20andtest_splitter_cw20_with_remainderintegration testsVersion Changes
splitter:2.2.0-beta.1->3.0.0-b.1set-amount-splitter:1.1.0-beta->2.0.0-b.1Notes
Will apply cw20 capabilities to any other requested splitter ADO, just need to have confirmation on the current implementation since it will be identical to the rest.
Future work
None
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests