Skip to content

Refactor: remove unused event, legacy getter, optimize vote query #106

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 20, 2025
Merged

Refactor: remove unused event, legacy getter, optimize vote query #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions contracts/DEVoterEscrow.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,8 @@ contract DEVoterEscrow is ReentrancyGuard, Ownable, Pausable, AccessControl {
uint256 releaseTime,
bool wasForced
);
event VoteCast(
address indexed user,
uint256 indexed repositoryId,
uint256 amount,
uint256 timestamp
);
// 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.
event FeeParametersUpdated(
uint256 newFeePercentage,
address indexed newFeeCollector,
Expand Down Expand Up @@ -379,7 +375,7 @@ contract DEVoterEscrow is ReentrancyGuard, Ownable, Pausable, AccessControl {

emit FeeBasisPointsUpdated(oldFeeBasisPoints, newFeeBasisPoints);
emit FeeParametersUpdated(
(newFeeBasisPoints * 100) / BASIS_POINTS_DENOMINATOR,
newFeeBasisPoints, // Now emits raw basis points for clarity instead of percentage
feeWallet,
msg.sender
);
Comment on lines +378 to 381
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Expand All @@ -397,7 +393,7 @@ contract DEVoterEscrow is ReentrancyGuard, Ownable, Pausable, AccessControl {

emit FeeWalletUpdated(oldFeeWallet, newFeeWallet);
emit FeeParametersUpdated(
(feeBasisPoints * 100) / BASIS_POINTS_DENOMINATOR,
feeBasisPoints, // Now emits raw basis points for clarity instead of percentage
newFeeWallet,
msg.sender
);
Expand Down Expand Up @@ -691,10 +687,8 @@ contract DEVoterEscrow is ReentrancyGuard, Ownable, Pausable, AccessControl {
return feeWallet;
}

function getFeePercentage() external view returns (uint256) {
// Convert basis points to percentage for backward compatibility
return (feeBasisPoints * 100) / BASIS_POINTS_DENOMINATOR;
}
// 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.
Comment on lines +690 to +691
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 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() with getFeeInfo() 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 use getFeeInfo() (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.


function getTokenAddress() external view returns (address) {
return address(token);
Expand Down
18 changes: 8 additions & 10 deletions contracts/DEVoterVoting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,22 +370,22 @@ contract DEVoterVoting is Ownable, ReentrancyGuard {
view
returns (uint256 amount, uint256 timestamp)
{
// Optimization: Return early if user hasn't voted for this repository.
if (!hasUserVoted[user][repositoryId]) {
return (0, 0);
}

amount = userVotesByRepository[repositoryId][user];
// Find timestamp from user votes array

// Optimization: Return timestamp immediately when found, avoiding unnecessary looping.
Vote[] storage votes = userVotes[user];
for (uint256 i = 0; i < votes.length; i++) {
if (votes[i].repositoryId == repositoryId) {
timestamp = votes[i].timestamp;
break;
return (amount, votes[i].timestamp);
}
}

return (amount, timestamp);
// Defensive: Should not reach here, but fallback to (amount, 0) if not found.
return (amount, 0);
}

/**
Expand Down Expand Up @@ -445,9 +445,7 @@ contract DEVoterVoting is Ownable, ReentrancyGuard {
remainingVotes[msg.sender][repositoryId] = amount;

// Update repository totals
if (repositoryVotes[repositoryId].totalVotes == 0) {
repositoryVotes[repositoryId].voterCount = 1;
} else {
if (!hasUserVoted[msg.sender][repositoryId]) {
repositoryVotes[repositoryId].voterCount++;
}
repositoryVotes[repositoryId].totalVotes += amount;
Expand Down