feat: updatable proposals#5
Draft
dan13ram wants to merge 38 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
5abbe4c to
a3bc597
Compare
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
a2b9aaa to
03e428f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
test/GovGasBenchmark.t.sol (1)
338-374: ⚡ Quick winExtract duplicated helper logic to shared test base (Lines 338-374).
_buildOrderedUpdateSignaturesand_mintTokensToUsersare duplicated across benchmark/fuzz suites. Moving them intoGov.t.sol(or a shared helper mixin) reduces drift risk when signature payloads change again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/GovGasBenchmark.t.sol` around lines 338 - 374, Move the duplicated helper functions _buildOrderedUpdateSignatures and _mintTokensToUsers out of GovGasBenchmark.t.sol into the shared test base (e.g., Gov.t.sol or a new helper mixin) so all benchmark/fuzz suites use the same implementations; ensure the moved _buildOrderedUpdateSignatures continues to reference UPDATE_PROPOSAL_TYPEHASH, governor.DOMAIN_SEPARATOR(), and _encodeSignature and that callers still pass proposer/nonce/deadline, and ensure _mintTokensToUsers continues to use token.mint(), auction, token.transferFrom(...) and otherUsers; after moving, remove the duplicates and update any imports/inheritance so tests compile and reference the single shared helper.test/GovFuzz.t.sol (1)
80-89: ⚡ Quick winImprove fuzz effectiveness by removing high-rejection assumption (Lines 80-89).
vm.assume(expiredOffset <= block.timestamp)drops a large portion of fuzz cases after bounding to365 days. Use a dynamic upper bound instead to keep the corpus dense.Proposed change
- // Bound to reasonable past range - expiredOffset = bound(expiredOffset, 1, 365 days); + // Bound to valid past range relative to current timestamp + uint256 maxExpiredOffset = block.timestamp < 365 days ? block.timestamp : 365 days; + expiredOffset = bound(expiredOffset, 1, maxExpiredOffset); ... - vm.assume(expiredOffset <= block.timestamp); uint256 deadline = block.timestamp - expiredOffset;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/GovFuzz.t.sol` around lines 80 - 89, The test currently uses vm.assume(expiredOffset <= block.timestamp) which rejects many fuzz cases; instead move the bounding of expiredOffset to after the vm.warp and replace bound(expiredOffset, 1, 365 days) with a dynamic upper bound using the current timestamp (e.g., bound(expiredOffset, 1, block.timestamp)), and remove the vm.assume line; adjust the order so you call deployMock(), mintVoter1(), createProposal(), vm.warp(... governor.proposalUpdatablePeriod() + governor.votingDelay()), then compute expiredOffset via bound(expiredOffset, 1, block.timestamp) and derive deadline = block.timestamp - expiredOffset.test/GovUpgrade.t.sol (2)
271-273: ⚡ Quick winConsider specifying the expected revert reason.
Using
vm.expectRevert()without a specific error could mask bugs if the upgrade fails for an unexpected reason. Specifying the exact error would improve test reliability and help identify regressions faster.♻️ Proposed enhancement
Looking at the Manager contract pattern in this codebase, the specific error should likely be related to the Manager's upgrade registration check. Consider adding the expected error message:
// Attempt upgrade without registration should fail vm.prank(address(treasury)); - vm.expectRevert(); + vm.expectRevert(); // TODO: Add specific error from Manager contract governor.upgradeTo(address(newGovernorImpl));Or if there's a specific error from the upgrade validation logic, use it:
- vm.expectRevert(); + vm.expectRevert(abi.encodeWithSignature("INVALID_UPGRADE(address,address)", address(governorImpl), address(newGovernorImpl)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/GovUpgrade.t.sol` around lines 271 - 273, Replace the generic vm.expectRevert() with a targeted expectRevert that asserts the exact revert reason thrown by the Manager/upgrade validation so the test fails only for the intended condition; locate the Manager contract's upgrade registration or validation revert string (the error emitted by the function that blocks unauthorized upgrades) and use vm.expectRevert(abi.encodePacked("<exact revert string>")) (or the appropriate bytes/error selector) immediately before vm.prank(address(treasury)); governor.upgradeTo(address(newGovernorImpl)) so the test checks for that specific revert from governor.upgradeTo.
15-17: 💤 Low valueConsider removing the redundant
setUpfunction.The override only calls
super.setUp()without any additional logic. When there's no child-specific setup, the function can be removed entirely and the parent'ssetUpwill be called automatically.♻️ Proposed cleanup
- function setUp() public override { - super.setUp(); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/GovUpgrade.t.sol` around lines 15 - 17, The setUp() override in GovUpgrade.t.sol is redundant because it only calls super.setUp(); remove the entire function declaration (the setUp() override that calls super.setUp()) so the parent setUp() will be used automatically; if you later need child-specific initialization, reintroduce an override that calls super.setUp() and adds the extra logic.src/governance/governor/IGovernor.sol (1)
314-320: ⚡ Quick winExpose the replacement successor in
IGovernor.
state()can now returnReplaced, but the interface does not expose the replacement id. The concrete contract already has a publicproposalIdReplacedBygetter via storage, so callers compiled againstIGovernorcannot follow the replacement chain from the interface alone.💡 Suggested addition
+ /// `@notice` The replacement proposal id for a replaced proposal + /// `@param` proposalId The original proposal id + function proposalIdReplacedBy(bytes32 proposalId) external view returns (bytes32);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/governance/governor/IGovernor.sol` around lines 314 - 320, The IGovernor interface must expose the replacement successor so callers can follow a Replaced state chain; add a public getter declaration matching the concrete contract's storage accessor (e.g., proposalIdReplacedBy) to IGovernor (signature like proposalIdReplacedBy(bytes32) external view returns (bytes32)) so callers compiled against IGovernor can retrieve the replacement proposal id when state() returns Replaced; update the interface near the other proposal getters (alongside getProposalSigners and proposalUpdatePeriodEnd).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/frontend-migration-guide.md`:
- Around line 331-338: The while-loop in getLatestProposalId incorrectly uses
the ethers v5 constant ethers.constants.HashZero; update the zero-hash check to
use the ethers v6 API by replacing ethers.constants.HashZero with
ethers.ZeroHash wherever getLatestProposalId calls governor.proposalIdReplacedBy
(and any similar replacement-chain checks) so the loop compares against the
correct zero hash constant.
In `@src/governance/governor/Governor.sol`:
- Around line 952-964: The update path currently allows arbitrarily many
signers; when handling an update in updateProposalBySigs (after receiving
newSigners from _validateUpdateSignaturesAndGetVotes but before calling
_replaceProposalWithSigners) enforce the same cap used by proposeBySigs by
checking newSigners.length <= MAX_PROPOSAL_SIGNERS and revert (e.g., with a new
or existing error) if exceeded; alternatively add the check inside
_validateUpdateSignaturesAndGetVotes so it never returns an oversized signer
array—this prevents unbounded gas/storage growth that would affect cancel() and
getProposalSigners().
- Around line 882-886: The loop in the helper copies old signer addresses into
proposalSigners[newProposalId] (using newSigners and _oldSigners), which
incorrectly carries prior approvals onto an unsigned replacement; remove the
copy so proposalSigners[newProposalId] remains empty (no push of _oldSigners
into newSigners) and ensure updateProposal()/updateProposalBySigs() flow
requires fresh signatures for the replacement (or explicitly document/implement
requiring updateProposalBySigs for persistent backing) so
getProposalSigners(newProposalId) and cancel() reflect only actual signers of
the new proposal.
In `@test/Gov.t.sol`:
- Around line 2728-2740: The test mints tokens to address(auction) so delegating
via token.delegate(address(wallet)) gives the ERC-1271 wallet zero votes; change
the minting so the wallet actually receives tokens before delegating (e.g. call
token.mint() while vm.prank(address(wallet)) or use the contract's
mintTo(wallet) helper if available) and make the same adjustment in the second
occurrence (the block around lines 2792-2800) so the wallet holds tokens prior
to token.delegate(address(wallet)) and the mixed-signer assertions exercise
contract voting weight.
- Around line 1489-1490: The tests never exercise non-zero proposalThreshold
because total supply after mintVoter1() is too small so
governor.updateProposalThresholdBps(1) still rounds proposalThreshold() to 0;
fix by increasing circulating supply before calling
governor.updateProposalThresholdBps (either mint additional tokens in
mintVoter1() or call an extra mint helper so totalSupply >= ceil(requiredBps *
denominator / 10000)), or use a much larger BPS value so that
proposalThreshold() > 0 for the current supply; update the test cases around
mintVoter1(), governor.updateProposalThresholdBps(...), and assertions on
proposalThreshold() to ensure the non-zero branch is actually reached.
---
Nitpick comments:
In `@src/governance/governor/IGovernor.sol`:
- Around line 314-320: The IGovernor interface must expose the replacement
successor so callers can follow a Replaced state chain; add a public getter
declaration matching the concrete contract's storage accessor (e.g.,
proposalIdReplacedBy) to IGovernor (signature like proposalIdReplacedBy(bytes32)
external view returns (bytes32)) so callers compiled against IGovernor can
retrieve the replacement proposal id when state() returns Replaced; update the
interface near the other proposal getters (alongside getProposalSigners and
proposalUpdatePeriodEnd).
In `@test/GovFuzz.t.sol`:
- Around line 80-89: The test currently uses vm.assume(expiredOffset <=
block.timestamp) which rejects many fuzz cases; instead move the bounding of
expiredOffset to after the vm.warp and replace bound(expiredOffset, 1, 365 days)
with a dynamic upper bound using the current timestamp (e.g.,
bound(expiredOffset, 1, block.timestamp)), and remove the vm.assume line; adjust
the order so you call deployMock(), mintVoter1(), createProposal(), vm.warp(...
governor.proposalUpdatablePeriod() + governor.votingDelay()), then compute
expiredOffset via bound(expiredOffset, 1, block.timestamp) and derive deadline =
block.timestamp - expiredOffset.
In `@test/GovGasBenchmark.t.sol`:
- Around line 338-374: Move the duplicated helper functions
_buildOrderedUpdateSignatures and _mintTokensToUsers out of
GovGasBenchmark.t.sol into the shared test base (e.g., Gov.t.sol or a new helper
mixin) so all benchmark/fuzz suites use the same implementations; ensure the
moved _buildOrderedUpdateSignatures continues to reference
UPDATE_PROPOSAL_TYPEHASH, governor.DOMAIN_SEPARATOR(), and _encodeSignature and
that callers still pass proposer/nonce/deadline, and ensure _mintTokensToUsers
continues to use token.mint(), auction, token.transferFrom(...) and otherUsers;
after moving, remove the duplicates and update any imports/inheritance so tests
compile and reference the single shared helper.
In `@test/GovUpgrade.t.sol`:
- Around line 271-273: Replace the generic vm.expectRevert() with a targeted
expectRevert that asserts the exact revert reason thrown by the Manager/upgrade
validation so the test fails only for the intended condition; locate the Manager
contract's upgrade registration or validation revert string (the error emitted
by the function that blocks unauthorized upgrades) and use
vm.expectRevert(abi.encodePacked("<exact revert string>")) (or the appropriate
bytes/error selector) immediately before vm.prank(address(treasury));
governor.upgradeTo(address(newGovernorImpl)) so the test checks for that
specific revert from governor.upgradeTo.
- Around line 15-17: The setUp() override in GovUpgrade.t.sol is redundant
because it only calls super.setUp(); remove the entire function declaration (the
setUp() override that calls super.setUp()) so the parent setUp() will be used
automatically; if you later need child-specific initialization, reintroduce an
override that calls super.setUp() and adds the extra logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdd6265e-2632-4e51-a195-961ac22e114d
📒 Files selected for processing (20)
docs/README.mddocs/deployment-workflows.mddocs/frontend-migration-guide.mddocs/governor-architecture.mddocs/governor-audit-readiness.mddocs/governor-proposal-lifecycle.mddocs/mainnet-v2-upgrade-runbook.mddocs/upgrade-runbook.mdpackage.jsonsrc/governance/governor/Governor.solsrc/governance/governor/IGovernor.solsrc/governance/governor/storage/GovernorStorageV3.solsrc/governance/governor/types/GovernorTypesV1.soltest/Gov.t.soltest/GovFuzz.t.soltest/GovGasBenchmark.t.soltest/GovUpgrade.t.soltest/MetadataRenderer.t.soltest/Token.t.soltest/utils/mocks/MockERC1271Wallet.sol
💤 Files with no reviewable changes (1)
- docs/mainnet-v2-upgrade-runbook.md
Comment on lines
+331
to
+338
| async function getLatestProposalId(proposalId) { | ||
| let currentId = proposalId; | ||
| let replacedBy = await governor.proposalIdReplacedBy(currentId); | ||
|
|
||
| // Follow replacement chain to get latest version | ||
| while (replacedBy !== ethers.constants.HashZero) { | ||
| currentId = replacedBy; | ||
| replacedBy = await governor.proposalIdReplacedBy(currentId); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ethers.js v6, what is the equivalent of ethers.constants.HashZero from ethers.js v5?
💡 Result:
In ethers.js v6, the equivalent of ethers.constants.HashZero from v5 is ethers.ZeroHash [1][2]. In v5, constants were grouped under the ethers.constants object [3]. In v6, these constants have been moved to the top-level ethers object [1][4]. Similarly, ethers.constants.AddressZero has been replaced by ethers.ZeroAddress [1][2].
Citations:
- 1: https://docs.ethers.org/v6/migrating/
- 2: https://github.com/ethers-io/ethers.js/blob/main/docs.wrm/migrating.wrm
- 3: https://registry.npmjs.org/%40ethersproject%2Fconstants
- 4: https://docs.ethers.org/v6/api/constants/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
nl -ba "$FILE" | sed -n '320,350p'Repository: BuilderOSS/nouns-protocol
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
cat -n "$FILE" | sed -n '320,350p'Repository: BuilderOSS/nouns-protocol
Length of output: 1010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
echo "== HashZero occurrences =="
rg -n "ethers\.constants\.HashZero" "$FILE" || true
echo "== ZeroHash occurrences =="
rg -n "ethers\.ZeroHash" "$FILE" || true
echo "== Context around HashZero =="
rg -n "ethers\.constants\.HashZero" "$FILE" -C 5 || trueRepository: BuilderOSS/nouns-protocol
Length of output: 624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
rg -n "ethers\.?js v6|ethers v6|\bv6\b|ZeroAddress|constants\.HashZero|HashZero|ethers\.ZeroHash" "$FILE" || true
# Also grab the section header(s) around line 331
sed -n '280,380p' "$FILE" | nl -ba -w2 -s': ' | head -n 120Repository: BuilderOSS/nouns-protocol
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
echo "== Around ethers.js v6 section (90-180) =="
sed -n '90,180p' "$FILE"
echo
echo "== Around snippet (300-370) =="
sed -n '300,370p' "$FILE"Repository: BuilderOSS/nouns-protocol
Length of output: 3969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="docs/frontend-migration-guide.md"
echo "== ethers.js section headings =="
rg -n "^#### Using ethers\.js v" "$FILE" || true
echo
echo "== ethers.constants.* occurrences =="
rg -n "ethers\.constants\." "$FILE" || trueRepository: BuilderOSS/nouns-protocol
Length of output: 225
Update replacement-chain zero-hash check to ethers v6 (ethers.ZeroHash).
docs/frontend-migration-guide.md (line 336) uses ethers.constants.HashZero (ethers v5 API); in ethers v6 this should be ethers.ZeroHash to prevent copy/paste failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/frontend-migration-guide.md` around lines 331 - 338, The while-loop in
getLatestProposalId incorrectly uses the ethers v5 constant
ethers.constants.HashZero; update the zero-hash check to use the ethers v6 API
by replacing ethers.constants.HashZero with ethers.ZeroHash wherever
getLatestProposalId calls governor.proposalIdReplacedBy (and any similar
replacement-chain checks) so the loop compares against the correct zero hash
constant.
Comment on lines
+882
to
+886
| address[] storage newSigners = proposalSigners[newProposalId]; | ||
| uint256 oldSignersLen = _oldSigners.length; | ||
| for (uint256 i; i < oldSignersLen; ++i) { | ||
| newSigners.push(_oldSigners[i]); | ||
| } |
There was a problem hiding this comment.
Do not carry signer approvals onto an unsigned replacement.
When updateProposal() reaches this helper, the loop on Lines 882-886 copies the old signer list onto newProposalId even though those addresses never signed the replacement payload. That makes getProposalSigners(newProposalId) inaccurate and gives stale signers cancel authority through cancel().
💡 Suggested fix
- address[] storage newSigners = proposalSigners[newProposalId];
- uint256 oldSignersLen = _oldSigners.length;
- for (uint256 i; i < oldSignersLen; ++i) {
- newSigners.push(_oldSigners[i]);
- }If signer backing should persist on the replacement, require fresh approvals via updateProposalBySigs() instead of reusing the old signer list.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| address[] storage newSigners = proposalSigners[newProposalId]; | |
| uint256 oldSignersLen = _oldSigners.length; | |
| for (uint256 i; i < oldSignersLen; ++i) { | |
| newSigners.push(_oldSigners[i]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/governance/governor/Governor.sol` around lines 882 - 886, The loop in the
helper copies old signer addresses into proposalSigners[newProposalId] (using
newSigners and _oldSigners), which incorrectly carries prior approvals onto an
unsigned replacement; remove the copy so proposalSigners[newProposalId] remains
empty (no push of _oldSigners into newSigners) and ensure
updateProposal()/updateProposalBySigs() flow requires fresh signatures for the
replacement (or explicitly document/implement requiring updateProposalBySigs for
persistent backing) so getProposalSigners(newProposalId) and cancel() reflect
only actual signers of the new proposal.
Comment on lines
+952
to
+964
| // If original proposal had signers, update must also have signers | ||
| if (proposalSigners[_proposalId].length > 0 && _proposerSignatures.length == 0) revert MUST_PROVIDE_SIGNATURES(); | ||
|
|
||
| bytes32 descriptionHash = keccak256(bytes(_description)); | ||
| bytes32 updatedProposalId = hashProposal(_targets, _values, _calldatas, descriptionHash, _proposer); | ||
|
|
||
| // Validate new signatures and collect votes (signers can be different from original) | ||
| (uint256 totalVotes, address[] memory newSigners) = | ||
| _validateUpdateSignaturesAndGetVotes(_proposalId, updatedProposalId, _proposer, _proposerSignatures); | ||
|
|
||
| if (totalVotes <= proposalThreshold()) revert VOTES_BELOW_PROPOSAL_THRESHOLD(); | ||
|
|
||
| bytes32 newProposalId = _replaceProposalWithSigners(_proposalId, oldProposal, _targets, _values, _calldatas, descriptionHash, newSigners); |
There was a problem hiding this comment.
Re-enforce MAX_PROPOSAL_SIGNERS on the update path.
proposeBySigs() caps signer count, but updateProposalBySigs() never does. A proposal can therefore start with a small signer set and then be updated with an arbitrarily large array, which makes cancel() and getProposalSigners() unbounded in gas/storage.
💡 Suggested fix
// If original proposal had signers, update must also have signers
if (proposalSigners[_proposalId].length > 0 && _proposerSignatures.length == 0) revert MUST_PROVIDE_SIGNATURES();
+ if (_proposerSignatures.length > MAX_PROPOSAL_SIGNERS) revert TOO_MANY_SIGNERS();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If original proposal had signers, update must also have signers | |
| if (proposalSigners[_proposalId].length > 0 && _proposerSignatures.length == 0) revert MUST_PROVIDE_SIGNATURES(); | |
| bytes32 descriptionHash = keccak256(bytes(_description)); | |
| bytes32 updatedProposalId = hashProposal(_targets, _values, _calldatas, descriptionHash, _proposer); | |
| // Validate new signatures and collect votes (signers can be different from original) | |
| (uint256 totalVotes, address[] memory newSigners) = | |
| _validateUpdateSignaturesAndGetVotes(_proposalId, updatedProposalId, _proposer, _proposerSignatures); | |
| if (totalVotes <= proposalThreshold()) revert VOTES_BELOW_PROPOSAL_THRESHOLD(); | |
| bytes32 newProposalId = _replaceProposalWithSigners(_proposalId, oldProposal, _targets, _values, _calldatas, descriptionHash, newSigners); | |
| // If original proposal had signers, update must also have signers | |
| if (proposalSigners[_proposalId].length > 0 && _proposerSignatures.length == 0) revert MUST_PROVIDE_SIGNATURES(); | |
| if (_proposerSignatures.length > MAX_PROPOSAL_SIGNERS) revert TOO_MANY_SIGNERS(); | |
| bytes32 descriptionHash = keccak256(bytes(_description)); | |
| bytes32 updatedProposalId = hashProposal(_targets, _values, _calldatas, descriptionHash, _proposer); | |
| // Validate new signatures and collect votes (signers can be different from original) | |
| (uint256 totalVotes, address[] memory newSigners) = | |
| _validateUpdateSignaturesAndGetVotes(_proposalId, updatedProposalId, _proposer, _proposerSignatures); | |
| if (totalVotes <= proposalThreshold()) revert VOTES_BELOW_PROPOSAL_THRESHOLD(); | |
| bytes32 newProposalId = _replaceProposalWithSigners(_proposalId, oldProposal, _targets, _values, _calldatas, descriptionHash, newSigners); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/governance/governor/Governor.sol` around lines 952 - 964, The update path
currently allows arbitrarily many signers; when handling an update in
updateProposalBySigs (after receiving newSigners from
_validateUpdateSignaturesAndGetVotes but before calling
_replaceProposalWithSigners) enforce the same cap used by proposeBySigs by
checking newSigners.length <= MAX_PROPOSAL_SIGNERS and revert (e.g., with a new
or existing error) if exceeded; alternatively add the check inside
_validateUpdateSignaturesAndGetVotes so it never returns an oversized signer
array—this prevents unbounded gas/storage growth that would affect cancel() and
getProposalSigners().
Comment on lines
+1489
to
+1490
| vm.prank(address(treasury)); | ||
| governor.updateProposalThresholdBps(1); |
There was a problem hiding this comment.
These threshold tests never leave the zero-threshold path.
After mintVoter1() the supply here is only 1, so updateProposalThresholdBps(1) and updateProposalThresholdBps(500) both make proposalThreshold() round down to 0. That means the cancel-boundary and creation-threshold assertions pass without exercising the non-zero threshold logic they claim to cover.
🛠️ Example adjustment
- governor.updateProposalThresholdBps(1);
+ governor.updateProposalThresholdBps(10_000);
...
- governor.updateProposalThresholdBps(500); // 5% threshold
+ governor.updateProposalThresholdBps(10_000); // 1 token supply => threshold of 1Also applies to: 2418-2421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Gov.t.sol` around lines 1489 - 1490, The tests never exercise non-zero
proposalThreshold because total supply after mintVoter1() is too small so
governor.updateProposalThresholdBps(1) still rounds proposalThreshold() to 0;
fix by increasing circulating supply before calling
governor.updateProposalThresholdBps (either mint additional tokens in
mintVoter1() or call an extra mint helper so totalSupply >= ceil(requiredBps *
denominator / 10000)), or use a much larger BPS value so that
proposalThreshold() > 0 for the current supply; update the test cases around
mintVoter1(), governor.updateProposalThresholdBps(...), and assertions on
proposalThreshold() to ensure the non-zero branch is actually reached.
Comment on lines
+2728
to
+2740
| // Mint to both wallet and voter1 | ||
| vm.prank(address(auction)); | ||
| token.mint(); // to wallet | ||
|
|
||
| mintVoter1(); // to voter1 EOA | ||
|
|
||
| vm.prank(address(wallet)); | ||
| token.delegate(address(wallet)); | ||
|
|
||
| vm.warp(block.timestamp + 1); | ||
|
|
||
| vm.prank(address(treasury)); | ||
| governor.updateProposalThresholdBps(1); |
There was a problem hiding this comment.
The ERC-1271 wallet never gets voting power in this mixed-signer test.
token.mint() under address(auction) mints to the auction, not to wallet, so the subsequent token.delegate(address(wallet)) delegates zero votes. With proposalThresholdBps(1) also rounding to 0, this test can still pass even if contract signers contribute no weight at all.
🛠️ Example adjustment
- vm.prank(address(auction));
- token.mint(); // to wallet
+ vm.startPrank(address(auction));
+ uint256 walletTokenId = token.mint();
+ token.transferFrom(address(auction), address(wallet), walletTokenId);
+ vm.stopPrank();
...
- governor.updateProposalThresholdBps(1);
+ governor.updateProposalThresholdBps(10_000);Also applies to: 2792-2800
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Gov.t.sol` around lines 2728 - 2740, The test mints tokens to
address(auction) so delegating via token.delegate(address(wallet)) gives the
ERC-1271 wallet zero votes; change the minting so the wallet actually receives
tokens before delegating (e.g. call token.mint() while vm.prank(address(wallet))
or use the contract's mintTo(wallet) helper if available) and make the same
adjustment in the second occurrence (the block around lines 2792-2800) so the
wallet holds tokens prior to token.delegate(address(wallet)) and the
mixed-signer assertions exercise contract voting weight.
64e8260 to
9127436
Compare
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
8027c1f to
3983757
Compare
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
bacd89e to
ba72aa6
Compare
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
6cc7aa8 to
f18bedf
Compare
dan13ram
force-pushed
the
feat/updatable-proposals
branch
from
722e730 to
410ce09
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation