OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.75k stars 11.76k forks source link

Add dynamic options to governance proposals #4005

Open RitzyDevBox opened 1 year ago

RitzyDevBox commented 1 year ago

I've been working on a multi-governor, I've had to pretty much duplicate the entire interface due to one small parameter change I need an additional parameter to accept data during the propose, currently i've hardcoded this

function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description,
    VoteChoices[] memory voteChoices
)

But if the new version were to accept a blob of bytes, the interface would be more versatile a it can include things that is related to the proposal and but not specifically for execution

function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description,
    bytes memory additionalData
)
Amxx commented 1 year ago

Hello @RitzyDevBox

Changing the propose interface is a breaking change that we want to avoid.


You could use a construction similar to GovernorCompatibilityBravo to add a second interface on top of the existing one. It could look like:

abstract contract MyGovernorModule is Governor {
    function propose(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) public virtual override(IGovernor, Governor) returns (uint256) {
        revert("disabled, please use the other one");
    } 

    function propose(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description,
        bytes memory additionalData
    ) public virtual returns (uint256) {
        uint256 proposalId = super.propose(targets, values, calldatas, description);
        // Do something with the additionalData
        return proposalId;
    }
}
RitzyDevBox commented 1 year ago

Yeah I know that would be a big breaking change. I have it working as you proposed currently. Openzepplin interfaces are widely used, and it would save a lot of audit time and money if all of this functionality was built into the interface.

I would almost take it a further notch and day wouldn't it make sense to include an additional bytes param in almost every method?

There shouldn't be much of an additional cost if by default the parameter is not used, the only down side is that it is taking away from the max bytes that can be placed in a contract.

frangio commented 1 year ago

include an additional bytes param in almost every method

It isn't as simple as adding an additional bytes param. We need to give it a semantics. In the case of vote params, we have params in COUNTING_MODE to help with that.

We're not going to add a bytes param without a really good idea of what it can be used for and how important those use cases are. Feel free to share what you're building for multiple choice.

RitzyDevBox commented 1 year ago

The current governor is limited its tightly coupling the behavior to the proposals and does not allow dynamic options.

There is a clear use case for the additional bytes parameter and that is to configure dynamic options related to the proposal. I will list a few use cases.

  1. Providing different choices for different proposal Proposal 1 Choices: A,B,C Proposal 2 Choices: D, E, F, G . This cannot be addressed by counting mode as the choices are dynamic with each proposal.

  2. Options can be used for declaring different proposal types.
    Standard, Emergency Vote, Multiple choice.

  3. Options which allow proposers with special roles to override default behavior such as the quorum, threshold.

frangio commented 1 year ago

Those are good examples! "Dynamic options" is a very clear category that is clearly not covered by the current design.

This cannot be addressed by counting mode as the choices are dynamic with each proposal.

Just to be clear, I didn't mean that counting mode addressed this, what I meant is that when we have a generic argument we need a way to communicate what kinds of values are accepted for that argument. If we add bytes data to proposals, the Govenor needs to specify for anyone interacting with it what values it is possible to pass as data and how they would be decoded and interpreted.


I think this is a potentially good idea, but we should set clear expectations: we are currently working on a 4.9 release which will not include this as it is a significant feature. After that we will work on a 5.0 release and I think we should consider this as a potential feature and if it requires breaking changes that would be a good moment to add it, but if it doesn't require breaking changes it would likely get pushed to a subsequent minor release.

RitzyDevBox commented 1 year ago

Thanks,

Yeah I def can wait for 5.0, I have a lot of refactoring to do anyway. I originally duplicated the entire design. But after a few of our conversations I realized it it all fits, that's the only thing that was outstanding.

On Fri, Feb 3, 2023, 7:59 PM Francisco @.***> wrote:

Those are good examples! "Dynamic options" is a very clear category that is clearly not covered by the current design.

This cannot be addressed by counting mode as the choices are dynamic with each proposal.

Just to be clear, I didn't mean that counting mode addressed this, what I meant is that when we have a generic argument we need a way to communicate what kinds of values are accepted for that argument. If we add bytes data to proposals, the Govenor needs to specify what values it is possible to pass as data and how they would be decoded and interpreted.

I think this is a potentially good idea, but we should set clear expectations: we are currently working on a 4.9 release which will not include this as it is a significant feature. After that we will work on a 5.0 release and I think we should consider this as a potential feature and if it requires breaking changes that would be a good moment to add it, but if it doesn't require breaking changes it would likely get pushed to a subsequent minor release.

— Reply to this email directly, view it on GitHub https://github.com/OpenZeppelin/openzeppelin-contracts/issues/4005#issuecomment-1416569381, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYR4EL3VRW7HBAMLVRHPAHLWVWSXRANCNFSM6AAAAAAUIDCLWM . You are receiving this because you were mentioned.Message ID: @.***>

RitzyDevBox commented 1 year ago

@frangio do you guys have a roadmap or eta for 5.0 release? I finished all my smart contract coding, and I'm on to the front-end but once I have the POC done, I'll be able to provide some help if you guys need assistance for the release.