5.0 Architecture Thread - Fix Governance Interface Architecture

[RitzyDevBox]

Open Zeppelins Interface makes assumptions about its implentation.

abstract contract IGovernorTimelock is IGovernor {
    event ProposalQueued(uint256 proposalId, uint256 eta);

    function timelock() public view virtual returns (address);

    function proposalEta(uint256 proposalId) public view virtual returns (uint256);

    function queue(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) public virtual returns (uint256 proposalId);
}

`
Queue and Execute don't take the proposalId in as a parameter, In OZ Governor, thats fine because the members hash to the Proposal. However this means that anyone consuming the interface has to use the same proposal.

This is a huge limitation as in other architectures such as GovernorA GovernorB the Proposals are sequential so the interfaces are not compatible for inheritance

For Deeper clarification look at the Execute Method:

/**
 * @dev See {IGovernor-execute}.
 */
function execute(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    bytes32 descriptionHash
) public payable virtual override returns (uint256) {
    uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);

    ProposalState status = state(proposalId);
    require(
        status == ProposalState.Succeeded || status == ProposalState.Queued,
        "Governor: proposal not successful"
    );
    _proposals[proposalId].executed = true;

    emit ProposalExecuted(proposalId);

    _beforeExecute(proposalId, targets, values, calldatas, descriptionHash);
    _execute(proposalId, targets, values, calldatas, descriptionHash);
    _afterExecute(proposalId, targets, values, calldatas, descriptionHash);

    return proposalId;
}

I would like to inherit from this method in my implemntation, However since my proposalId is not based on your implemtation my code will due to the following lines:

    uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);

[RitzyDevBox]

I'll leave a comment on the full architecture propose for 5.0

1. Change the IGovernor Interface to take in the proposalId.

    abstract contract IGovernor {  
     // Adjusted to take in params as discussed in previous tickets
        function propose(
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description,
            bytes memory params
        ) public virtual returns (uint256 proposalId);
   
    /** handles taking care of creating the proposal. 
     *       in the OZ Compatibility:
     *           this will just hash the content and add coreProposal
     *       in the GovernorA Compatibility:
     *            it will handle enumerating the proposal Id
     *            this will store the additional required information in ExtendedCoreProposals
    // 
    function _createProposal(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description,
        bytes memory params
    ) public internal returns (uint256 proposalId);
   
    function execute(
        uint256 proposalId,
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) public payable virtual returns (uint256);    
   

   }

and in the Timelock:

    abstract contract IGovernorTimelock is IGovernor {

        function queue(
            uint256 proposalId,
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            bytes32 descriptionHash
        ) public virtual returns (uint256);
    }

3. Creation of a new Module which handles the default upgrade from 4 -> 5.0 for the example I will call this IOZGovernorCompatibilityGovernor :

   abstract contract IOZGovernorCompatibilityGovernor is IERC165, IGovernor {

    function hashProposal(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) public pure virtual returns (uint256);
   
    function _defaultProposeParams() internal view virtual returns (bytes memory) {
      return "";
    }
   
   
    /**
    * @dev See {IGovernor-execute}.
    */
    function execute(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) public payable virtual override returns (uint256) {
        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
        return super.execute(proposalId, targets, values, calldatas, descriptionHash);
    }
   

   }

The following would also occur in the Time lock.

[RitzyDevBox]

There are other things that should probably be changed as well such as:

function votingPeriod() public view virtual returns (uint256);
function votingDelay() public view virtual returns (uint256);

and pretty much every other function should be scoped to the proposal or have some identifier. Currently the Voting Period, and delay are all tied down to the governor, Ideally this architecture should be flexible enough to handle different types of votes.

e.g For instance some operations should be scoped to only certain members within the organization,

Image someone wanted to create emergency operations and only allow trusted interal members to handle proposing or voting on paticular internal things.

Currently the Time Delays, and voting periods get votes etc, are tied into the governor, when really that is something that should be more dynamic. Emergency votes may want shorter delays etc for critical operations.

[frangio]

function execute(
     uint256 proposalId,
     address[] memory targets,
     uint256[] memory values,
     bytes[] memory calldatas,
     bytes32 descriptionHash
 ) public payable virtual returns (uint256);

How do you ensure that the parameters match with what was included in the original call to propose? If you do that by hashing, you don't need the proposalId there. If you do that through data in storage, this is what the GovernorCompatibilityBravo does but it offers an alternative interface execute(uint256 proposalId) because it's not necessary to pass the remaining parameters,
they are implicit.

---

Currently the Time Delays, and voting periods get votes etc, are tied into the governor, when really that is something that should be more dynamic. Emergency votes may want shorter delays etc for critical operations.

Conceptually this makes sense, but I don't agree with the proposed implementation.

The Governor will have permissions assigned to it, over treasury, over upgrades, etc. The parameters are tied to those permissions, e.g.: "the system can be upgraded with a voting period of 14 days and 40% quorum". I should not be able to choose the parameters when I create a proposal, they should be determined by what the proposal does, and what permissions it requires. So I think when it comes to having different kinds of proposals that require different parameters, that should be implemented by having different Governor instances, which have different permissions assigned to them.

[RitzyDevBox]

The original interface is designed to be backwards compatible with your design. So in the OZ case it would verify that the hast matches the hash of the parameters passed in. In compound governance style they would expose a method from the parent which called the base. And the proposalId would look up the stored values on chain. If someone tries to call the explicit endpoint it can either validate the parameters match the internal or fail ans enforce you only call the endpoint which exposes the singular proposal I'd. As for the second question, I've abstracted a vote type and I use the params added to the proposal to handle processing that vote type. Based off that vote type controls quorum, threshold and delays. Separating governor's is a not an good choice, as potentially multiple groups can manage the same resource but have differently role levels etc. Ownership should be long to the same governor, while permissions to modify certain parameters are left to the validations of access to a particular call.

…

On Tue, Jun 27, 2023, 10:23 PM Francisco ***@***.***> wrote: function execute( uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) public payable virtual returns (uint256); How do you ensure that the parameters match with what was included in the original call to propose? If you do that by hashing, you don't need the proposalId there. If you do that through data in storage, this is what the GovernorCompatibilityBravo does but it offers an alternative interface execute(uint256 proposalId) because it's not necessary to pass the remaining parameters, they are implicit. ------------------------------ Currently the Time Delays, and voting periods get votes etc, are tied into the governor, when really that is something that should be more dynamic. Emergency votes may want shorter delays etc for critical operations. Conceptually this makes sense, but I don't agree with the proposed implementation. The Governor will have permissions assigned to it, over treasury, over upgrades, etc. The parameters are tied to those permissions, e.g.: "the system can be upgraded with a voting period of 14 days and 40% quorum". I should not be able to choose the parameters when I create a proposal, they should be determined by what the proposal does, and what permissions it requires. So I think when it comes to having different kinds of proposals that require different parameters, that should be implemented by having different Governor instances, which have different permissions assigned to them. — Reply to this email directly, view it on GitHub <#4383 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYR4EL6B2L2OBE2TX3OOB4DXNOIQPANCNFSM6AAAAAAZQ7NOT4> . You are receiving this because you authored the thread.Message ID: ***@***.***>