Splitter config lock #757
Splitter config lock #757
Conversation
WalkthroughThis pull request introduces a standardized approach to error handling across multiple Andromeda finance and kernel contracts, specifically focusing on the Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
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: 0
🧹 Nitpick comments (2)
contracts/finance/andromeda-conditional-splitter/src/contract.rs (1)
232-232: Consider adding descriptive error messages for lock-related errors.The error messages are currently set to
None. Adding descriptive messages would improve the user experience by clearly indicating why the operation failed.- ContractError::ContractLocked { msg: None } + ContractError::ContractLocked { + msg: Some("Cannot update thresholds while the splitter is locked".to_string()) + }Also applies to: 264-264
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
250-250: Standardize error message handling across functions.For consistency with
execute_send_cw20, consider adding descriptive error messages to other lock-related errors.- ContractError::ContractLocked { msg: None } + ContractError::ContractLocked { + msg: Some("Cannot perform this operation while the splitter is locked".to_string()) + }Also applies to: 338-338, 369-369, 404-404
📜 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 (12)
contracts/finance/andromeda-conditional-splitter/src/contract.rs(2 hunks)contracts/finance/andromeda-conditional-splitter/src/testing/tests.rs(1 hunks)contracts/finance/andromeda-fixed-amount-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-fixed-amount-splitter/src/contract.rs(5 hunks)contracts/finance/andromeda-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-splitter/src/contract.rs(4 hunks)contracts/finance/andromeda-weighted-distribution-splitter/Cargo.toml(1 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs(7 hunks)contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs(5 hunks)contracts/os/andromeda-kernel/src/execute.rs(1 hunks)contracts/os/andromeda-kernel/src/query.rs(1 hunks)packages/std/src/error.rs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/finance/andromeda-fixed-amount-splitter/Cargo.toml
- contracts/finance/andromeda-splitter/Cargo.toml
- contracts/finance/andromeda-weighted-distribution-splitter/Cargo.toml
- contracts/os/andromeda-kernel/src/query.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Contracts
- GitHub Check: Schemas
- GitHub Check: Linting
- GitHub Check: Summary
🔇 Additional comments (13)
contracts/finance/andromeda-splitter/src/contract.rs (1)
Line range hint
167-172: LGTM! Good security enhancement.The added check prevents configuration changes while the splitter is locked, addressing the security concern mentioned in the PR objectives. The descriptive error message is helpful for users.
contracts/finance/andromeda-fixed-amount-splitter/src/contract.rs (1)
167-172: LGTM! Security check properly implemented.The lock validation is correctly placed before any state modifications, preventing configuration changes while the contract is locked.
Also applies to: 303-308
contracts/finance/andromeda-weighted-distribution-splitter/src/contract.rs (3)
279-284: LGTM! Security check properly implemented.The lock validation is correctly placed and includes a descriptive error message.
137-137: Add test coverage for lock-related error cases.While the lock validation is implemented correctly, it would be beneficial to add test cases that verify the behavior when attempting to modify locked contracts.
Also applies to: 179-179, 225-225, 389-389, 427-427, 464-464
Line range hint
257-262: Security Review: Lock Validation ImplementationThe implementation effectively addresses the security concern by:
- Adding lock validation before processing configurations
- Preventing unauthorized modifications during locked states
- Using descriptive error messages in critical paths
Recommendations for improvement:
- Standardize error message handling across all lock-related errors
- Add comprehensive test coverage for lock-related scenarios
- Consider adding logging for security-relevant events
Also applies to: 167-172, 303-308, 279-284
packages/std/src/error.rs (1)
46-47: LGTM! Enhanced error reporting for contract locks.The addition of an optional message field to the
ContractLockedvariant improves error reporting by allowing contracts to provide context-specific details about why an operation was rejected due to a lock.contracts/finance/andromeda-conditional-splitter/src/testing/tests.rs (1)
223-223: LGTM! Updated test assertion to match new error structure.The test assertion has been correctly updated to match the new
ContractLockederror variant that includes an optional message field.contracts/os/andromeda-kernel/src/execute.rs (1)
7-7: LGTM! Improved fund validation strategy.Replacing
merge_coinswithhas_coins_mergedshifts the focus from merging operations to validation, which is a safer approach for handling funds.contracts/finance/andromeda-weighted-distribution-splitter/src/testing/tests.rs (5)
330-330: LGTM! Updated test assertion intest_execute_update_lock_already_locked.Test assertion correctly updated to match the new
ContractLockederror variant structure.
604-604: LGTM! Updated test assertion intest_execute_remove_recipient_contract_locked.Test assertion correctly updated to match the new
ContractLockederror variant structure.
827-827: LGTM! Updated test assertion intest_update_recipient_weight_locked_contract.Test assertion correctly updated to match the new
ContractLockederror variant structure.
1280-1283: LGTM! Updated test assertion intest_execute_add_recipient_locked_contract.Test assertion correctly updated to match the new
ContractLockederror variant structure.
1498-1498: LGTM! Updated test assertion intest_execute_update_recipients_contract_locked.Test assertion correctly updated to match the new
ContractLockederror variant structure.
Motivation
The splitter contracts' locks could be bypassed by providing a config during
Send.Implementation
Added this check whenever a config is provided:
Testing
None
Version Changes
splitter:2.3.0-b.2->2.3.0-b.3fixed-amount-splitter:1.2.0-b.1->1.2.0-b.2weighted-distribution-splitter:2.1.0-b.2->2.1.0-b.3Notes
None
Future work
None
Summary by CodeRabbit
Bug Fixes
ContractLockederror to include optional message fieldChores
1.2.0-b.1→1.2.0-b.22.3.0-b.2→2.3.0-b.32.1.0-b.2→2.1.0-b.3Refactor