feat: Splitter Send Config #686
Conversation
|
Warning Rate limit exceeded@crnbarr93 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request includes updates to several 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 and nitpick comments (6)
contracts/finance/andromeda-weighted-distribution-splitter/src/testing/mock_querier.rs (2)
17-46: Consider enhancing mock_dependencies_custom reusabilityWhile the implementation is solid, consider these improvements:
- Make the
ado_typeconfigurable instead of hardcoding "splitter"- Add error handling for the instantiation result instead of unwrap()
Here's a suggested improvement:
pub fn mock_dependencies_custom( contract_balance: &[Coin], + ado_type: Option<String>, ) -> OwnedDeps<MockStorage, MockApi, WasmMockQuerier> { // ... existing setup code ... mock_info("sender", &[]), InstantiateMsg { - ado_type: "splitter".to_string(), + ado_type: ado_type.unwrap_or_else(|| "splitter".to_string()), ado_version: "test".to_string(), kernel_address: MOCK_KERNEL_CONTRACT.to_string(), owner: None, }, ) - .unwrap(); + .map_err(|e| panic!("Failed to instantiate ADOContract: {}", e))?; deps }
70-91: Clean up unused variables and magic numbersA few suggestions for improvement:
- Remove the unused
contract_addrbinding inhandle_query- Either document or remove the magic number
2fortokens_left_to_burnHere's the suggested cleanup:
match &request { QueryRequest::Wasm(WasmQuery::Smart { contract_addr, msg: _, }) => { - let _ = contract_addr.as_str(); MockAndromedaQuerier::default().handle_query(&self.base, request) } _ => MockAndromedaQuerier::default().handle_query(&self.base, request), }contracts/finance/andromeda-set-amount-splitter/src/mock.rs (1)
32-40: Add documentation for the new config parameterThe implementation correctly adds the optional config parameter, but it would be helpful to document its purpose and usage.
Add documentation above the function:
+ /// Executes a send message with optional recipient configuration + /// + /// # Arguments + /// * `app` - Mutable reference to MockApp + /// * `sender` - Address of the sender + /// * `funds` - Slice of Coin to be sent + /// * `config` - Optional vector of AddressAmount for custom recipient configurationpackages/andromeda-finance/src/weighted_splitter.rs (1)
47-47: Update documentation for Send variantThe Send variant now supports dynamic configuration, but this isn't reflected in the documentation.
Update the documentation comment:
- /// Divides any attached funds to the message amongst the recipients list. + /// Divides any attached funds to the message amongst the recipients list. + /// If config is provided, uses the custom recipient configuration instead of the stored recipients. + /// # Arguments + /// * `config` - Optional vector of AddressWeight that overrides the stored recipient configurationpackages/andromeda-finance/src/splitter.rs (1)
56-56: LGTM! Consider adding documentation for the new parameter.The addition of the optional config parameter aligns well with the PR objectives and maintains backward compatibility. The implementation leverages existing validation logic.
Consider adding documentation for the
configparameter:/// Divides any attached funds to the message amongst the recipients list. + /// @param config Optional recipient configuration to override the stored recipients list Send { config: Option<Vec<AddressPercent>> },contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
158-177: Clarify remainder funds handling in commentsThe comments about remaining funds going to the sender need clarification.
Update the comments to clearly explain the business logic:
-// Why does the remaining funds go the the sender of the executor of the splitter? -// Is it considered tax(fee) or mistake? -// Discussion around caller of splitter function in andromedaSPLITTER smart contract. -// From tests, it looks like owner of smart contract (Andromeda) will recieve the rest of funds. -// If so, should be documented +// Any remaining funds (dust) after distribution are returned to the message sender +// This handles rounding differences and ensures no funds are locked in the contract
📜 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 (24)
contracts/finance/andromeda-set-amount-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-set-amount-splitter/src/contract.rs(4 hunks)contracts/finance/andromeda-set-amount-splitter/src/mock.rs(2 hunks)contracts/finance/andromeda-set-amount-splitter/src/testing/tests.rs(7 hunks)contracts/finance/andromeda-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-splitter/src/contract.rs(4 hunks)contracts/finance/andromeda-splitter/src/mock.rs(2 hunks)contracts/finance/andromeda-splitter/src/testing/tests.rs(7 hunks)contracts/finance/andromeda-weighted-distribution-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs(3 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/testing/mock_querier.rs(1 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/testing/mod.rs(1 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs(3 hunks)ibc-tests/tests/crowdfund.rs(1 hunks)packages/andromeda-finance/src/set_amount_splitter.rs(1 hunks)packages/andromeda-finance/src/splitter.rs(1 hunks)packages/andromeda-finance/src/weighted_splitter.rs(1 hunks)tests-integration/tests/auction_app.rs(1 hunks)tests-integration/tests/crowdfund_app.rs(1 hunks)tests-integration/tests/kernel.rs(1 hunks)tests-integration/tests/kernel_orch.rs(1 hunks)tests-integration/tests/marketplace_app.rs(1 hunks)tests-integration/tests/set_amount_splitter.rs(1 hunks)tests-integration/tests/splitter.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- contracts/finance/andromeda-weighted-distribution-splitter/src/testing/mod.rs
- contracts/finance/andromeda-splitter/Cargo.toml
- contracts/finance/andromeda-weighted-distribution-splitter/Cargo.toml
🔇 Additional comments (29)
contracts/finance/andromeda-weighted-distribution-splitter/src/testing/mock_querier.rs (3)
1-12: LGTM: Imports are well-organized and complete
The imports cover all necessary dependencies for mock testing functionality.
54-68: LGTM: Solid Querier implementation
The implementation includes proper error handling and follows the standard Querier trait pattern.
48-52: Verify the purpose of tokens_left_to_burn field
The tokens_left_to_burn field appears to be unused in the current implementation. If it's not needed, consider removing it to avoid confusion.
contracts/finance/andromeda-set-amount-splitter/src/testing/tests.rs (7)
179-187: Code update correctly handles the new config parameter
The initialization of recip3 and config_recipient appropriately reflects the addition of the optional config parameter in ExecuteMsg::Send. This allows the test to verify sending funds with a custom configuration.
198-198: ExecuteMsg::Send message updated with config: None
The msg variable is correctly updated to include config: None, ensuring that the default configuration is used when no custom configuration is provided.
Line range hint 239-256: Test executes Send without custom configuration successfully
The test validates that when config is None, the contract uses the state-stored recipient configuration. The expected response is accurately constructed and compared.
257-305: Test executes Send with custom configuration successfully
The test correctly constructs a custom config and verifies that the contract processes it appropriately. The steps include generating AMP messages, constructing the expected response, and asserting equality with the actual response.
334-334: ExecuteMsg::Send used with config: None in test
The message is properly constructed with config: None to test the default configuration scenario.
408-408: Serialization of ExecuteMsg::Send with config: None is correct
The to_json_binary function correctly serializes the ExecuteMsg::Send { config: None } message, ensuring that the packet contains the proper execution message.
484-484: ExecuteMsg::Send message correctly uses config: None
The test case accurately sets up the message to test sending funds without a custom configuration.
tests-integration/tests/marketplace_app.rs (1)
389-389: Updated function call to mock_splitter_send_msg(None)
The call to mock_splitter_send_msg now includes None as an argument, aligning with the updated function signature that accepts an optional config parameter.
contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs (3)
13-13: Added necessary imports for testing
The inclusion of BankMsg, Coin, CosmosMsg, DepsMut, QuerierWrapper, and SubMsg is appropriate for handling bank messages and sub-messages within the tests.
24-40: Initialization function init added for test setup
The init function provides a clean and reusable way to initialize the contract state for tests, enhancing maintainability and readability of test code.
1522-1619: Test test_execute_send correctly validates send functionality
The test case thoroughly checks the Send execution with and without a custom configuration. It verifies the distribution of funds according to weights and confirms that the contract handles the optional config parameter appropriately.
contracts/finance/andromeda-set-amount-splitter/Cargo.toml (1)
3-3: Version updated to 1.1.0-beta
The version bump reflects the new functionality added to the package, adhering to semantic versioning practices.
contracts/finance/andromeda-splitter/src/mock.rs (2)
31-38: execute_send method signature updated with optional config
The execute_send function correctly accepts the config parameter, enabling tests to pass custom configurations to the Send execution message.
63-64: mock_splitter_send_msg function updated to accept config
The function signature aligns with changes in the ExecuteMsg::Send struct, allowing for optional configuration in mock messages.
contracts/finance/andromeda-set-amount-splitter/src/mock.rs (1)
64-65: LGTM! Implementation aligns with PR objectives
The mock message constructor correctly implements the new config parameter, maintaining consistency with the contract changes.
tests-integration/tests/kernel.rs (1)
78-78: LGTM! Changes correctly adapt to new API.
The modification properly handles the API change while maintaining the focus on kernel integration testing.
packages/andromeda-finance/src/set_amount_splitter.rs (1)
59-59: LGTM: Optional config parameter addition is well-designed
The addition of config: Option<Vec<AddressAmount>> to the Send variant provides flexibility while maintaining backward compatibility through the Option type.
Let's verify the message structure usage across the codebase:
✅ Verification successful
Let's verify the usage of the Send message in the set-amount-splitter contract specifically to ensure compatibility:
Let's check the execute_send implementation specifically:
Based on the verification results, I can now provide the final response:
Optional config parameter is correctly implemented and used
The verification confirms that:
- The
Sendmessage's optional config parameter is properly handled in the execute_send implementation - Test cases cover both scenarios (with and without config)
- The change maintains backward compatibility as evidenced by test cases using
config: None - The implementation is consistent with the contract's purpose and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct Send message construction to ensure compatibility
rg -A 2 "ExecuteMsg::Send"
Length of output: 17842
Script:
#!/bin/bash
# Check the implementation of execute_send function
ast-grep --pattern 'fn execute_send($_) {
$$$
}'
# Check test cases specifically in set-amount-splitter
rg -A 5 "ExecuteMsg::Send" "contracts/finance/andromeda-set-amount-splitter/src/testing"
Length of output: 3146
Script:
#!/bin/bash
# Check the execute_send implementation in set-amount-splitter
rg -A 20 "fn execute_send" "contracts/finance/andromeda-set-amount-splitter/src"
Length of output: 3991
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (2)
165-169: Add config validation and verify constraints
The config validation issue is similar to the splitter contract.
Let's verify the constraints are consistent across contracts:
#!/bin/bash
# Description: Compare validation logic between contracts
rg "validate_recipient_list" --type rustLine range hint 181-193: LGTM: Clean implementation of recipient iteration
The recipient iteration logic is well-implemented with proper error handling and clear fund distribution logic.
contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs (2)
224-227: LGTM! Function signature updated to support dynamic recipient configuration.
The addition of the optional config parameter aligns with the PR objectives, allowing users to specify custom recipient configurations when sending messages.
Line range hint 252-377: LGTM! Funds distribution logic is secure and accurate.
The implementation:
- Uses checked arithmetic operations for safety
- Correctly calculates individual shares based on weights
- Properly handles remainder funds by returning them to sender
contracts/finance/andromeda-splitter/src/testing/tests.rs (1)
274-284: LGTM! Comprehensive test coverage for the new config functionality.
The test suite has been properly extended to cover:
- Sending with custom config
- Verifying correct fund distribution with custom config
- Maintaining existing functionality with default config
Also applies to: 342-377
tests-integration/tests/auction_app.rs (1)
341-341: LGTM: Updated test case to use optional config parameter.
The test case correctly uses the new optional config parameter for the splitter's send functionality.
tests-integration/tests/kernel_orch.rs (1)
1517-1517: LGTM: Updated IBC test to use optional config parameter.
The test case correctly uses the new optional config parameter for the splitter's send functionality in an IBC context.
tests-integration/tests/crowdfund_app.rs (2)
843-843: LGTM! The change aligns with the optional configuration feature.
The modification correctly implements the new optional configuration support by passing None to mock_splitter_send_msg, which tests the default behavior using state values.
Line range hint 1-844: Verify test coverage for optional configuration scenarios.
While the existing tests cover various scenarios, we should verify if there are test cases that explicitly validate the behavior when a custom configuration is provided (i.e., when Some(config) is passed to mock_splitter_send_msg).
✅ Verification successful
Based on the search results, I can now provide a final response:
Optional configuration scenarios are adequately covered through integration tests.
The codebase shows comprehensive test coverage for splitter configurations:
- The
mock_splitter_send_msgfunction accepts an optional configuration parameter (Option<Vec<AddressPercent>>) in both the cross-chain-swap and splitter contracts. - Integration tests in
crowdfund_app.rs,marketplace_app.rs, andauction_app.rsdemonstrate usage of splitter with both default (None) and custom configurations. - The test
test_crowdfund_app_native_with_ado_recipientspecifically validates the behavior with a configured splitter recipient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are test cases that cover the custom configuration scenario
# Search for test functions that might test the custom configuration
rg -A 5 "test_.*_with.*config"
# Search for usage of mock_splitter_send_msg with Some(...) to verify custom config testing
ast-grep --pattern 'mock_splitter_send_msg(Some($_))'
Length of output: 88
Script:
#!/bin/bash
# Let's search for all usages of mock_splitter_send_msg to understand how it's being used
rg "mock_splitter_send_msg" -B 2 -A 2
# Also search for test cases involving splitter configuration
rg "test.*splitter" -B 2 -A 2
# Look for the implementation of mock_splitter_send_msg
ast-grep --pattern 'fn mock_splitter_send_msg($_) {
$$$
}'
Length of output: 8183
contracts/finance/andromeda-weighted-distribution-splitter/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)
CHANGELOG.md (1)
30-30: Enhance the changelog entry to be more descriptive.While the entry follows the correct format, it could be more informative about the feature's purpose and impact. Consider expanding it to:
- - Added optional config for Send in Splitter contracts [(#686)](https://github.com/andromedaprotocol/andromeda-core/pull/686) + - Added optional config for Send in Splitter contracts, enabling dynamic recipient configurations without state updates [(#686)](https://github.com/andromedaprotocol/andromeda-core/pull/686) + - Bumped splitter from 2.1.4 to 2.2.0 + - Bumped set-amount-splitter from 1.0.3-beta to 1.1.0-beta + - Bumped weighted-distribution-splitter from 2.0.3-beta to 2.1.0-betaThis provides more context about the feature's functionality and documents the version changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests-integration/tests/splitter.rs (1)
71-71: Remove unnecessary.clone()The token variable is cloned but only used once more. Consider removing the
.clone()as it's not needed here.- .execute_send(&mut router, owner.clone(), &[token.clone()], None) + .execute_send(&mut router, owner.clone(), &[token], None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests-integration/tests/splitter.rs(2 hunks)
🔇 Additional comments (1)
tests-integration/tests/splitter.rs (1)
21-21: LGTM: Balance increase necessary for multiple test cases
The increase in initial balance from 1000 to 10000 "uandr" is appropriate as we need sufficient funds to test both the default and custom configuration scenarios.
CHANGELOG.md
Outdated
| @@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Added Validator Staking ADO [(#330)](https://github.com/andromedaprotocol/andromeda-core/pull/330) | |||
| - Added Restake and Redelegate to Validator Staking [(#622)](https://github.com/andromedaprotocol/andromeda-core/pull/622) | |||
| - Added andromeda-math and andromeda-account packages[(#672)](https://github.com/andromedaprotocol/andromeda-core/pull/672) | |||
| - Added optional config for Send in Splitter contracts [(#686)](https://github.com/andromedaprotocol/andromeda-core/pull/686) | |||
There was a problem hiding this comment.
This looks like it's under the wrong section? Release 3 was completed so this will be under Unreleased
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
contracts/finance/andromeda-set-amount-splitter/src/mock.rs (2)
24-32: Enhance test coverage fordefault_recipientparameterThe
instantiatemethod now includes adefault_recipientparameter. Ensure that unit tests are updated or added to verify the behavior whendefault_recipientis provided and when it isNone.
38-45: Expand tests forexecute_sendwithconfigparameterWith the addition of the
configparameter inexecute_send, it's important to test various scenarios, including cases with and without theconfig. Consider enhancing the test suite to cover these cases.packages/andromeda-finance/src/weighted_splitter.rs (4)
22-23: Clarify documentation fordefault_recipientfieldThe comment for
default_recipientstates that it defaults to the message sender. To improve clarity, consider specifying that it is an optional field and elaborating on the default behavior whenNone.Suggested revision:
/// An optional address that will receive any surplus funds. If `None`, the message sender will receive the surplus. pub default_recipient: Option<Recipient>,
33-33: Add documentation fordefault_recipientinInstantiateMsgThe
default_recipientfield inInstantiateMsglacks documentation. Adding a comment will improve understanding for developers interacting with the contract.Suggested addition:
/// An optional default recipient for surplus funds during instantiation. /// If not provided, the sender will be used as the default recipient. pub default_recipient: Option<Recipient>,
43-44: Document the newUpdateDefaultRecipientvariant inExecuteMsgThe
UpdateDefaultRecipientexecute message lacks documentation. Including a comment will enhance code readability and maintainability.Suggested addition:
/// Update the default recipient. Only executable by the contract owner when the contract is not locked. UpdateDefaultRecipient { recipient: Option<Recipient> },
52-52: Update documentation forSendvariant to reflect newconfigparameterThe
Sendvariant now accepts an optionalconfigparameter, but the documentation hasn't been updated accordingly. Consider revising the comment to describe the new functionality.Suggested revision:
/// Divides any attached funds to the message amongst the recipients list. /// Optionally accepts a custom configuration of recipients. Send { config: Option<Vec<AddressWeight>> },contracts/finance/andromeda-splitter/src/contract.rs (2)
Line range hint
165-185: Optimize nested loops inexecute_sendfor better performanceThe nested loops iterating over
info.fundsandsplitter_recipientsinexecute_sendcould potentially lead to performance issues if the number of funds or recipients increases in the future. Although limits are currently enforced, consider optimizing this section to improve computational efficiency and mitigate any potential DoS vectors.
194-202: Clarify and document the handling of remainder fundsThe code sends any remainder funds to the
default_recipientor back to theinfo.senderifdefault_recipientis not set. It would be beneficial to document this behavior clearly so that users are aware of how surplus funds are managed during distribution.contracts/finance/andromeda-set-amount-splitter/src/contract.rs (1)
184-187: LGTM: Clean implementation of dynamic recipient configurationThe changes effectively implement the flexible recipient configuration feature while maintaining backward compatibility through the optional config parameter.
Consider extracting the recipient selection logic into a separate function for better readability:
- let splitter_recipients = if let Some(config) = config { - validate_recipient_list(deps.as_ref(), config.clone())?; - config - } else { - splitter.recipients - }; + fn get_validated_recipients(deps: Deps, config: Option<Vec<AddressAmount>>, splitter: &Splitter) -> Result<Vec<AddressAmount>, ContractError> { + match config { + Some(config) => { + validate_recipient_list(deps, config.clone())?; + Ok(config) + } + None => Ok(splitter.recipients.clone()), + } + } + let splitter_recipients = get_validated_recipients(deps.as_ref(), config, &splitter)?;Also applies to: 213-218
contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs (2)
24-41: Consider making the init helper function more flexible.The
inithelper function could be more reusable by accepting parameters for the recipient address and weight instead of hardcoding them.Consider this refactor:
-pub const OWNER: &str = "creator"; +pub const OWNER: &str = "creator"; +pub const DEFAULT_WEIGHT: u128 = 100; -fn init(deps: DepsMut) -> Response { +fn init( + deps: DepsMut, + recipient_addr: Option<String>, + weight: Option<u128>, +) -> Response { let mock_recipient: Vec<AddressWeight> = vec![AddressWeight { - recipient: Recipient::from_string(String::from("some_address")), - weight: Uint128::new(100), + recipient: Recipient::from_string( + recipient_addr.unwrap_or_else(|| String::from("some_address")) + ), + weight: Uint128::new(weight.unwrap_or(DEFAULT_WEIGHT)), }];
1551-1649: Enhance test coverage and readability of send functionality.While the test covers the basic functionality, consider these improvements:
- Add constants for the weights instead of magic numbers
- Add explicit test cases for remainder handling
- Add test cases for invalid configurations:
- Empty config vector
- Config with zero total weight
- Config with invalid addresses
Consider this refactor for the weight constants:
+const WEIGHT_10_PERCENT: u128 = 10; +const WEIGHT_20_PERCENT: u128 = 20; +const WEIGHT_50_PERCENT: u128 = 50; #[test] fn test_execute_send() { // ... - let recip_weight1 = 10; // 10% - let recip_weight2 = 20; // 20% - let recip_weight3 = 50; // 50% + let recip_weight1 = WEIGHT_10_PERCENT; + let recip_weight2 = WEIGHT_20_PERCENT; + let recip_weight3 = WEIGHT_50_PERCENT;Would you like me to generate additional test cases for error scenarios and remainder handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
contracts/finance/andromeda-set-amount-splitter/src/contract.rs(7 hunks)contracts/finance/andromeda-set-amount-splitter/src/mock.rs(3 hunks)contracts/finance/andromeda-set-amount-splitter/src/testing/tests.rs(13 hunks)contracts/finance/andromeda-splitter/src/contract.rs(8 hunks)contracts/finance/andromeda-splitter/src/mock.rs(3 hunks)contracts/finance/andromeda-splitter/src/testing/tests.rs(19 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs(7 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs(31 hunks)ibc-tests/tests/crowdfund.rs(2 hunks)packages/andromeda-finance/src/set_amount_splitter.rs(3 hunks)packages/andromeda-finance/src/splitter.rs(3 hunks)packages/andromeda-finance/src/weighted_splitter.rs(3 hunks)tests-integration/tests/auction_app.rs(2 hunks)tests-integration/tests/crowdfund_app.rs(2 hunks)tests-integration/tests/kernel.rs(2 hunks)tests-integration/tests/kernel_orch.rs(3 hunks)tests-integration/tests/marketplace_app.rs(2 hunks)tests-integration/tests/set_amount_splitter.rs(2 hunks)tests-integration/tests/splitter.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- tests-integration/tests/kernel.rs
- ibc-tests/tests/crowdfund.rs
- tests-integration/tests/set_amount_splitter.rs
- contracts/finance/andromeda-splitter/src/testing/tests.rs
- tests-integration/tests/marketplace_app.rs
- tests-integration/tests/splitter.rs
- tests-integration/tests/auction_app.rs
- packages/andromeda-finance/src/splitter.rs
- tests-integration/tests/kernel_orch.rs
- packages/andromeda-finance/src/set_amount_splitter.rs
🔇 Additional comments (15)
contracts/finance/andromeda-splitter/src/mock.rs (4)
24-32: LGTM: Updated instantiate method with default_recipient parameter
The addition of the default_recipient parameter to the instantiate method is correctly implemented and passed through to the InstantiateMsg. This enhancement increases flexibility for testing different scenarios.
38-45: LGTM: Updated execute_send method with config parameter
The modification to accept an optional config parameter in execute_send aligns with the updated contract functionality. The config is properly passed to mock_splitter_send_msg.
61-68: LGTM: Included default_recipient in mock_splitter_instantiate_msg
The mock_splitter_instantiate_msg function correctly includes the default_recipient parameter, ensuring that all instantiation parameters are properly set in the mock.
72-73: LGTM: Updated mock_splitter_send_msg to accept config
The function now accepts an optional config parameter and constructs the ExecuteMsg::Send message accordingly. This change ensures consistency with the contract's updated execute message structure.
contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs (3)
170-211: LGTM: Added execute_update_default_recipient function
The new execute_update_default_recipient function correctly handles updating the default_recipient, including necessary validations and ensuring that the contract is not locked during the update.
Line range hint 272-296: Potential missing validation in execute_send function
The execute_send function accepts a config parameter, but the provided config is not validated. This could lead to potential issues if the config contains invalid data.
114-117:
Add validation for the provided config parameter
While the logic correctly uses the provided config if present, there is no validation to ensure that the config adheres to expected constraints. The provided config should be validated to prevent issues such as:
- Empty recipient list.
- Duplicate recipients.
- Invalid weights (e.g., total weight exceeding allowed limits).
Consider adding validation similar to the one used in execute_update_recipients.
contracts/finance/andromeda-set-amount-splitter/src/mock.rs (1)
72-73: LGTM: Updated mock_set_amount_splitter_send_msg to include config
The function now accepts an optional config parameter and constructs the ExecuteMsg::Send message accordingly. This change ensures consistency with the contract's updated execute message structure.
contracts/finance/andromeda-splitter/src/contract.rs (2)
143-149: Validation of provided config in execute_send
Good job adding validation for the provided config using validate_recipient_list. This addresses previous concerns about missing validation and ensures the config adheres to expected constraints.
287-328: LGTM: Added execute_update_default_recipient function
The implementation of execute_update_default_recipient correctly handles updating the default_recipient, including necessary validations and ensuring the contract is not locked.
contracts/finance/andromeda-set-amount-splitter/src/contract.rs (3)
55-55: LGTM: Default recipient field added to Splitter struct
The addition of the default_recipient field provides a clean way to handle remainder funds, aligning with the PR's objective of flexible recipient configuration.
Also applies to: 63-63
141-182: LGTM: Well-implemented default recipient update function
The function includes all necessary validations:
- Authorization check
- Lock expiration verification
- Recipient validation
- Non-payable check
The implementation is clean and follows best practices.
259-267: LGTM: Clean implementation of remainder funds handling
The code elegantly handles remainder funds by using the default_recipient with a fallback to the sender's address.
contracts/finance/andromeda-set-amount-splitter/src/testing/tests.rs (1)
182-190: LGTM: Comprehensive test coverage for new functionality
The test cases thoroughly cover:
- Default recipient behavior
- Custom config usage
- Remainder funds handling
- Error scenarios
Also applies to: 260-309
tests-integration/tests/crowdfund_app.rs (1)
95-101: LGTM: Integration test updates
The integration test changes correctly reflect the contract modifications while maintaining test coverage.
Also applies to: 848-848
| @@ -41,6 +58,7 @@ fn test_update_app_contract() { | |||
| lock_time: None, | |||
| kernel_address: MOCK_KERNEL_CONTRACT.to_string(), | |||
| owner: None, | |||
| default_recipient: None, | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for default_recipient functionality
The codebase shows that default_recipient is a significant feature implemented across multiple splitter contracts with dedicated update functionality (execute_update_default_recipient), but lacks comprehensive test coverage. The implementation suggests the following important behaviors that need testing:
- Setting and updating the default recipient
- Sending funds when there are no other recipients (falls back to default recipient)
- Interaction between default recipient and configured recipients
Key locations needing test coverage:
contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rscontracts/finance/andromeda-splitter/src/contract.rscontracts/finance/andromeda-set-amount-splitter/src/contract.rs
🔗 Analysis chain
Add test coverage for default_recipient functionality.
The default_recipient field has been added to the Splitter struct, but there are no test cases that verify its behavior when set to a non-None value. Consider adding test cases to cover:
- Setting a default recipient
- Sending funds when there are no other recipients
- Interaction between default recipient and configured recipients
Also applies to: 131-131, 152-152, 210-210, 255-255, 301-301, 346-346, 425-425, 451-451, 519-519, 583-583, 594-594, 708-708, 719-719, 751-751, 804-804, 815-815, 883-883, 956-956, 1021-1021, 1064-1064, 1128-1128, 1207-1207, 1274-1274, 1339-1339, 1411-1411, 1467-1467, 1521-1521, 1594-1594
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of default_recipient functionality
# Search for test cases using default_recipient
rg -l 'default_recipient:\s*Some\(' .
# Search for any usage of default_recipient other than None
rg 'default_recipient:(?!\s*None)' .
Length of output: 389
Script:
#!/bin/bash
# Description: Check for test coverage of default_recipient functionality with corrected regex
# Search for test cases using default_recipient with Some value
rg --pcre2 'default_recipient:\s*Some\(' .
# Search for any test cases or code that sets or uses default_recipient
rg -A 3 'default_recipient' .
# Look for any test functions that might test default recipient functionality
ast-grep --pattern 'fn test_$_default_recipient$_'
Length of output: 36980
Script:
#!/bin/bash
# Description: Check for test coverage of default_recipient functionality in contract implementations
# Search for contract implementations using default_recipient
rg -A 5 'fn execute_update_default_recipient|fn execute_send.*default_recipient' .
# Search for test functions that might test default recipient functionality (broader pattern)
rg -B 2 -A 5 'test.*default.*recipient|test.*recipient.*default' .
# Look for any test functions that verify sending behavior with default recipient
ast-grep --pattern 'fn test_$_send$_'
Length of output: 1951
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
11-11: Add version information to the changelog entry.The PR includes version updates for several components. Consider adding this information to make the changelog more informative:
- Added optional config for Send in Splitter contracts [(#686)](https://github.com/andromedaprotocol/andromeda-core/pull/686) + Added optional config for Send in Splitter contracts [(#686)](https://github.com/andromedaprotocol/andromeda-core/pull/686) + - splitter: 2.1.4 -> 2.2.0 + - set-amount-splitter: 1.0.3-beta -> 1.1.0-beta + - weighted-distribution-splitter: 2.0.3-beta -> 2.1.0-beta
Motivation
Closes #682
Implementation
Added an optional config to the send message, if it's None, the state values are used.
Testing
Added a case with config in
test_execute_sendVersion Changes
splitter:2.1.4->2.2.0-beta.1set-amount-splitter:1.0.3-beta->1.1.0-betaweighted-distributon-splitter:2.0.3-beta->2.1.0-betaNotes
None
Future work
#681 #683
Summary by CodeRabbit
New Features
execute_sendfunctionality to accept a custom configuration for recipient addresses and weights, allowing for more flexible fund distribution.Sendmessage structure to include an optional configuration parameter across various contracts.Bug Fixes
Tests
execute_send.