feat: governance proposal validator specs #701

0xOneTony · 2025-05-12T17:09:12Z

Description

Following the design doc ethereum-optimism/design-docs#260, this PR details the specs for the new ProposalValidator contract

ben-chain

I have reviewed this up to about the halfway mark which covers most of the business logic. Will return to the rest, but this feels like enough meat to start making improvements.

Main open item I need to figure out is how we populate the election calldata. Need to investigate how Agora does this and will return here

ben-chain · 2025-05-13T21:37:56Z

specs/experimental/gov-proposal-validator.md

+
+Submits a `GovernanceFund` or `CouncilBudget` proposal type that transfers OP tokens for approval and voting.
+
+- MUST only be called for `GovernanceFund` or `CouncilBudget` proposal type


Just to be explicit -- these two types should only split out/separate to the extent that it's useful for frontends to differentiate between them. Other than that, all onchain logic should be identical between these two types. I don't see anything that suggests otherwise yet but felt worth a call-out.

Definitely, since these 2 types require the same checks there wont be any difference when it comes to smart contract logic

ben-chain · 2025-05-13T21:49:40Z

specs/experimental/gov-proposal-validator.md

+- MUST provide a valid attestation UID
+- MUST NOT transfer any tokens or change any allowances
+- MUST emit `ProposalSubmitted` event
+- MUST store proposal data


Let's try to specify what exactly we mean by "proposal data" in this context. I don't think it requires all arguments to submitProposal to be stored. Once I review the actual functions, I will try to return to this comment and specify what I think it would be.

I agree, we will make this more descriptive on what the "proposal data" are. In the meantime I will give some context here. We use this mapping to store each proposal data

specs/specs/experimental/gov-proposal-validator.md

Lines 265 to 271 in 2465cca

`_proposals`

A mapping that stores each submitted proposals’ data based on its `proposalHash`. The proposal hash is produced by hashing the ABI encoded `targets` array, the `values` array, the `calldatas` array and the `description`

```solidity

mapping(bytes32 => ProposalSubmissionData) private _proposals;

```

where ProposalSubmissionData are defined here

specs/specs/experimental/gov-proposal-validator.md

Lines 283 to 303 in 2465cca

`ProposalSubmissionData`

A struct that holds all the data for a single proposal. Consists of:

- `proposer`: The address that submitted the proposal

- `proposalType`: The type of the proposal

- `proposalTypeConfigurator`: The voting type for the proposal

- `inVoting`: Returns true if the proposal has already been submitted for voting

- `delegateApprovals`: Mapping of addresses that approved the specific proposal

- `approvalsCounter`: The number of approvals the specific proposal has received

```solidity

struct ProposalSubmissionData {

address proposer;

ProposalType proposalType;

uint8 proposalTypeConfigurator;

bool inVoting;

mapping(address => bool) delegateApprovals;

uint256 approvalsCounter;

}

```

Do you think we could remove any of the structs properties?

Thanks yes, I made this comment before reading further down, it makes more sense with the struct specified.

Do you think we could remove any of the structs properties?

proposalTypeConfigurator, pending other discussions on this topic :)

ben-chain · 2025-05-13T22:29:31Z

specs/experimental/gov-proposal-validator.md

+- MUST only be called for `ProtocolOrGovernorUpgrade` , `MaintenanceUpgradeProposals`, or `CouncilMemberElections` types
+- MUST be called by an approved address
+- MUST check if the proposal is a duplicate
+- MUST provide a valid proposal type configurator


This should not be necessary I believe. There is a 1:1 mapping of ProposalType as defined in this spec to the ProposalTypesConfigurator uint in the existing governor. This is defined in column G here

Thank you for pointing this out, we initially thought to not have this, and tie the ProposalTypeConfigurtator to each ProposalType like you mentioned, however we were not sure if these are possible to change in the future or have a ProposalType to be able to choose between more than 1 ProposalTypeConfigurator thats why we defined it as is.
If these ProposalTypeConfigurators WILL NOT change any time in the future we could use the mapping and remove the check, this would make things much more simple for us.

I think it is very low probability that we break 1:1 invariant any time soon. Further, it is always possible to add additional ProposalTypes to implement the same functionality and maintain the 1:1 invariant. IMO this might be a better pattern anyway, so my intuition is the simpler thing is better.

ben-chain · 2025-05-13T22:36:45Z

specs/experimental/gov-proposal-validator.md

+    address[] memory _targets,
+    uint256[] memory _values,
+    bytes[] memory _calldatas,


Suggested change

address[] memory _targets,

uint256[] memory _values,

bytes[] memory _calldatas,

In the future this may change, but for MVP, none of these proposal types are ever expected to create a call from the governor. So instead what we want to do is either:

enforce that submitProposal stores [0x00..00],[0],[0x] for any proposal submitted via this function

enforce that moveToVote only passes those zero values as calldata to the OptimismGovernor.propose function
Depending on which feels cleaner/easier :)

Actually, I am realizing that there is an exception to the above for Approval votes (elections) because we need to specify a number of options. The options should still be no-ops but there needs to be some submission step here which I don't fully understand. Effectively, what I'm saying above that we just enforce no-op still applies, but those no-ops will within this struct, which is embedded in an array in the overall proposal struct for this type.. I will follow up and figure out exactly how this logic works, I am having trouble understanding. For reference, here is an example election proposal; I'm struggling to decode the data, need a night's sleep first 😅

ben-chain · 2025-05-13T22:38:02Z

specs/experimental/gov-proposal-validator.md

+    bytes[] memory _calldatas,
+    string memory _description,
+    ProposalType _proposalType,
+    uint8 _proposalTypeConfiguration,


Suggested change

uint8 _proposalTypeConfiguration,

See above comment, _proposalType is sufficient to infer this value

ben-chain · 2025-05-13T23:10:38Z

specs/experimental/gov-proposal-validator.md

+- Proposal MUST have gathered X amount of approvals by top delegates
+- Proposal MUST be moved to vote during a valid voting cycle
+- MUST check if proposal has already moved for voting
+- MUST check if the total amount of tokens that can possible be distributed during this voting cycle does not go over the `VotingCycleData.votingCycleDistributionLimit`


Let's discuss on the call tomorrow the implementation complexity of this approach. We are comfortable with a per-proposal-only cap if it simplifies things a lot.

ben-chain · 2025-05-13T23:13:11Z

specs/experimental/gov-proposal-validator.md

+    returns (uint256 governorProposalId_)
+```
+
+`canSignOff`


nit: can we call this canApproveProposal, feels a bit more obvious what it is than "sign off"

I agree, will change it!

ben-chain · 2025-05-13T23:18:11Z

specs/experimental/gov-proposal-validator.md

+    address[] memory _targets,
+    uint256[] memory _values,
+    bytes[] memory _calldata,
+    string memory description


Suggested change

address[] memory _targets,

uint256[] memory _values,

bytes[] memory _calldata,

string memory description

string memory description,

ProposalType memory _proposalType,

address[] memory candidates, // only for elections

address _recipient, // only for funding proposals

uint256 _amount // only for funding proposals

I think it will be something like this, may want to break out a few of these functions/signature overload or whatever

ben-chain · 2025-05-13T23:20:43Z

specs/experimental/gov-proposal-validator.md

+- MUST emit `ProposalMovedToVote` event
+
+```solidity
+function moveToVote(


I can't think of a reason that we would need it right now, but this smells like a place where traditionally there would be some sort of nonce stored and incremented during proposal submission and then provided to moveToVote. I can't think of what would go wrong without it though. It would mean you can't submit identical proposals multiple times, but changing the description is also very easy. Let's discuss live.

ben-chain · 2025-05-13T23:57:48Z

specs/experimental/gov-proposal-validator.md

+function setDistributionThreshold(uint256 _threshold) external
+```
+
+`setProposalTypeApprovalThreshold`


Oh I see, we made this configurable. I think that's okay, it would be nice if we can default to 4 in the constructor so we don't have to remember to call this unless we change in the future

* fix: linting * fix: remove type configurator as input

* fix: linting * fix: remove type configurator as input * fix: submit functions * fix: define approval timing * feat: add voting module info * fix: proposal uniquness * fix: move to vote * feat: add new event for voting module data * fix: proposals mapping description * fix: namings * fix: descriptions

* fix: improvements * feat: improve move to vote * fix: naming * fix: remove storage of encoded data

ben-chain

Looking good! I think the biggest concern I realized in this review is the delegate approval threshold being dynamic / changing each voting cycle as delegations change. I proposed one way to do it that seems ok, but let's discuss in discord how painful this is to implement in practice

ben-chain · 2025-06-02T21:46:21Z

specs/experimental/gov-proposal-validator.md

+
+### Public Functions
+
+`submitProtocolOrGovernorUpgradeProposal`


fwiw if this is killing you I'm fine with just submitUpgradeProposal or submitRegularUpgradeProposal or something :P

perfect! I prefer the sorter version aswell just wanted to be as verbose but I think submitUpgradeProposal is fine aswell

ben-chain · 2025-06-02T21:48:17Z

specs/experimental/gov-proposal-validator.md

+
+## Roles
+
+The contract has a single `owner` role (Optimism Foundation) with permissions to:


Something I didn't think to ask -- do we read this storage address to check for the EAS attestations as well? That should probs be in this list if so. If not, curious how it's done otherwise

Yes we do read the address to make sure that the attester is the owner address

ben-chain · 2025-06-02T21:52:50Z

specs/experimental/gov-proposal-validator.md

+```solidity
+function submitProtocolOrGovernorUpgradeProposal(
+    uint128 _criteriaValue,
+    string[] memory _optionDescriptions,


I believe the options should always be yes/no here for upgrades, so we can omit from this method and hardcode in the function. Following up w/ gov team if there is a third abstain option, but I believe there is not.

beautiful! will adjust to the 2 options

Whoops, I was incorrect -- it should still be hardcoded, but options are yes/no/abstain

ben-chain · 2025-06-02T21:53:24Z

specs/experimental/gov-proposal-validator.md

+) external returns (bytes32 proposalHash_);
+```
+
+Approval Voting Module


Suggested change

Approval Voting Module

**Approval Voting Module**

nit

ben-chain · 2025-06-02T21:55:54Z

specs/experimental/gov-proposal-validator.md

+
+Submits a Protocol or Governor Upgrade proposal for approval and voting.
+
+- MUST be called by an approved address


Suggested change

- MUST be called by an approved address

- MUST be called by an `owner`-approved address

clarity nit assuming my question on the owner role is yes

ben-chain · 2025-06-02T22:07:39Z

specs/experimental/gov-proposal-validator.md

+    uint248 _againstThreshold,
+    bool _isRelativeToVotableSupply,


We haven't documented these values here, but I believe they can be omitted from this method. I believe that this should always be 12%, true -- meaning, if more than 12% of votable supply chooses veto option, the proposal is rejected. I think hardcoding should be fine

@ben-chain Perfect this makes things more simple for the specific proposal, do we know if these numbers could change in the future?

Same story as previous comment -- I will follow up tomorrow. It is definitely always true but there might be a few options to %.

ben-chain · 2025-06-04T17:16:10Z

specs/experimental/gov-proposal-validator.md

+This requires the user who submits the proposal to provide some additional data related to the proposal.
+
+For the `ProposalSettings` of the voting module, these are:
+- `uint128 criteriaValue`: Since the passing criteria type is always "Threshold" for this proposal type


I believe that this should be a fixed % of votable supply. So I think we can remove it as an argument and just pass fixed % of the votable supply oracle value

ben-chain · 2025-06-04T17:18:18Z

specs/experimental/gov-proposal-validator.md

+
+- This type does not require any checks and is being forwarded to the Governor contracts, this should happen atomically.
+
+For `CouncilMemberElections`, `GovernanceFund` and `CouncilBudget` types:


Elections do not do fund transfers. So I believe that the _optionsAmounts/recipients should be checked as zero in the CouncilMemberElections case, same as upgrades case

Correct, for the CouncilMemberElections we will be checking to make sure these fields are empty, will update the document to mention this

ben-chain · 2025-06-04T17:44:08Z

specs/experimental/gov-proposal-validator.md

+
+`setMinimumVotingPower`
+
+Sets the minimum voting power a delegate must have in order to be eligible to approve a proposal.


Technically if the canApproveProposal check is based on the start of the voting cycle, wouldn't this need to be indexed by voting cycle or something?

If the associated complexity is significant, we can think about other options. But if there's a voting cycle storage object, it might just be that you setMinimumVotingPower(_power, _cycleId) or something here instead

ben-chain · 2025-06-04T17:52:54Z

specs/experimental/gov-proposal-validator.md

+struct VotingCycleData {
+    uint256 startingBlock;
+    uint256 duration;
+    uint256 votingCycleDistributionLimit;


So I guess we could put the approvalThreshold here to do it per-cycle? I guess the issue would then be the OF needs to set it quickly, or we might have the wrong value used in a canApprove check.

Maybe we can block delegate approvals until the threshold is set. So the flow would be:

initial voting cycles have a new approvalThreshold field, all are initialized to 0

once a cycle has started and we know what the threshold should be and set to that value

approveProposal requires the above step was complete/nonzero value

Does that feel doable? Any other ideas?

Agreed to keep this as is for the MVP

* fix: submit protocol or governor upgrade function name * fix: remove options from upgrade proposals * fix: bold for modules info title * fix: specify proposer approved address must be attested by the owner * fix: optimistic voting module description * fix: move to vote cases description * fix: wording on approval module criteria value

0xOneTony · 2025-06-05T18:40:21Z

specs/experimental/gov-proposal-validator.md

+- `uint128 criteriaValue`: Since the passing criteria type is always "Threshold", for this proposal type,
+this value will be the percentage that will be used to calculate the fraction of the votable supply that
+the proposal will need in votes in order to pass.


@ben-chain If the % is fixed we can even remove this as an input and hardcode it

* fix: top100 delegate calculation to use EAS * fix: function seperation * fix: function seperation * fix: headers

ben-chain

This is looking great. I'd like us to fill in the non-maintenance upgrade method for completion, but everything else looks spot on, no comments.

ben-chain · 2025-06-16T21:05:59Z

specs/experimental/gov-proposal-validator.md

+
+`submitUpgradeProposal`
+
+Submits a Protocol/Governor Upgrade or a Maintenance Upgrade proposal to move for voting.


This should have a method spec, right?

I think it would be the same exact signature and requirements, minus can move straight to voting if all submission checks pass

The idea is to have the same function for both of these types since most of the logic is the same, however we improved the text and params here! d23b082

* fix: submit upgrade proposal * fix: text

* fix: spec inconsistencies * fix: linting

mds1

Approving on behalf of Ben to unblock merge

* fix: can approve proposal visibility * fix: event name * fix: proposal data field name * fix: voting cycle data field name * fix: submit funding proposal invariant * fix: criteria value description * fix: spelling * fix: approved proposer schema * fix: against threshold description

alcueca · 2025-08-01T04:57:58Z

specs/experimental/gov-proposal-validator.md

+- MUST use the `Optimistic` Voting Module
+- MUST provide a valid againstThreshold
+- MUST provide a valid attestation UID
+- MUST NOT execute any operations


What's the definition of operations here?

This should be removed from here since these proposals are using the Optimistic voting module. It is also mentioned for the Council Member Elections proposals but it should stay there (will rephrase it to be more clear). Basically the Approval voting module has this ProposalOption struct https://github.com/voteagora/optimism-governor/blob/3f8a2ea29d2c91a176758c72a213d499424ee2fc/src/modules/ApprovalVotingModule.sol#L35-L41
that can include some arbitary calldata, so the meaning of operations is basically that these calldata should be empty and thus not execute any further operations.
An other example of the Approval voting module which for this case should include some calldata is for the funding proposal types where we have to execute a transfer call if the proposal passes.

0xOneTony added 5 commits May 9, 2025 17:49

feat: proposal validator spec

ed94cc6

feat: add proposal type data struct

1b3be99

fix: comments

06a2df6

fix: nit improvements

a64f159

fix: linting

2465cca

0xOneTony requested review from a team as code owners May 12, 2025 17:09

0xOneTony marked this pull request as draft May 12, 2025 17:09

0xOneTony changed the title ~~feat: governance validator specs~~ feat: governance proposal validator specs May 12, 2025

0xOneTony marked this pull request as ready for review May 12, 2025 17:19

ben-chain reviewed May 14, 2025

View reviewed changes

0xOneTony and others added 5 commits May 19, 2025 17:44

fix: linting (#32)

18b9c60

fix: remove type configurator as input (#33)

c2d7c2e

* fix: linting * fix: remove type configurator as input

fix: linting

1d82078

feat: improvements (#36)

e87c473

* fix: improvements * feat: improve move to vote * fix: naming * fix: remove storage of encoded data

0xOneTony requested a review from ben-chain June 2, 2025 12:40

ben-chain reviewed Jun 4, 2025

View reviewed changes

0xOneTony commented Jun 5, 2025

View reviewed changes

0xOneTony and others added 5 commits June 10, 2025 17:49

fix: submit upgrade function (#38)

63e75da

fix: top100 delegates calculation to use EAS (#39)

5e8ee3a

* fix: top100 delegate calculation to use EAS * fix: function seperation * fix: function seperation * fix: headers

fix: linting

de95558

fix: spelling

4af7e01

fix: approve proposal description

3961306

ben-chain reviewed Jun 16, 2025

View reviewed changes

0xOneTony added 2 commits June 17, 2025 21:17

fix: submit upgrade proposal (#40)

d23b082

* fix: submit upgrade proposal * fix: text

fix: pp spec inconsistencies (#41)

46af875

* fix: spec inconsistencies * fix: linting

mds1 approved these changes Jul 15, 2025

View reviewed changes

0xOneTony mentioned this pull request Jul 28, 2025

feat(governance): proposal validator ethereum-optimism/optimism#16861

Closed

alcueca reviewed Aug 1, 2025

View reviewed changes

0xOneTony marked this pull request as draft August 21, 2025 14:47


		Submits a `GovernanceFund` or `CouncilBudget` proposal type that transfers OP tokens for approval and voting.

		- MUST only be called for `GovernanceFund` or `CouncilBudget` proposal type

	`_proposals`

	A mapping that stores each submitted proposals’ data based on its `proposalHash`. The proposal hash is produced by hashing the ABI encoded `targets` array, the `values` array, the `calldatas` array and the `description`

	```solidity
	mapping(bytes32 => ProposalSubmissionData) private _proposals;
	```

	`ProposalSubmissionData`

	A struct that holds all the data for a single proposal. Consists of:

	- `proposer`: The address that submitted the proposal
	- `proposalType`: The type of the proposal
	- `proposalTypeConfigurator`: The voting type for the proposal
	- `inVoting`: Returns true if the proposal has already been submitted for voting
	- `delegateApprovals`: Mapping of addresses that approved the specific proposal
	- `approvalsCounter`: The number of approvals the specific proposal has received

	```solidity
	struct ProposalSubmissionData {
	address proposer;
	ProposalType proposalType;
	uint8 proposalTypeConfigurator;
	bool inVoting;
	mapping(address => bool) delegateApprovals;
	uint256 approvalsCounter;
	}
	```

	address[] memory _targets,
	uint256[] memory _values,
	bytes[] memory _calldatas,

-    address[] memory _targets,
-    uint256[] memory _values,
-    bytes[] memory _calldata,
-    string memory description
+    string memory description,
+    ProposalType memory _proposalType,
+    address[] memory candidates, // only for elections
+    address _recipient, // only for funding proposals
+    uint256 _amount // only for funding proposals


		### Public Functions

		`submitProtocolOrGovernorUpgradeProposal`


		## Roles

		The contract has a single `owner` role (Optimism Foundation) with permissions to:


		Submits a Protocol or Governor Upgrade proposal for approval and voting.

		- MUST be called by an approved address

	- MUST be called by an approved address
	- MUST be called by an `owner`-approved address


		- This type does not require any checks and is being forwarded to the Governor contracts, this should happen atomically.

		For `CouncilMemberElections`, `GovernanceFund` and `CouncilBudget` types:


		`setMinimumVotingPower`

		Sets the minimum voting power a delegate must have in order to be eligible to approve a proposal.


		`submitUpgradeProposal`

		Submits a Protocol/Governor Upgrade or a Maintenance Upgrade proposal to move for voting.

feat: governance proposal validator specs #701

Are you sure you want to change the base?

feat: governance proposal validator specs #701

Uh oh!

Conversation

0xOneTony commented May 12, 2025

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment