feat(governance): proposal validator

[0xOneTony]

Description

This PR introduces the ProposalValidator contract designed to enable permissionless proposals in the Optimism
governance system, as detailed in the design doc and technical spec.

[ben-chain]

I have to go AFK, so submitting this WIP review for now. This is looking good, I will have some follow up comments for the gov team and there are some discussion points for us to touch on in my comments :D

[ben-chain]

This is interesting. I initially misinterpreted this as being about proxy-related ownership, which I think it is not. But: it does make me wonder how much complexity we introduce with these sorts of stateful config updates via the owner.

I hadn't thought about it initially, but -- if the OF can simply setAuthorizedProposer, then in theory, this entire contract could be immutable/non-upgradable/non-configurable. setAuthorizedProposer is basically a superset of the upgrade functionality here.

There are pros and cons to this, maybe it is worth a bigger discussion in chat. There are obviously some cons around in-flight proposals, indexers, etc. But actually with indexers for the Governor, we've seen breaking changes cause issues with historic votes. Separating addresses could have benefits there.

[ben-chain]

can't we return the struct here or is that an anti-pattern?

[0xOneTony]

I believe it is a good practice to return the fields instead of the struct for the getter function.

[ben-chain]

nit

Suggested change

	/// @notice The max amount of tokens that can be distributed in a proposal.
	/// @notice The max amount of tokens that can be distributed in a single proposal.

[ben-chain]

If I'm reading right, this is for the first voting cycle specifically, is that correct?

Unless I'm missing a security property or something, I'd recommend to remove from initialization and just have owner use the setters. We will always set voting cycles in batches/groups/seasons at a time, so no reason to have a special case. But LMK if you had something else in mind with these args

[ben-chain]

Personally I am comfortable with removing this check since ProposalValidator_InvalidUpgradeProposalType is thrown above if it's not a value we know to be true.

If we want to keep it -- feels like checking codesize(proposalTypeConfig.module) != 0 would be more robust than just the name length.

[corydickson]

A possible safer option is that the proposal type id for the approval module is known as well as the address. Is making this constant the best for now? Or adding some role for adding new proposal types / modules if things are redeployed?

[0xOneTony]

@ben-chain the idea of this check is to mimic the Governor to make sure that a valid submitted proposal will not revert in case it moves to the Governor. It also acts as a safety in case someone changes the voting module data on the ProposalTypeConfigurator
https://github.com/voteagora/optimism-governor/blob/3f8a2ea29d2c91a176758c72a213d499424ee2fc/src/OptimismGovernor.sol#L438

@corydickson The problem is we cant support a completely new module without redeploying cause each proposal has their unique set of rules that the contract will need to adapt for the specific proposal type that use the new module. As long as the voting module's logic doesn't change, then we can support a "different variation" of an existing module by simply updating the id of the configurator for the "new" module with setProposalTypeData.

[ben-chain]

can we add the _againstThreshold into the randomness here? I can see having to re-submit due to incorrect threshold and running into a collision

[ben-chain]

Oh, I see, we can't easily because this is matching the existing Governor Implementation, which assumed no against threshold change like that...

Are there any other security risks associated with the potential for collision here? If not, I guess we can just ignore this issue, and simply add whitespace to the description if we need to re-submit lol.

[ben-chain]

I am going back on myself again -- I just noticed that we are already forced to introduce idInConfigurator in other codepaths, so I think we should do my original suggestion

[0xOneTony]

We intentionally aim to generate the exact same proposal id as the one that will be created in the Governor, this helps us verify that the proposals are the same in both contracts and that the same proposal has not been already proposed to the Governor. If we want to avoid collisions the easiest way that also works currently would be to re-submit with a slight change in the description like you mentioned

[corydickson]

I think just adding the proposer to the description would be a good way handle this.

[ben-chain]

Is this worth making into an internal function? Seems to be shared with the other moveToVote functions

[ben-chain]

I will check in with the gov team -- there actually may be a desire for an artificially lower max.

[0xOneTony]

That would be great!

[ben-chain]

same as above -- I'm fine with deleting, if we keep, should probably check module codesize instead

[corydickson]

This might be a small nit but instead of imposing this check in submitting the proposal should this be moved to _buildApprovalModuleOptions just because this is an always required to check in the approval module and then if we add different proposal types it reduces a duplicated code pattern. Same thing for the optionsLength.

[0xOneTony]

Hmmm I am not sure about this check because this is not needed for example on the Council Member elections even though it uses the Approval module, but I believe we should add the check bellow now that you mentioned it if (optionsLength == 0 || optionsLength > type(uint8).max)

[corydickson]

I do somewhat worry here that we are overloading proposalType as the PTC currently linked to the OP governor has it's own set of proposalTypes that do not use modules etc but don't necessarily map 1-to-1 on the enum:

 enum ProposalType {
        ProtocolOrGovernorUpgrade,
        MaintenanceUpgrade,
        CouncilMemberElections,
        GovernanceFund,
        CouncilBudget
    }

I think if we ensure that these two things map onto each other there will be significantly less confusion. Apologies if this has been addressed already in the spec somewhere

[0xiamflux]

This set of ProposalTypes is linked to the ones defined in the ProposalTypeConfigurator only by the ProposalTypeData.idInConfigurator, they are *not * a 1:1 match as some of the ProposalTypesin the validator share the same id in the configurator, this is because while the validator’s types deal with individual types the ones in the configurator are more focused on the voting process (quorum, approval threshold and the module that should handle it).

[corydickson]

For sure, I was raising this to discuss the potential of coalescing them.

[corydickson]

should this be updated after _proposeToGovernor in case the transaction to create the proposal on the governor reverts?

[opgitgovernance]

This pr has been automatically marked as stale and will be closed in 5 days if no updates