test: voting contract integration test #2
Conversation
WalkthroughThe previous greeting-related asynchronous test in Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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: 5
🧹 Nitpick comments (3)
tests/test_basics.rs (3)
31-31: Add comment explaining vote parametersThe vote parameter
is_vote: truedoesn't provide context on what it means. Adding a comment would make the test more understandable.- .args_json(json!({"is_vote": true})) + // Attempt to vote "yes" on the proposal + .args_json(json!({"is_vote": true}))
34-34: Clarify error assertion messageThe error assertion message doesn't explain why the failure is expected. A more descriptive message would make the test easier to understand.
- assert!(outcome.is_failure(), "{:#?}", outcome.into_result().unwrap_err()); + assert!(outcome.is_failure(), "Vote should fail before user registration. Error: {:#?}", outcome.into_result().unwrap_err());
36-37: Add more complete proposal verificationThe test only verifies that the proposal string matches, but should also verify other proposal properties like the vote count or deadline.
- let contract_proposal = contract.view("get_proposal").args_json(json!({})).await?; - assert_eq!(contract_proposal.json::<String>()?, proposal); + // Verify all proposal properties + let contract_proposal = contract.view("get_proposal").args_json(json!({})).await?; + let proposal_data = contract_proposal.json::<serde_json::Value>()?; + + // Verify the proposal text + assert_eq!(proposal_data["text"].as_str().unwrap(), proposal, "Proposal text doesn't match"); + + // Verify other properties like vote counts, deadline, etc. + assert_eq!(proposal_data["deadline_timestamp_ms"].as_u64().unwrap(), deadline_timestamp_ms, "Deadline doesn't match"); + assert_eq!(proposal_data["yes_votes"].as_u64().unwrap(), 0, "Initial yes votes should be 0"); + assert_eq!(proposal_data["no_votes"].as_u64().unwrap(), 0, "Initial no votes should be 0");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/test_basics.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
tests/test_basics.rs
[error] 13-13: Rust compiler error E0433: use of unresolved module or unlinked crate 'near_workspaces'. Suggestion: add 'near_workspaces' crate to Cargo.toml.
[error] 18-18: Rust compiler error E0277: cannot add integer to 'std::time::SystemTime'. The trait 'std::ops::Add<{integer}>' is not implemented for 'std::time::SystemTime'. Use 'std::time::Duration' for addition.
[error] 6-6: Rust compiler error E0277: the size for values of type '[u8]' cannot be known at compile time. The trait 'std::marker::Sized' is not implemented for '[u8]'. Unsized locals are unstable.
🔇 Additional comments (1)
tests/test_basics.rs (1)
4-10:⚠️ Potential issueFix unsized type error
The use of unsized
&[u8]as a local variable is causing a compilation error. The contract bytes should be wrapped in a sized type likeVec<u8>or a reference to it.#[tokio::test] async fn test_contract_is_operational() -> Result<(), Box<dyn std::error::Error>> { - let contract_wasm = near_workspaces::compile_project("./").await?; + let contract_wasm = near_workspaces::compile_project("./").await?; - test_initialization(&contract_wasm).await?; + test_initialization(contract_wasm.as_slice()).await?; Ok(()) }Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: Test
[error] 6-6: Rust compiler error E0277: the size for values of type '[u8]' cannot be known at compile time. The trait 'std::marker::Sized' is not implemented for '[u8]'. Unsized locals are unstable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_basics.rs (3)
25-27:⚠️ Potential issueFix typo in JSON field name
There's a typo in the JSON field name for the deadline timestamp.
.args_json(json!({ "proposal": proposal, - "deadline_timestamp_md": deadline_timestamp_ms, + "deadline_timestamp_ms": deadline_timestamp_ms, }))
37-43: 🛠️ Refactor suggestionAdd user registration step before voting
The test expects the vote to fail, but doesn't explain why. It's likely that users need to register before voting.
Expand the test to include the complete user flow:
let user_account = sandbox.dev_create_account().await?; + + // First try voting without registration (should fail) let outcome = user_account .call(contract.id(), "vote") .args_json(json!({"is_vote": true})) .transact() .await?; - assert!(outcome.is_failure(), "{:#?}", outcome.into_result().unwrap_err()); + assert!(outcome.is_failure(), "Vote should fail before user registration. Error: {:#?}", outcome.into_result().unwrap_err()); + + // Register the user (if the contract has such functionality) + let register_outcome = user_account + .call(contract.id(), "register_voter") + .args_json(json!({})) + .transact() + .await?; + assert!(register_outcome.is_success(), "User registration failed: {:#?}", register_outcome); + + // Now try voting after registration (should succeed) + let vote_outcome = user_account + .call(contract.id(), "vote") + .args_json(json!({"is_vote": true})) + .transact() + .await?; + assert!(vote_outcome.is_success(), "Vote failed after registration: {:#?}", vote_outcome);This provides a complete test of the voting workflow and clearly documents the expected behavior.
1-2:⚠️ Potential issueAdd the missing near_workspaces import
The imports look good, but the code is using
near_workspaceswithout having it as a dependency according to the pipeline failures.Add the
near_workspacesdependency to your Cargo.toml file:[dev-dependencies] + near-workspaces = "0.9.0"This will resolve the pipeline errors showing "use of unresolved module or unlinked crate
near_workspaces".
🧹 Nitpick comments (2)
tests/test_basics.rs (2)
18-21: Improved timestamp calculationThe timestamp calculation looks good, but could be made a bit more readable.
Consider simplifying the timestamp calculation:
- let deadline_timestamp_ms = (SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_millis() + 10 * 60 * 1000) as u64; + // Current time + 10 minutes in milliseconds + let deadline_timestamp_ms = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("Time went backwards") + .as_millis() + .checked_add(10 * 60 * 1000) + .expect("Timestamp overflow") + as u64;This makes the code more robust by using
expectwith meaningful error messages andchecked_addto prevent overflow.
31-35: Add more comprehensive contract state verificationThe current checks verify only the proposal and deadline. Consider adding more assertions to validate the complete contract state.
let contract_deadline = contract.view("get_deadline_timestamp").args_json(json!({})).await?; assert_eq!(contract_deadline.json::<u64>()?, deadline_timestamp_ms); let contract_proposal = contract.view("get_proposal").args_json(json!({})).await?; assert_eq!(contract_proposal.json::<String>()?, proposal); + + // Add assertions for initial voting state + let vote_count = contract.view("get_vote_count").args_json(json!({})).await?; + assert_eq!(vote_count.json::<u64>()?, 0, "Initial vote count should be zero"); + + let voters_count = contract.view("get_voters_count").args_json(json!({})).await?; + assert_eq!(voters_count.json::<u64>()?, 0, "Initial voters count should be zero");These additional checks ensure the contract is initialized with the expected default values for voting-related state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/test_basics.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
tests/test_basics.rs
[error] 13-13: failed to resolve: use of unresolved module or unlinked crate near_workspaces. Suggestion: add near_workspaces to Cargo.toml using cargo add near_workspaces.
[error] 6-6: failed to resolve: use of unresolved module or unlinked crate near_workspaces. Suggestion: add near_workspaces to Cargo.toml using cargo add near_workspaces.
[error] 6-6: the size for values of type [u8] cannot be known at compilation time. The trait Sized is not implemented for [u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type [u8] cannot be known at compilation time. The trait Sized is not implemented for [u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type [u8] cannot be known at compilation time. The trait Sized is not implemented for [u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type [u8] cannot be known at compilation time. The trait Sized is not implemented for [u8]. All local variables must have a statically known size.
🔇 Additional comments (1)
tests/test_basics.rs (1)
4-10:⚠️ Potential issueFix unsized type error in the contract_wasm handling
The pipeline is failing with "the size for values of type
[u8]cannot be known at compilation time" errors.Modify the function to use a Vec instead of [u8]:
#[tokio::test] async fn test_contract_is_operational() -> Result<(), Box<dyn std::error::Error>> { - let contract_wasm = near_workspaces::compile_project("./").await?; + let contract_wasm: Vec<u8> = near_workspaces::compile_project("./").await?; test_initialization(&contract_wasm).await?; Ok(()) }Similarly, update the helper function signature:
-async fn test_initialization(contract_wasm: &[u8]) -> Result<(), Box<dyn std::error::Error>> { +async fn test_initialization(contract_wasm: &Vec<u8>) -> Result<(), Box<dyn std::error::Error>> {This will resolve the sized type requirement for local variables.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: Test
[error] 6-6: failed to resolve: use of unresolved module or unlinked crate
near_workspaces. Suggestion: addnear_workspacesto Cargo.toml usingcargo add near_workspaces.
[error] 6-6: the size for values of type
[u8]cannot be known at compilation time. The traitSizedis not implemented for[u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type
[u8]cannot be known at compilation time. The traitSizedis not implemented for[u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type
[u8]cannot be known at compilation time. The traitSizedis not implemented for[u8]. All local variables must have a statically known size.
[error] 6-6: the size for values of type
[u8]cannot be known at compilation time. The traitSizedis not implemented for[u8]. All local variables must have a statically known size.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_vote.rs (1)
6-23: Well-structured test for permission enforcementThis test correctly verifies that non-validator accounts cannot vote on the contract. The test follows a clear pattern:
- Deploy and initialize the contract
- Create a non-validator user account
- Attempt to vote and verify the operation fails
The error handling with
?operator and the clear assertion message are good practices.Consider enhancing the assertion message to be more specific about why the test should fail:
assert!( outcome.is_failure(), - "{:#?}", + "Non-validator accounts should not be able to vote: {:#?}", outcome.into_result().unwrap_err() );tests/test_init.rs (1)
6-23: Effective contract initialization verificationThis test properly validates that the contract's state after initialization matches the expected values:
- The deadline timestamp is correctly stored and retrievable
- The proposal string is properly initialized and accessible
The test is well-structured with clear assertions and proper error handling.
Consider using a single view call pattern for consistency between the two contract calls:
let contract_deadline = contract .view("get_deadline_timestamp") .args_json(json!({})) .await?; assert_eq!( contract_deadline.json::<u64>()?, init_args.deadline_timestamp_ms ); -let contract_proposal = contract.view("get_proposal").args_json(json!({})).await?; +let contract_proposal = contract + .view("get_proposal") + .args_json(json!({})) + .await?; assert_eq!(contract_proposal.json::<String>()?, init_args.proposal);tests/utils/mod.rs (1)
12-35: Effective contract deployment utilityThe
deploy_voting_contractfunction provides a useful abstraction for deploying and initializing the contract in a test environment. It correctly:
- Compiles the contract from the current project
- Creates a sandbox test environment
- Deploys the contract
- Initializes it with test values
- Returns all necessary components for testing
Consider making the function more flexible by allowing parameterization of the proposal and deadline:
pub async fn deploy_voting_contract( + proposal: Option<String>, + deadline_offset_ms: Option<u64> ) -> Result<(Contract, Worker<Sandbox>, InitArgs), Box<dyn std::error::Error>> { let contract_wasm = near_workspaces::compile_project("./").await?; let sandbox = near_workspaces::sandbox().await?; let contract = sandbox.dev_deploy(&contract_wasm).await?; // Initialize contract let init_args = InitArgs { - proposal: "test_proposal".to_string(), + proposal: proposal.unwrap_or_else(|| "test_proposal".to_string()), deadline_timestamp_ms: (SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_millis() - + 10 * 60 * 1000) as u64, + + deadline_offset_ms.unwrap_or(10 * 60 * 1000)) as u64, }; // Rest of the function remains the sameThis would allow tests to customize these values when needed while maintaining the default behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
tests/test_basics.rs(0 hunks)tests/test_init.rs(1 hunks)tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_basics.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_vote.rs (1)
tests/utils/mod.rs (1)
deploy_voting_contract(12-35)
tests/test_init.rs (1)
tests/utils/mod.rs (1)
deploy_voting_contract(12-35)
🔇 Additional comments (4)
tests/test_vote.rs (1)
1-5: LGTM: Appropriate imports and module usageClean import of
serde_json::jsonfor JSON serialization and modular organization using the utility module.tests/test_init.rs (1)
1-5: LGTM: Clean imports and modular organizationGood use of the utility module and appropriate JSON serialization imports.
tests/utils/mod.rs (2)
1-5: LGTM: Appropriate imports for contract testingGood selection of imports for NEAR SDK, workspaces, JSON serialization, and time handling.
6-10: Well-defined initialization arguments structThe
InitArgsstruct is properly defined with the NEAR serialization attribute and contains the necessary fields for contract initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/contracts/mock-staking-pool/src/lib.rs (5)
1-8: Imports and constants look good, but consider adding more descriptive constantsThe imports are appropriate for the contract functionality. The
VOTE_GASconstant is well-defined, but consider adding more descriptive constants for other magic numbers that might appear in the contract, such as minimum staking amounts.
9-12: External contract interface should specify return typeThe external contract trait is properly defined, but the
votemethod doesn't specify a return type. Although it might not return anything, it's a best practice to explicitly specify()as the return type for clarity.trait VotingContract { - fn vote(&mut self, is_vote: bool); + fn vote(&mut self, is_vote: bool) -> (); }
42-53: Staking method is well-implemented but lacks event emissionThe staking implementation correctly handles deposits and updates balances. Consider adding event logging (using
near_sdk::log!) to provide transparency for users and enable off-chain tracking of staking actions.pub fn stake(&mut self) { let amount = env::attached_deposit().as_yoctonear(); require!(amount > 0u128, "Invalid stake amount"); let account_id = env::predecessor_account_id(); let balance = self.internal_account_staked_balance(&account_id); self.accounts.insert(account_id.clone(), balance + amount); self.total_staked_balance += amount; self.internal_restake(); + near_sdk::log!("Account {} staked {}", account_id, amount); }
69-76: Owner-only method should use #[private] decoratorThe voting method is described in comments as an "Owner's method" and correctly verifies ownership, but it would be clearer to use the
#[private]decorator to make this requirement explicit in the contract interface./// Owner's method. /// Calls `vote(is_vote)` on the given voting contract account ID on behalf of the pool. +#[private] pub fn vote(&mut self, voting_account_id: AccountId, is_vote: bool) -> Promise { - self.assert_owner(); ext_voting::ext(voting_account_id) .with_static_gas(VOTE_GAS) .vote(is_vote) }
87-90: Consider adding more view methods for contract state inspectionThe contract lacks view methods to expose its state (total staked balance, individual account balances, etc.). Adding these would make it more testable and usable in integration tests.
fn assert_owner(&self) { require!(env::predecessor_account_id() == self.owner_id, "Not owner"); } +/// Returns the staked balance of the given account. +pub fn get_account_staked_balance(&self, account_id: AccountId) -> U128 { + U128(self.internal_account_staked_balance(&account_id)) +} + +/// Returns the total staked balance. +pub fn get_total_staked_balance(&self) -> U128 { + U128(self.total_staked_balance) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
tests/contracts/mock-staking-pool/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.gitignore(1 hunks)tests/contracts/mock-staking-pool/Cargo.toml(1 hunks)tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- tests/contracts/mock-staking-pool/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (3)
tests/contracts/mock-staking-pool/src/lib.rs (3)
14-27: Contract state structure is well-definedThe contract state structure is well-organized with appropriate storage keys and fields. Using
PanicOnDefaultis a good practice to prevent accidental initialization without proper parameters.
29-40: Initialization method follows best practicesThe initialization method is properly decorated with
#[init]and#[private], and correctly sets up all contract state. The use ofLookupMapfor accounts is an appropriate choice for storing account balances.
78-85: Internal helper methods are well-implementedThe helper methods for account balance retrieval and restaking are well-implemented. The account balance retrieval properly handles the case when an account doesn't exist, and the restaking method correctly creates a staking promise.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/contracts/mock-staking-pool/src/lib.rs (1)
59-71: Unstake must return tokens or be documented as a mockThe current
unstakeimplementation only updates internal balances and restakes the pool's total, but never returns the unstaked amount to the caller nor requests an actual protocol-level unstake.You should either implement the transfer/unstake logic or clearly document that this is a simplified mock:
+ /// Note: This is a test-only mock. `unstake` updates balances but + /// does not perform protocol-level unstake or transfer tokens back to the user. pub fn unstake(&mut self, amount: U128) -> Promise { let amount = amount.0; require!(amount > 0u128, "Invalid unstake amount"); let account_id = env::predecessor_account_id(); let balance = self.internal_account_staked_balance(&account_id); require!(balance >= amount, "Not enough stake"); self.accounts.insert(account_id, balance - amount); self.total_staked_balance -= amount; self.internal_restake() }
🧹 Nitpick comments (4)
tests/contracts/mock-staking-pool/src/lib.rs (4)
89-100: Improve error handling in callbackThe
on_stake_actioncallback only logs a generic "Stake action failed" message without providing any details about the error. This makes debugging difficult.Consider enhancing the error handling to include more specific information:
#[private] pub fn on_stake_action(&self, #[callback_result] result: Result<String, PromiseError>) { if result.is_err() { - log!("Stake action failed"); + match result.unwrap_err() { + PromiseError::Failed => log!("Stake action failed: execution failed"), + PromiseError::NotReady => log!("Stake action failed: promise not ready"), + } return; } log!( "Validator stake amount: {}", env::validator_stake(&env::current_account_id()) ); }
46-57: Consider adding deposit limit for testing safetyThe
deposit_and_stakemethod accepts any non-zero amount without an upper limit. For a test contract, it might be beneficial to add a maximum deposit limit to prevent accidental large deposits.#[payable] pub fn deposit_and_stake(&mut self) -> Promise { let amount = env::attached_deposit().as_yoctonear(); require!(amount > 0u128, "Invalid stake amount"); + // For test safety, limit maximum deposit + require!(amount <= 100_000_000_000_000_000_000_000_000, "Deposit exceeds test limit"); let account_id = env::predecessor_account_id(); let balance = self.internal_account_staked_balance(&account_id); self.accounts.insert(account_id, balance + amount); self.total_staked_balance += amount; self.internal_restake() }
102-104: Optimize account balance lookupThe current implementation of
internal_account_staked_balancedereferences the result of thegetoperation, which is slightly less efficient and requires an extraunwrap_or.Consider using
get_or_defaultfor a more concise and potentially more efficient implementation:fn internal_account_staked_balance(&self, account_id: &AccountId) -> Balance { - *self.accounts.get(account_id).unwrap_or(&0u128) + self.accounts.get(account_id).copied().unwrap_or_default() }
82-87: Rename method to better reflect its purposeThe method
get_staked_balancereturns both the validator stake and total staked balance, but its name suggests it only returns a single balance.Consider renaming this method to better reflect what it returns:
- pub fn get_staked_balance(&self) -> (U128, U128) { + pub fn get_stake_info(&self) -> (U128, U128) { ( U128::from(env::validator_stake(&env::current_account_id()).as_yoctonear()), U128::from(self.total_staked_balance), ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)tests/test_vote.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_vote.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (1)
tests/contracts/mock-staking-pool/src/lib.rs (1)
75-80: 🛠️ Refactor suggestionAdd validation for voting contract before invocation
The
votemethod directly calls the external voting contract without validating if it exists first. In a production environment, this could lead to wasted gas if the contract doesn't exist.Consider adding a check using
Promise::new(voting_account_id.clone()).account_id()to verify the account exists before proceeding:pub fn vote(&mut self, voting_account_id: AccountId, is_vote: bool) -> Promise { self.assert_owner(); + // First verify the voting contract exists + Promise::new(voting_account_id.clone()) + .account_id() + .then( ext_voting::ext(voting_account_id) .with_static_gas(VOTE_GAS) .vote(is_vote) + ) }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/contracts/mock-staking-pool/src/lib.rs (1)
64-76: Unstake does not return tokens – document or implementAs noted in previous reviews,
unstakeonly updates internal balances and
callsinternal_restake, leaving the user without their tokens.
Either make this explicit in docs or transfer the amount back via
Promise::new(account_id).transfer(amount). (Tagging as duplicate of the
prior comment for traceability.)
🧹 Nitpick comments (8)
.gitignore (2)
1-1: Good: Recursivetargetdirectory ignore
The pattern**/targetcorrectly ignores all nested Cargo/buildtargetdirectories across workspaces. As a nitpick, you could add a trailing slash (**/target/) to clarify that only directories (not files named “target”) are being ignored.
2-3: Ignore compiled WASM artifact directories
Ignoringtests/resandresensures your generated WebAssembly outputs aren’t committed. If these folders always live at the repository root, consider anchoring them with a leading slash (e.g.,/tests/res/and/res/) to avoid accidentally ignoring any otherresdirectories deeper in the tree.src/lib.rs (3)
55-58: Consider deferring theformat!evaluation to panic-only path
require!eagerly evaluates its second argument, soformat!("{} is not a validator", account_id)allocates even when the check passes (i.e. for every honest validator vote).
For hot paths it’s preferable to avoid the cost by passing a&strliteral or building the string lazily only on failure.- require!(stake > 0, format!("{} is not a validator", account_id)); + require!( + stake > 0, + "{} is not a validator", + account_id // interpolated only if the requirement fails + );
97-108: Epoch-height cache may skip the very firstping()
last_epoch_heightstarts at0.
If the contract is initialised during epoch0, the very first call toping()(or any
method that calls it) in the same epoch will not refresh stakes because
cur_epoch_height == self.last_epoch_height.
Initialisinglast_epoch_heightto the current epoch removes this edge case:- last_epoch_height: 0, + last_epoch_height: env::epoch_height(),This change does not affect correctness but avoids a surprising no-op on the first call.
168-178:set_validator_stakeneedn’t require&mut selfThe helper only mutates the mock validator map in
utils, not the contract
instance. Accepting&selfmakes the method callable on an immutable
reference and avoids needless &mut borrows in tests:- pub fn set_validator_stake(&mut self, validator_account_id: AccountId, amount: U128) { + pub fn set_validator_stake(&self, validator_account_id: AccountId, amount: U128) {(
utils::set_validator_stakealready performs the mutation inside storage. )src/utils.rs (1)
35-46: Repeated full-map (de)serialization can be expensiveEvery call to
validator_stake/validator_total_stakeunder the
integration-testfeature deserialises the whole map from storage. In large
test scenarios this dominates gas/time.If you keep this design, consider caching the map in
Statefor the duration
of the test, or store stakes in individual keys
(e.g.__validator__:<account_id>). Either approach avoids O(n) work for
each lookup and makes tests that iterate over many validators much faster.makefile (2)
1-1: Drop the superfluous quotes inRUSTFLAGSIncluding the double quotes here pushes them into the environment variable,
resulting in"being passed to the compiler and breaking flag
concatenation in some shells.-RUSTFLAGS = "-C link-arg=-s" +RUSTFLAGS = -C link-arg=-s
22-25: Avoid redundantrustup target addon every build
rustup target add wasm32-unknown-unknownis idempotent but incurs network
latency when executed in CI for every make invocation. Cache the check or
move it to project setup (e.g. injustfile,ci.yml, or a dedicated
make setuptarget) to speed up incremental builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
tests/contracts/mock-staking-pool/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignore(1 hunks)Cargo.toml(1 hunks)makefile(1 hunks)src/events.rs(1 hunks)src/lib.rs(9 hunks)src/utils.rs(1 hunks)tests/contracts/mock-staking-pool/Cargo.toml(1 hunks)tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/events.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- tests/contracts/mock-staking-pool/Cargo.toml
- tests/test_vote.rs
- tests/utils/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/contracts/mock-staking-pool/src/lib.rs (2)
src/lib.rs (2)
vote(51-86)set_validator_stake(171-173)src/utils.rs (1)
set_validator_stake(29-33)
src/lib.rs (2)
src/utils.rs (4)
validator_stake(35-40)validator_total_stake(42-47)set_validator_stake(29-33)get_validator_stake(20-26)tests/contracts/mock-staking-pool/src/lib.rs (1)
set_validator_stake(18-18)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (1)
tests/contracts/mock-staking-pool/src/lib.rs (1)
2-2:✅ Verification successful
near_sdk::storemay not containLookupMapin older SDK versions
LookupMaplives undernear_sdk::collectionsup to v4.x and was moved to
near_sdk::storeonly in v5.
If your workspace still depends on < 5.0, the current import fails to compile.-use near_sdk::store::LookupMap; +use near_sdk::collections::LookupMap // v4.x +// use near_sdk::store::LookupMap // v5.xPlease verify the SDK version in
Cargo.tomland choose the correct path.
🏁 Script executed:
#!/bin/bash # Locate all Cargo.toml files and display any near-sdk dependency lines to determine the SDK version grep -R "near-sdk" -n . | grep "Cargo.toml"Length of output: 226
No changes needed for
LookupMapimport in v5.12.0
The project’sCargo.tomlspecifiesnear-sdk = "5.12.0"(including intests/contracts/mock-staking-pool/Cargo.toml), soLookupMapis correctly imported fromnear_sdk::store.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/utils/mod.rs (6)
30-37: Consider handling time calculation error more safely.The
unwrap()onduration_since(UNIX_EPOCH)assumes the system time is always after the UNIX epoch. While this is typically true, consider using.expect("System time is before UNIX EPOCH")to provide a clearer error message if this ever fails.let init_args = VotingInitArgs { proposal: "test_proposal".to_string(), deadline_timestamp_ms: (SystemTime::now() .duration_since(UNIX_EPOCH) - .unwrap() + .expect("System time is before UNIX EPOCH") .as_millis() + 10 * 60 * 1000) as u64, };
39-43: Consider capturing the initialization outcome.The initialization result is discarded with
let _ = .... Consider capturing this value to make it available for tests that might need to inspect it.- let _ = contract + let init_outcome = contract .call("new") .args_json(json!(init_args)) .transact() .await?;
59-65: Use.expect()with a descriptive message for error handling.The hardcoded public key is unlikely to fail parsing, but for better error messages, replace
unwrap()withexpect()that includes a descriptive message.let init_args = MockStakingPoolInitArgs { owner_id: owner.id().clone(), stake_public_key: PublicKey::from_str( "ed25519:6E8sCci9badyRkXb3JoRpBj5p8C6Tw41ELDZoiihKEtp", ) - .unwrap(), + .expect("Failed to parse hardcoded public key"), voting_account_id, };
66-70: Consider capturing the initialization outcome.Similar to the voting contract initialization, consider capturing the result instead of discarding it.
- let _ = contract + let init_outcome = contract .call("new") .args_json(json!(init_args)) .transact() .await?;
85-91: Fix inconsistent indentation and use.expect()for better error handling.The indentation of
.unwrap()is inconsistent with the rest of the code. Also, as with the other instances, consider using.expect()for better error messages.let init_args = MockStakingPoolInitArgs { owner_id: owner.id().clone(), stake_public_key: PublicKey::from_str( "ed25519:6E8sCci9badyRkXb3JoRpBj5p8C6Tw41ELDZoiihKEtp", ) - .unwrap(), + .expect("Failed to parse hardcoded public key"), voting_account_id, };
135-136: Improve error handling for root account retrieval.The
unwrap()call for the root account should be replaced with proper error handling to make the code more robust.- let root = sandbox.root_account().unwrap(); + let root = sandbox.root_account().expect("Failed to get sandbox root account");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
makefile(1 hunks)src/lib.rs(9 hunks)tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)tests/test_init.rs(1 hunks)tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib.rs
- tests/test_init.rs
- makefile
- tests/contracts/mock-staking-pool/src/lib.rs
- tests/test_vote.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (4)
tests/utils/mod.rs (4)
97-102: Good practice: Asserting transaction success.The explicit assertion of transaction success with a detailed error message is a good practice. This helps catch issues early with clear diagnostics.
1-8: Clean and focused imports.The imports are well-organized and appropriately scoped for the functionality in this module.
9-20: Well-structured contract initialization argument structs.The
VotingInitArgsandMockStakingPoolInitArgsstructs are clearly defined with appropriate serialization attributes.
109-117: Well-designed environment setup functions.The
setup_envandsetup_env_manyfunctions provide a clean API for tests to set up their test environment with single or multiple staking pool contracts.Also applies to: 119-128
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
35-35: Step name should reflect the actual command
The step is still named “Run cargo test” even though it now invokesmake test. For clarity, update thenameto something like “Run tests” or “Run make test”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks)makefile(1 hunks)src/lib.rs(9 hunks)tests/test_init.rs(1 hunks)tests/test_vote.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_init.rs
- makefile
- src/lib.rs
- tests/test_vote.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Code Linter
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/utils.rs (4)
7-8: Consider handling deserialization errors more gracefully.Using
unwrap()will panic if deserialization fails. While this might be acceptable for integration tests where failing fast is useful, consider using a more robust error handling approach if this code could be used in other contexts.- |validators| near_sdk::borsh::from_slice(&validators).unwrap(), + |validators| near_sdk::borsh::from_slice(&validators).expect("Failed to deserialize validators map"),
15-16: Consider handling serialization errors more gracefully.Using
unwrap()will panic if serialization fails. Consider usingexpect()with a descriptive message.- &near_sdk::borsh::to_vec(&validators).unwrap(), + &near_sdk::borsh::to_vec(&validators).expect("Failed to serialize validators map"),
19-23: Consider handling deserialization errors more gracefully in get_validator_total_stake.Same issue with
unwrap()as mentioned earlier.- .map_or(0, |amount| near_sdk::borsh::from_slice(&amount).unwrap()) + .map_or(0, |amount| near_sdk::borsh::from_slice(&amount).expect("Failed to deserialize total stake"))
25-31: Consider handling serialization errors more gracefully in set_validator_total_stake.Same issue with
unwrap()as mentioned earlier.- &near_sdk::borsh::to_vec(&amount).unwrap(), + &near_sdk::borsh::to_vec(&amount).expect("Failed to serialize total stake"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/lib.rs(9 hunks)src/utils.rs(1 hunks)tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib.rs
- tests/test_vote.rs
- tests/utils/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (3)
src/utils.rs (3)
33-40: Properly handles the case when a validator doesn't exist.Good use of
unwrap_or_default()to safely handle the case where a validator doesn't have a stake yet.
58-63: Clean implementation of feature-based delegation.The function elegantly delegates to the appropriate implementation based on the compilation feature.
65-70: Clean implementation of feature-based delegation for total stake.The function elegantly delegates to the appropriate implementation based on the compilation feature.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_vote.rs (3)
110-132: Clarify test assertions and error messagesThe conditional assertions in this loop are checking if the voting process completes after a certain number of votes (200 out of 300), but the error message and logic could be clearer.
Improve the assertion error messages and clarify the test's intent:
for (index, staking_pool_contract) in staking_pool_contracts.iter().enumerate() { sandbox.fast_forward(500).await?; let block = sandbox.view_block().await?; let outcome = owner .call(voting_contract.id(), "ping") .gas(Gas::from_tgas(300)) .transact() .await?; if index <= 200 { assert!( outcome.is_success(), - "{:#?}", + "Ping should succeed while voting is still in progress (vote {}/201). Error: {:#?}", + index + 1, outcome.into_result().unwrap_err() ); } else { assert!( outcome.is_failure(), - "Ping should failed since vote is finished", + "Ping should fail after vote #{} since voting is complete (requires 201/300 votes)", + index + 1 ); break; }
149-156: Reduce excessive logging in loop iterationsPrinting detailed vote information for every iteration in a loop with 300 iterations will generate a lot of output, making it difficult to parse test results and potentially slowing down test execution.
Consider reducing the verbosity by logging only at specific intervals:
- println!("validator #{} voted at epoch ({})", index, block.epoch_id()); - println!( - "Votes: {:#?}", - voting_contract - .view("get_votes") - .await? - .json::<HashMap<AccountId, String>>()? - ); + // Log only at the beginning, end, and at specific intervals + if index == 0 || index == 200 || index % 50 == 0 { + println!("validator #{} voted at epoch ({})", index, block.epoch_id()); + println!( + "Votes: {:#?}", + voting_contract + .view("get_votes") + .await? + .json::<HashMap<AccountId, String>>()? + ); + }
1-8: Consider importing specific utility functionsThe current approach imports all functions from the utils module. For better clarity and to avoid potential name conflicts, consider importing specific functions that are used in this file.
mod utils; - use utils::*; + use utils::{create_account, deploy_voting_contract, setup_env, setup_env_many};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
makefile(1 hunks)tests/test_vote.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- makefile
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_vote.rs (1)
tests/utils/mod.rs (4)
deploy_voting_contract(22-43)setup_env(110-126)create_account(151-163)setup_env_many(128-149)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
tests/test_vote.rs
Outdated
| #[tokio::test] | ||
| async fn test_simple_vote() -> Result<(), Box<dyn std::error::Error>> { | ||
| let (staking_pool_contract, voting_contract, sandbox, owner) = setup_env().await?; | ||
|
|
||
| let alice = create_account(&sandbox, "alice", 10000).await?; | ||
| let outcome = alice | ||
| .call(staking_pool_contract.id(), "deposit_and_stake") | ||
| .args_json(json!({})) | ||
| .gas(Gas::from_tgas(250)) | ||
| .deposit(NearToken::from_near(1000)) | ||
| .transact() | ||
| .await?; | ||
| assert!( | ||
| outcome.is_success(), | ||
| "{:#?}", | ||
| outcome.into_result().unwrap_err() | ||
| ); | ||
|
|
||
| let staked_balance = voting_contract | ||
| .view("get_validator_stake") | ||
| .args_json(json!({ | ||
| "validator_account_id": staking_pool_contract.id() | ||
| })) | ||
| .await?; | ||
| println!( | ||
| "user account: {}, {:#?}", | ||
| alice.id(), | ||
| staked_balance.json::<String>()? | ||
| ); | ||
|
|
||
| let outcome = owner | ||
| .call(staking_pool_contract.id(), "vote") | ||
| .args_json(json!({ | ||
| "voting_account_id": voting_contract.id(), | ||
| "is_vote": true | ||
| })) | ||
| .gas(Gas::from_tgas(200)) | ||
| .transact() | ||
| .await?; | ||
| assert!( | ||
| outcome.is_success(), | ||
| "{:#?}", | ||
| outcome.into_result().unwrap_err() | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test validation for vote recording
The test successfully verifies the staked balance, but doesn't verify that the vote was actually recorded after calling the vote method. Adding verification of the vote state would make this test more comprehensive.
Add verification of the vote state after the vote operation:
assert!(
outcome.is_success(),
"{:#?}",
outcome.into_result().unwrap_err()
);
+ // Verify the vote was recorded
+ let votes = voting_contract
+ .view("get_votes")
+ .await?
+ .json::<HashMap<AccountId, String>>()?;
+ assert!(votes.contains_key(staking_pool_contract.id()), "Vote not recorded");
+ assert_eq!(votes.get(staking_pool_contract.id()).unwrap(), "1000", "Incorrect vote amount");
Ok(())
}🤖 Prompt for AI Agents
In tests/test_vote.rs around lines 34 to 80, the test_simple_vote function
verifies staking but does not check if the vote was recorded after calling the
vote method. To fix this, add a call to the voting_contract to query the vote
state for the owner or relevant account after the vote transaction completes.
Then assert that the vote state matches the expected value (e.g., true) to
ensure the vote was properly recorded.
tests/test_vote.rs
Outdated
| #[tokio::test] // it takes around 50 minutes | ||
| async fn test_many_votes() -> Result<(), Box<dyn std::error::Error>> { | ||
| let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(300).await?; | ||
|
|
||
| let alice = create_account(&sandbox, "alice", 50000).await?; | ||
|
|
||
| for staking_pool_contract in staking_pool_contracts.iter() { | ||
| let outcome = alice | ||
| .call(staking_pool_contract.id(), "deposit_and_stake") | ||
| .args_json(json!({})) | ||
| .gas(Gas::from_tgas(250)) | ||
| .deposit(NearToken::from_near(100)) | ||
| .transact() | ||
| .await?; | ||
| assert!( | ||
| outcome.is_success(), | ||
| "{:#?}", | ||
| outcome.into_result().unwrap_err() | ||
| ); | ||
|
|
||
| let total_staked = voting_contract.view("get_validator_total_stake").await?; | ||
| println!( | ||
| "total staked: {}, {:#?}", | ||
| alice.id(), | ||
| total_staked.json::<String>()? | ||
| ); | ||
| } | ||
|
|
||
| for (index, staking_pool_contract) in staking_pool_contracts.iter().enumerate() { | ||
| sandbox.fast_forward(500).await?; | ||
| let block = sandbox.view_block().await?; | ||
|
|
||
| let outcome = owner | ||
| .call(voting_contract.id(), "ping") | ||
| .gas(Gas::from_tgas(300)) | ||
| .transact() | ||
| .await?; | ||
|
|
||
| if index <= 200 { | ||
| assert!( | ||
| outcome.is_success(), | ||
| "{:#?}", | ||
| outcome.into_result().unwrap_err() | ||
| ); | ||
| } else { | ||
| assert!( | ||
| outcome.is_failure(), | ||
| "Ping should failed since vote is finished", | ||
| ); | ||
| break; | ||
| } | ||
|
|
||
| let outcome = owner | ||
| .call(staking_pool_contract.id(), "vote") | ||
| .args_json(json!({ | ||
| "voting_account_id": voting_contract.id(), | ||
| "is_vote": true | ||
| })) | ||
| .gas(Gas::from_tgas(200)) | ||
| .transact() | ||
| .await?; | ||
|
|
||
| assert!( | ||
| outcome.is_success(), | ||
| "{:#?}", | ||
| outcome.into_result().unwrap_err() | ||
| ); | ||
| println!("validator #{} voted at epoch ({})", index, block.epoch_id()); | ||
| println!( | ||
| "Votes: {:#?}", | ||
| voting_contract | ||
| .view("get_votes") | ||
| .await? | ||
| .json::<HashMap<AccountId, String>>()? | ||
| ); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize long-running test execution
This test takes approximately 50 minutes to run, which is excessively long for a unit test that might be run frequently during development. Long-running tests can slow down development cycles and CI pipelines.
Consider these improvements:
- Parameterize the test to allow running with fewer staking pools during regular testing:
#[tokio::test] // it takes around 50 minutes
async fn test_many_votes() -> Result<(), Box<dyn std::error::Error>> {
- let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(300).await?;
+ // Use fewer contracts (e.g., 10) for regular testing, more for comprehensive testing
+ let staking_pool_count = option_env!("FULL_TEST").map_or(10, |_| 300);
+ let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(staking_pool_count).await?;
// Rest of the test...
// Adjust the assertion accordingly
- if index <= 200 {
+ if index <= staking_pool_count * 2 / 3 { // Maintain the same proportion for vote completion-
Consider splitting the test into smaller, more focused tests that test specific aspects of the functionality.
-
Use batching or parallelization where possible to speed up operations.
📝 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.
| #[tokio::test] // it takes around 50 minutes | |
| async fn test_many_votes() -> Result<(), Box<dyn std::error::Error>> { | |
| let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(300).await?; | |
| let alice = create_account(&sandbox, "alice", 50000).await?; | |
| for staking_pool_contract in staking_pool_contracts.iter() { | |
| let outcome = alice | |
| .call(staking_pool_contract.id(), "deposit_and_stake") | |
| .args_json(json!({})) | |
| .gas(Gas::from_tgas(250)) | |
| .deposit(NearToken::from_near(100)) | |
| .transact() | |
| .await?; | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| let total_staked = voting_contract.view("get_validator_total_stake").await?; | |
| println!( | |
| "total staked: {}, {:#?}", | |
| alice.id(), | |
| total_staked.json::<String>()? | |
| ); | |
| } | |
| for (index, staking_pool_contract) in staking_pool_contracts.iter().enumerate() { | |
| sandbox.fast_forward(500).await?; | |
| let block = sandbox.view_block().await?; | |
| let outcome = owner | |
| .call(voting_contract.id(), "ping") | |
| .gas(Gas::from_tgas(300)) | |
| .transact() | |
| .await?; | |
| if index <= 200 { | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| } else { | |
| assert!( | |
| outcome.is_failure(), | |
| "Ping should failed since vote is finished", | |
| ); | |
| break; | |
| } | |
| let outcome = owner | |
| .call(staking_pool_contract.id(), "vote") | |
| .args_json(json!({ | |
| "voting_account_id": voting_contract.id(), | |
| "is_vote": true | |
| })) | |
| .gas(Gas::from_tgas(200)) | |
| .transact() | |
| .await?; | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| println!("validator #{} voted at epoch ({})", index, block.epoch_id()); | |
| println!( | |
| "Votes: {:#?}", | |
| voting_contract | |
| .view("get_votes") | |
| .await? | |
| .json::<HashMap<AccountId, String>>()? | |
| ); | |
| } | |
| Ok(()) | |
| } | |
| #[tokio::test] // it takes around 50 minutes | |
| async fn test_many_votes() -> Result<(), Box<dyn std::error::Error>> { | |
| // Use fewer contracts (e.g., 10) for regular testing, more for comprehensive testing | |
| let staking_pool_count = option_env!("FULL_TEST").map_or(10, |_| 300); | |
| let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(staking_pool_count).await?; | |
| let alice = create_account(&sandbox, "alice", 50000).await?; | |
| for staking_pool_contract in staking_pool_contracts.iter() { | |
| let outcome = alice | |
| .call(staking_pool_contract.id(), "deposit_and_stake") | |
| .args_json(json!({})) | |
| .gas(Gas::from_tgas(250)) | |
| .deposit(NearToken::from_near(100)) | |
| .transact() | |
| .await?; | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| let total_staked = voting_contract.view("get_validator_total_stake").await?; | |
| println!( | |
| "total staked: {}, {:#?}", | |
| alice.id(), | |
| total_staked.json::<String>()? | |
| ); | |
| } | |
| for (index, staking_pool_contract) in staking_pool_contracts.iter().enumerate() { | |
| sandbox.fast_forward(500).await?; | |
| let block = sandbox.view_block().await?; | |
| let outcome = owner | |
| .call(voting_contract.id(), "ping") | |
| .gas(Gas::from_tgas(300)) | |
| .transact() | |
| .await?; | |
| if index <= staking_pool_count * 2 / 3 { // Maintain the same proportion for vote completion | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| } else { | |
| assert!( | |
| outcome.is_failure(), | |
| "Ping should failed since vote is finished", | |
| ); | |
| break; | |
| } | |
| let outcome = owner | |
| .call(staking_pool_contract.id(), "vote") | |
| .args_json(json!({ | |
| "voting_account_id": voting_contract.id(), | |
| "is_vote": true | |
| })) | |
| .gas(Gas::from_tgas(200)) | |
| .transact() | |
| .await?; | |
| assert!( | |
| outcome.is_success(), | |
| "{:#?}", | |
| outcome.into_result().unwrap_err() | |
| ); | |
| println!("validator #{} voted at epoch ({})", index, block.epoch_id()); | |
| println!( | |
| "Votes: {:#?}", | |
| voting_contract | |
| .view("get_votes") | |
| .await? | |
| .json::<HashMap<AccountId, String>>()? | |
| ); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In tests/test_vote.rs around lines 82 to 160, the test_many_votes function runs
for about 50 minutes, which is too long for regular unit testing. To fix this,
parameterize the test to accept a smaller number of staking pools for faster
runs during development, and keep the large number for more exhaustive CI runs.
Additionally, split the test into smaller tests focusing on specific behaviors
like staking, voting, and pinging to isolate failures and reduce runtime. Where
possible, batch or parallelize calls such as deposits and votes to speed up
execution without changing test logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_vote.rs (3)
9-32: Fix redundant sandbox creationThere's a redundant sandbox creation at line 22 which is inconsistent with the test setup. The voting contract is deployed to the first sandbox, but you're trying to create a user account in a separate sandbox environment.
Apply this diff to use the initially created sandbox:
async fn test_non_validator_cannot_vote() -> Result<(), Box<dyn std::error::Error>> { let sandbox = near_workspaces::sandbox().await?; let (contract, _) = deploy_voting_contract( &sandbox, (SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_millis() + 10 * 60 * 1000) as u64, ) .await?; - let sandbox = near_workspaces::sandbox().await?; let user_account = sandbox.dev_create_account().await?; let outcome = user_account .call(contract.id(), "vote") .args_json(json!({"is_vote": true})) .transact() .await?; assert!(outcome.is_failure(),); Ok(()) }
34-80: Enhance test validation for vote recordingThe test successfully verifies the staking balance and that the vote transaction completes successfully, but it doesn't verify that the vote was actually recorded in the contract state.
Add verification of the vote state after the vote operation:
assert!( outcome.is_success(), "{:#?}", outcome.into_result().unwrap_err() ); + // Verify the vote was recorded + let votes = voting_contract + .view("get_votes") + .await? + .json::<HashMap<AccountId, String>>()?; + assert!(votes.contains_key(staking_pool_contract.id()), "Vote not recorded"); + assert_eq!(votes.get(staking_pool_contract.id()).unwrap(), "1000", "Incorrect vote amount"); Ok(()) }
82-160: Optimize long-running test executionThis test takes approximately 50 minutes to run, which is extremely long for a unit test that might be run frequently during development.
Consider these improvements:
- Parameterize the test to allow running with fewer staking pools during regular testing:
#[tokio::test] // it takes around 50 minutes async fn test_many_votes() -> Result<(), Box<dyn std::error::Error>> { - let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(300).await?; + // Use fewer contracts (e.g., 10) for regular testing, more for comprehensive testing + let staking_pool_count = option_env!("FULL_TEST").map_or(10, |_| 300); + let (staking_pool_contracts, voting_contract, sandbox, owner) = setup_env_many(staking_pool_count).await?; // Rest of the test... // Adjust the assertion accordingly - if index <= 200 { + if index <= staking_pool_count * 2 / 3 { // Maintain the same proportion for vote completion
Consider splitting the test into smaller, more focused tests that test specific aspects of the functionality.
Use batching or parallelization where possible to speed up operations.
🧹 Nitpick comments (4)
tests/test_vote.rs (4)
162-213: Improve error assertion in expiration testThe assertion on line 210 is checking for a specific error message using string contains, which can be fragile if the error message changes.
Consider using a more robust approach to verify the error condition:
- assert!(outcome.into_result().unwrap_err().to_string().contains("Smart contract panicked: Voting deadline has already passed")); + // Extract the error and check its properties + let err = outcome.into_result().unwrap_err(); + // Check error type and code if available + assert!(matches!(err.kind(), near_workspaces::types::ErrorKind::Contract(_))); + assert!(err.to_string().contains("Voting deadline has already passed"), + "Expected deadline error message, got: {}", err);This approach provides better error messages and is slightly more structured in checking the error type first.
52-63: Add more descriptive debug messageThe debug print statement could be more descriptive about what value it's displaying.
println!( - "user account: {}, {:#?}", + "Staked balance for validator {}: {:#?}", alice.id(), staked_balance.json::<String>()? );
102-108: Improve debug message clarityThe print statement about total staked amount should reference the staking pool account rather than the alice account.
println!( - "total staked: {}, {:#?}", + "Total staked after deposit from {}: {:#?}", alice.id(), total_staked.json::<String>()? );
149-157: Consider reducing debug output in loopPrinting detailed vote information for all 300 iterations creates excessive output that makes it difficult to find important information.
Consider limiting the output to key points in the test:
println!("validator #{} voted at epoch ({})", index, block.epoch_id()); - println!( - "Votes: {:#?}", - voting_contract - .view("get_votes") - .await? - .json::<HashMap<AccountId, String>>()? - ); + // Only print detailed vote information occasionally to reduce output volume + if index % 50 == 0 || index > 190 { + println!( + "Votes at milestone {}: {:#?}", + index, + voting_contract + .view("get_votes") + .await? + .json::<HashMap<AccountId, String>>()? + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_vote.rs (1)
tests/utils/mod.rs (4)
deploy_voting_contract(22-43)setup_env(110-127)create_account(152-164)setup_env_many(129-150)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (1)
tests/test_vote.rs (1)
1-8: LGTM: Imports and module setup are appropriateThe imports and module setup look correct for these integration tests, utilizing the shared utilities from the utils module.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_vote.rs (3)
9-32: Fix redundant sandbox creationThe redundant second sandbox creation at line 22 is still present. This creates confusion since the voting contract is deployed to the first sandbox, but the user account is created in a different sandbox instance.
34-80: Enhance test validation for vote recordingThe test successfully verifies the staked balance, but doesn't verify that the vote was actually recorded after calling the vote method. Adding verification of the vote state would make this test more comprehensive.
82-160: Optimize long-running test executionThis test takes approximately 50 minutes to run, which is excessively long for a unit test that might be run frequently during development. Long-running tests can slow down development cycles and CI pipelines.
🧹 Nitpick comments (1)
tests/test_vote.rs (1)
1-380: Consider refactoring for improved maintainabilityThe test suite is comprehensive but could benefit from reducing code duplication and improving consistency:
Extract common staking operations: Multiple tests repeat similar staking setup patterns that could be extracted into helper functions.
Define constants for magic numbers: Hardcoded values like
1000 NEAR,100 NEAR, gas amounts, and timeouts should be extracted as named constants.Standardize gas usage: Gas amounts are specified inconsistently across similar operations.
Example improvements:
// At the top of the file const DEFAULT_STAKE_AMOUNT: u128 = 1000; const LARGE_STAKE_AMOUNT: u128 = 100; const VOTE_GAS: u64 = 200; const STAKE_GAS: u64 = 250; // Helper function for common staking pattern async fn setup_staking_for_user( user: &Account, pool: &Contract, amount: u128, ) -> Result<(), Box<dyn std::error::Error>> { let outcome = user .call(pool.id(), "deposit_and_stake") .args_json(json!({})) .gas(Gas::from_tgas(STAKE_GAS)) .deposit(NearToken::from_near(amount)) .transact() .await?; assert!(outcome.is_success(), "{:#?}", outcome.into_result().unwrap_err()); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (3)
tests/test_vote.rs (3)
162-218: Well-implemented vote expiration testThis test effectively validates the vote expiration functionality with proper setup, time manipulation, and specific error message verification. The test flow is logical and comprehensive.
220-289: Comprehensive vote withdrawal testThis test thoroughly validates the vote withdrawal functionality with proper verification of vote state before and after withdrawal operations. The test flow is well-structured and covers the withdrawal scenario comprehensively.
291-380: Excellent edge case coverage for unstaking after votingThis test effectively validates the important edge case where unstaking occurs after voting. It properly verifies that votes persist after unstaking and that subsequent votes correctly update the vote state. The test logic is sound and comprehensive.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tests/contracts/mock-staking-pool/src/lib.rs (3)
57-57: Pass a reference toinsertinstead of movingaccount_id.
LookupMap::insertexpects&AccountId. Moving the value forces an allocation and breaks subsequent use. Use a reference:- self.accounts.insert(account_id, balance + amount); + self.accounts.insert(&account_id, balance + amount);
71-71: Pass a reference toinsertinstead of movingaccount_id.Same issue as in the deposit path -
LookupMap::insertexpects&AccountId.- self.accounts.insert(account_id.clone(), balance - amount); + self.accounts.insert(&account_id, balance - amount);
108-108:internal_account_staked_balancedereferences an integer reference.
LookupMap::getreturnsOption<Balance>, notOption<&Balance>. The current code fails to compile (expected u128, found &u128).- *self.accounts.get(account_id).unwrap_or(&0u128) + self.accounts.get(account_id).unwrap_or(0u128)tests/test_vote.rs (2)
22-22: Fix redundant sandbox creation.You're creating a second sandbox instance after already creating one at line 11. This is unnecessary and potentially confusing since the voting contract is deployed to the first sandbox.
- let sandbox = near_workspaces::sandbox().await?; - let user_account = sandbox.dev_create_account().await?; + let user_account = sandbox.dev_create_account().await?;
59-72: Enhance test validation for vote recording.The test successfully verifies the staked balance, but doesn't verify that the vote was actually recorded after calling the vote method. Adding verification of the vote state would make this test more comprehensive.
Add verification of the vote state after the vote operation:
assert!( outcome.is_success(), "{:#?}", outcome.into_result().unwrap_err() ); + // Verify the vote was recorded + let votes = voting_contract + .view("get_votes") + .await? + .json::<HashMap<AccountId, String>>()?; + assert!(votes.contains_key(staking_pool_contract.id()), "Vote not recorded"); Ok(())
🧹 Nitpick comments (3)
tests/scripts/vote.sh (1)
5-5: Fix variable interpolation quoting.The surrounding quotes actually unquote the variable, which can cause unexpected behavior if the variable contains spaces or special characters.
- VALIDATOR_ID="mock-validator-"${i}".testnet" + VALIDATOR_ID="mock-validator-${i}.testnet"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 5-5: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh (2)
7-7: Fix variable interpolation quoting.The surrounding quotes actually unquote the variable, which can cause unexpected behavior if the variable contains spaces or special characters.
- VALIDATOR_ID="mock-validator-"${i}".testnet" + VALIDATOR_ID="mock-validator-${i}.testnet"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
13-13: Complete the incomplete comment.The comment on line 13 appears to be incomplete and should be finished or removed.
- # stake some NEAR to + # stake some NEAR to the validator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/contracts/mock-staking-pool/src/lib.rs(1 hunks)tests/scripts/create-validators.sh(1 hunks)tests/scripts/vote.sh(1 hunks)tests/test_vote.rs(1 hunks)tests/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/scripts/create-validators.sh (1)
tests/contracts/mock-staking-pool/src/lib.rs (2)
new(40-48)deposit_and_stake(51-61)
tests/test_vote.rs (1)
tests/utils/mod.rs (4)
deploy_voting_contract(24-45)setup_env(112-131)create_account(156-168)setup_env_many(133-154)
tests/contracts/mock-staking-pool/src/lib.rs (2)
src/lib.rs (2)
vote(51-86)set_validator_stake(171-173)src/utils.rs (1)
set_validator_stake(43-56)
🪛 Shellcheck (0.10.0)
tests/scripts/vote.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 5-5: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (3)
tests/contracts/mock-staking-pool/src/lib.rs (1)
74-74: Excellent implementation of token transfer in unstake.The unstake method now properly transfers tokens back to the user, addressing the previous concern about mock behavior consistency.
tests/test_vote.rs (2)
78-79: Well-handled long-running test optimization.Good use of the
#[ignore]attribute for the 50-minute test. This prevents it from running during regular development while still keeping it available for comprehensive testing when needed.
342-346: Good test coverage for unstaking behavior.The test properly verifies that votes remain recorded even after unstaking, which is important for testing the voting contract's behavior with dynamic stake changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_staking_pool.rs (4)
9-26: Consider more robust balance assertionsThe balance range check
(8999..9000)may be too narrow and could lead to flaky tests if gas costs vary. Consider using a wider range or calculating expected balance more precisely.- assert!((8999..9000).contains(&balance)); + // Account for gas fees with more generous range + assert!((8990..9010).contains(&balance), "Expected balance around 9000 NEAR, got {}", balance);
27-44: Enhance test coverage with staking pool state verificationWhile the balance check verifies unstaking worked, consider also verifying the staking pool's internal state (e.g., total staked amount) to ensure complete test coverage.
Add verification of staking pool state after unstaking:
let balance = alice.view_account().await?.balance.as_near(); assert!((9999..10000).contains(&balance)); + + // Verify staking pool shows zero staked amount + let total_staked = staking_pool_contract + .view("get_total_staked_balance") + .await? + .json::<String>()?; + assert_eq!(total_staked, "0");
49-57: Extract duplicate timestamp logic to utility functionThe timestamp calculation logic is duplicated from
setup_env. Consider extracting this to a utility function for better maintainability.In
tests/utils/mod.rs, add:pub fn get_future_timestamp_ms(minutes_ahead: u64) -> u64 { (SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_millis() + minutes_ahead * 60 * 1000) as u64 }Then use it here:
- let (new_voting_contract, _) = deploy_voting_contract( - &sandbox, - (SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_millis() - + 10 * 60 * 1000) as u64, - ) - .await?; + let (new_voting_contract, _) = deploy_voting_contract( + &sandbox, + get_future_timestamp_ms(10), + ) + .await?;
9-104: Consider extracting magic numbers to constantsThe test uses magic numbers (1000, 10000) that could be made more maintainable as named constants.
Add constants at the top of the file:
mod utils; +const ALICE_INITIAL_BALANCE: u128 = 10000; +const STAKE_AMOUNT: u128 = 1000; +const GAS_LIMIT: u64 = 250; + #[tokio::test]Then use these constants throughout the tests to improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_staking_pool.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_staking_pool.rs (1)
tests/utils/mod.rs (3)
create_account(156-168)deploy_voting_contract(24-45)setup_env(112-131)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (2)
tests/test_staking_pool.rs (2)
1-7: LGTM: Clean imports and module structureThe imports are well-organized and appropriate for integration testing with NEAR blockchain. The local utils module import suggests good test code organization.
90-101: Good cross-contract state verificationExcellent test design! Verifying that the voting contract correctly shows the validator stake demonstrates proper integration between the staking pool and voting contracts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/scripts/create-proposal.sh (2)
1-1: Enforce strict mode and portable shebang
Use a more portable shebang and enable strict error handling to catch failures early:-#!/bin/bash +#!/usr/bin/env bash +set -euo pipefailThis will make the script exit on errors, treat unset variables as failures, and handle pipes correctly.
10-10: Safely construct JSON args and verify deployment
Inline JSON concatenation risks injection and quoting bugs. Use a here-doc orjqto build the payload, and check the deployment’s exit status:read -r -d '' INIT_ARGS <<EOF { "proposal": "$PROPOSAL_DESCRIPTION", "deadline_timestamp_ms": $DEADLINE_TIMESTAMP } EOF if ! near contract deploy \ "$VOTING_ACCOUNT_ID" \ use-file validator_voting.wasm \ with-init-call new \ json-args "$INIT_ARGS" \ prepaid-gas '100.0 Tgas' \ attached-deposit '0 NEAR' \ network-config testnet \ sign-with-legacy-keychain \ send; then echo "ERROR: Contract deployment failed" >&2 exit 1 fiThis improves readability, prevents JSON quoting pitfalls, and ensures failures are caught.
tests/scripts/vote.sh (2)
1-4: Enable strict error handling in the script.Add
set -euo pipefailright after the shebang to ensure the script exits on errors, undefined variables, and pipeline failures, preventing silent failures.
6-6: Refine variable assignment quoting.Change:
VALIDATOR_ID="mock-validator-"${i}".testnet"to:
VALIDATOR_ID="mock-validator-${i}.testnet"This avoids unintentional word splitting and improves readability.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh (3)
1-5: Enable strict error handling in the script.Add
set -euo pipefailafter the shebang to catch errors, undefined variables, and pipeline failures early.
8-8: Refine variable assignment quoting.Replace:
VALIDATOR_ID="mock-node-"${i}".testnet"with:
VALIDATOR_ID="mock-node-${i}.testnet"to ensure consistent quoting and prevent unexpected splitting.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
10-10: Remove or update commented-out code.The commented
# near account create-account sponsor-by-faucet-serviceline appears outdated. Remove or update this example to keep the script clean and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/scripts/create-proposal.sh(1 hunks)tests/scripts/create-validators.sh(1 hunks)tests/scripts/vote.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/scripts/create-validators.sh (1)
tests/contracts/mock-staking-pool/src/lib.rs (2)
new(40-48)deposit_and_stake(51-61)
🪛 Shellcheck (0.10.0)
tests/scripts/create-validators.sh
[warning] 8-8: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/vote.sh
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (1)
tests/scripts/create-validators.sh (1)
13-13: Verify the contract file path.Ensure
mock_staking_pool.wasmis correctly referenced relative to the script’s location. If the file resides elsewhere (e.g., undertests/contracts/mock-staking-pool/target/), update the path accordingly to avoid runtime errors.
Summary by CodeRabbit
near-workspacesdevelopment dependency..gitignoreto exclude all nestedtargetdirectories and added exclusions fortests/resandres.cargo-near.