feat: Restake & Redelegate for Validator Staking #622
Conversation
WalkthroughThe pull request includes significant updates across multiple files, primarily focusing on enhancements to the validator staking functionality. Key changes involve the introduction of restaking and redelegation features, modifications to the vesting mechanism, and updates to the changelog. Several new methods and error variants have been added to improve functionality and error handling. Additionally, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (11)
packages/std/Cargo.toml (1)
14-14: Consider documenting the distribution featureThe new
distributionfeature adds Osmosis chain integration capabilities. Consider adding documentation about:
- When to use this feature
- Its relationship with validator staking
- Prerequisites for using Osmosis chain functionality
Would you like me to help create a feature documentation template?
packages/andromeda-finance/src/validator_staking.rs (1)
30-31: Consider using a newtype pattern for better type safety.While the
restakefield documentation indicates a default of false, consider making this more explicit in the type system.Here's a suggested improvement:
- /// Defaults to false - restake: Option<bool>, + #[serde(default)] + restake: bool,This makes the default more explicit and removes the need to handle
Nonecases in the implementation.packages/std/src/common/distribution.rs (2)
65-81: Consider adding validation helpers for Coin structureWhile the structure correctly mirrors the Cosmos SDK Coin type, consider adding helper methods for:
- Validating denomination format
- Parsing and validating amount strings
- Preventing negative amounts
Example helper methods:
impl Coin { pub fn validate(&self) -> Result<(), String> { // Validate denom format if self.denom.is_empty() { return Err("denomination cannot be empty".to_string()); } // Parse and validate amount self.amount.parse::<u128>() .map_err(|_| "invalid amount format".to_string())?; Ok(()) } }
1-103: Consider implementing comprehensive message validation moduleWhile the message structures are well-defined, consider creating a separate validation module to centralize all message validation logic. This would help ensure consistent validation across all distribution and staking operations.
This validation module could:
- Validate address formats
- Ensure amounts are within acceptable ranges
- Implement cross-field validation rules
- Provide reusable validation utilities
contracts/finance/andromeda-validator-staking/src/mock.rs (1)
Line range hint
1-143: Missing mock implementation forRedelegatefunctionality.The PR objectives mention adding
Redelegatefunctionality, but there's no corresponding mock implementation (mock_execute_redelegate) in this file. This is needed for comprehensive testing of the new feature.Add the following mock implementation:
+pub fn mock_execute_redelegate( + src_validator: Addr, + dst_validator: Addr, + amount: Uint128, +) -> ExecuteMsg { + ExecuteMsg::Redelegate { + src_validator, + dst_validator, + amount, + } +}Also, consider adding a helper method to
MockValidatorStaking:+impl MockValidatorStaking { + pub fn execute_redelegate( + &self, + app: &mut MockApp, + sender: Addr, + src_validator: Addr, + dst_validator: Addr, + amount: Uint128, + ) -> ExecuteResult { + let msg = mock_execute_redelegate(src_validator, dst_validator, amount); + self.execute(app, &msg, sender, &[]) + } +}contracts/finance/andromeda-validator-staking/src/testing/tests.rs (1)
Line range hint
1-174: Critical: Missing test coverage for new featuresThe PR adds Redelegate and Restake functionalities, but there are no test cases covering these new features. Specifically missing:
- Test cases for the Redelegate functionality
- Test cases for the Restake functionality (both true and false cases)
- Test cases for the DefaultValidator query message
Would you like me to help generate comprehensive test cases for these new features? Here's what we should cover:
Redelegate tests:
- Successful redelegation
- Redelegation with amount exceeding limit
- Unauthorized redelegation
- Invalid validator scenarios
Restake tests:
- Successful restaking of claimed rewards
- Restaking with insufficient rewards
- Verification of execute_stake call in reply
DefaultValidator query tests:
- Successful query
- Response validation
Let me know if you'd like me to provide the implementation for any of these test cases.
ibc-tests/tests/validator_staking.rs (1)
Line range hint
1-214: Add test coverage for the redelegation functionality.The PR adds redelegation support but there are no tests covering this functionality. Please add test cases to verify:
- Basic redelegation between validators
- Redelegation amount limits
- Error cases (e.g., exceeding limits, invalid validators)
- Edge cases (e.g., redelegating to the same validator)
Would you like me to help create a comprehensive test suite for the redelegation functionality?
packages/std/src/error.rs (1)
132-133: Consider enhancing the error message for better clarity.The error message could be more descriptive to help users understand the context where events were expected.
- #[error("EmptyEvents")] + #[error("No events found when events were expected")]contracts/finance/andromeda-validator-staking/src/contract.rs (3)
Line range hint
284-353: Incorrect handling of restaking rewards inexecute_claimThe
execute_claimfunction saves the entire delegationresintoRESTAKING_QUEUE, which may not contain the necessary data for restaking and could lead to deserialization errors.Proposed fix: Save only necessary reward information
Modify the code to save the accumulated rewards and validator information needed for restaking:
let restake = restake.unwrap_or(false); // Only one denom is allowed to be restaked at a time let res = if restake && res.accumulated_rewards.len() == 1 { + let reward_info = RestakeRewardInfo { + delegator: delegator.clone(), + validator: validator.clone(), + rewards: res.accumulated_rewards.clone(), + }; + RESTAKING_QUEUE.save(deps.storage, &reward_info)?; Response::new() .add_submessage(SubMsg::reply_always( withdraw_msg, ReplyId::RestakeReward.repr(), )) .add_attribute("action", "validator-claim-reward") .add_attribute("validator", validator.to_string()) } else { Response::new() .add_message(withdraw_msg) .add_attribute("action", "validator-claim-reward") .add_attribute("validator", validator.to_string()) };Additionally, define the
RestakeRewardInfostruct instate.rs:pub struct RestakeRewardInfo { pub delegator: Addr, pub validator: Addr, pub rewards: Vec<Coin>, }
339-353: Inconsistent behavior when restaking with multiple reward denomsIn
execute_claim, ifrestakeis true but there are multiple reward denominations, the function ignores the restake request without notifying the user, which may lead to confusion.Suggestion: Inform the user when restake is not possible due to multiple denominations
Provide feedback to the user when restaking cannot proceed:
} else { + if restake && res.accumulated_rewards.len() != 1 { + return Err(ContractError::CustomError { + val: "Restaking is only supported when there is exactly one reward denomination.".to_string(), + }); + } Response::new()This change ensures the user is aware that restaking was not performed due to the presence of multiple reward denominations.
478-482: Error handling in event attribute parsingIn
on_validator_unstake, ifcompletion_timeis not found in the event attributes,payout_atremains asTimestamp::default(), which may not be the intended behavior.Suggestion: Handle missing
completion_timeappropriatelyAdd error handling to ensure
completion_timeis found:let mut payout_at = None; for attr in attributes { if attr.key == "completion_time" { let completion_time = DateTime::parse_from_rfc3339(&attr.value).unwrap(); let seconds = completion_time.timestamp() as u64; let nanos = completion_time.timestamp_subsec_nanos() as u64; payout_at = Some(Timestamp::from_seconds(seconds).plus_nanos(nanos)); } } - let payout_at = payout_at.unwrap_or(Timestamp::default()); + let payout_at = payout_at.ok_or(ContractError::CustomError { + val: "completion_time attribute not found in events".to_string(), + })?;This ensures that the contract does not proceed with a default timestamp if the
completion_timeis missing, preventing potential issues with fund release timings.
📜 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 (14)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)contracts/finance/andromeda-validator-staking/Cargo.toml(2 hunks)contracts/finance/andromeda-validator-staking/src/contract.rs(12 hunks)contracts/finance/andromeda-validator-staking/src/mock.rs(1 hunks)contracts/finance/andromeda-validator-staking/src/state.rs(1 hunks)contracts/finance/andromeda-validator-staking/src/testing/tests.rs(1 hunks)contracts/os/andromeda-kernel/src/mock.rs(1 hunks)ibc-tests/tests/validator_staking.rs(2 hunks)packages/andromeda-finance/src/validator_staking.rs(3 hunks)packages/std/Cargo.toml(3 hunks)packages/std/src/common/distribution.rs(1 hunks)packages/std/src/common/mod.rs(1 hunks)packages/std/src/error.rs(2 hunks)
🔇 Additional comments (20)
contracts/finance/andromeda-validator-staking/src/state.rs (2)
4-4: LGTM!
The import of FullDelegation is properly organized and consistent with its usage in the new constant.
10-10:
Consider using Vec for RESTAKING_QUEUE
The current implementation stores only a single FullDelegation, which might be limiting if multiple delegations need to be queued for restaking. Consider using Vec<FullDelegation> similar to how UNSTAKING_QUEUE is implemented.
Apply this diff to support multiple delegations:
-pub const RESTAKING_QUEUE: Item<FullDelegation> = Item::new("restaking_queue");
+pub const RESTAKING_QUEUE: Item<Vec<FullDelegation>> = Item::new("restaking_queue");Let's verify if multiple delegations might need to be queued:
contracts/finance/andromeda-validator-staking/Cargo.toml (1)
24-24: LGTM: Distribution feature addition
The addition of the "distribution" feature to andromeda-std is appropriate for implementing the new Redelegate and Restake functionalities.
packages/std/Cargo.toml (2)
3-3: Verify semver compliance for breaking changes
The version bump to 1.2.4-b.1 indicates a beta release. Given the addition of new staking functionality, consider if this warrants a minor version bump to 1.3.0-b.1 instead, following semver guidelines.
#!/bin/bash
# Check if there are any breaking changes in the codebase
# Look for removed or renamed public functions/types
# Search for removed or modified public functions
rg -B 2 -A 2 'pub fn|pub struct|pub enum'
# Check the changelog for breaking changes
cat CHANGELOG.md43-46: Verify dependency versions and security implications
The new dependencies introduce Osmosis chain integration and Protocol Buffers support:
osmosis-std-derive v0.15.3: Verify compatibility with the Osmosis chain version used in productionprost v0.11.2: Check for any known security advisories
✅ Verification successful
Based on the GitHub security advisory results, I can generate the final response since we have definitive information about security vulnerabilities for prost.
Dependencies are using secure versions
The version of prost being used (0.11.2) is well above the vulnerable version range (< 0.6.1) that had a critical security issue related to out-of-bounds write, which was patched in version 0.6.1.
While we couldn't verify the latest versions due to cargo command limitations in the sandbox, the current versions appear to be secure based on:
prost v0.11.2is significantly newer than the vulnerable version- No other security advisories were found for more recent versions
For additional confidence, you may want to:
- Verify Osmosis chain compatibility in your deployment environment
- Check the latest versions in the official crate registry at crates.io
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories and latest versions
# Check for security advisories in dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: RUST, package: "prost") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check latest versions
cargo search osmosis-std-derive --limit 1
cargo search prost --limit 1
Length of output: 755
contracts/os/andromeda-kernel/src/mock.rs (1)
23-23: LGTM! Improved environment naming clarity.
The change from "andromeda" to "andromeda-local" better indicates that this is a local/test environment, which is a good practice for preventing confusion.
packages/andromeda-finance/src/validator_staking.rs (3)
58-59: LGTM! Query message is well-structured.
The DefaultValidator query is properly implemented with correct return type specification using the returns attribute.
23-27: LGTM! Verify redelegation constraints.
The Redelegate message structure is well-designed with appropriate optionality for fields. The required dst_validator ensures a valid target for redelegation.
Let's verify the validator-related constraints in the contract implementation:
✅ Verification successful
Redelegation implementation properly validates validators and amounts
The implementation shows appropriate validation:
- Both source and destination validators are checked using
is_validatorfunction - Source validator defaults to stored DEFAULT_VALIDATOR if not provided
- The
is_validatorfunction properly queries validator status and returns error for invalid validators - Amount validation is handled through querier delegation checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for validator validation and redelegation amount checks
# Expected: Implementation should validate both validators and check redelegation amounts
# Search for validator validation
ast-grep --pattern 'fn $_ ($_, $validator: $_) {
$$$
is_validator($_, $validator)
$$$
}'
# Search for amount validation in redelegation
rg -A 5 'fn.*execute.*[Rr]edelegate.*amount.*'
Length of output: 231
Script:
#!/bin/bash
# Let's try a different approach to find validator-related validation logic
# Search for validator validation patterns
rg -A 10 "validator.*validate|validate.*validator" --type rust
# Search for redelegation implementation
rg -A 15 "fn.*[Rr]edelegate" --type rust
# Search for any validator-related error handling
rg "Error.*validator|validator.*Error" --type rust
# Look for staking-related messages and validation
rg -A 10 "StakingMsg::(Begin|End)Redelegate" --type rust
Length of output: 2541
Script:
#!/bin/bash
# Let's examine the implementation of is_validator and the complete redelegation function
# Check the is_validator implementation
rg -A 10 "fn is_validator" --type rust
# Get more context around the redelegation implementation
rg -B 5 -A 20 "fn execute_redelegate" --type rust
# Look for any validation-related constants or error definitions
rg "pub.*Error.*validator|pub.*enum.*Error" --type rust
Length of output: 3525
76-79: Consider adding validator address validation.
While the response struct is well-structured, ensure that the contract implementation validates that default_validator is actually a validator address before returning it.
Let's verify the validation implementation:
Cargo.toml (1)
32-34: Verify if additional features are needed for staking functionality
Given the new Redelegate and Restake functionalities being added, please verify if additional features need to be enabled in andromeda-std.
✅ Verification successful
No additional features needed for staking functionality
Based on the verification results:
- The
distributionfeature is already defined in andromeda-std and is properly gated - Staking-related functionality in the contracts is using regular imports and doesn't require special feature flags
- The existing error types and distribution module support validator operations without additional feature requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if staking-related features exist in andromeda-std
# Check features in andromeda-std's Cargo.toml
echo "Checking available features in andromeda-std:"
cat "./packages/std/Cargo.toml" | rg '^\[features\]' -A 10
# Look for staking-related modules or types
echo -e "\nChecking for staking-related modules:"
fd -e rs . "./packages/std/src" -x rg -l 'staking|delegate|validator'
Length of output: 582
Script:
#!/bin/bash
# Let's examine the content of the files that matched our search
echo "Checking staking-related content in error.rs:"
cat "./packages/std/src/error.rs" | rg -A 2 -B 2 'staking|delegate|validator'
echo -e "\nChecking staking-related content in distribution.rs:"
cat "./packages/std/src/common/distribution.rs" | rg -A 2 -B 2 'staking|delegate|validator'
# Let's also check if there are any staking-related types or traits
echo -e "\nChecking for staking-related types and traits:"
ast-grep --pattern 'struct $_ {
$$$
staking: $_,
$$$
}'
# Check for any delegation-related messages or types
ast-grep --pattern 'enum $_ {
$$$
Delegate$_,
$$$
}'
Length of output: 1555
Script:
#!/bin/bash
# Let's check the actual feature usage in the code being modified
echo "Checking feature usage in the modified code:"
rg -l 'andromeda_std::.*distribution|andromeda_std::.*staking' ./contracts/
# Let's also check if there are any distribution-related features being used
echo -e "\nChecking distribution feature usage:"
rg '#\[cfg\(feature = "distribution"\)\]' -A 5 ./packages/std/src/
# Check for any staking-related imports or uses
echo -e "\nChecking staking-related imports:"
rg '^use .*staking' ./contracts/
Length of output: 2674
packages/std/src/common/distribution.rs (4)
1-2: LGTM: Appropriate use of CosmwasmExt
The import of CosmwasmExt is correct for defining blockchain message types.
34-63: LGTM: Consistent message definition pattern
The message structure follows the established pattern and correctly defines the delegator reward withdrawal operation.
83-103: LGTM: Correctly implements Cosmos SDK staking message
The message structure properly implements the unbonding delegation cancellation message from the Cosmos SDK.
#!/bin/bash
# Verify consistency with other staking message definitions
rg -g '*.rs' 'proto_message\(type_url = "/cosmos\.staking\.v1beta1\.'3-32: Verify proto message URL namespace consistency
The message structure is well-defined, but let's ensure the proto message URLs follow the established pattern.
✅ Verification successful
Proto message URLs are consistent within the distribution module
The proto message URLs for distribution-related messages consistently follow the /andromeda.distribution.v1beta1 namespace pattern within the distribution.rs file. All distribution messages (MsgSetWithdrawAddress, MsgWithdrawDelegatorReward, and their responses) maintain this consistency. The only different namespaces used are for standard Cosmos types (cosmos.base.v1beta1 and cosmos.staking.v1beta1), which is correct as these are core Cosmos SDK types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check consistency of proto message URLs across the codebase
rg -g '*.rs' 'proto_message\(type_url = "/andromeda\.distribution\.v1beta1\.'
# Check for any other distribution message definitions
rg -g '*.proto' 'package andromeda.distribution'
Length of output: 289
Script:
#!/bin/bash
# Search for all proto message URLs in the distribution.rs file
rg -g 'distribution.rs' '#\[proto_message\(type_url = ".*"\)'
# Search for other proto files that might define distribution messages
fd -e proto | xargs cat
# Search for any other distribution-related message definitions
rg -g '*.rs' 'distribution.*Message'
Length of output: 873
contracts/finance/andromeda-validator-staking/src/testing/tests.rs (1)
170-170: LGTM: Restake parameter added correctly
The restake parameter has been properly added to the ExecuteMsg::Claim message in the unauthorized claim test.
ibc-tests/tests/validator_staking.rs (2)
Line range hint 28-41: Verify test environment configuration for validator operations.
The test environment uses a local Terra testnet. Please ensure:
- The network configuration supports validator redelegation operations
- The test mnemonic has sufficient permissions and funds
- The gas settings are appropriate for complex operations like redelegation
Let's verify the network configuration:
#!/bin/bash
# Check network configuration files for validator settings
rg -A 5 "validator|staking|redelegat" --type yaml180-180:
Add test coverage for restaking with unbonded validators.
The test should verify the behavior of restaking with unbonded validators. Consider adding test cases to:
- Verify that restaking attempts with unbonded validators are properly handled
- Ensure appropriate error messages are returned
Let's check if there are any validation checks for unbonded validators in the contract:
packages/std/src/common/mod.rs (1)
4-5: LGTM! Verify feature flag configuration.
The addition of the distribution module with conditional compilation is well-structured and follows Rust best practices. The placement maintains alphabetical ordering of modules.
Let's verify the feature flag setup and module usage:
✅ Verification successful
Let me gather more information about the feature flag configuration by searching in other potential Cargo.toml files within the packages directory.
Let me check the content of the distribution module to understand its implementation and dependencies.
Based on the gathered information, I can now provide the final response:
✅ Feature flag and module implementation verified
The distribution module is properly implemented with:
- Correct feature flag configuration in packages/std/Cargo.toml with required dependencies (
prostandosmosis-std-derive) - Well-structured protobuf message definitions for validator staking operations
- Proper implementation of distribution-related messages (SetWithdrawAddress, WithdrawDelegatorReward)
- Clean integration with no current usage, indicating this is a new module ready for consumption
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify feature flag configuration and module usage
# Check if the distribution feature is properly declared in Cargo.toml
echo "Checking feature declaration in Cargo.toml:"
rg '"distribution"' -A 5 -B 5 'Cargo.toml'
# Check if the distribution module is used in validator staking implementation
echo "Checking distribution module usage:"
rg 'use .*distribution' packages/
# Check for any TODO or FIXME comments related to distribution
echo "Checking for pending tasks:"
rg -i 'todo|fixme|hack|xxx|bug.*distribution' packages/
Length of output: 1024
Script:
#!/bin/bash
# Search for Cargo.toml files in packages directory and check for distribution feature
echo "Checking all Cargo.toml files in packages:"
fd Cargo.toml packages/ --exec grep -A 5 -B 5 'features\|distribution' {}
# Check if the distribution module is imported or used in any way
echo "Checking for any distribution module references:"
rg -l 'distribution' packages/std/src/
Length of output: 9775
Script:
#!/bin/bash
# Check the content of the distribution module
echo "Content of distribution module:"
cat packages/std/src/common/distribution.rs
# Check for any imports or usage of the distribution module in other files
echo -e "\nUsage of distribution module:"
rg -C 3 'use .*distribution|use super::distribution' packages/std/src/
Length of output: 2849
packages/std/src/error.rs (2)
96-97: LGTM! Well-structured error variant for redelegation amount validation.
The InvalidRedelegationAmount error variant is well-designed with descriptive fields that will help users understand the exact cause of validation failures during redelegation operations.
135-136: LGTM! Clear error variant for unstaking queue validation.
The EmptyUnstakingQueue error variant is well-defined and its message clearly indicates the issue.
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "andromeda-validator-staking" | |||
| version = "0.2.3" | |||
| version = "0.3.0-b.1" | |||
There was a problem hiding this comment.
Version bump requires comprehensive testing
The version increase from 0.2.3-b.2 to 0.3.0-b.1 indicates breaking changes with the addition of Redelegate and Restake features. However, the PR notes that no tests were conducted, which is concerning for a major version change.
Please ensure that:
- Comprehensive tests are added for the new features
- Existing functionality is verified to work with the changes
- Breaking changes are documented in CHANGELOG.md
crnbarr93
changed the base branch from
development
to
connor/validator-staking-fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
contracts/finance/andromeda-validator-staking/src/contract.rs (2)
Line range hint
292-366: Consider handling multiple reward denominationsThe current implementation only allows restaking when there's exactly one reward denomination. Consider supporting multiple denominations by:
- Allowing the user to specify which denomination to restake
- Restaking all denominations in separate transactions
- let res = if restake && res.accumulated_rewards.len() == 1 { + let res = if restake { + // Handle multiple denominations if needed + let rewards_to_restake = match res.accumulated_rewards.len() { + 1 => res.accumulated_rewards, + _ => vec![res.accumulated_rewards[0].clone()], // Or allow user to specify which one + };
Line range hint
491-507: Improve error handling for completion time parsingThe completion time parsing in
on_validator_unstakeusesunwrap()which could panic. Consider proper error handling:- let completion_time = DateTime::parse_from_rfc3339(&attr.value).unwrap(); + let completion_time = DateTime::parse_from_rfc3339(&attr.value) + .map_err(|_| ContractError::InvalidCompletionTime { + time: attr.value.clone(), + })?;tests-integration/tests/validator_staking.rs (3)
72-74: TODO comments need to be addressedThere are multiple TODO comments about error downcasting that need to be addressed. This could impact the robustness of error handling in the tests.
Would you like me to help implement proper error downcasting for these test cases?
Also applies to: 289-293
165-234: Enhance test coverage for restaking functionalityWhile the test covers the happy path, consider adding the following test cases:
- Attempting to restake with insufficient rewards
- Verification of emitted events/responses
- Multiple consecutive restaking operations
- Restaking with different reward amounts
Here's a suggested addition to test insufficient rewards:
+ // Test restaking with insufficient rewards + validator_staking + .execute_claim_reward(&mut router, owner.clone(), Some(validator_1), Some(true)) + .unwrap_err();
236-362: Improve redelegation test coverage and maintainabilityThe test has good coverage of basic error cases, but consider the following improvements:
- Test redelegating to the same validator (should fail)
- Test multiple consecutive redelegations
- Extract hardcoded error message strings into constants
Here's a suggested improvement:
+ const INVALID_REDELEGATION_AMOUNT: &str = "10000"; + const MAX_REDELEGATION_AMOUNT: &str = "1000"; + assert_eq!( err, ContractError::InvalidRedelegationAmount { - amount: "10000".to_string(), - max: "1000".to_string() + amount: INVALID_REDELEGATION_AMOUNT.to_string(), + max: MAX_REDELEGATION_AMOUNT.to_string() } ); + // Test redelegating to the same validator + let err = validator_staking + .execute_redelegate( + &mut router, + owner.clone(), + None, + validator_2.clone(), // Already delegated to validator_2 + None, + ) + .unwrap_err() + .downcast::<ContractError>() + .unwrap(); + assert_eq!(err, ContractError::InvalidRedelegation {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
contracts/finance/andromeda-validator-staking/src/contract.rs(11 hunks)contracts/finance/andromeda-validator-staking/src/mock.rs(3 hunks)tests-integration/tests/validator_staking.rs(5 hunks)
🔇 Additional comments (9)
contracts/finance/andromeda-validator-staking/src/mock.rs (4)
29-39: LGTM! Implementation follows established patterns.
The execute_redelegate method is well-structured and consistent with other execute methods in the mock implementation.
57-57: LGTM! Signature update matches requirements.
The addition of the restake parameter aligns with the PR objectives.
Also applies to: 59-59
136-137: LGTM! Simple and correct implementation.
The mock function correctly constructs the claim message with the new restake parameter.
120-130: Verify validator optionality requirements.
The dst_validator parameter is non-optional while src_validator is optional. Please verify if this aligns with the underlying blockchain's requirements and typical redelegation patterns.
✅ Verification successful
Let me gather more information about the redelegation implementation by checking the contract execution logic.
Let me check the underlying CosmosSDK staking module's redelegation message structure to verify if this aligns with the blockchain's requirements.
Based on the gathered evidence, I can now provide the final response:
The non-optional dst_validator parameter is correctly implemented
The implementation aligns with the CosmosSDK staking module's requirements where:
- The destination validator must be specified for redelegation (non-optional)
- The source validator is optional, defaulting to the current validator
- The amount is optional, defaulting to the full delegated amount
This is verified by:
- The CosmosSDK staking message structure in the contract
- The validation logic that checks destination validator's validity
- Integration tests showing correct usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if destination validator is always required in other redelegation implementations
# Search for other redelegation message definitions
rg -A 5 "Redelegate.*\{" --type rust
# Search for redelegation tests to understand usage patterns
rg -A 10 "test.*redelegate" --type rust
Length of output: 3213
Script:
#!/bin/bash
# Check the execute_redelegate function implementation and its validation logic
ast-grep --pattern 'fn execute_redelegate($$$) {
$$$
}'
# Check if there are any validation tests for redelegation
rg -B 5 -A 15 "execute_redelegate" --type rust
Length of output: 12250
Script:
#!/bin/bash
# Check the CosmosSDK staking message structure being used
rg -A 5 "StakingMsg::Redelegate" --type rust
# Check if there are any validation or error handling related to dst_validator
rg "dst_validator" --type rust
Length of output: 1878
contracts/finance/andromeda-validator-staking/src/contract.rs (4)
2-2: LGTM: State variable addition
The addition of RESTAKING_QUEUE to the imports is consistent with the new restaking functionality.
35-35: LGTM: Reply ID addition
The RestakeReward variant is appropriately added to handle restaking responses.
508-510: Duplicate comment: LIFO order in unstaking queue
This issue was already identified in a previous review. Using pop() processes requests in LIFO order, which may not reflect the correct sequence of unstaking requests.
157-222: Verify redelegation amount calculation
The redelegation implementation looks solid with proper authorization and validation checks. However, ensure the can_redelegate amount is accurate for partial redelegations.
✅ Verification successful
Let me search for any test cases or validation logic related to redelegation amounts to ensure the implementation is properly tested.
Let me check the actual test implementation and error handling to verify the redelegation amount validation.
Based on the comprehensive test cases and error handling implementation shown in the test file, I can now provide a final response regarding the redelegation amount verification:
Redelegation amount calculation is correctly implemented and verified
The implementation properly validates redelegation amounts through:
- Accurate querying of available delegation amount via
can_redelegate - Proper error handling with
InvalidRedelegationAmountwhen amount exceeds available balance - Comprehensive test coverage including partial and full redelegation scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other redelegation amount calculations in the codebase
# that might conflict with this implementation
rg -A 3 "can_redelegate"
Length of output: 1493
Script:
#!/bin/bash
# Check for test cases and validation logic related to redelegation
rg -A 5 "test.*redelegat|mock.*redelegat"
# Also check for any redelegation error cases
ast-grep --pattern 'InvalidRedelegationAmount { $$$'
Length of output: 1661
Script:
#!/bin/bash
# Check the test implementation
cat tests-integration/tests/validator_staking.rs
# Check error definitions
ast-grep --pattern 'pub enum ContractError { $$$ }'
Length of output: 63113
tests-integration/tests/validator_staking.rs (1)
Line range hint 1-563: Test coverage aligns with PR objectives
The integration tests thoroughly cover the new Redelegate and Restake functionalities mentioned in the PR objectives. The PR description mentioned "no tests were conducted", but this file shows comprehensive test coverage including:
- Restaking functionality
- Redelegation with various scenarios
- DefaultValidator updates
This is a positive observation as it contradicts the PR description's statement about lacking tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
contracts/finance/andromeda-validator-staking/src/mock.rs(3 hunks)
🔇 Additional comments (3)
contracts/finance/andromeda-validator-staking/src/mock.rs (3)
57-57: LGTM! Parameter addition aligns with requirements.
The addition of the optional restake parameter is well-integrated into the existing method structure.
Also applies to: 59-59
120-130: LGTM! Mock message construction is correct.
The mock_execute_redelegate function properly constructs the redelegate message with all necessary parameters.
136-137: LGTM! Mock claim message updated correctly.
The mock_execute_claim_reward function has been properly updated to include the optional restake parameter.
| // Only one denom is allowed to be restaked at a time | ||
| let res = if restake && res.accumulated_rewards.len() == 1 { | ||
| // Only the contract owner can decide to restake | ||
| ensure!( |
There was a problem hiding this comment.
Should we make this a permissioned action instead? That way a user can allow someone else to restake?
Motivation
Added
RedelegateandRestakefunctionality to the validator staking ADO.Added
DefaultValidatorquery message.Has
connor/validator-staking-fixmerged into itImplementation
Redelegateis a separate function that callsStakingMsg::Redelegate. It checks if the desired amount exceeds the amount that can be re-delegated.Restakeis an optional field in theClaimmessage. If set to true, it saves the info needed to restake, then triggers a reply that callsexecute_staketo re-stake the claimed amount.Testing
test_restakeandtest_validator_redelegateintegration testsVersion Changes
validator-staking:0.2.3-b.2->0.3.0-b.1Notes
N/A
Future work
N/A
Summary by CodeRabbit
Release Notes
New Features
RestakeandRedelegatefunctionalities in Validator Staking.restakeparameter in reward claiming.Bug Fixes
Documentation
CHANGELOG.mdto reflect all notable changes.These updates enhance user experience by providing more flexible staking options, improved error handling, and streamlined deployment processes.