feat: add core compounder contract #126
Conversation
WalkthroughThe recent updates bring forth an auto compounder contract for vesting native tokens, enhancing security and efficiency in fund management within a multisig setup. These changes introduce new functionalities for instantiating, executing, and querying contract states. Additionally, improvements have been made in the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (7)
Cargo.lockis excluded by:!**/*.lockcontracts/core-compounder/Cargo.lockis excluded by:!**/*.lockcontracts/core-compounder/Cargo.tomlis excluded by:!**/*.tomlcontracts/core-compounder/schema/execute_msg.jsonis excluded by:!**/*.jsoncontracts/core-compounder/schema/instantiate_msg.jsonis excluded by:!**/*.jsoncontracts/core-compounder/schema/query_msg.jsonis excluded by:!**/*.jsoncontracts/core-compounder/schema/vesting_account_response.jsonis excluded by:!**/*.json
Files selected for processing (11)
- Cookbook.md (1 hunks)
- contracts/core-compounder/.cargo/config (1 hunks)
- contracts/core-compounder/.gitignore (1 hunks)
- contracts/core-compounder/README.md (1 hunks)
- contracts/core-compounder/examples/schema.rs (1 hunks)
- contracts/core-compounder/src/contract.rs (1 hunks)
- contracts/core-compounder/src/errors.rs (1 hunks)
- contracts/core-compounder/src/lib.rs (1 hunks)
- contracts/core-compounder/src/msg.rs (1 hunks)
- contracts/core-compounder/src/state.rs (1 hunks)
- contracts/core-compounder/src/testing.rs (1 hunks)
Files skipped from review due to trivial changes (3)
- contracts/core-compounder/.cargo/config
- contracts/core-compounder/.gitignore
- contracts/core-compounder/src/testing.rs
Additional comments: 14
contracts/core-compounder/src/lib.rs (1)
- 1-7: The module structure and conditional compilation directive for the testing module are correctly implemented, adhering to Rust's best practices.
contracts/core-compounder/src/errors.rs (1)
- 1-13: The error definitions in
errors.rsare correctly implemented, using thethiserrorcrate for clear and concise error handling, which adheres to Rust's best practices.contracts/core-compounder/examples/schema.rs (1)
- 1-16: The schema generation script in
schema.rsis correctly implemented, using thecosmwasm_schemacrate to generate JSON schemas for the contract's message structures, adhering to best practices for CosmWasm contract development.contracts/core-compounder/src/state.rs (1)
- 1-24: The state management in
state.rsis correctly implemented, usingcw_storage_plusfor state items andHashSetfor managing unique manager addresses, adhering to best practices for CosmWasm contract development.contracts/core-compounder/README.md (1)
- 1-39: The documentation in
README.mdis well-written and provides a clear and accurate overview of the Compounder contract's functionality, modes, and role-based operations.contracts/core-compounder/src/msg.rs (1)
- 1-61: The message structures in
msg.rsare correctly implemented, covering a comprehensive range of contract operations and adhering to best practices for CosmWasm contract message design.contracts/core-compounder/src/contract.rs (8)
- 12-18: The instantiation function correctly validates the input parameters, ensuring that the list of managers is not empty and that all addresses are valid. This is crucial for preventing misconfiguration of the contract at deployment.
- 40-46: The execute function correctly dispatches the incoming message to the appropriate handler based on the message type. This pattern is standard and effective for handling different contract actions.
- 66-90: The
update_managersfunction correctly implements authorization checks to ensure that only the admin can update the list of managers. This is a critical security measure to prevent unauthorized changes to the contract's state.- 92-105: The
set_autocompounder_modefunction correctly implements authorization checks and updates the auto compounder mode state. This function is essential for controlling the contract's behavior based on the admin's decisions.- 108-133: The
withdrawfunction correctly implements authorization checks and constructs a message for withdrawing funds. It's important to ensure that only the admin can perform withdrawals, maintaining control over the contract's funds.- 136-157: The
unstakefunction correctly implements authorization checks and constructs messages for unstaking tokens. This function is crucial for managing the contract's staked tokens in a secure manner.- 162-197: The
stakefunction correctly implements authorization checks, allowing both managers and the admin to stake tokens. It also validates the total shares to prevent invalid stake distributions. This function is key to the contract's functionality, enabling the staking of tokens in a controlled and secure manner.- 261-279: The query functions correctly implement the logic for returning the auto compounder mode and the admin and managers' information. These functions are essential for external parties to understand the contract's current state and configuration.
Cookbook.md
Outdated
| ```javascript | ||
| { | ||
| "admin": "cosmos1...", | ||
| "managers": ["cosmos1...", "cosmos1..."] | ||
| } | ||
| ``` | ||
|
|
||
| ### 8.2 Execute | ||
|
|
||
| #### Admin functions | ||
|
|
||
| - **SetAutoCompounderMode** sets the auto compounder mode | ||
|
|
||
| ```javascript | ||
| { | ||
| "set_auto_compounder_mode": { | ||
| "mode": "true" // true or false | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| - **Withdraw** allows to withdraw the funds from the contract | ||
|
|
||
| ```javascript | ||
| { | ||
| "withdraw": { | ||
| "amount": "1000000" | ||
| "recipient": "cosmos1..." | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| - **unstakes** allows to unstake the funds from the contract | ||
|
|
||
| ```javascript | ||
| { | ||
| "unstake": { | ||
| "unstake_msgs": [ | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "amount": "1000000" | ||
| }, | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "amount": "1000000" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| - **update managers** allows to update the managers of the contract | ||
|
|
||
| ```javascript | ||
| { | ||
| "update_managers": { | ||
| "managers": ["cosmos1...", "cosmos1..."] | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| #### Manager functions | ||
|
|
||
| - **stake** allows to stake the funds from the contract. The shares are normalized | ||
|
|
||
| ```javascript | ||
| { | ||
| "stake": { | ||
| "stake_msgs": [ | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "share": "1000000" | ||
| }, | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "share": "1000000" | ||
| } | ||
| ] | ||
| }, | ||
| "amount": "1000000" | ||
| } | ||
| ``` | ||
|
|
||
| ### 8.3 Query | ||
|
|
||
| - **auto compounder mode** returns wether the auto compounder mode is enabled or not | ||
|
|
||
| ```javascript | ||
| { | ||
| "auto_compounder_mode": {} | ||
| } | ||
| ``` | ||
|
|
||
| - **AdminAndManagers** returns the admin and managers of the contract | ||
|
|
||
| ```javascript | ||
| { | ||
| "admin_and_managers": {} | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The documentation for the "Auto compounder" contract is well-structured and aligns with the PR objectives of enhancing security and efficiency in staking re-delegation processes. However, there are a few areas that could be improved for clarity and consistency:
-
Instantiate Section (Lines 665-671): The description "This contract implements vesting accounts for the native tokens." seems to be a copy-paste error from the previous section. It would be more appropriate to describe the specific purpose of the auto compounder contract here, focusing on its role in managing staking re-delegation securely.
-
Execute Section - Withdraw Function (Lines 688-697): There's a missing comma at the end of line 693. This could lead to confusion or errors if someone tries to use this JSON snippet as is. It should be corrected to ensure syntactical accuracy.
-
General Consistency: Ensure that the documentation style, such as the use of backticks for code snippets and the indentation of JSON objects, is consistent throughout this section and matches the style used in the rest of the Cookbook.md file. This includes consistent use of comments within the JSON snippets to explain parameters or expected values.
-
Clarity on Manager Functions (Lines 728-747): It would be beneficial to add a brief explanation about what it means for shares to be "normalized" in the context of staking. This could help users understand how the
stakefunction operates and what they should expect when using it.
Improving these areas will enhance the readability and usability of the documentation for developers and users interacting with the auto compounder contract.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
artifacts/core_compounder.wasmis excluded by:!**/*.wasmcontracts/core-compounder/schema/execute_msg.jsonis excluded by:!**/*.jsoncontracts/core-compounder/schema/query_msg.jsonis excluded by:!**/*.json
Files selected for processing (3)
- artifacts/checksums.txt (1 hunks)
- contracts/core-compounder/src/contract.rs (1 hunks)
- contracts/core-compounder/src/testing.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contracts/core-compounder/src/contract.rs
Additional comments: 6
artifacts/checksums.txt (1)
- 4-4: The addition of the
core_compounder.wasmchecksum follows the project's standards for tracking WebAssembly binaries. This update is crucial for ensuring the integrity of the newly introduced contract.contracts/core-compounder/src/testing.rs (5)
- 14-90: This test function
invalid_manangers_initializationcorrectly checks for various invalid initialization scenarios, ensuring that the contract initialization logic is robust against incorrect inputs. It's well-structured and covers essential failure cases, including empty managers and invalid admin addresses.- 92-301: The
test_execute_permissionfunction thoroughly tests permissions for various contract actions, such as staking, unstaking, withdrawing, and updating managers. It ensures that only authorized roles can perform specific actions, which is crucial for the contract's security. The tests are comprehensive and cover a wide range of scenarios.- 304-436: The
test_share_calcfunction tests the share calculation logic during staking to multiple validators with different shares. It's a critical test that ensures the staking logic divides tokens correctly among validators based on the specified shares. The test is well-designed and covers several scenarios, including equal and unequal shares.- 439-463: The
test_withdrawfunction checks the withdrawal functionality, ensuring that only authorized roles can perform withdrawals. This test is straightforward and validates an essential aspect of the contract's security regarding fund management.- 467-517: The
test_unstakefunction validates the unstaking functionality, ensuring that the contract correctly handles unstaking requests to multiple validators. It's an important test that verifies the contract's ability to handle unstaking actions correctly, including handling different amounts and validators.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockartifacts/core_compounder.wasmis excluded by:!**/*.wasm
Files selected for processing (2)
- Cookbook.md (5 hunks)
- artifacts/checksums.txt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- artifacts/checksums.txt
Additional comments: 5
Cookbook.md (5)
- 517-523: The instantiation message correctly specifies the admin and managers, aligning with the contract's design for role separation. This is essential for managing permissions within a multisig setup.
- 551-567: The
unstakefunction documentation uses the term "unstakes" which might be a typo or inconsistency in naming. Consider using "unstake" for consistency with the function name.- 570-577: The
update managersfunction documentation should use camelCase for consistency with other function names in the documentation, e.g.,updateManagers. Consistency in naming conventions improves readability and maintainability.- "update managers": { + "updateManagers": {
- 604-610: The query for the auto compounder mode is correctly documented, providing a clear example of how to check if the auto compounder mode is enabled or not.
- 612-618: The query for retrieving the admin and managers of the contract is well-documented, offering a straightforward way to obtain this information. This is crucial for understanding the contract's permission structure.
|
|
||
| ```javascript | ||
| { | ||
| "stake": { | ||
| "stake_msgs": [ | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "share": "1000000" | ||
| }, | ||
| { | ||
| "validator": "cosmosvaloper1...", | ||
| "share": "1000000" | ||
| } | ||
| ] | ||
| }, | ||
| "amount": "1000000" |
There was a problem hiding this comment.
The stake function documentation mentions that "The shares are normalized," but it lacks clarity on what "normalized" means in this context. Adding a brief explanation would help users understand how the function operates and what to expect when using it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
artifacts/core_compounder.wasmis excluded by:!**/*.wasm
Files selected for processing (2)
- artifacts/checksums.txt (1 hunks)
- contracts/core-compounder/src/contract.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- artifacts/checksums.txt
- contracts/core-compounder/src/contract.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by:!**/*.lock
Files selected for processing (1)
- artifacts/checksums.txt (1 hunks)
Additional comments: 1
artifacts/checksums.txt (1)
- 2-2: The addition of the checksum for
core_compounder.wasmis consistent with the file's purpose of tracking checksums for wasm files. Ensure the checksum is correctly generated and matches the actual wasm file to prevent any integrity issues.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockcontracts/core-compounder/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (8)
- contracts/broker-bank/src/contract.rs (2 hunks)
- contracts/broker-bank/src/lib.rs (1 hunks)
- contracts/broker-bank/src/msgs.rs (1 hunks)
- contracts/core-compounder/examples/schema.rs (1 hunks)
- contracts/core-compounder/src/contract.rs (1 hunks)
- contracts/core-compounder/src/lib.rs (1 hunks)
- contracts/core-compounder/src/msg.rs (1 hunks)
- contracts/core-compounder/src/testing.rs (1 hunks)
Additional comments: 16
contracts/core-compounder/src/lib.rs (3)
- 1-1: LGTM!
- 2-2: LGTM!
- 4-5: Correct use of conditional compilation for the
testingmodule.contracts/broker-bank/src/lib.rs (1)
- 1-5: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-12]
The removal of the
#[cfg(test)]attribute from thetutilmodule means it will now be included in the production build. Please verify if this was intentional. Iftutilis meant only for testing, consider restoring the#[cfg(test)]attribute.contracts/core-compounder/examples/schema.rs (1)
- 1-17: The schema generation script is correctly implemented. Consider adding error handling instead of using
unwrap()for a more robust solution, especially in larger or more complex scripts.contracts/broker-bank/src/msgs.rs (1)
- 43-45: The addition of the
IsHaltedquery message is correctly implemented and enhances the contract's functionality.contracts/core-compounder/src/testing.rs (10)
- 23-31: The
TestCaseExecstruct is well-defined for encapsulating test case parameters. However, consider adding documentation comments to explain the purpose of each field, especially for complex or non-obvious ones likeresp_msgsandcontract_funds_start. This will improve code readability and maintainability.- 236-252: The
CosmosMsgExtstruct and its implementations (PartialEq,Display) are cleverly designed for comparingCosmosMsginstances in a human-readable format. This is a good practice for test assertions that involve complex structures. Well done!- 33-121: The test
test_assert_ownereffectively checks that owner-gated execute calls fail when the sender is not the contract owner. It's comprehensive and covers multiple scenarios. However, ensure that the error messages checked in the assertions are aligned with the actual contract implementation to avoid false negatives in the tests.- 123-234: The
exec_withdrawtest case is well-structured and covers both success and failure scenarios for withdrawal operations. It's good to see the use of mock queriers for setting up contract balances. To further improve, consider adding more detailed comments explaining the rationale behind each test case, especially for complex scenarios involving contract funds and expected response messages.- 254-303: The
exec_toggle_halttest case thoroughly checks the functionality of toggling the halt status of the contract. It includes both success and error cases, which is commendable. One suggestion is to add assertions for the state change after the toggle operation to ensure that the contract's state is correctly updated.- 309-352: The
exec_edit_opers_addtest case effectively tests the addition of operators. It's good to see checks for both the response and the updated state. To enhance this test, consider adding negative test cases, such as attempting to add an operator that already exists, to ensure robust error handling.- 354-407: The
exec_edit_opers_removetest case is well-implemented, covering the removal of operators from the permissions set. Similar to the previous comment, consider adding negative test cases, such as attempting to remove an operator that does not exist, to ensure the contract handles these scenarios gracefully.- 409-454: The
exec_stake_haltedtest case checks staking operations under normal and halted conditions. This is crucial for ensuring the contract behaves correctly under different states. It's well-implemented, but adding comments to explain the rationale behind each test scenario would improve readability.- 456-624: The
exec_staketest case thoroughly tests various staking scenarios, including success cases with different shares and failure cases with invalid senders or empty stake messages. This comprehensive approach is commendable. To further improve, consider adding tests for edge cases, such as staking with minimal amounts or to non-existent validators, to ensure robustness.- 626-764: The
exec_unstaketest case covers unstaking operations, including success scenarios and failure cases where the sender lacks the necessary permissions. It's well-structured and provides a good coverage of the unstake functionality. Similar to previous comments, adding tests for edge cases, such as unstaking more than the available balance, would enhance the test suite.
| // TODO: test ExecuteMsg::EditOpers | ||
| // TODO: ownership query | ||
| // pub fn get_ownership(storage: &dyn Storage) -> StdResult<Ownership<Addr>> |
There was a problem hiding this comment.
These TODO comments indicate unfinished work related to testing ExecuteMsg::EditOpers and querying ownership. It's important to address these to ensure comprehensive test coverage of the contract's functionalities.
Would you like me to help implement these tests or open a GitHub issue to track this task?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- Cookbook.md (5 hunks)
- contracts/README.md (2 hunks)
- contracts/core-compounder/README.md (1 hunks)
Additional comments: 3
contracts/README.md (1)
- 17-18: The addition of the core-compounder contract to the README is clear and aligns with the PR objectives. However, consider adding a more detailed description or a link to further documentation for users who might be interested in learning more about its functionalities and use cases.
contracts/core-compounder/README.md (1)
- 49-49: The description for the Withdraw function could be improved for clarity. Consider specifying that it allows the admin to withdraw funds, to make it clear who has the permission to perform this action.
Cookbook.md (1)
- 582-598: The documentation mentions that "The shares are normalized" in the context of staking, but it lacks clarity on what "normalized" means. Adding a brief explanation would help users understand how the function operates and what to expect when using it.
| - **Withdraw** allows to withdraw the funds from the contract | ||
|
|
||
| ```javascript | ||
| { | ||
| "withdraw": { | ||
| "amount": "1000000" | ||
| "recipient": "cosmos1..." |
There was a problem hiding this comment.
The JSON snippet for the Withdraw function is missing a comma at the end of line 545. This syntax error could lead to confusion or errors if someone tries to use this snippet as is. It should be corrected to ensure syntactical accuracy.
"withdraw": {
"amount": "1000000",
"recipient": "cosmos1..."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - **Withdraw** allows to withdraw the funds from the contract | |
| ```javascript | |
| { | |
| "withdraw": { | |
| "amount": "1000000" | |
| "recipient": "cosmos1..." | |
| - **Withdraw** allows to withdraw the funds from the contract | |
| ```javascript | |
| { | |
| "withdraw": { | |
| "amount": "1000000", | |
| "recipient": "cosmos1..." |
There was a problem hiding this comment.
There's some duplicate code here
| @@ -40,7 +40,9 @@ pub enum QueryMsg { | |||
| /// operator set is "halted". | |||
| #[returns(PermsStatus)] | |||
| Perms {}, | |||
| // TODO: feat(broker-bank): Logs query | |||
There was a problem hiding this comment.
TODO not yet implemented, so we should keep the comment and open an issue
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to path filters (9)
Cargo.lockis excluded by:!**/*.lockCargo.tomlis excluded by:!**/*.tomlartifacts/broker_bank.wasmis excluded by:!**/*.wasmartifacts/broker_staking.wasmis excluded by:!**/*.wasmartifacts/core_token_vesting_v2.wasmis excluded by:!**/*.wasmartifacts/shifter.wasmis excluded by:!**/*.wasmcontracts/broker-bank/Cargo.tomlis excluded by:!**/*.tomlcontracts/broker-staking/Cargo.lockis excluded by:!**/*.lockcontracts/broker-staking/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (16)
- artifacts/checksums.txt (1 hunks)
- contracts/README.md (3 hunks)
- contracts/broker-bank/README.md (1 hunks)
- contracts/broker-bank/src/contract.rs (3 hunks)
- contracts/broker-bank/src/lib.rs (1 hunks)
- contracts/broker-bank/src/tutil.rs (2 hunks)
- contracts/broker-staking/.cargo/config (1 hunks)
- contracts/broker-staking/.gitignore (1 hunks)
- contracts/broker-staking/README.md (1 hunks)
- contracts/broker-staking/examples/schema.rs (1 hunks)
- contracts/broker-staking/src/contract.rs (1 hunks)
- contracts/broker-staking/src/lib.rs (1 hunks)
- contracts/broker-staking/src/msg.rs (1 hunks)
- contracts/broker-staking/src/testing.rs (1 hunks)
- contracts/core-token-vesting-v2/src/contract.rs (2 hunks)
- contracts/core-token-vesting-v2/src/state.rs (1 hunks)
Files skipped from review due to trivial changes (3)
- contracts/broker-staking/.cargo/config
- contracts/broker-staking/.gitignore
- contracts/core-token-vesting-v2/src/contract.rs
Files skipped from review as they are similar to previous changes (2)
- contracts/broker-bank/src/contract.rs
- contracts/broker-bank/src/lib.rs
Additional comments: 26
contracts/broker-staking/src/lib.rs (1)
- 1-5: LGTM! The module declarations are correctly defined.
contracts/broker-bank/README.md (1)
- 1-5: The description of the
broker-bankcontract is clear and concise.contracts/broker-staking/examples/schema.rs (1)
- 1-17: The schema generation script is correctly implemented and uses appropriate imports for message types.
artifacts/checksums.txt (1)
- 1-14: The checksum entries are correctly added and updated. The formatting is consistent with expected standards.
contracts/broker-bank/src/tutil.rs (1)
- 1-11: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-62]
The testing utilities are well-organized with appropriate conditional compilation for non-WASM targets. The setup functions for contracts are correctly implemented.
contracts/broker-staking/src/msg.rs (1)
- 1-61: The message types for the
broker-stakingcontract are well-defined, with clear documentation comments explaining each operation. The use ofBTreeSetfordenomsin theWithdrawmessage is a good practice to ensure uniqueness.contracts/core-token-vesting-v2/src/state.rs (1)
- 62-70: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-67]
The
vested_amountmethod in theVestingAccountimplementation is correctly calculated and clearly written. The explicit subtraction syntax enhances readability and maintainability.contracts/README.md (1)
- 9-37: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-47]
The updates to the README file correctly include new broker contracts and improve formatting for better readability. The content is well-organized and clear.
contracts/broker-staking/README.md (1)
- 66-66: The phrase "allows to withdraw" might be improved for clarity. Consider rephrasing to explicitly state the subject performing the action.
Consider rephrasing to "allows the admin to withdraw" for clarity.
contracts/broker-staking/src/contract.rs (6)
- 1-6: Ensure that all imported modules and functions are used within the file. Unused imports can lead to confusion and clutter the codebase.
Verification successful
All imported modules and functions (
assert_not_halted,edit_opers,execute_update_ownership,query_perms_status,toggle_halt,withdraw,withdraw_all) are utilized withincontracts/broker-staking/src/contract.rs. No unused imports were found.* 20-34: The `instantiate` function correctly initializes the contract state. Ensure that all necessary validations are performed on the input parameters to prevent any potential issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if all imported modules and functions are utilized. rg 'assert_not_halted|edit_opers|execute_update_ownership|query_perms_status|toggle_halt|withdraw|withdraw_all' contracts/broker-staking/src/contract.rsLength of output: 715
Consider adding validation for the
msg.to_addrsandmsg.opersfields to ensure they meet specific criteria (e.g., non-empty, valid addresses).
- 36-63: The
executefunction dispatches various contract actions based on the message type. It's crucial to ensure that permissions are correctly enforced for each action to prevent unauthorized access.Verify that permission checks (e.g., ownership or operator status) are correctly implemented for sensitive actions such as
Withdraw,ToggleHalt, andUpdateOwnership.
- 65-87: The
unstakefunction allows the contract owner to unstake funds. Ensure that the amount to unstake is validated against the contract's staked balance to prevent underflow or other issues.Consider adding a check to ensure that the total unstake amount does not exceed the contract's current staked balance.
- 89-132: The
stakefunction allows operators to stake funds. It's important to validate theamountandstake_msgsto ensure they are within acceptable ranges and conditions.Verify that the
amountto stake is validated against the contract's available balance to prevent over-staking. Additionally, ensure that thestake_msgsare validated for correctness.
- 160-179: The
queryfunction handles various query messages. Ensure that the responses are correctly formatted and contain all necessary information for each query type.Verify that the query responses for
Perms,Ownership, andIsHaltedcontain accurate and complete information.contracts/broker-staking/src/testing.rs (11)
- 3-11: Consider organizing imports to improve readability. Grouping standard library imports, third-party crates, and local module imports separately can make the code cleaner.
- 23-31: The
TestCaseExecstruct is well-defined for encapsulating test case parameters. However, consider adding documentation comments to explain the purpose of each field, especially for complex or non-obvious ones. This will enhance maintainability and readability for other developers.- 36-120: The test
test_assert_ownereffectively checks owner-gated execute calls. However, it's recommended to separate the test cases into individual test functions or use a parameterized testing framework likerstestfor clarity and easier debugging.- 124-233: In
exec_withdraw, consider verifying the correctness of theBankMsg::Sendamounts in the success cases. Currently, the test checks for the presence of expected messages but does not validate the amounts being sent, which is crucial for withdrawal functionality.- 236-252: The
CosmosMsgExtstruct and its implementations are a clever way to compareCosmosMsginstances. However, adding a brief comment explaining its purpose and usage would be beneficial for maintainability.- 254-303: In
exec_toggle_halt, the test effectively toggles the halt status and verifies the operation. However, it's recommended to split the error and success cases into separate test functions for clarity. Additionally, consider verifying the state change directly from the contract's storage instead of relying solely on query responses to ensure the state is correctly updated.- 309-352: For
exec_edit_opers_add, it's good practice to also verify the absence of the new member in the permissions set before the operation. This ensures the test covers the state change more comprehensively.- 355-407: In
exec_edit_opers_remove, consider adding a check to ensure the member to be removed exists in the permissions set before the operation. This addition would make the test more robust by covering the initial state more thoroughly.- 410-454: The test
exec_stake_haltedcorrectly checks the behavior when staking operations are halted. However, it would be beneficial to add a test case that verifies the successful staking operation whenIS_HALTEDis set tofalse, to ensure both states are covered.- 457-619: In
exec_stake, the test cases cover various scenarios effectively. However, for the fail cases, consider verifying that no state changes occurred in the contract as a result of the failed operation. This can be done by checking the contract's storage or balances before and after the operation.- 621-754: For
exec_unstake, similar to theexec_staketest, it's recommended to verify that no state changes occurred in the contract for the fail cases. Additionally, consider adding a test case to verify the behavior when attempting to unstake more than the available balance, to ensure robust error handling.
| This smart contract handles account abstraction to enable certain staking transaction messages | ||
| to be called by a subset of "operators", while the funds can only be withdrawn by the contract owner. | ||
|
|
||
| This is useful if you want a mutlisig to manage a large allocation of funds while |
There was a problem hiding this comment.
There's a typo in "mutlisig" which should be corrected to "multisig".
- This is useful if you want a mutlisig to manage a large allocation of funds while
+ This is useful if you want a multisig to manage a large allocation of funds whileCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This is useful if you want a mutlisig to manage a large allocation of funds while | |
| This is useful if you want a multisig to manage a large allocation of funds while |
|
|
||
| - stake funds to validators in the proportion given | ||
|
|
||
| This way, only the multisig can maange the funds, and the seed keys of the |
There was a problem hiding this comment.
The word "maange" is misspelled. It should be corrected to "manage".
- This way, only the multisig can maange the funds, and the seed keys of the
+ This way, only the multisig can manage the funds, and the seed keys of theCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| This way, only the multisig can maange the funds, and the seed keys of the | |
| This way, only the multisig can manage the funds, and the seed keys of the |
| } | ||
| ``` | ||
|
|
||
| - **unstakes** allows to unstake the funds from the contract |
There was a problem hiding this comment.
The function name "unstakes" seems to be inconsistent with the naming convention used for other functions. Function names should be verbs and correctly spelled.
- - **unstakes** allows to unstake the funds from the contract
+ - **Unstake** allows to unstake the funds from the contractCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - **unstakes** allows to unstake the funds from the contract | |
| - **Unstake** allows to unstake the funds from the contract |
|
|
||
| ##### Manager functions | ||
|
|
||
| - **stake** allows to stake the funds from the contract. The shares are normalized |
There was a problem hiding this comment.
The function name "stake" should start with an uppercase letter to maintain consistency with other function names in the document.
- - **stake** allows to stake the funds from the contract. The shares are normalized
+ - **Stake** allows to stake the funds from the contract. The shares are normalizedCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - **stake** allows to stake the funds from the contract. The shares are normalized | |
| - **Stake** allows to stake the funds from the contract. The shares are normalized |
|
|
||
| #### Query | ||
|
|
||
| - **auto compounder mode** returns wether the auto compounder mode is enabled or not |
There was a problem hiding this comment.
The word "wether" is misspelled. It should be corrected to "whether".
- - **auto compounder mode** returns wether the auto compounder mode is enabled or not
+ - **auto compounder mode** returns whether the auto compounder mode is enabled or notCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - **auto compounder mode** returns wether the auto compounder mode is enabled or not | |
| - **auto compounder mode** returns whether the auto compounder mode is enabled or not |
|
|
||
| ### Deployed Contract Info | ||
|
|
||
| TODO for mainnet/testnet |
There was a problem hiding this comment.
The "TODO for mainnet/testnet" placeholder suggests that information is missing. It's important to complete this section before the final release.
Would you like assistance in completing this section?
contracts/broker-bank/README.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| # contracts/broker-bank | |||
|
|
|||
| Account abstration to enable funds to be held and sent to a whitelisted set of | |||
There was a problem hiding this comment.
There's a typo in "abstration"; it should be "abstraction".
- Account abstration to enable funds to be held and sent to a whitelisted set of
+ Account abstraction to enable funds to be held and sent to a whitelisted set ofCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Account abstration to enable funds to be held and sent to a whitelisted set of | |
| Account abstraction to enable funds to be held and sent to a whitelisted set of |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- Cookbook.md (5 hunks)
Additional comments: 4
Cookbook.md (4)
- 6-34: The reorganization of section headings and subheadings improves the document's structure and readability. The addition of "Airdrop token vesting" and "Auto compounder" contracts is clearly marked and follows the established documentation format.
- 437-509: The "Airdrop token vesting" contract documentation is well-structured, providing clear instructions for instantiation, execution, and querying. However, ensure that the example JSON snippets are correctly formatted and that any specific details about the contract's functionality are accurately described.
- 511-618: The "Auto compounder" contract documentation aligns with the PR objectives of enhancing security and efficiency in staking re-delegation processes. It's well-structured, covering instantiation, execution (including admin and manager functions), and querying. Ensure that the JSON snippets are syntactically correct and that the descriptions accurately reflect the contract's capabilities.
- 582-598: The documentation for the stake function mentions that "The shares are normalized," but it lacks clarity on what "normalized" means in this context. Adding a brief explanation would help users understand how the function operates and what to expect when using it.
Consider adding a sentence or two explaining the normalization process for shares in the context of staking to enhance clarity for the readers.
|
|
||
| ```javascript | ||
| { | ||
| "withdraw": { | ||
| "amount": "1000000" | ||
| "recipient": "cosmos1..." | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
There's a missing comma at the end of line 546 in the JSON snippet for the Withdraw function. This syntax error could lead to confusion or errors if someone tries to use this snippet as is. It should be corrected to ensure syntactical accuracy.
{
"withdraw": {
"amount": "1000000",
+ "recipient": "cosmos1..."
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - **Withdraw** allows to withdraw the funds from the contract | |
| ```javascript | |
| { | |
| "withdraw": { | |
| "amount": "1000000" | |
| "recipient": "cosmos1..." | |
| } | |
| } | |
| ``` | |
| - **Withdraw** allows to withdraw the funds from the contract | |
| ```javascript | |
| { | |
| "withdraw": { | |
| "amount": "1000000", | |
| "recipient": "cosmos1..." | |
| } | |
| } |
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
This contracts allows to let anyone handle our staking re-delegation to avoid risk of having the seeds of a big account leaked.
Summary by CodeRabbit
New Features
Compoundercontract for managing funds within a multisig setup, including permissions for admins and managers.Documentation
Refactor
Tests
Chores
.gitignoreto ignore unnecessary files and directories.