Skip to content

feat: add submit funding proposal #411

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.

Sign up for GitHub

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

Jump to bottom
Merged
0xChin merged 22 commits into from
Jun 23, 2025
Merged

feat: add submit funding proposal #411

0xChin merged 22 commits into from
Jun 23, 2025

Conversation

@0xChin
Copy link
Member

@0xChin 0xChin commented Jun 9, 2025 •
edited
Loading

Closes OPT-785

🚨 Spec-Breaking Changes

This implementation includes two significant deviations from the original specification that require discussion and approval:

1. Changed proposalVotingModule Type: uint8 → address

Original Spec: ProposalTypeData.proposalVotingModule was defined as uint8
Implementation: Changed to address

Justification:

  • We are not integrating with the OptimismGovernor's proposal type system (which uses uint8 IDs)
  • Our governance flow is independent and uses its own proposal type management
  • Direct address reference eliminates unnecessary indirection and is more gas-efficient
  • Provides better type safety and clarity in our architecture

Impact: All structs, interfaces, and function signatures now use address instead of uint8

2. Interpreted criteriaValue as Absolute Token Count (Not Percentage)

Original Spec/NatSpec: Described criteriaValue as "percentage that will be used to calculate the fraction of the votable supply"
Implementation: Treats criteriaValue as absolute token count required for options to pass

Justification:

  • ApprovalVotingModule code clearly expects absolute token amounts (direct comparison: optionVotes[i] >= settings.criteriaValue)
  • No percentage conversion logic exists anywhere in the codebase
  • Voting weights are passed as raw token amounts, not percentages
  • Absolute token thresholds are more predictable and commonly used in governance systems
  • The original NatSpec appears to be misleading/incorrect based on actual implementation

Impact: Functions expect token amounts (e.g., 1000 ether) rather than percentage values

Note: These changes align the implementation with the actual ApprovalVotingModule contract behavior and our independent governance architecture.

@0xChin 0xChin requested a review from 0xOneTony June 9, 2025 20:16
@0xChin 0xChin self-assigned this Jun 9, 2025
@linear
Copy link

linear bot commented Jun 9, 2025

OPT-785 Add submitFundingProposal function

0xOneTony
0xOneTony requested changes Jun 10, 2025
View reviewed changes
Copy link
Member

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also still thinking about storing only the address instead of the voting module type. Afaik the address of the voting module can change for a specific type so this becomes an issue if we dont have also a function to change that address. I will get back to this.

0xOneTony
0xOneTony requested changes Jun 11, 2025
View reviewed changes
0xOneTony
0xOneTony requested changes Jun 16, 2025
View reviewed changes
Copy link
Member

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some names and tests that are the same lets check all of them to make sure we dont repeat code

0xOneTony
0xOneTony requested changes Jun 18, 2025
View reviewed changes
Copy link
Member

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests need some further improvement, also added a comment about the for loops on the logic side

packages/contracts-bedrock/test/governance/ProposalValidator.t.sol Outdated

// Expect ProposalSubmitted event
vm.expectEmit(address(validator));
emit ProposalSubmitted(expectedHash, rando, description, ProposalValidator.ProposalType.GovernanceFund);
Copy link
Member

@0xOneTony 0xOneTony Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if we fuzzed the proposer instead of using a "random" address

Copy link
Member Author

@0xChin 0xChin Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test: fuzz proposer

packages/contracts-bedrock/test/governance/ProposalValidator.t.sol Outdated
Comment on lines 895 to 917
vm.prank(owner);
validator.setProposalTypeData(
ProposalValidator.ProposalType.GovernanceFund,
ProposalValidator.ProposalTypeData({
requiredApprovals: PROPOSAL_REQUIRED_APPROVALS,
proposalVotingModule: 3
})
);

criteriaValue = 50;
optionsDescriptions = new string[](2);
optionsDescriptions[0] = "Option A";
optionsDescriptions[1] = "Option B";

optionsRecipients = new address[](2);
optionsRecipients[0] = makeAddr("recipient1");
optionsRecipients[1] = makeAddr("recipient2");

optionsAmounts = new uint256[](2);
optionsAmounts[0] = 1000 ether;
optionsAmounts[1] = 500 ether;

description = "Test funding proposal";
Copy link
Member

@0xOneTony 0xOneTony Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setup is repeated in ProposalValidator_SubmitFundingProposal_Test aswell, maybe defining the proposal type data is something that could be added in the Init contract since this will probably be needed to happen in most of the tests?

Copy link
Member Author

@0xChin 0xChin Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore(test): remove unused setup variables

Copy link
Member Author

@0xChin 0xChin Jun 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that these values will mostly be fuzzed and that the values can vary depending on the proposal that is being made, for example funding proposals require being given amount, while council member election proposal don't, I'm not sure having them in the Init is the way to go. wdyt?

Copy link
Member

@0xOneTony 0xOneTony Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with the usage of _setGovernanceFundProposalType its enough, the rest of the hardcoded values should be fuzzed where possible

@0xOneTony 0xOneTony mentioned this pull request Jun 19, 2025
Merged
0xOneTony
0xOneTony requested changes Jun 23, 2025
View reviewed changes
0xOneTony
0xOneTony approved these changes Jun 23, 2025
View reviewed changes
Copy link
Member

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! 🔥

@0xChin 0xChin merged commit bebd2e5 into sc-feat/permissionless-proposals Jun 23, 2025
2 checks passed
0xOneTony pushed a commit that referenced this pull request Jul 29, 2025
@0xChin @0xOneTony
feat: add submit funding proposal (#411)
1ade987 
* 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

* fix: remove duplicated tests

* perf: optimiza for loops usage

* 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
0xOneTony pushed a commit that referenced this pull request Jul 29, 2025
@0xChin @0xOneTony
feat: add submit funding proposal (#411)
6118380 
* 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

* fix: remove duplicated tests

* perf: optimiza for loops usage

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xOneTony 0xOneTony 0xOneTony approved these changes

Assignees

@0xChin 0xChin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@0xChin @0xOneTony