feat: add submit upgrade proposal #429
Merged
Conversation
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
0xOneTony
requested changes
Jun 26, 2025
| proposalDescription = "Protocol Upgrade Proposal"; | ||
| } | ||
|
|
||
| function testFuzz_submitUpgradeProposal_succeeds( |
Member
There was a problem hiding this comment.
I think the logic of this function gets too complex for a unit tests, lets have 2 different tests 1 for maintenance upgrade and 1 for protocol or governor upgrade
Member
Author
There was a problem hiding this comment.
| function testFuzz_submitUpgradeProposal_invalidProposalType_reverts(uint8 proposalTypeValue) public { | ||
| // Bound to proposal types that are NOT upgrade proposals (2, 3, 4) | ||
| // Valid upgrade proposal types are ProtocolOrGovernorUpgrade (0) and MaintenanceUpgrade (1) | ||
| proposalTypeValue = uint8(bound(proposalTypeValue, 2, 4)); |
Member
There was a problem hiding this comment.
to cover more cases I would change this to assume != 0 && != 1
Member
Author
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
fix: broken tests out of enum bounds had to change it bc tests were breaking when going out of bounds. will limit to test all the proposal types that are not the valid ones
Member
There was a problem hiding this comment.
can you explain a bit more why they fail? if we keep it 2-4 then we dont tests the case where type is >4 which is also an invalid proposal type
| _validateAttestation(_attestationUid, _proposalType); | ||
|
|
||
| // Validate againstThreshold is non-zero and within bounds for percentage-based thresholds | ||
| if (_againstThreshold == 0 || _againstThreshold > 10000) { |
Member
There was a problem hiding this comment.
would be good to explain a bit what the 10000 represents in a comment
Member
Author
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
why is this not fixed in the current PR?
Member
Author
There was a problem hiding this comment.
packages/contracts-bedrock/src/governance/ProposalValidator.sol
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0xOneTony
requested changes
Jun 30, 2025
| contract ProposalValidator_SubmitUpgradeProposal_Test is ProposalValidator_Init { | ||
| string proposalDescription; | ||
|
|
||
| event ProposalVotingModuleData(bytes32 indexed proposalHash, bytes encodedVotingModuleData); |
Member
There was a problem hiding this comment.
We should declare this in the init with the other events
Member
Author
There was a problem hiding this comment.
| function testFuzz_submitUpgradeProposal_invalidProposalType_reverts(uint8 proposalTypeValue) public { | ||
| // Bound to proposal types that are NOT upgrade proposals (2, 3, 4) | ||
| // Valid upgrade proposal types are ProtocolOrGovernorUpgrade (0) and MaintenanceUpgrade (1) | ||
| proposalTypeValue = uint8(bound(proposalTypeValue, 2, 4)); |
Member
There was a problem hiding this comment.
can you explain a bit more why they fail? if we keep it 2-4 then we dont tests the case where type is >4 which is also an invalid proposal type
0xOneTony
approved these changes
Jun 30, 2025
0xOneTony
added a commit
that referenced
this pull request
Jul 29, 2025
* feat: add submitFundingProposal function * fix: compiler errors * test: add submitFundingProposal tests * chore: run pre-pr * chore: add comments for submitFundingProposal * docs: improve natspec * feat: add validation for options length in ProposalValidator * feat: get votingModuleAddress from ProposalTypeConfigurator * feat: check for proposal existance on submission * test: add InvalidOptionsLength tests * chore: run pre-pr * feat: add submitCouncilMemberElectionsProposal function * chore: run pre-pr * fix: remove duplicated tests * test(fuzz): use fuzz testing for happy paths * feat: add check for criteria value < optionslength * perf: optimiza for loops usage * test: fuzz invalid attestationUid * test: fuzz invalid proposer * fix: lack of attestation existance validation * test: fuzz exceeded max options test * test: fuzz unapproved attester * test: reduce upper bound for array size * test: remove duplicated test * fix: update criteria value validation logic in ProposalValidator * refactor(test): use helper functions for duplicated logic * refactor: declare approvalVotingModule variable * test: increment the assertions in council memeber election tests * refactor(test): improve tests legibility * test: use fuzzing instead of individual tests for edge cases * test: improve submitFundingProposal assertions * test: fuzz proposer * chore(test): remove unused setup variables * test: use fuzzing for sad submitFundingProposal paths * chore: remove redundant tests * test: use fuzzing for exceeded amount * chore: run pre-pr * refactor: normalize funding proposal voting modules in global variable * test: fuzz proposal types for revert cases * refactor: use minimal values for funding proposal revert cases tests * refactor: use more descriptive variables for testing * chore: run pre-pr * refactor: use variables instead of hardcoded values * chore: rename voting module depending on configurator instead of internal proposal type * chore: improve tests naming * test: expect proposal type configurator calls * feat: add submitUpgradeProposal function * refactor: improve variable names consistency * test: add submitUpgradeProposal tests * chore: run pre-pr * refactpr(test): separate submitUpgradePropowal tests depending on proposal type * test: improve coverage on InvalidProposalType tests * chore: improve code legibility * chore: improve comments Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * fix: merge conflicts * fix: broken compile for missing variable * fix: broken tests out of enum bounds * fix: pre-pr * fix: correct order for event emission * refactor(test): declare events in Init contract * refactor: use constants for code legibility --------- Signed-off-by: Chiin <[email protected]> Co-authored-by: 0xOneTony <[email protected]>
0xOneTony
added a commit
that referenced
this pull request
Jul 29, 2025
* feat: add submitFundingProposal function * fix: compiler errors * test: add submitFundingProposal tests * chore: run pre-pr * chore: add comments for submitFundingProposal * docs: improve natspec * feat: add validation for options length in ProposalValidator * feat: get votingModuleAddress from ProposalTypeConfigurator * feat: check for proposal existance on submission * test: add InvalidOptionsLength tests * chore: run pre-pr * feat: add submitCouncilMemberElectionsProposal function * chore: run pre-pr * fix: remove duplicated tests * test(fuzz): use fuzz testing for happy paths * feat: add check for criteria value < optionslength * perf: optimiza for loops usage * test: fuzz invalid attestationUid * test: fuzz invalid proposer * fix: lack of attestation existance validation * test: fuzz exceeded max options test * test: fuzz unapproved attester * test: reduce upper bound for array size * test: remove duplicated test * fix: update criteria value validation logic in ProposalValidator * refactor(test): use helper functions for duplicated logic * refactor: declare approvalVotingModule variable * test: increment the assertions in council memeber election tests * refactor(test): improve tests legibility * test: use fuzzing instead of individual tests for edge cases * test: improve submitFundingProposal assertions * test: fuzz proposer * chore(test): remove unused setup variables * test: use fuzzing for sad submitFundingProposal paths * chore: remove redundant tests * test: use fuzzing for exceeded amount * chore: run pre-pr * refactor: normalize funding proposal voting modules in global variable * test: fuzz proposal types for revert cases * refactor: use minimal values for funding proposal revert cases tests * refactor: use more descriptive variables for testing * chore: run pre-pr * refactor: use variables instead of hardcoded values * chore: rename voting module depending on configurator instead of internal proposal type * chore: improve tests naming * test: expect proposal type configurator calls * feat: add submitUpgradeProposal function * refactor: improve variable names consistency * test: add submitUpgradeProposal tests * chore: run pre-pr * refactpr(test): separate submitUpgradePropowal tests depending on proposal type * test: improve coverage on InvalidProposalType tests * chore: improve code legibility * chore: improve comments Co-authored-by: 0xOneTony <[email protected]> Signed-off-by: Chiin <[email protected]> * fix: merge conflicts * fix: broken compile for missing variable * fix: broken tests out of enum bounds * fix: pre-pr * fix: correct order for event emission * refactor(test): declare events in Init contract * refactor: use constants for code legibility --------- Signed-off-by: Chiin <[email protected]> Co-authored-by: 0xOneTony <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes OPT-826, OPT-827, OPT-880