feat: validator voting contract #1
Conversation
|
|
||
| /// Method for validators to vote or withdraw the vote. | ||
| /// Votes for if `is_vote` is true, or withdraws the vote if `is_vote` is false. | ||
| pub fn vote(&mut self, is_vote: bool) { |
There was a problem hiding this comment.
Can we change the code as following?
pub fn vote(&mut self, is_vote: bool) {
let account_id = env::predecessor_account_id();
if is_vote {
self.votes.insert(&account_id, &0); // or Balance::MAX
} else {
self.votes.remove(&account_id);
}
self.ping();
}There was a problem hiding this comment.
This won't work well.
In this way, if ping() is already invoked in the current epoch, the total_voted_stake will not be updated right away after a new vote().
There was a problem hiding this comment.
I think remove the condition if cur_epoch_height != self.last_epoch_height can solve this issue, this is unnecessary
There was a problem hiding this comment.
I'm fine with the condition to save gas cost for vote() function call. It's not necessary to update stake balances very frequently.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update introduces the initial project setup for a NEAR smart contract enabling stake-weighted validator voting on proposals. It adds comprehensive CI workflows for building, formatting, linting, and testing. The contract logic, configuration files, build instructions, and documentation are established, with unit tests and a commented integration test scaffold included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant NEAR_Env
User->>Contract: new(proposal, deadline)
Note right of Contract: Initializes contract state
User->>Contract: vote(is_vote)
Contract->>NEAR_Env: Query validator stake for caller
Contract->>Contract: Update votes & total_voted_stake
Contract->>Contract: Check if threshold met
Contract->>Contract: Emit Voted or VoteWithdrawn event
User->>Contract: ping()
Contract->>NEAR_Env: Query all validator stakes
Contract->>Contract: Update votes & total_voted_stake
Contract->>Contract: Check if threshold met
Contract->>Contract: Emit ProposalApproved event if threshold met
User->>Contract: get_total_voted_stake()
Contract-->>User: (voted_stake, total_stake)
User->>Contract: get_votes()
Contract-->>User: votes map
User->>Contract: get_result()
Contract-->>User: result timestamp or None
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 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 (10)
.github/workflows/build.yml (1)
13-20: Add caching to speed up CI builds.
To reduce build times, consider caching the Cargo registry and generated artifacts between runs. For example:+ - name: Cache Cargo registry + uses: actions/cache@v3 + with: + path: ~/.cargo/registry + key: ${{ runner.os }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }} + - name: Cache build artifacts + uses: actions/cache@v3 + with: + path: target + key: ${{ runner.os }}-cargo-target-${{ hashFiles('**/Cargo.lock') }}This can significantly speed up subsequent builds.
README.md (1)
3-3: Fix missing article in description.
Change “Validators can callvotefunction” to “Validators can call thevotefunction” for grammatical clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ... specific proposal. Validators can callvotefunction to vote for yes or no wit...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
.github/workflows/test.yml (1)
13-16: Speed up CI by caching dependencies and build artifacts.
Introduce caching after the checkout step in each job. For example:+ - name: Cache Cargo registry + uses: actions/cache@v3 + with: + path: ~/.cargo/registry + key: ${{ runner.os }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }} + - name: Cache build artifacts + uses: actions/cache@v3 + with: + path: target + key: ${{ runner.os }}-cargo-target-${{ hashFiles('**/Cargo.lock') }}This will significantly reduce job execution time on subsequent runs.
Also applies to: 21-27, 32-35
tests/test_basics.rs (1)
1-29: Entire test file is commented-out – enable or delete to avoid bit-rot
cargo testwill silently ignore this file because every line is commented. Either
- uncomment the code (and add
near-workspacesback todev-dependencies) so that the integration test actually protects the contract, or- remove the file / gate it behind
#[cfg(feature = "e2e")]to keep the test suite signal-to-noise ratio high.Running dead code through CI wastes time and may mislead new contributors.
Cargo.toml (2)
6-10: Fill in therepositoryfieldTooling such as
cargo about, NEAR explorer metadata andcargo-nearpick up the
repositoryURL to surface links back to source. Leaving a placeholder degrades UX
for downstream integrators.
44-45: Upgrade Tokio – 1.12 is ~3 years oldTokio 1.12 lacks many perf & soundness fixes. Unless you have a lock-in
constraint, bumping to the latest 1.x (currently 1.37) is a drop-in upgrade.-tokio = { version = "1.12.0", features = ["full"] } +tokio = { version = "1.37.0", features = ["full"] }src/lib.rs (4)
31-39: Initializelast_epoch_heightwith the current epochSetting it to
0means the very firstvote()triggers an unnecessary full
re-scan inping(). Capturingenv::epoch_height()once saves gas on the first
call.- last_epoch_height: 0, + last_epoch_height: env::epoch_height(),
41-50: Duplicate deadline / result checks – consider DRYing
vote()andping()repeat the samerequire!guards. Extracting an internal
assert_voting_open()would reduce byte-code size and future maintenance
foot-guns.
75-95:ping()reallocates and re-serialises the vote map every epoch
std::mem::takefollowed by reinsertion forces two full writes of the map per
epoch. When switching toUnorderedMap, you can iterate keys in-place and only
touch entries whose stake actually changed, cutting gas roughly in half.
103-107: Emit an event when voting finalisesExternal indexers and off-chain clients have no efficient way to learn that
resultflipped toSome(_). Emitting a JSON-serialised event via
near_sdk::env::log_strkeeps the contract stateless yet discoverable.if self.total_voted_stake > total_stake * 2 / 3 { self.result = Some(env::block_timestamp_ms()); env::log_str(&format!("VOTING_FINISHED:{}", self.result.unwrap())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)rust-toolchain.toml(1 hunks)src/lib.rs(1 hunks)tests/test_basics.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ... specific proposal. Validators can call vote function to vote for yes or no wit...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (3)
.gitignore (1)
1-1: Ignore build artifacts as expected.
The simplified.gitignorecorrectly excludes thetargetdirectory where Rust artifacts are placed.rust-toolchain.toml (1)
1-4: Toolchain specification is clear and appropriate.
Pinning the stable channel withrustfmtand thewasm32-unknown-unknowntarget ensures consistent builds and formatting across all environments.Cargo.toml (1)
41-45: Enablenear-workspacesfor integration testsThe scaffold in
tests/test_basics.rsdepends onnear-workspaces, but the crate
is commented out here. Uncomment to avoid compilation errors when the test is
re-enabled.-# near-workspaces = { version = "0.18", features = ["unstable"] } +near-workspaces = { version = "0.18", features = ["unstable"] }
| proposal: String, | ||
| deadline_timestamp_ms: Timestamp, | ||
| votes: HashMap<AccountId, Balance>, | ||
| total_voted_stake: Balance, | ||
| result: Option<Timestamp>, | ||
| last_epoch_height: EpochHeight, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Persisting votes in std::collections::HashMap is gas-inefficient
Each insert/remove rewrites the whole map because the entire structure is stored
as a single Borsh blob. With >300 validators this quickly balloons write costs.
Prefer near_sdk::collections::UnorderedMap/LookupMap, which stores each entry
under its own key and scales O(1) per operation.
Refactor sketch:
-use std::collections::HashMap;
-...
-votes: HashMap<AccountId, Balance>,
+use near_sdk::collections::UnorderedMap;
+...
+votes: UnorderedMap<AccountId, Balance>,There was a problem hiding this comment.
There're indeed potential gas issues at scale. That's something we'd like to test in integration test in another PR to confirm whether the gas assumption works well or not.
There was a problem hiding this comment.
@linguists That's a sensible approach. Integration testing will provide empirical data on the actual gas costs of the current implementation. This can help determine if the optimization is necessary given your specific validator set size and usage patterns. Let me know if you'd like any guidance on setting up those tests when you get to that PR.
✏️ Learnings added
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.294Z
Learning: The team is aware of potential gas inefficiency with using `std::collections::HashMap` instead of `near_sdk::collections::UnorderedMap` or `LookupMap` for storing validator votes in the validator-voting contract. They plan to test the actual gas usage in integration tests in a future PR before deciding whether to implement this optimization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib.rs (1)
20-24:HashMapin contract state is gas-inefficient (repeat observation)
std::collections::HashMapstores the whole map as one Borsh blob,
causing every insert/remove to rewrite all votes.UnorderedMapor
LookupMapstores each key under its own trie node and scales O(1).Previous reviews captured this, so tagging as duplicate.
🧹 Nitpick comments (2)
src/events.rs (2)
17-32: Consider owning the payload insideEventto drop the lifetime parameterStoring references means every call-site has to keep the referenced
values alive until the event is emitted, which complicates usage and
prevents the enum from being passed around or queued for later logging.Making the fields
AccountId,String,U64,U128, … (owned
values) removes the need for the<'a>lifetime and produces a more
ergonomic API with negligible extra gas (serialization already copies
the data).-pub enum Event<'a> { - Voted { - validator_id: &'a AccountId, - }, +pub enum Event { + Voted { + validator_id: AccountId, + }, ... }
35-45: Handle (de-)serialization errors explicitly & avoid a doublejson!pass
json!(self)allocates aValue, which is then re-wrapped in another
Valuebefore being stringified. This creates two intermediate
allocations and silently ignores serialization errors.- let json = json!(self); - let event_json = json!({ - "standard": EVENT_STANDARD, - "version": EVENT_STANDARD_VERSION, - "event": json["event"], - "data": [json["data"]] - }) - .to_string(); + // single pass – fails early if serialization ever breaks + let event_json = serde_json::to_string(&serde_json::json!({ + "standard": EVENT_STANDARD, + "version": EVENT_STANDARD_VERSION, + "event": self, // `serde` extracts the tag automatically + })) + .expect("event serialization must succeed");This keeps the gas footprint unchanged while guaranteeing that any
serialization regression surfaces immediately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/events.rs(1 hunks)src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
src/lib.rs (1)
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.294Z
Learning: The team is aware of potential gas inefficiency with using `std::collections::HashMap` instead of `near_sdk::collections::UnorderedMap` or `LookupMap` for storing validator votes in the validator-voting contract. They plan to test the actual gas usage in integration tests in a future PR before deciding whether to implement this optimization.
🔇 Additional comments (1)
src/lib.rs (1)
73-83:check_result()is skipped on vote withdrawal – re-evaluate?When a validator withdraws (
is_vote == false) the contract does not
re-runcheck_result(). If >⅔ stake had already been reached within
the same epoch and a large withdrawal pushes it back under the
threshold, the contract will not notice until the next epoch’sping().If this is intentional (i.e. once a super-majority is reached in-epoch
the proposal is irrevocably approved), please add a comment. Otherwise
consider invokingself.check_result()unconditionally after
total_voted_stakeis updated.
NEAR Validator Voting Contract
The purpose of this contract is for validators to vote on any specific proposal. Validators can call
votefunction to vote for yes or no with the staked amount on the validator. If there are more than 2/3 of the stake at any given moment voting for yes, the voting is done. After the voting is finished or the voting deadline has passed, no one can further modify the contract. The voting contract is recommended to be pinged every epoch to make sure the latest stake is updated in the contract.Summary by CodeRabbit