Conversation
WalkthroughThe changes add support for voting through staking pool accounts by verifying their ownership. A new external contract interface is introduced to query the staking pool's owner. The voting method now optionally accepts a staking pool ID, performs an asynchronous ownership check, and only allows voting if the caller owns the staking pool. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Contract
participant StakingPool
Caller->>Contract: vote(is_vote, staking_pool_id)
alt staking_pool_id is Some
Contract->>StakingPool: get_owner_id() (async call)
StakingPool-->>Contract: owner_account_id
Contract->>Contract: check_owner_id_match(owner_account_id, callback_result)
alt owner matches caller
Contract->>Contract: proceed with voting
else owner does not match
Contract->>Caller: abort with error
end
else staking_pool_id is None
Contract->>Contract: proceed with voting (using predecessor)
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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: 2
🔭 Outside diff range comments (1)
src/lib.rs (1)
220-488: Add tests for staking pool voting functionalityThe new staking pool voting feature lacks test coverage. Please add tests for:
- Voting through a staking pool with valid ownership
- Rejection when the caller doesn't own the staking pool
- Handling of cross-contract call failures
- Edge cases with staking pool ID validation
Would you like me to help generate comprehensive test cases for the new functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(4 hunks)
🔇 Additional comments (3)
src/lib.rs (3)
6-8: LGTM!The additional imports for cross-contract functionality are appropriate.
17-17: LGTM!The gas allocation of 5 TGas is reasonable for a simple getter function.
32-35: LGTM!The external contract trait is well-defined with a clear interface for querying staking pool ownership.
src/lib.rs
Outdated
| pub fn vote(&mut self, is_vote: bool, staking_pool_id: Option<AccountId>) { | ||
| self.ping(); | ||
| let account_id = env::predecessor_account_id(); | ||
| let account_id = if let Some(pool_id) = staking_pool_id { | ||
| let strs = pool_id.as_str().split(".").collect::<Vec<&str>>(); | ||
| require!( | ||
| strs.len() == 3 && strs[1] == "pool" && strs[2] == "near", | ||
| "New staking_pool_id must be in the format <pool_id>.pool.near" | ||
| ); | ||
| ext_staking_pool::ext(pool_id.clone()) | ||
| .with_static_gas(GET_OWNER_ID_GAS) | ||
| .get_owner_id() | ||
| .then( | ||
| Self::ext(env::current_account_id()) | ||
| .check_owner_id_match(env::predecessor_account_id()), | ||
| ); | ||
| pool_id | ||
| } else { | ||
| env::predecessor_account_id() | ||
| }; |
There was a problem hiding this comment.
Critical: Vote is processed before ownership verification completes
The current implementation has a race condition where votes are counted before staking pool ownership is verified. The asynchronous call to get_owner_id doesn't block execution, so the vote proceeds immediately using the staking pool ID.
This is a security vulnerability that allows anyone to vote on behalf of any staking pool.
The voting logic needs to be moved into the callback to ensure ownership is verified first. Consider this architectural approach:
- Move the actual voting logic into a separate internal method
- In the main
votemethod, ifstaking_pool_idis provided, only initiate the async call - In
check_owner_id_match, after successful verification, call the internal voting method - For direct voting (no staking pool), call the internal voting method directly
Additionally, the hardcoded format validation <pool_id>.pool.near may be too restrictive. Consider making this configurable or removing it entirely, relying only on the ownership check.
🤖 Prompt for AI Agents
In src/lib.rs lines 71 to 89, the vote function processes votes before verifying
staking pool ownership due to asynchronous calls not blocking execution.
Refactor by moving the voting logic into a separate internal method. Modify the
vote method to only initiate the async ownership verification call if
staking_pool_id is provided, and call the internal voting method directly if
not. Then, in the check_owner_id_match callback, after successful verification,
invoke the internal voting method to process the vote. Also, consider removing
or making the staking_pool_id format validation configurable instead of
hardcoding the "<pool_id>.pool.near" pattern.
src/lib.rs
Outdated
| pub fn check_owner_id_match( | ||
| &self, | ||
| owner_account_id: AccountId, | ||
| #[callback_result] owner_account_id_result: Result<AccountId, PromiseError>, | ||
| ) { | ||
| require!( | ||
| owner_account_id == owner_account_id_result.unwrap(), | ||
| "Voting is only allowed for the staking pool owner" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle cross-contract call failures gracefully
The callback method will panic if the cross-contract call fails. This could be exploited or cause unexpected failures.
Apply this diff to handle errors properly:
- pub fn check_owner_id_match(
- &self,
- owner_account_id: AccountId,
- #[callback_result] owner_account_id_result: Result<AccountId, PromiseError>,
- ) {
- require!(
- owner_account_id == owner_account_id_result.unwrap(),
- "Voting is only allowed for the staking pool owner"
- );
- }
+ pub fn check_owner_id_match(
+ &self,
+ owner_account_id: AccountId,
+ #[callback_result] owner_account_id_result: Result<AccountId, PromiseError>,
+ ) {
+ match owner_account_id_result {
+ Ok(pool_owner) => {
+ require!(
+ owner_account_id == pool_owner,
+ "Voting is only allowed for the staking pool owner"
+ );
+ }
+ Err(_) => {
+ env::panic_str("Failed to retrieve staking pool owner");
+ }
+ }
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn check_owner_id_match( | |
| &self, | |
| owner_account_id: AccountId, | |
| #[callback_result] owner_account_id_result: Result<AccountId, PromiseError>, | |
| ) { | |
| require!( | |
| owner_account_id == owner_account_id_result.unwrap(), | |
| "Voting is only allowed for the staking pool owner" | |
| ); | |
| } | |
| pub fn check_owner_id_match( | |
| &self, | |
| owner_account_id: AccountId, | |
| #[callback_result] owner_account_id_result: Result<AccountId, PromiseError>, | |
| ) { | |
| match owner_account_id_result { | |
| Ok(pool_owner) => { | |
| require!( | |
| owner_account_id == pool_owner, | |
| "Voting is only allowed for the staking pool owner" | |
| ); | |
| } | |
| Err(_) => { | |
| env::panic_str("Failed to retrieve staking pool owner"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib.rs around lines 58 to 67, the method check_owner_id_match currently
unwraps the Result from a cross-contract call, which will panic if the call
fails. To fix this, modify the method to handle the Err case gracefully by
checking if owner_account_id_result is Ok and comparing the contained AccountId
with owner_account_id, otherwise handle the error case without panicking, for
example by returning early or logging an error, ensuring the contract does not
panic on cross-contract call failures.
|
Close in favor of #5 |
1 participant
There is a problem that the new staking pool does not have the interface to call voting function and we need to handle it separately.
Summary by CodeRabbit
New Features
Bug Fixes
Tests