Refactor: remove unused event, legacy getter, optimize vote query #106
Conversation
…d add rationale comments
WalkthroughThis change removes a legacy voting event and a fee percentage accessor, updates fee event payloads to emit basis points, adds an early-return guard in a vote lookup function, and modifies voter count update logic during vote casting. No public function signatures were added or changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant VV as DEVoterVoting
rect rgb(240,248,255)
note over U,VV: getUserVoteForRepository (early guard)
U->>VV: getUserVoteForRepository(user, repositoryId)
alt No entry for user/repository
VV-->>U: (0, 0)
else Entry exists
VV->>VV: read amount, timestamp
VV-->>U: (amount, timestamp)
end
end
sequenceDiagram
autonumber
actor U as User
participant VV as DEVoterVoting
rect rgb(245,255,240)
note over U,VV: castVote (voterCount update condition changed)
U->>VV: castVote(repositoryId, amount)
VV->>VV: validate + set hasUserVoted[msg.sender][repositoryId] = true
alt First-time voter check
note right of VV: Increment voterCount only if !hasUserVoted before set
VV->>VV: if (!hasUserVoted[msg.sender][repositoryId]) increment voterCount
end
VV->>VV: update amounts, totals, emit VoteCasted
VV-->>U: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/DEVoterEscrow.sol (1)
679-683: Wrong event for per‑user release timestamp updatesEmitting VotingPeriodUpdated here overloads semantics and omits the affected user. Emit a dedicated event including the user.
+ event ReleaseTimestampUpdated(address indexed user, uint256 oldReleaseTimestamp, uint256 newReleaseTimestamp, address indexed updatedBy); @@ - emit VotingPeriodUpdated(oldTimestamp, newReleaseTimestamp, msg.sender); + emit ReleaseTimestampUpdated(user, oldTimestamp, newReleaseTimestamp, msg.sender);contracts/DEVoterVoting.sol (1)
441-452: voterCount never increments due to order of operationsYou set hasUserVoted to true, then check if (!hasUserVoted), which is always false. voterCount won’t increase for first‑time voters.
- hasUserVoted[msg.sender][repositoryId] = true; - userVotesByRepository[repositoryId][msg.sender] = amount; - - // Initialize remaining votes for withdrawal tracking - remainingVotes[msg.sender][repositoryId] = amount; - - // Update repository totals - if (!hasUserVoted[msg.sender][repositoryId]) { - repositoryVotes[repositoryId].voterCount++; - } + // Capture pre-existing state before setting the flag + bool firstVote = !hasUserVoted[msg.sender][repositoryId]; + hasUserVoted[msg.sender][repositoryId] = true; + userVotesByRepository[repositoryId][msg.sender] = amount; + + // Initialize remaining votes for withdrawal tracking + remainingVotes[msg.sender][repositoryId] = amount; + + // Update repository totals + if (firstVote) { + repositoryVotes[repositoryId].voterCount++; + } repositoryVotes[repositoryId].totalVotes += amount;
🧹 Nitpick comments (6)
contracts/DEVoterEscrow.sol (4)
76-77: Clarify scope of event commentThe comment implies the whole system only uses VoteCasted, but DEVoterVoting still emits VoteCast. Make it explicit this applies to this contract only.
- // Removed unused VoteCast event. Only VoteCasted is used for voting actions. - // Rationale: Only one event is needed for vote actions to avoid confusion and redundancy. + // Removed unused VoteCast event in this contract. This contract uses only VoteCasted for voting actions. + // Rationale: Within this contract, a single vote event avoids confusion and redundancy.
370-372: Redundant non‑negativity check on uint256newFeeBasisPoints is uint256; the “cannot be negative” require is dead code.
- require(newFeeBasisPoints >= MIN_FEE_BASIS_POINTS, "Fee cannot be negative");
154-161: Unreachable fee > amount guard (with 5% cap)Given MAX_FEE_BASIS_POINTS=500 (5%), feeAmount > amount cannot occur. Keep if you want belt‑and‑suspenders, else trim for bytecode size.
// Ensure fee doesn't exceed the amount - if (feeAmount > amount) { - feeAmount = amount; - } + // Not needed with 5% cap; retained if future caps change
633-639: simulateVoteCast should report remaining power, not total escrowThis currently overstates voting power once the user has voted on other repos.
return ( escrow.isActive && isVotingPeriodActive(user), - escrow.amount, + escrow.amount - escrow.votesCast, getRemainingVotingTime(user) );contracts/DEVoterVoting.sol (2)
191-200: Validate against remaining voting power to avoid later revertUse Escrow’s remaining balance instead of total escrow to pre‑catch “used votes” cases and save gas on invalid casts.
- // Check if user has sufficient escrow balance - // Get user's escrow amount from the public mapping - (bool isActive, uint256 escrowAmount,,,,) = escrowContract.escrows(user); - if (!isActive) { - return (false, "No active escrow"); - } - - if (amount > escrowAmount) { - return (false, "Insufficient escrow balance"); - } + // Check remaining voting power from escrow + uint256 remainingPower = escrowContract.getRemainingVoteBalance(user); + if (remainingPower == 0) { + return (false, "No remaining voting power"); + } + if (amount > remainingPower) { + return (false, "Insufficient remaining voting power"); + }
373-389: Optional: O(1) timestamp lookupTo avoid scanning userVotes, consider tracking vote timestamps per repo:
- Add mapping(address => mapping(uint256 => uint256)) voteTimestampByRepository;
- Set it in castVote; read it here to return (amount, timestamp) without the loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/DEVoterEscrow.sol(4 hunks)contracts/DEVoterVoting.sol(2 hunks)
🔇 Additional comments (1)
contracts/DEVoterVoting.sol (1)
373-389: Early return guard is a good fast‑pathThe short‑circuit on hasUserVoted avoids unnecessary work. LGTM.
| newFeeBasisPoints, // Now emits raw basis points for clarity instead of percentage | ||
| feeWallet, | ||
| msg.sender | ||
| ); |
There was a problem hiding this comment.
Event parameter now represents basis points; rename for correctness
FeeParametersUpdated’s first param is named newFeePercentage but you now emit raw basis points. Rename the parameter to avoid operational confusion for indexers and dashboards. Signature stays identical (uint256,address,address), so ABI compatibility is preserved.
- event FeeParametersUpdated(
- uint256 newFeePercentage,
- address indexed newFeeCollector,
- address indexed updatedBy
- );
+ event FeeParametersUpdated(
+ uint256 newFeeBasisPoints,
+ address indexed newFeeCollector,
+ address indexed updatedBy
+ );No change needed at emit sites beyond optionally removing the inline percentage-to-bps comments.
Also applies to: 396-399
🤖 Prompt for AI Agents
In contracts/DEVoterEscrow.sol around lines 378-381 and 396-399, the
FeeParametersUpdated event currently names its first parameter newFeePercentage
but the emitted value is raw basis points; rename the event parameter to
newFeeBasisPoints to reflect the actual units while keeping the signature
(uint256,address,address) and parameter order unchanged to preserve ABI
compatibility, and update any inline comments or NatSpec referring to percentage
to indicate basis points instead.
| // Removed legacy getFeePercentage getter for clarity. Use getFeeInfo for raw basis points. | ||
| // Rationale: getFeeInfo provides all fee details in basis points, which is the standard for smart contracts. |
There was a problem hiding this comment.
🧩 Analysis chain
Getter removal looks good; check for external/internal usages
Removing getFeePercentage is fine; getFeeInfo is the right replacement. Please verify nothing still calls the old getter.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any lingering references to the removed getter
rg -nP '\bgetFeePercentage\s*\(' -S || echo "OK: no references found"Length of output: 283
Fix remaining getFeePercentage usages in tests and docs
- test/DEVoterEscrow.ts:501 — replace
await dEVoterEscrow.read.getFeePercentage()withgetFeeInfo()and derive the percentage from basis points (use big-number-safe math; percent = feeBps / 100). Re-run tests. - docs/FeeCalculationSystem.md:130,225 — remove/replace
getFeePercentage()references and update examples to usegetFeeInfo()(describe BPS → percentage conversion).
🤖 Prompt for AI Agents
In contracts/DEVoterEscrow.sol around lines 690-691 the legacy getFeePercentage
getter was removed; update usages accordingly: in test/DEVoterEscrow.ts at ~line
501 replace await dEVoterEscrow.read.getFeePercentage() with await
dEVoterEscrow.read.getFeeInfo(), extract feeBps from the returned struct and
compute percentage using big-number-safe math (percent = feeBps.div(100) or
equivalent bn arithmetic) and update any assertions to compare the derived
percentage; in docs/FeeCalculationSystem.md at lines ~130 and ~225 remove or
replace getFeePercentage() examples with getFeeInfo() and add a short note
showing BPS → percentage conversion (percentage = feeBps / 100, or show bn-safe
division), then re-run the test suite to confirm all tests pass.
Here’s a summary of the tasks completed
DEVoterEscrow.sol
VoteCastevent, clarifying that onlyVoteCastedis used for voting actions.FeeParametersUpdatedevent to emit raw basis points instead of percentage for clarity.getFeePercentagegetter, with comments explaining thatgetFeeInfoshould be used for fee details.DEVoterVoting.sol
voterCountincastVoteto accurately track unique voters.getUserVoteForRepositoryfunction to return early when a match is found and avoid unnecessary looping if the user hasn’t voted, with comments explaining the rationale.All changes were documented with comments for clarity and maintainability.
Summary by CodeRabbit