-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add Proposal Validator #367
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.
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
feat: add Proposal Validator #367
Conversation
packages/contracts-bedrock/interfaces/governance/IDelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/interfaces/governance/IDelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/interfaces/governance/IDelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/interfaces/governance/IDelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/DelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/DelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/DelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/DelegatesProposalValidator.sol
Outdated
Show resolved
Hide resolved
0xOneTony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, there is a confusion around proposalId of ProposalValidator and OptimismGovernor also for proposalType.
We should define as ProposalType the actuall proposal type, e.g. governance funds proposal, protocol or governance upgrade proposal etc.. (this is a new thing nothing to do with the proposalType that already exist on the governor).
Then ProposalVotingType should be the existing proposalType that is reffering to the ProposalTypeConfigurator. I know it is kinda confusing cause we cant edit the code on the governor side for now but still we should differentiate these 2 on our contracts
0xOneTony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments! nice job 🔥
packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]>
0xOneTony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small details but overall great job! 🔥 Didnt check the tests yet will do when we have the fixes
packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/interfaces/governance/IProposalValidator.sol
Outdated
Show resolved
Hide resolved
| error ProposalValidator_AlreadyApproved(); | ||
|
|
||
| /// @notice Thrown when attempting to move a proposal to vote that is already in voting. | ||
| error ProposalValidator_AlreadyProposed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could change the name of the error to be more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried lol chore: improve errors naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more something like ProposalValidator_AlreadyMovedToVote since AlreadyProposed can be confused with the submit proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like saying that it is inVoting given that we use that naming for the proposals that have been moved to vote:
| /// @notice Data structure for storing proposal information. | |
| /// @param proposer The address that submitted the proposal. | |
| /// @param targets Target addresses for proposal calls. | |
| /// @param values ETH values for proposal calls. | |
| /// @param calldatas Function data for proposal calls. | |
| /// @param description Description of the proposal. | |
| /// @param proposalType Type of the proposal from the ProposalType enum. | |
| /// @param inVoting Whether the proposal has been moved to the voting phase. | |
| /// @param delegateApprovals Mapping of delegate addresses to their approval status. | |
| /// @param remainingApprovalsRequired Number of approvals still needed before voting. | |
| struct ProposalData { | |
| address proposer; | |
| address[] targets; | |
| uint256[] values; | |
| bytes[] calldatas; | |
| string description; | |
| ProposalType proposalType; | |
| bool inVoting; | |
| mapping(address => bool) delegateApprovals; | |
| uint256 remainingApprovalsRequired; | |
| } |
| /// @param _calldatas Function data for proposal calls. | ||
| /// @param _proposalType Type of the proposal. | ||
| /// @param _attestationUid The UID of the attestation proving eligibility. | ||
| function _validateProposal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this name and description is not that accurate, maybe _validateAttestation is better, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to make all validation there in the future, that's the reason why targets, values, and calldatas are provided but not used yet
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]>
Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]>
0xOneTony
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
* feat: add initial interface and logic * refactor: remove installed governor submodule * chore: remove xERC20 * feat: add proposal routing full flow * feat: check voting power and required proposals * refactor: rename to ProposalValidator * feat: add EAS validation for certain Proposal Types * chore: fix attestation schema approved address naming Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * chore: remove management functions * chore: run pre-pr * refacto: follow style guide for function parameters and return variables * docs: add natspec, remove unused errors * chore: remove management functions from interface * chore: make voting token immutable * perf: make governor immutable * feat: add validator management functions * chore: add comments for imports in ProposalValidator * test: add unit tests * fix: semgrep warnings * chore: rename MaintenanceUpgradeProposals --> MaintenanceUpgrade * chore(semgrep): add excluded governance files * chore: fix coding style * chore: add ImmutableProposalTypeData * chore: improve errors naming * docs: improve natspec Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * docs: add technical explanation on attestation validation function * feat: add _proposalTypeData mapping * chore: keep private functions consistency * chore: improve required attestation naming * docs: improve documents legibility Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * chore: fix immutable variables coding style * chore: explain governor external call * docs: explicit Validator/Governor contract interaction in events natspec --------- Signed-off-by: Chiin <[email protected]> Co-authored-by: 0xOneTony <[email protected]>
* feat: add initial interface and logic * refactor: remove installed governor submodule * chore: remove xERC20 * feat: add proposal routing full flow * feat: check voting power and required proposals * refactor: rename to ProposalValidator * feat: add EAS validation for certain Proposal Types * chore: fix attestation schema approved address naming Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * chore: remove management functions * chore: run pre-pr * refacto: follow style guide for function parameters and return variables * docs: add natspec, remove unused errors * chore: remove management functions from interface * chore: make voting token immutable * perf: make governor immutable * feat: add validator management functions * chore: add comments for imports in ProposalValidator * test: add unit tests * fix: semgrep warnings * chore: rename MaintenanceUpgradeProposals --> MaintenanceUpgrade * chore(semgrep): add excluded governance files * chore: fix coding style * chore: add ImmutableProposalTypeData * chore: improve errors naming * docs: improve natspec Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * docs: add technical explanation on attestation validation function * feat: add _proposalTypeData mapping * chore: keep private functions consistency * chore: improve required attestation naming * docs: improve documents legibility Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * chore: fix immutable variables coding style * chore: explain governor external call * docs: explicit Validator/Governor contract interaction in events natspec --------- Signed-off-by: Chiin <[email protected]> Co-authored-by: 0xOneTony <[email protected]>
Closes OPT-757, OPT-758