-
Notifications
You must be signed in to change notification settings - Fork 1
feat: update voting rules #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant Events
User->>Contract: Casts vote (internal_vote)
Contract->>Contract: Update votes map with VotedStake (vote + stake)
Contract->>Contract: Update total_voted_stake and yes_stake
User->>Contract: Calls ping
Contract->>Contract: Recalculate stakes for all votes
Contract->>Contract: Update total_voted_stake and yes_stake
Contract->>Contract: Call check_result
alt Approval conditions met
Contract->>Events: Emit ProposalApproved with stake details
else Rejection conditions met
Contract->>Events: Emit ProposalRejected with stake details
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/lib.rs (2)Learnt from: linguists Learnt from: linguists ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (9)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/lib.rs (2)
18-23: Add missing Borsh serialization traits to theVoteenumThe
Voteenum needs to implement Borsh serialization traits since it's used withinVotedStakewhich requires both borsh and json serialization.#[near(serializers = [json])] +#[derive(PartialEq)] #[serde(rename_all = "lowercase")] pub enum Vote { Yes, No, }Additionally, update the
#[near(serializers = [json])]annotation to include borsh:-#[near(serializers = [json])] +#[near(serializers = [borsh, json])]
214-218: Fix the conversion fromVotedStaketoU128The code attempts to convert the entire
VotedStakestruct toU128, but should only convert thestakefield.pub fn get_votes(&self) -> HashMap<AccountId, U128> { self.votes .iter() - .map(|(account_id, stake)| (account_id.clone(), (*stake).into())) + .map(|(account_id, voted_stake)| (account_id.clone(), voted_stake.stake.into())) .collect() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/events.rs(1 hunks)src/lib.rs(6 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
src/events.rs (1)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
src/lib.rs (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
🧬 Code Graph Analysis (1)
src/lib.rs (2)
src/utils.rs (2)
validator_stake(58-63)validator_total_stake(65-70)tests/contracts/mock-staking-pool/src/lib.rs (2)
vote(15-15)vote(81-87)
🪛 GitHub Actions: Build
src/lib.rs
[error] 182-182: Cannot find value total_voted_stake in this scope. Did you mean self.total_voted_stake?
[error] 25-25: Trait BorshSerialize is not implemented for Vote. Required by #[near(serializers = [borsh, json])] macro.
[error] 25-25: Trait BorshDeserialize is not implemented for Vote. Required by #[near(serializers = [borsh, json])] macro.
[error] 27-27: Trait BorshSchema is not implemented for Vote used in field vote.
[error] 84-84: Binary operation == cannot be applied to type Vote. Missing implementation of PartialEq for Vote. Consider adding #[derive(PartialEq)].
[error] 142-142: Trait Default is not implemented for &VotedStake. unwrap_or_default() cannot be called on Option<&VotedStake>.
[error] 151-151: Binary operation == cannot be applied to type Vote. Missing implementation of PartialEq for Vote. Consider adding #[derive(PartialEq)].
[error] 217-217: Trait From<VotedStake> is not implemented for U128. Cannot convert VotedStake into U128.
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
src/events.rs (1)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
src/lib.rs (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
🧬 Code Graph Analysis (1)
src/lib.rs (2)
src/utils.rs (2)
validator_stake(58-63)validator_total_stake(65-70)tests/contracts/mock-staking-pool/src/lib.rs (2)
vote(15-15)vote(81-87)
🪛 GitHub Actions: Build
src/lib.rs
[error] 182-182: Cannot find value total_voted_stake in this scope. Did you mean self.total_voted_stake?
[error] 25-25: Trait BorshSerialize is not implemented for Vote. Required by #[near(serializers = [borsh, json])] macro.
[error] 25-25: Trait BorshDeserialize is not implemented for Vote. Required by #[near(serializers = [borsh, json])] macro.
[error] 27-27: Trait BorshSchema is not implemented for Vote used in field vote.
[error] 84-84: Binary operation == cannot be applied to type Vote. Missing implementation of PartialEq for Vote. Consider adding #[derive(PartialEq)].
[error] 142-142: Trait Default is not implemented for &VotedStake. unwrap_or_default() cannot be called on Option<&VotedStake>.
[error] 151-151: Binary operation == cannot be applied to type Vote. Missing implementation of PartialEq for Vote. Consider adding #[derive(PartialEq)].
[error] 217-217: Trait From<VotedStake> is not implemented for U128. Cannot convert VotedStake into U128.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Linter
🔇 Additional comments (1)
src/events.rs (1)
23-31: Event structure properly updatedThe addition of the
yes_stakefield to theProposalApprovedevent correctly reflects the new voting mechanism that tracks yes votes separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib.rs(16 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
src/lib.rs (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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 context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
src/lib.rs (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (14)
src/lib.rs (14)
18-24: LGTM: Vote enum properly configured for serialization and comparison.The addition of
PartialEq,Clone, andDebugtraits along with Borsh and JSON serialization support is correctly implemented for the Vote enum.
26-30: LGTM: VotedStake struct properly designed.The struct correctly encapsulates both vote type and stake amount, with appropriate serialization annotations.
46-49: LGTM: Contract state updated to support new voting logic.The changes correctly:
- Update votes map to store VotedStake instead of just Balance
- Add yes_stake tracking for separate accounting
- Change result from Option to Option
80-89: Verify stake accounting logic in ping method.The ping method now resets both
total_voted_stakeandyes_staketo 0 and recalculates them. The logic looks correct but needs verification of the stake accounting.
130-134: LGTM: Voting deadline enforcement added correctly.The addition of deadline checking in
internal_voteis appropriate - voting should be prohibited after the deadline while allowingpingto update stakes.
137-141: LGTM: Validator stake validation is correct.Properly validates that the account has positive stake before allowing voting.
143-158: LGTM: Stake calculation logic is sound.The logic correctly:
- Calculates yes stake contribution based on vote type
- Retrieves previous vote and stake values
- Handles the case where no previous vote exists
166-174: LGTM: Stake accounting and vote storage updated correctly.The accounting logic properly:
- Updates total_voted_stake by adding new stake and subtracting previous stake
- Updates yes_stake by adding new yes contribution and subtracting previous yes contribution
- Stores the new VotedStake entry
190-192: LGTM: Early return for deadline check is appropriate.Correctly prevents result determination before the deadline has passed.
194-221: LGTM: Updated proposal approval logic and event emission.The new logic correctly:
- Requires total voted stake > 1/3 of total stake (quorum requirement)
- Requires yes stake > 2/3 of total voted stake (supermajority of participants)
- Sets result to true/false instead of timestamp
- Emits appropriate ProposalApproved or ProposalRejected events with detailed metrics
230-236: LGTM: View method correctly returns stake tuple.The method properly returns (yes_stake, total_voted_stake, total_stake) as documented.
241-251: LGTM: Vote retrieval method updated correctly.Properly maps VotedStake entries to (Vote, stake) tuples for external consumption.
254-256: LGTM: Result method updated to return boolean.Correctly returns Option instead of Option to match the new result semantics.
798-801: Verify test logic for validator vote changes.The test shows validator 4 voting No, then Yes, but the vote count remains at 3. This suggests validator 4 was already in the votes map. Please verify this is the intended behavior.
This appears correct assuming the duplicate validator issue in line 784 is fixed - validator 4 changing their vote shouldn't increase the vote count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
tests/scripts/vote.sh (1)
11-15: Missingset -ecan hide failed votesIf the first call reverts the script will still attempt to read totals, yielding misleading numbers.
The strict-mode guards recommended earlier (set -euo pipefail) apply here as well.
🧹 Nitpick comments (3)
tests/scripts/create-proposal.sh (1)
7-7: Dead code: commented account-creation leaves the script half-doneIf the voting account does not exist beforehand the subsequent
near contract deploywill fail.
Either delete the line entirely (less noise) or keep the command active and guard it with an idempotency check.tests/scripts/create-validators.sh (1)
4-4: Consistent variable quoting
$VOTING_ACCOUNT_IDmight one day contain characters that the shell expands (e.g.,*).
Surround it with double quotes everywhere it is used, just like you already do for$VALIDATOR_ID.tests/scripts/vote.sh (1)
5-9: Only one iteration – intentional?
for i in {1..1}loops exactly once; if this was meant to be a quick smoke test, consider dropping the loop entirely for clarity or restore the previous range.
📜 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)
🧠 Learnings (4)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/vote.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-validators.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-proposal.sh (1)
Learnt from: linguists
PR: #1
File: README.md:10-11
Timestamp: 2025-05-14T03:12:48.262Z
Learning: For NEAR contracts, cargo near build (non-reproducible-wasm) is acceptable for local development, while cargo near build reproducible-wasm is recommended for production builds that will be deployed.
🧬 Code Graph Analysis (1)
tests/scripts/create-proposal.sh (2)
tests/contracts/mock-staking-pool/src/lib.rs (1)
new(40-48)src/lib.rs (1)
new(58-73)
🪛 Shellcheck (0.10.0)
tests/scripts/vote.sh
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh
[warning] 8-8: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/vote.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-validators.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-proposal.sh (1)
Learnt from: linguists
PR: #1
File: README.md:10-11
Timestamp: 2025-05-14T03:12:48.262Z
Learning: For NEAR contracts, cargo near build (non-reproducible-wasm) is acceptable for local development, while cargo near build reproducible-wasm is recommended for production builds that will be deployed.
🧬 Code Graph Analysis (1)
tests/scripts/create-proposal.sh (2)
tests/contracts/mock-staking-pool/src/lib.rs (1)
new(40-48)src/lib.rs (1)
new(58-73)
🪛 Shellcheck (0.10.0)
tests/scripts/vote.sh
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh
[warning] 8-8: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Linter
- GitHub Check: Tests
🔇 Additional comments (1)
tests/scripts/create-validators.sh (1)
7-8: Brace expansion{4..10}is Bash-specificThe script declares
#!/bin/bash, so this is OK locally, but portability to Dash/Zsh or CI environments that default to/bin/shwill break.
If portability matters useseq 4 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
makefile (1)
3-3: Declare explicit.PHONYtargets to silencecheckmakeand avoid name-collision pitfallsGNU make will treat
all,lint,validator-voting, etc. as real files unless they’re marked phony, which can lead to confusing “Nothing to be done” messages if a file with the same name is ever created.checkmakeis already warning about this.+ .PHONY: all lint validator-voting validator-voting-test test all: lint validator-voting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)makefile(1 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
🪛 checkmake (0.2.2)
makefile
[warning] 3-3: Target "all" should be declared PHONY.
(phonydeclared)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
🪛 checkmake (0.2.2)
makefile
[warning] 3-3: Target "all" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (1)
makefile (1)
3-3: Confirm that droppingvalidator-voting-testfrom the defaultalltarget doesn’t break CI expectations
make teststill buildsvalidator-voting-test, but many CI pipelines invokemake allby default. If your pipeline previously relied onmake allto compile the test Wasm, it will now silently skip that step.Please verify the CI configuration (e.g., GitHub Actions, GitLab CI) and update it if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/scripts/create-validators.sh (1)
15-15: Manual JSON construction is error-prone (see prior review)
Still concatenating JSON by hand; any quote/back-slash inOWNER_IDet al. will break the payload. Previous review already proposed ajq -nrefactor.tests/scripts/vote.sh (1)
9-9: Unsafe JSON concatenation – switch tojq -n(previously flagged)
This remains the same manual concat pattern already called out earlier.tests/scripts/create-proposal.sh (2)
1-1: Add strict-mode guards (previously suggested)
Still missingset -euo pipefail.
7-7: Manual JSON string building – same issue as before
Still vulnerable to quoting/escaping problems; migrate tojq -n.Also applies to: 15-15
🧹 Nitpick comments (5)
tests/scripts/create-validators.sh (2)
1-1: Add strict-mode guards to avoid silent failures
This script touches live test-net accounts; exiting early on error is essential.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
6-6: Fragile quoting – SC2027
The mixed"..."$var"...pattern removes quoting around the expanded value, defeating the point of the quotes and tripping Shellcheck. Use a single quoted string with${}interpolation.-export VOTING_ACCOUNT_ID="mock-proposal-"$SUFFIX".testnet" +export VOTING_ACCOUNT_ID="mock-proposal-${SUFFIX}.testnet" - VALIDATOR_ID="mock-node-"$SUFFIX"-"${i}".testnet" + VALIDATOR_ID="mock-node-${SUFFIX}-${i}.testnet"Also applies to: 10-10
tests/scripts/vote.sh (2)
1-1: Enable strict mode for safer execution
Same rationale as the other scripts—abort on first error to protect funds.#!/bin/bash +set -euo pipefail +IFS=$'\n\t'
4-4: SC2027 – surrounding quotes unquote the variable
Adopt consistent${}interpolation to keep the value quoted.-export VOTING_ACCOUNT_ID="mock-proposal-"$SUFFIX".testnet" +export VOTING_ACCOUNT_ID="mock-proposal-${SUFFIX}.testnet" - VALIDATOR_ID="mock-node-"$SUFFIX"-"${i}".testnet" + VALIDATOR_ID="mock-node-${SUFFIX}-${i}.testnet"Also applies to: 7-7
tests/scripts/create-proposal.sh (1)
4-4: SC2027 – quoting issue for interpolated variable
Same pattern; keep the entire string quoted.-export VOTING_ACCOUNT_ID="mock-proposal-"$SUFFIX".testnet" +export VOTING_ACCOUNT_ID="mock-proposal-${SUFFIX}.testnet"
📜 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)
🧠 Learnings (3)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/vote.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-proposal.sh (1)
Learnt from: linguists
PR: #1
File: README.md:10-11
Timestamp: 2025-05-14T03:12:48.262Z
Learning: For NEAR contracts, cargo near build (non-reproducible-wasm) is acceptable for local development, while cargo near build reproducible-wasm is recommended for production builds that will be deployed.
🧬 Code Graph Analysis (1)
tests/scripts/create-validators.sh (2)
tests/contracts/mock-staking-pool/src/lib.rs (1)
new(40-48)src/lib.rs (1)
new(58-73)
🪛 Shellcheck (0.10.0)
tests/scripts/vote.sh
[warning] 4-4: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-proposal.sh
[warning] 4-4: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 10-10: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 10-10: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: linear-protocol/validator-voting-contract#1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/vote.sh (2)
Learnt from: linguists
PR: #8
File: src/lib.rs:0-0
Timestamp: 2025-06-25T09:08:36.725Z
Learning: In the validator voting contract (src/lib.rs), the voting system intentionally handles "no" votes and zero-stake "yes" votes differently: validators who vote "no" are removed from the votes map and not re-inserted, while validators who voted "yes" but got kicked out (have 0 stake) are kept in the votes map with 0 stake to preserve their original voting intent. This asymmetric behavior in ping() vs internal_vote() is by design, not an inconsistency.
Learnt from: linguists
PR: #1
File: src/lib.rs:12-18
Timestamp: 2025-05-14T03:23:47.323Z
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.
tests/scripts/create-proposal.sh (1)
Learnt from: linguists
PR: #1
File: README.md:10-11
Timestamp: 2025-05-14T03:12:48.262Z
Learning: For NEAR contracts, cargo near build (non-reproducible-wasm) is acceptable for local development, while cargo near build reproducible-wasm is recommended for production builds that will be deployed.
🧬 Code Graph Analysis (1)
tests/scripts/create-validators.sh (2)
tests/contracts/mock-staking-pool/src/lib.rs (1)
new(40-48)src/lib.rs (1)
new(58-73)
🪛 Shellcheck (0.10.0)
tests/scripts/vote.sh
[warning] 4-4: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 7-7: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-proposal.sh
[warning] 4-4: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
tests/scripts/create-validators.sh
[warning] 6-6: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 10-10: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
[warning] 10-10: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Code Linter
🔇 Additional comments (1)
tests/scripts/create-proposal.sh (1)
9-9: Undefined variableARGS– script prints nothing
echo $ARGSreferences a variable that is never set (should beINIT_ARGS).-echo $ARGS +echo "$INIT_ARGS"Likely an incorrect or invalid review comment.
Update Voting Rules
ping()after deadline;Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Scripts