fix: vote from new staking pool contract#5
Conversation
WalkthroughThe voting mechanism was updated to support voting on behalf of staking pool owners via cross-contract calls. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant StakingPool
User->>Contract: vote(is_vote, staking_pool_id)
Contract->>StakingPool: get_owner_id()
StakingPool-->>Contract: owner_id
Contract->>Contract: on_get_owner_id(owner_id, staking_pool_id, is_vote, result)
Contract->>Contract: internal_vote(is_vote, owner_id)
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (2)
src/lib.rs (2)
17-17: Consider making the gas amount configurable or justifying the 5 Tgas allocation.The hardcoded gas amount of 5 Tgas for
get_owner_idcalls may be excessive for a simple getter method. Most view methods require significantly less gas.Consider reducing the gas allocation or making it configurable:
-const GET_OWNER_ID_GAS: Gas = Gas::from_tgas(5); +const GET_OWNER_ID_GAS: Gas = Gas::from_tgas(2);Or make it configurable through contract initialization if different staking pool contracts have varying gas requirements.
94-112: Verify error handling and consider logging for debugging.The callback method correctly validates the owner ID but could benefit from better error handling and logging.
Consider adding logging for debugging and more specific error messages:
pub fn on_get_owner_id( &mut self, pool_owner_id: AccountId, staking_pool_id: AccountId, is_vote: bool, #[callback_result] pool_owner_id_result: Result<AccountId, PromiseError>, ) { if let Ok(actual_owner_id) = pool_owner_id_result { require!( pool_owner_id == actual_owner_id, - "Voting is only allowed for the staking pool owner" + format!("Voting is only allowed for the staking pool owner. Expected: {}, Got: {}", actual_owner_id, pool_owner_id) ); self.internal_vote(is_vote, staking_pool_id); } else { - env::panic_str("Failed to get the staking pool owner id"); + env::panic_str(&format!("Failed to get the staking pool owner id from {}: {:?}", staking_pool_id, pool_owner_id_result)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib.rs(14 hunks)tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/contracts/mock-staking-pool/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (actions)
🔇 Additional comments (4)
src/lib.rs (4)
6-8: LGTM! Necessary imports added for cross-contract functionality.The imports correctly include
ext_contractandPromiseErrorwhich are needed for the new cross-contract call functionality.
32-35: LGTM! External contract interface correctly defined.The
StakingPoolContracttrait properly defines the expected interface for cross-contract calls toget_owner_id.
72-92: Good refactoring: Separating vote update logic into a dedicated method.The
pingmethod correctly handles vote recalculation when epoch height changes and includes proper validation checks for voting deadline and result status.The logic correctly:
- Validates voting is still active
- Updates stakes only when epoch changes
- Removes validators with zero stake
- Checks for voting result after updates
115-150: LGTM! Internal voting logic properly refactored.The
internal_votemethod correctly encapsulates the core voting logic and maintains all the original functionality including proper stake validation, vote tracking, and event emission.The refactoring properly:
- Calls
ping()to ensure votes are up-to-date- Validates validator stake
- Updates vote tallies correctly
- Emits appropriate events
- Maintains invariants
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib.rs (1)
61-71: Security concern: Staking pool validation still missing.The implementation continues to accept any
staking_pool_idwithout validation, which was flagged in previous reviews. This poses a security risk as malicious contracts could be called.The security validation concern from previous reviews remains unaddressed. Consider implementing an allowlist or validation pattern as suggested in the previous review.
🧹 Nitpick comments (2)
src/lib.rs (2)
18-18: Consider adjusting gas allocation for cross-contract calls.The gas allocation of 5 TGas for
get_owner_idcalls may be insufficient for complex staking pool contracts or network congestion scenarios.Consider increasing the gas allocation or making it configurable:
-const GET_OWNER_ID_GAS: Gas = Gas::from_tgas(5); +const GET_OWNER_ID_GAS: Gas = Gas::from_tgas(10);
96-114: Consider improving error handling in the callback.The callback method handles the happy path well but could provide more informative error messages for debugging failed cross-contract calls.
Consider enhancing error handling:
} else { - env::panic_str("Failed to get the staking pool owner id"); + env::panic_str(&format!("Failed to get owner id from staking pool {}: {:?}", staking_pool_id, pool_owner_id_result)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build
- GitHub Check: Tests
- GitHub Check: Code Linter
- GitHub Check: Analyze (actions)
🔇 Additional comments (5)
src/lib.rs (5)
6-9: LGTM - Appropriate imports for cross-contract functionality.The added imports (
ext_contract,Promise,PromiseError) are necessary for the new cross-contract voting verification mechanism.
33-37: LGTM - Clean external contract interface definition.The external contract interface for staking pools is well-defined and follows NEAR SDK conventions.
73-94: LGTM - Well-implemented stake update mechanism.The
pingmethod correctly handles epoch changes and updates validator stakes. The logic properly:
- Validates voting constraints
- Recalculates stakes for all voters
- Removes validators with zero stake
- Updates the result check
116-152: LGTM - Clean refactoring of voting logic.The internal voting method properly encapsulates the core voting logic and maintains all the original validation and event emission functionality.
247-249: LGTM - Clean test helper functions.The helper functions improve test readability and maintainability.
Also applies to: 258-260
| fn vote(contract: &mut Contract, is_vote: bool, staking_pool_id: &AccountId) { | ||
| contract.on_get_owner_id( | ||
| pool_owner(), | ||
| staking_pool_id.clone(), | ||
| is_vote, | ||
| Ok(pool_owner()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test implementation bypasses the actual vote method.
The test helper directly calls on_get_owner_id instead of testing the complete flow through the vote method. This doesn't test the cross-contract call mechanism and could miss integration issues.
Consider implementing proper integration tests that exercise the full cross-contract call flow, or add unit tests that mock the external contract responses:
#[test]
fn test_vote_with_valid_staking_pool() {
// Set up mock external contract calls
// Test the actual vote method
// Verify the promise is created correctly
}🤖 Prompt for AI Agents
In src/lib.rs around lines 302 to 309, the current test helper bypasses the full
vote method by directly calling on_get_owner_id, missing the cross-contract call
flow. To fix this, replace the direct call with tests that invoke the vote
method itself, using mocks or integration test setups to simulate and verify the
external contract calls and promise creation, ensuring the entire voting process
is properly exercised and validated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/lib.rs (2)
61-71: Duplicate security concern: Staking pool validation missing.The current implementation makes cross-contract calls to any provided
staking_pool_idwithout validation, creating a potential security vulnerability where malicious contracts could be called.Additionally, this remains a breaking change without backward compatibility handling. Consider making the
staking_pool_idparameter optional to maintain backward compatibility.#!/bin/bash # Verify if there are existing callers of the vote method that would break rg -A 3 -B 3 "\.vote\(" --type rust
302-309: Duplicate issue: Test bypasses the actual vote method.The test helper still directly calls
on_get_owner_idinstead of testing the complete cross-contract call flow through thevotemethod. This doesn't exercise the promise creation and external contract call mechanism.Consider adding integration tests that mock the external contract responses or test the promise creation:
#[test] fn test_vote_creates_promise() { let context = get_context(&pool_owner()); set_context(&context); let mut contract = get_contract(); // This should test that vote() returns a Promise let promise = contract.vote(true, validator(0)); // Add assertions about the promise structure if possible }
🧹 Nitpick comments (1)
src/lib.rs (1)
18-18: Consider adding documentation for the gas constant.The gas amount seems reasonable for a simple view call, but adding a comment explaining the purpose would improve code clarity.
+/// Gas allocated for calling get_owner_id on staking pool contracts const GET_OWNER_ID_GAS: Gas = Gas::from_tgas(5);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Tests
- GitHub Check: Build
- GitHub Check: Code Linter
- GitHub Check: Analyze (actions)
🔇 Additional comments (7)
src/lib.rs (7)
6-9: LGTM: Import additions support the new cross-contract functionality.The added imports (
ext_contract,Promise,PromiseError) are necessary for the new cross-contract call implementation.
33-36: LGTM: External contract interface is minimal and appropriate.The interface correctly defines the required method for staking pool ownership verification.
73-94: LGTM: Ping method correctly updates validator stakes.The method properly checks voting conditions and updates stakes when the epoch changes. The logic for recalculating stakes and removing inactive validators is sound.
96-114: LGTM: Callback method has proper error handling.The ownership verification logic is correct, properly validating that the caller matches the pool owner before proceeding with the vote.
116-152: LGTM: Internal vote logic correctly preserved.The refactored method maintains the original voting logic while now using the
pingmethod to ensure up-to-date validator stakes. The event emission and validation logic remain intact.
247-249: LGTM: Test helper functions support the new testing approach.The helper functions provide clean abstractions for test setup.
Also applies to: 258-260
314-321: Tests updated consistently with new voting flow.All test cases have been properly updated to use the new test helper function that simulates the staking pool ownership verification.
Also applies to: 327-339, 344-351, 376-384, 404-407, 422-426, 442-446, 489-493, 500-504
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/lib.rs (2)
302-309: Test implementation still bypasses the actual vote method.The test helper continues to call
on_get_owner_iddirectly instead of testing the complete cross-contract call flow through thevotemethod, as noted in previous reviews.This approach doesn't validate that:
- The Promise is created correctly
- Gas allocation is sufficient
- The cross-contract call mechanism works end-to-end
Consider implementing integration tests that mock the external contract responses or use a proper test framework that supports cross-contract call testing.
62-71: Security concern: Staking pool validation still missing.The implementation accepts any
staking_pool_idwithout validation, which was flagged in previous reviews. This creates a security risk where malicious contracts could be called.Additionally, consider the user experience impact - this breaking change removes the ability for validators to vote directly without specifying a staking pool.
Consider implementing an allowlist or validation pattern:
+ // Add validation before cross-contract call + require!( + staking_pool_id.as_str().ends_with(".poolv1.near") || + staking_pool_id.as_str().ends_with(".pool.near"), + "Invalid staking pool contract" + );
🧹 Nitpick comments (1)
src/lib.rs (1)
96-114: Consider more graceful error handling in callback.The callback method panics on cross-contract call failure, which may not provide the best user experience.
Consider logging the error and returning a more informative message:
} else { - env::panic_str("Failed to get the staking pool owner id"); + env::log_str(&format!("Failed to verify staking pool ownership for {}", staking_pool_id)); + env::panic_str("Staking pool verification failed - please ensure the pool contract is valid"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Code Linter
- GitHub Check: Tests
- GitHub Check: Build
🔇 Additional comments (6)
src/lib.rs (6)
6-9: LGTM: Imports are appropriate for the new functionality.The added imports support the cross-contract calling mechanism and promise handling needed for the new voting flow.
18-18: LGTM: Gas allocation appears reasonable.The gas allocation of 5 TGas for the
get_owner_idcall is appropriate for a simple view method cross-contract call.
20-23: LGTM: Standard NEAR external contract interface pattern.The external contract interface follows NEAR best practices for cross-contract calls.
73-94: LGTM: Ping method implements proper vote stake updates.The method correctly handles epoch changes, recalculates validator stakes, and maintains voting integrity by removing validators with zero stake.
116-117: LGTM: Clean separation of voting logic.Moving the core voting logic to an internal method provides good separation of concerns and maintains the original voting behavior.
174-174: LGTM: Proper separation of view methods.The separate impl block for view methods follows NEAR conventions and improves code organization.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/lib.rs (3)
62-71: Security concern: Staking pool validation still missing.The method accepts any
staking_pool_idwithout validation, which could allow malicious contracts to be called. This security issue was flagged in previous reviews but remains unaddressed.Consider implementing validation such as:
+use std::collections::HashSet; +// Add to contract state +allowlisted_pools: HashSet<AccountId>, pub fn vote(&mut self, is_vote: bool, staking_pool_id: AccountId) -> Promise { + require!( + self.allowlisted_pools.contains(&staking_pool_id) || + staking_pool_id.as_str().ends_with(".poolv1.near"), + "Staking pool not authorized" + ); ext_staking_pool::ext(staking_pool_id.clone())
62-71: Breaking change: Method signature incompatible with existing callers.The addition of the required
staking_pool_id: AccountIdparameter is a breaking change that will cause runtime failures for existing code callingvote(is_vote).To maintain backward compatibility while supporting the new functionality:
-pub fn vote(&mut self, is_vote: bool, staking_pool_id: AccountId) -> Promise { +pub fn vote(&mut self, is_vote: bool, staking_pool_id: Option<AccountId>) -> Promise { + let pool_id = staking_pool_id.unwrap_or_else(|| env::predecessor_account_id()); ext_staking_pool::ext(staking_pool_id.clone())This allows both direct validator voting (when
None) and staking pool owner voting (whenSome).
302-309: Test helper bypasses actual cross-contract call flow.The test helper directly calls
on_get_owner_idinstead of exercising the completevote→ cross-contract call → callback sequence, which doesn't test the integration properly.Replace the direct callback invocation with proper integration testing:
-fn vote(contract: &mut Contract, is_vote: bool, staking_pool_id: &AccountId) { - contract.on_get_owner_id( - pool_owner(), - staking_pool_id.clone(), - is_vote, - Ok(pool_owner()), - ); -} +fn vote(contract: &mut Contract, is_vote: bool, staking_pool_id: &AccountId) { + // Set up mock context for cross-contract calls + let promise = contract.vote(is_vote, staking_pool_id.clone()); + // Test that promise is created correctly + // Then simulate the callback with mocked external contract response +}
🧹 Nitpick comments (1)
src/lib.rs (1)
116-117: Clarification needed: Vote attribution logic.The
internal_votemethod records votes against thestaking_pool_idrather than the owner account. While this appears intentional for staking pool voting, it should be documented to clarify the vote attribution model.Consider adding a comment to clarify:
/// Internal method for voting. +/// Note: Votes are attributed to the staking_pool_id, not the owner account. fn internal_vote(&mut self, is_vote: bool, account_id: AccountId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build
- GitHub Check: Tests
- GitHub Check: Code Linter
- GitHub Check: Analyze (actions)
🔇 Additional comments (2)
src/lib.rs (2)
73-94: LGTM! Well-implemented epoch-based vote updates.The
pingmethod correctly handles epoch changes by recalculating stakes for all voters and maintains proper invariants.
96-114: Solid implementation of owner verification.The callback method properly verifies staking pool ownership before allowing votes and provides clear error handling.
linguists
changed the title
4 participants
This fix makes the voting compatible with
*.pool.nearstaking pool contracts.Instead of letting the staking pool contract call the voting contract, in the voting contract, we verify that the caller is the owner of a given staking pool contract.
Summary by CodeRabbit
New Features
Refactor
Tests