From a310dd929fc136669fd84baa5024d437ac1c9603 Mon Sep 17 00:00:00 2001 From: LHerskind <16536249+LHerskind@users.noreply.github.com> Date: Mon, 22 Sep 2025 10:39:38 +0000 Subject: [PATCH] chore: fix misc issues for clarity --- l1-contracts/src/governance/GSE.sol | 21 ------------------- l1-contracts/src/governance/Governance.sol | 15 +++++-------- .../src/governance/interfaces/IGovernance.sol | 10 ++++----- .../libraries/AddressSnapshotLib.sol | 3 --- .../governance/libraries/ConfigurationLib.sol | 4 +--- .../src/governance/libraries/Errors.sol | 12 ----------- .../src/governance/libraries/ProposalLib.sol | 1 - .../compressed-data/Configuration.sol | 9 ++++---- .../src/shared/libraries/BN254Lib.sol | 4 ++-- .../governance-proposer/signal.t.sol | 7 ------- .../governance-proposer/voteWithsig.t.sol | 7 ------- .../governance/governance/dropProposal.t.sol | 8 +++---- .../test/governance/governance/execute.t.sol | 4 ++-- .../proposallib/voteTabulation.tree | 2 -- .../governance/scenarios/noVoteAndExit.t.sol | 2 +- l1-contracts/test/harnesses/TestConstants.sol | 4 ++-- 16 files changed, 27 insertions(+), 86 deletions(-) diff --git a/l1-contracts/src/governance/GSE.sol b/l1-contracts/src/governance/GSE.sol index c67141f82197..818f555d0430 100644 --- a/l1-contracts/src/governance/GSE.sol +++ b/l1-contracts/src/governance/GSE.sol @@ -835,25 +835,4 @@ contract GSE is IGSE, GSECore { return attesters; } - - function _getInstanceStoreWithAttester(address _instance, address _attester) - internal - view - returns (InstanceAttesterRegistry storage, bool, address) - { - InstanceAttesterRegistry storage store = instances[_instance]; - bool attesterExists = store.attesters.contains(_attester); - address instanceAddress = _instance; - - if ( - !attesterExists && getLatestRollup() == _instance - && instances[BONUS_INSTANCE_ADDRESS].attesters.contains(_attester) - ) { - store = instances[BONUS_INSTANCE_ADDRESS]; - attesterExists = true; - instanceAddress = BONUS_INSTANCE_ADDRESS; - } - - return (store, attesterExists, instanceAddress); - } } diff --git a/l1-contracts/src/governance/Governance.sol b/l1-contracts/src/governance/Governance.sol index 17a3aa9e52db..7ac9696269f3 100644 --- a/l1-contracts/src/governance/Governance.sol +++ b/l1-contracts/src/governance/Governance.sol @@ -7,7 +7,7 @@ import { Proposal, ProposalState, Configuration, - ProposeConfiguration, + ProposeWithLockConfiguration, Withdrawal } from "@aztec/governance/interfaces/IGovernance.sol"; import {IPayload} from "@aztec/governance/interfaces/IPayload.sol"; @@ -417,7 +417,7 @@ contract Governance is IGovernance { * @return The id of the proposal */ function proposeWithLock(IPayload _proposal, address _to) external override(IGovernance) returns (uint256) { - ProposeConfiguration memory proposeConfig = configuration.getProposeConfig(); + ProposeWithLockConfiguration memory proposeConfig = configuration.getProposeConfig(); _initiateWithdraw(msg.sender, _to, proposeConfig.lockAmount, proposeConfig.lockDelay); return _propose(_proposal, address(this)); } @@ -439,7 +439,7 @@ contract Governance is IGovernance { * @param _amount The amount of power to vote with, which must be less than the available power. * @param _support The support of the vote. */ - function vote(uint256 _proposalId, uint256 _amount, bool _support) external override(IGovernance) returns (bool) { + function vote(uint256 _proposalId, uint256 _amount, bool _support) external override(IGovernance) { ProposalState state = getProposalState(_proposalId); require(state == ProposalState.Active, Errors.Governance__ProposalNotActive()); @@ -463,8 +463,6 @@ contract Governance is IGovernance { } emit VoteCast(_proposalId, msg.sender, _support, _amount); - - return true; } /** @@ -479,7 +477,7 @@ contract Governance is IGovernance { * * @param _proposalId The id of the proposal to execute. */ - function execute(uint256 _proposalId) external override(IGovernance) returns (bool) { + function execute(uint256 _proposalId) external override(IGovernance) { ProposalState state = getProposalState(_proposalId); require(state == ProposalState.Executable, Errors.Governance__ProposalNotExecutable()); @@ -497,8 +495,6 @@ contract Governance is IGovernance { } emit ProposalExecuted(_proposalId); - - return true; } /** @@ -508,7 +504,7 @@ contract Governance is IGovernance { * * @param _proposalId The id of the proposal to mark as `Dropped`. */ - function dropProposal(uint256 _proposalId) external override(IGovernance) returns (bool) { + function dropProposal(uint256 _proposalId) external override(IGovernance) { CompressedProposal storage self = proposals[_proposalId]; require(self.cachedState != ProposalState.Dropped, Errors.Governance__ProposalAlreadyDropped()); require(getProposalState(_proposalId) == ProposalState.Droppable, Errors.Governance__ProposalCannotBeDropped()); @@ -516,7 +512,6 @@ contract Governance is IGovernance { self.cachedState = ProposalState.Dropped; emit ProposalDropped(_proposalId); - return true; } /** diff --git a/l1-contracts/src/governance/interfaces/IGovernance.sol b/l1-contracts/src/governance/interfaces/IGovernance.sol index d9eae9002657..a51624b59015 100644 --- a/l1-contracts/src/governance/interfaces/IGovernance.sol +++ b/l1-contracts/src/governance/interfaces/IGovernance.sol @@ -19,13 +19,13 @@ enum ProposalState { Expired } -struct ProposeConfiguration { +struct ProposeWithLockConfiguration { Timestamp lockDelay; uint256 lockAmount; } struct Configuration { - ProposeConfiguration proposeConfig; + ProposeWithLockConfiguration proposeConfig; Timestamp votingDelay; Timestamp votingDuration; Timestamp executionDelay; @@ -88,9 +88,9 @@ interface IGovernance { function finalizeWithdraw(uint256 _withdrawalId) external; function propose(IPayload _proposal) external returns (uint256); function proposeWithLock(IPayload _proposal, address _to) external returns (uint256); - function vote(uint256 _proposalId, uint256 _amount, bool _support) external returns (bool); - function execute(uint256 _proposalId) external returns (bool); - function dropProposal(uint256 _proposalId) external returns (bool); + function vote(uint256 _proposalId, uint256 _amount, bool _support) external; + function execute(uint256 _proposalId) external; + function dropProposal(uint256 _proposalId) external; function isPermittedInGovernance(address _caller) external view returns (bool); function isAllBeneficiariesAllowed() external view returns (bool); diff --git a/l1-contracts/src/governance/libraries/AddressSnapshotLib.sol b/l1-contracts/src/governance/libraries/AddressSnapshotLib.sol index 1023ee664913..274a2c0c5e7f 100644 --- a/l1-contracts/src/governance/libraries/AddressSnapshotLib.sol +++ b/l1-contracts/src/governance/libraries/AddressSnapshotLib.sol @@ -27,7 +27,6 @@ struct Index { // AddressSnapshotLib error AddressSnapshotLib__IndexOutOfBounds(uint256 index, uint256 size); // 0xd789b71a -error AddressSnapshotLib__AddressNotInSet(address addr); error AddressSnapshotLib__CannotAddAddressZero(); /** @@ -73,8 +72,6 @@ library AddressSnapshotLib { /** * @notice Removes a address from the set by address * - * @dev This function is only used in tests. - * * @param _self The storage reference to the set * @param _address The address of the address to remove * @return bool True if the address was removed, false if it wasn't found diff --git a/l1-contracts/src/governance/libraries/ConfigurationLib.sol b/l1-contracts/src/governance/libraries/ConfigurationLib.sol index 730f231fa67c..6bb5e6921810 100644 --- a/l1-contracts/src/governance/libraries/ConfigurationLib.sol +++ b/l1-contracts/src/governance/libraries/ConfigurationLib.sol @@ -46,7 +46,7 @@ library ConfigurationLib { * @dev We specify `memory` here since it is called on outside import for validation * before writing it to state. */ - function assertValid(Configuration memory _self) internal pure returns (bool) { + function assertValid(Configuration memory _self) internal pure { require(_self.quorum >= QUORUM_LOWER, Errors.Governance__ConfigurationLib__QuorumTooSmall()); require(_self.quorum <= QUORUM_UPPER, Errors.Governance__ConfigurationLib__QuorumTooBig()); @@ -85,7 +85,5 @@ library ConfigurationLib { require(_self.gracePeriod >= TIME_LOWER, Errors.Governance__ConfigurationLib__TimeTooSmall("GracePeriod")); require(_self.gracePeriod <= TIME_UPPER, Errors.Governance__ConfigurationLib__TimeTooBig("GracePeriod")); - - return true; } } diff --git a/l1-contracts/src/governance/libraries/Errors.sol b/l1-contracts/src/governance/libraries/Errors.sol index 00e4e20e69b2..0894ca32fff6 100644 --- a/l1-contracts/src/governance/libraries/Errors.sol +++ b/l1-contracts/src/governance/libraries/Errors.sol @@ -17,9 +17,7 @@ library Errors { error Governance__GovernanceProposerCannotBeSelf(); error Governance__CallerNotSelf(address caller, address self); error Governance__CallerCannotBeSelf(); - error Governance__NoCheckpointsFound(); error Governance__InsufficientPower(address voter, uint256 have, uint256 required); - error Governance__InvalidConfiguration(); error Governance__CannotWithdrawToAddressZero(); error Governance__WithdrawalNotInitiated(); error Governance__WithdrawalAlreadyClaimed(); @@ -45,12 +43,6 @@ library Errors { error Governance__ConfigurationLib__TimeTooSmall(string name); error Governance__ConfigurationLib__TimeTooBig(string name); - error Governance__ProposalLib__ZeroMinimum(); - error Governance__ProposalLib__ZeroVotesNeeded(); - error Governance__ProposalLib__MoreVoteThanExistNeeded(); - error Governance__ProposalLib__ZeroYeaVotesNeeded(); - error Governance__ProposalLib__MoreYeaVoteThanExistNeeded(); - error EmpireBase__FailedToSubmitRoundWinner(IPayload payload); error EmpireBase__InstanceHaveNoCode(address instance); error EmpireBase__InsufficientSignals(uint256 signalsCast, uint256 signalsNeeded); @@ -63,7 +55,6 @@ library Errors { error EmpireBase__RoundTooOld(uint256 roundNumber, uint256 currentRoundNumber); error EmpireBase__RoundTooNew(uint256 roundNumber, uint256 currentRoundNumber); error EmpireBase__SignalAlreadyCastForSlot(Slot slot); - error GovernanceProposer__PayloadHaveNoCode(IPayload payload); error GovernanceProposer__GSEPayloadInvalid(); error CoinIssuer__InsufficientMintAvailable(uint256 available, uint256 needed); // 0xa1cc8799 @@ -92,7 +83,4 @@ library Errors { error GSE__ProofOfPossessionAlreadySeen(bytes32 hashedPK1); error Delegation__InsufficientPower(address, uint256, uint256); - - error Governance__BlsKeyInvalidG1Point(uint256[2]); - error Governance__BlsKeyInvalidG2Point(uint256[4]); } diff --git a/l1-contracts/src/governance/libraries/ProposalLib.sol b/l1-contracts/src/governance/libraries/ProposalLib.sol index d2b22d420bbc..9cd39a011ed9 100644 --- a/l1-contracts/src/governance/libraries/ProposalLib.sol +++ b/l1-contracts/src/governance/libraries/ProposalLib.sol @@ -14,7 +14,6 @@ enum VoteTabulationReturn { } enum VoteTabulationInfo { - MinimumEqZero, TotalPowerLtMinimum, VotesNeededEqZero, VotesNeededGtTotalPower, diff --git a/l1-contracts/src/governance/libraries/compressed-data/Configuration.sol b/l1-contracts/src/governance/libraries/compressed-data/Configuration.sol index da3fcf3651be..f965defc62e4 100644 --- a/l1-contracts/src/governance/libraries/compressed-data/Configuration.sol +++ b/l1-contracts/src/governance/libraries/compressed-data/Configuration.sol @@ -2,7 +2,7 @@ // Copyright 2024 Aztec Labs. pragma solidity >=0.8.27; -import {Configuration, ProposeConfiguration} from "@aztec/governance/interfaces/IGovernance.sol"; +import {Configuration, ProposeWithLockConfiguration} from "@aztec/governance/interfaces/IGovernance.sol"; import {CompressedTimestamp, CompressedTimeMath} from "@aztec/shared/libraries/CompressedTimeMath.sol"; import {Timestamp} from "@aztec/shared/libraries/TimeMath.sol"; import {SafeCast} from "@oz/utils/math/SafeCast.sol"; @@ -48,9 +48,10 @@ library CompressedConfigurationLib { function getProposeConfig(CompressedConfiguration storage _compressed) internal view - returns (ProposeConfiguration memory) + returns (ProposeWithLockConfiguration memory) { - return ProposeConfiguration({lockDelay: _compressed.lockDelay.decompress(), lockAmount: _compressed.lockAmount}); + return + ProposeWithLockConfiguration({lockDelay: _compressed.lockDelay.decompress(), lockAmount: _compressed.lockAmount}); } /** @@ -87,7 +88,7 @@ library CompressedConfigurationLib { */ function decompress(CompressedConfiguration memory _compressed) internal pure returns (Configuration memory) { return Configuration({ - proposeConfig: ProposeConfiguration({ + proposeConfig: ProposeWithLockConfiguration({ lockDelay: _compressed.lockDelay.decompress(), lockAmount: _compressed.lockAmount }), diff --git a/l1-contracts/src/shared/libraries/BN254Lib.sol b/l1-contracts/src/shared/libraries/BN254Lib.sol index 3bf47a702b90..d1ce05165730 100644 --- a/l1-contracts/src/shared/libraries/BN254Lib.sol +++ b/l1-contracts/src/shared/libraries/BN254Lib.sol @@ -236,8 +236,8 @@ library BN254Lib { mstore(add(freeMem, 0x60), xx) // (N + 1) / 4 = 0xc19139cb84c680a6e14116da060561765e05aa45a1c72a34f082305b61f3f52 mstore(add(freeMem, 0x80), 0xc19139cb84c680a6e14116da060561765e05aa45a1c72a34f082305b61f3f52) - // N = 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47 - mstore(add(freeMem, 0xA0), 0x30644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd47) + // N = BASE_FIELD_ORDER + mstore(add(freeMem, 0xA0), BASE_FIELD_ORDER) callSuccess := staticcall(sub(gas(), 2000), 5, freeMem, 0xC0, freeMem, 0x20) x := mload(freeMem) hasRoot := eq(xx, mulmod(x, x, BASE_FIELD_ORDER)) diff --git a/l1-contracts/test/governance/governance-proposer/signal.t.sol b/l1-contracts/test/governance/governance-proposer/signal.t.sol index e5f1eadb8dea..170131dc2506 100644 --- a/l1-contracts/test/governance/governance-proposer/signal.t.sol +++ b/l1-contracts/test/governance/governance-proposer/signal.t.sol @@ -15,13 +15,6 @@ contract SignalTest is GovernanceProposerBase { address internal proposer = address(0x1234567890); Fakerollup internal validatorSelection; - // Skipping this test since the it matches the for now skipped check in `EmpireBase::signal` - function skip__test_WhenProposalHoldNoCode() external { - // it revert - vm.expectRevert(abi.encodeWithSelector(Errors.GovernanceProposer__PayloadHaveNoCode.selector, proposal)); - governanceProposer.signal(proposal); - } - modifier whenProposalHoldCode() { proposal = IPayload(address(this)); _; diff --git a/l1-contracts/test/governance/governance-proposer/voteWithsig.t.sol b/l1-contracts/test/governance/governance-proposer/voteWithsig.t.sol index 1dc86938e3fb..c89dc5132c5b 100644 --- a/l1-contracts/test/governance/governance-proposer/voteWithsig.t.sol +++ b/l1-contracts/test/governance/governance-proposer/voteWithsig.t.sol @@ -27,13 +27,6 @@ contract SignalWithSigTest is GovernanceProposerBase { validatorSelection = new Fakerollup(); } - // Skipping this test since the it matches the for now skipped check in `EmpireBase::signal` - function skip__test_WhenProposalHoldNoCode() external { - // it revert - vm.expectRevert(abi.encodeWithSelector(Errors.GovernanceProposer__PayloadHaveNoCode.selector, proposal)); - governanceProposer.signalWithSig(proposal, signature); - } - modifier whenProposalHoldCode() { proposal = IPayload(address(this)); signature = createSignature(privateKey, proposal); diff --git a/l1-contracts/test/governance/governance/dropProposal.t.sol b/l1-contracts/test/governance/governance/dropProposal.t.sol index 674986f87be9..339d4e613b2c 100644 --- a/l1-contracts/test/governance/governance/dropProposal.t.sol +++ b/l1-contracts/test/governance/governance/dropProposal.t.sol @@ -15,7 +15,7 @@ contract DropProposalTest is GovernanceBase { _stateDroppable("empty", _governanceProposer); assertEq(governance.getProposal(proposalId).cachedState, ProposalState.Pending); assertEq(governance.getProposalState(proposalId), ProposalState.Droppable); - assertTrue(governance.dropProposal(proposalId)); + governance.dropProposal(proposalId); assertEq(governance.getProposal(proposalId).cachedState, ProposalState.Dropped); vm.expectRevert(abi.encodeWithSelector(Errors.Governance__ProposalAlreadyDropped.selector)); @@ -28,7 +28,7 @@ contract DropProposalTest is GovernanceBase { { // it revert _stateExecutable("empty", _voter, _totalPower, _votesCast, _yeas); - assertTrue(governance.execute(proposalId)); + governance.execute(proposalId); assertEq(governance.getProposal(proposalId).cachedState, ProposalState.Executed); vm.expectRevert(abi.encodeWithSelector(Errors.Governance__ProposalCannotBeDropped.selector)); @@ -90,7 +90,7 @@ contract DropProposalTest is GovernanceBase { { // it revert _stateExecutable("empty", _voter, _totalPower, _votesCast, _yeas); - assertTrue(governance.execute(proposalId)); + governance.execute(proposalId); assertEq(governance.getProposalState(proposalId), ProposalState.Executed); } @@ -111,7 +111,7 @@ contract DropProposalTest is GovernanceBase { _stateDroppable("empty", _governanceProposer); assertEq(governance.getProposal(proposalId).cachedState, ProposalState.Pending); assertEq(governance.getProposalState(proposalId), ProposalState.Droppable); - assertTrue(governance.dropProposal(proposalId)); + governance.dropProposal(proposalId); assertEq(governance.getProposal(proposalId).cachedState, ProposalState.Dropped); assertEq(governance.getProposalState(proposalId), ProposalState.Dropped); } diff --git a/l1-contracts/test/governance/governance/execute.t.sol b/l1-contracts/test/governance/governance/execute.t.sol index c04c4c28258b..6a25479f15f3 100644 --- a/l1-contracts/test/governance/governance/execute.t.sol +++ b/l1-contracts/test/governance/governance/execute.t.sol @@ -65,7 +65,7 @@ contract ExecuteTest is GovernanceBase { { // it revert _stateExecutable("empty", _voter, _totalPower, _votesCast, _yeas); - assertTrue(governance.execute(proposalId)); + governance.execute(proposalId); assertEq(governance.getProposalState(proposalId), ProposalState.Executed); } @@ -131,7 +131,7 @@ contract ExecuteTest is GovernanceBase { vm.expectEmit(true, true, true, true, address(governance)); emit IGovernance.ProposalExecuted(proposalId); - assertTrue(governance.execute(proposalId)); + governance.execute(proposalId); proposal = governance.getProposal(proposalId); diff --git a/l1-contracts/test/governance/governance/proposallib/voteTabulation.tree b/l1-contracts/test/governance/governance/proposallib/voteTabulation.tree index f0bf61a43f57..8493955bebb4 100644 --- a/l1-contracts/test/governance/governance/proposallib/voteTabulation.tree +++ b/l1-contracts/test/governance/governance/proposallib/voteTabulation.tree @@ -1,6 +1,4 @@ VoteTabulationTest -├── when minimum config eq 0 -│ └── it return (Invalid, MinimumEqZero) └── when minimum gt 0 ├── when total power lt minimum │ └── it return (Rejected, TotalPowerLtMinimum) diff --git a/l1-contracts/test/governance/governance/scenarios/noVoteAndExit.t.sol b/l1-contracts/test/governance/governance/scenarios/noVoteAndExit.t.sol index 005368af29b1..c6d51a387904 100644 --- a/l1-contracts/test/governance/governance/scenarios/noVoteAndExit.t.sol +++ b/l1-contracts/test/governance/governance/scenarios/noVoteAndExit.t.sol @@ -14,7 +14,7 @@ contract NoVoteAndExitTest is GovernanceBase { function test_CannotVoteAndExit(address _voter, uint256 _totalPower, uint256 _votesCast, uint256 _yeas) external { bytes32 _proposalName = "empty"; - vm.assume(_voter != address(0)); + vm.assume(_voter != address(0) && _voter != address(governance)); proposal = proposals[_proposalName]; proposalId = proposalIds[_proposalName]; diff --git a/l1-contracts/test/harnesses/TestConstants.sol b/l1-contracts/test/harnesses/TestConstants.sol index cf540d40242b..f20d09fac0b8 100644 --- a/l1-contracts/test/harnesses/TestConstants.sol +++ b/l1-contracts/test/harnesses/TestConstants.sol @@ -9,7 +9,7 @@ import {Bps} from "@aztec/core/libraries/rollup/RewardLib.sol"; import {StakingQueueConfig} from "@aztec/core/libraries/compressed-data/StakingQueueConfig.sol"; import {IRewardDistributor} from "@aztec/governance/interfaces/IRewardDistributor.sol"; import {RewardBoostConfig, IBoosterCore} from "@aztec/core/reward-boost/RewardBooster.sol"; -import {Configuration, ProposeConfiguration} from "@aztec/governance/interfaces/IGovernance.sol"; +import {Configuration, ProposeWithLockConfiguration} from "@aztec/governance/interfaces/IGovernance.sol"; import {Timestamp} from "@aztec/shared/libraries/TimeMath.sol"; import {SlasherFlavor} from "@aztec/core/interfaces/ISlasher.sol"; @@ -50,7 +50,7 @@ library TestConstants { function getGovernanceConfiguration() internal pure returns (Configuration memory) { return Configuration({ - proposeConfig: ProposeConfiguration({lockDelay: Timestamp.wrap(60 * 60 * 24 * 30), lockAmount: 1e24}), + proposeConfig: ProposeWithLockConfiguration({lockDelay: Timestamp.wrap(60 * 60 * 24 * 30), lockAmount: 1e24}), votingDelay: Timestamp.wrap(60), votingDuration: Timestamp.wrap(60 * 60), executionDelay: Timestamp.wrap(60),