chore: scale down by half decimals#1105
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughReorganized imports in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/server/src/server/routes/voting.rs(1 hunks)examples/CRISP/server/src/server/token_holders/etherscan.rs(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
📚 Learning: 2024-10-16T09:51:10.038Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/committee_meta.rs:43-43
Timestamp: 2024-10-16T09:51:10.038Z
Learning: In `packages/ciphernode/router/src/committee_meta.rs`, avoid suggesting making the `on_event` method in `CommitteMetaFeature` asynchronous or adding error handling for the `write` operation to the data store.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
📚 Learning: 2024-10-22T03:41:21.226Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/repositories.rs:40-71
Timestamp: 2024-10-22T03:41:21.226Z
Learning: In `packages/ciphernode/router/src/repositories.rs`, prefer to keep method implementations as they are if they are straightforward and maintainable, even if refactoring could reduce duplication.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
📚 Learning: 2024-10-22T03:42:14.057Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/context.rs:94-97
Timestamp: 2024-10-22T03:42:14.057Z
Learning: In `packages/ciphernode/router/src/context.rs`, avoid adding complexity for batching checkpoint operations in code; rely on the database's batching capabilities instead.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
🧬 Code graph analysis (1)
examples/CRISP/server/src/server/token_holders/etherscan.rs (2)
crates/config/src/chain_config.rs (1)
rpc_url(27-30)crates/support/host/src/lib.rs (1)
url(137-137)
⏰ 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). (9)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: crisp_unit
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (4)
examples/CRISP/server/src/server/routes/voting.rs (1)
7-17: LGTM! Clean import reorganization.The import statements have been reorganized and grouped for better readability with no functional changes.
examples/CRISP/server/src/server/token_holders/etherscan.rs (3)
23-23: LGTM! Standard ERC20 function addition.Adding
decimals()to the ERC20Votes interface is correct and necessary for the vote scaling implementation.
464-477: Implementation looks correct.The
get_decimalshelper method correctly follows the same pattern asget_past_voteswith appropriate error handling. However, as noted in the previous comment, this should be called once before the loop inverify_voting_powerrather than for each voter.
874-891: Test is still marked#[ignore]and has not been enabled.The code snippet in this review comment shows the test without the
#[ignore]attribute, but line 874 in the actual file still contains#[ignore]. If this test is intended to be enabled as part of the PR, remove the#[ignore]attribute first. The decimal-scaling logic (dividing by10^(decimals/2)) is present in the code and valid, but the verification request is premature until the test is actually uncommented.Likely an incorrect or invalid review comment.
ctrlc03
force-pushed
the
chore/scale-down
branch
from
c97b484 to
48f2bb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/CRISP/server/src/server/token_holders/etherscan.rs (1)
337-356: Voters passing threshold may be included with scaled balance of "0".When
threshold < scale_factor, voters who pass the threshold check at line 340 can have theirscaled_votesrounded down to 0 at line 341 due to integer division. For example:
- Token with 18 decimals →
scale_factor = 10^9- If
threshold = 10^8raw units- A voter with
votes = 10^8passes the threshold- But
scaled_votes = 10^8 / 10^9 = 0- This voter is stored with
balance: "0"This could cause issues if downstream consumers expect all included voters to have non-zero balances, or if UI/voting logic relies on balance values.
Consider one of these approaches:
Option 1: Ensure minimum scaled balance of 1 for voters above threshold:
if votes >= threshold { let scaled_votes = votes / scale_factor; + // Ensure voters above threshold have at least balance of 1 + let final_votes = if scaled_votes.is_zero() { U256::from(1) } else { scaled_votes }; token_holders.push(TokenHolder { address: voter.address.to_string(), - balance: scaled_votes.to_string(), + balance: final_votes.to_string(), }); }Option 2: Apply threshold comparison after scaling (if threshold represents scaled values):
- if votes >= threshold { let scaled_votes = votes / scale_factor; + if scaled_votes >= threshold { token_holders.push(TokenHolder { address: voter.address.to_string(), balance: scaled_votes.to_string(), }); + } - }Choose based on whether
thresholdis meant to represent raw voting power (current behavior, Option 1) or scaled voting power (Option 2).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/server/src/server/token_holders/etherscan.rs(5 hunks)
⏰ 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). (10)
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_dev
- GitHub Check: rust_unit
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: crisp_unit
- GitHub Check: test_net
- GitHub Check: Build & Push Image
🔇 Additional comments (2)
examples/CRISP/server/src/server/token_holders/etherscan.rs (2)
23-23: LGTM! Standard ERC20 decimals interface added correctly.The addition of the
decimals()method to theERC20Votesinterface follows the ERC20 standard and is necessary for the scaling logic.
465-478: LGTM! Helper function follows established patterns.The
get_decimalsimplementation correctly mirrors the structure ofget_past_votesand includes appropriate error handling with context.
2 participants
scale down voting power to fit into smaller values
Summary by CodeRabbit
New Features
Bug Fixes
Stability
✏️ Tip: You can customize this high-level summary in your review settings.