feat: Restake Permission #640
Conversation
… test from unit to integration test
WalkthroughThe pull request introduces multiple enhancements to the Changes
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: 1
🧹 Outside diff range and nitpick comments (5)
contracts/finance/andromeda-validator-staking/src/mock.rs (2)
45-53: Consider documenting valid action stringsThe implementation looks good, but since the PR mentions that "restake" is hardcoded, it would be helpful to document this.
Add a doc comment to clarify valid action strings:
+ /// Executes a permission action + /// Currently supported actions: + /// - "restake": Allows permitted actors to perform restaking
Line range hint
33-189: Consider future extensibility of permission systemThe PR mentions potential future work to extend permissions to the
Claimaction. The current implementation provides a good foundation for this extension through the generic action string parameter.However, to make the permission system more maintainable and type-safe, consider:
- Using an enum for action types instead of strings
- Adding a permission registry to track and validate supported actions
contracts/finance/andromeda-validator-staking/src/contract.rs (3)
298-301: Avoid unnecessary mutable reference todepsIt appears that
depsis not modified in theexecute_claimfunction. Consider passingdepsas an immutable reference to avoid unnecessary mutability.Apply this diff:
- mut deps, + deps,
310-310: Remove unnecessary clone ofenvCloning
envis unnecessary when accessing the contract address. Accessenv.contract.addressdirectly to improve efficiency.Apply this diff:
- let delegator = env.clone().contract.address; + let delegator = env.contract.address;
346-351: Consider using immutabledepsreference inis_permissionedIf
is_permissioneddoes not require a mutabledepsreference, consider passingdeps.as_ref()instead ofdeps.branch()to avoid unnecessary cloning.Apply this diff:
- ADOContract::default().is_permissioned( - deps.branch(), + ADOContract::default().is_permissioned( + deps.as_ref(), env, RESTAKING_ACTION, info.sender, )?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
contracts/finance/andromeda-validator-staking/src/contract.rs(5 hunks)contracts/finance/andromeda-validator-staking/src/mock.rs(3 hunks)contracts/finance/andromeda-validator-staking/src/testing/tests.rs(0 hunks)packages/andromeda-finance/src/validator_staking.rs(1 hunks)tests-integration/tests/validator_staking.rs(5 hunks)
💤 Files with no reviewable changes (1)
- contracts/finance/andromeda-validator-staking/src/testing/tests.rs
✅ Files skipped from review due to trivial changes (1)
- packages/andromeda-finance/src/validator_staking.rs
🔇 Additional comments (10)
contracts/finance/andromeda-validator-staking/src/mock.rs (2)
12-16: LGTM: Required imports for permission functionality
The new imports appropriately bring in the necessary types for implementing the permission system.
33-43: LGTM: Permission setting implementation
The method correctly implements permission setting functionality. However, let's verify the action string handling.
✅ Verification successful
Implementation verified - Action string handling is appropriate
The execute_set_permission method correctly handles action strings by:
- Using the standard permissioning message structure from the ADO contract base
- Properly passing through the action string to the underlying permissioning system
- Following the established pattern where actions are validated at runtime by the permissioning module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the hardcoded "restake" action string usage
# as mentioned in PR objectives
# Search for hardcoded "restake" action strings
rg -l "\"restake\"" --type rust
# Search for any constants or enums defining action names
ast-grep --pattern 'const $CONST: &str = "restake"'
Length of output: 437
Script:
#!/bin/bash
# Let's try a broader search for action strings and permission handling
# Search for action string patterns in the codebase
rg -l "action.*String" --type rust
# Search for permission-related code
rg -l "Permission" --type rust
# Look for test files that might validate action strings
fd -e rs -e toml . | xargs rg -l "permission|action"
Length of output: 10114
Script:
#!/bin/bash
# Let's examine the permission and action handling in the validator staking contract
# Check the validator staking contract implementation
rg -A 5 "action: String" contracts/finance/andromeda-validator-staking/src/contract.rs
# Look for action string validation or constants
rg -A 5 "pub const.*ACTION" contracts/finance/andromeda-validator-staking/src/
# Check test cases for permission actions
rg -A 10 "execute_set_permission" contracts/finance/andromeda-validator-staking/src/testing/tests.rs
Length of output: 270
Script:
#!/bin/bash
# Let's check the base implementation and common actions
# Check the base permissioning implementation
rg -A 5 "action: String" packages/std/src/ado_contract/permissioning.rs
# Look for common action definitions
rg -A 5 "pub const" packages/std/src/common/actions.rs
# Check the mock implementation context
rg -A 10 "mock_set_permission" contracts/finance/andromeda-validator-staking/src/mock.rs
Length of output: 1318
contracts/finance/andromeda-validator-staking/src/contract.rs (2)
13-13: Importing RESTAKING_ACTION constant
The addition of RESTAKING_ACTION is appropriate for the new restaking functionality.
361-365: Verify the necessity of the ownership check
The ownership check may be redundant if permissions are already enforced earlier. Verify whether this check is necessary or if it can be consolidated to improve code clarity.
To confirm the necessity of this check, review the authorization logic for potential redundancy.
tests-integration/tests/validator_staking.rs (6)
6-6: Use of AndrAddr for Address Handling
The addition of use andromeda_std::amp::AndrAddr; is appropriate for managing addresses within the module, ensuring consistency with the Andromeda standard.
24-27: Adding 'other' Wallet for Unauthorized Access Testing
Including the wallet "other" enhances the test's ability to verify unauthorized access scenarios, specifically when attempting to claim rewards without proper permissions.
89-96: Testing Unauthorized Claim Correctly
The implementation correctly tests that an unauthorized wallet ("other") receives a ContractError::Unauthorized when attempting to claim rewards. This ensures that access control is properly enforced.
182-186: Adding Wallets for Permissioned Action Testing
Introducing "permissioned_actor" and "random_actor" wallets allows for comprehensive testing of permissioned actions versus unauthorized attempts, improving the robustness of the test suite.
243-248: Updating execute_claim_reward with Restake Option
The addition of the Some(true) parameter enables the restake functionality within the execute_claim_reward method. Ensure that all calls to this function are updated accordingly and that the restake logic is properly handled.
266-309: Comprehensive Testing of Permissioned Restake Functionality
The test effectively verifies that only permissioned actors can perform the restake action:
- Permissions are set using
execute_set_permissionfor the"permissioned_actor". - An unauthorized attempt by
"random_actor"correctly results in aContractError::Unauthorized. - The
"permissioned_actor"successfully performs theexecute_claim_rewardwith restake.
This enhances the test coverage and confirms that permission checks are functioning as expected.
Motivation
@crnbarr93 's suggestion
Implementation
Contract owner should permission the
restakeaction, then set permission to actors to restake on his behalf. Normal claiming is still restricted to contract owner only.Testing
Added integration test.
Version Changes
validator-staking: No changeNotes
The "restake" action is hard-coded, not derived from an
ExecuteMsgname.Future work
Give permission for
Claimas well?Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation