Skip to content
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

Missing GovernorBravo compatibility #3877

Closed
RitzyDevBox opened this issue Dec 15, 2022 · 16 comments · Fixed by #4090
Closed

Missing GovernorBravo compatibility #3877

RitzyDevBox opened this issue Dec 15, 2022 · 16 comments · Fixed by #4090
Labels
area: governance
Milestone
4.9

Comments

@RitzyDevBox
Copy link

The GovernorCompatibilityBravo.sol contract seems to be missing alot of the the public interface members for bravo

such as:

/// @notice The latest proposal for each proposer
mapping (address => uint) public latestProposalIds;
@frangio
Copy link
Contributor

frangio commented Dec 21, 2022 •
edited
Loading

Thanks for pointing that out. We may have missed some public getters.

Are you using GovernorCompatibilityBravo? Would appreciate if you could chime in on #3833.

@frangio frangio added this to the 4.9 milestone Dec 21, 2022
@frangio frangio changed the title Proper Bravo Compatibility Missing GovernorBravo compatibility Dec 21, 2022
@FriskyHamTitz
Copy link

FriskyHamTitz commented Dec 22, 2022 •
edited
Loading

Yeah I use bravo compatibility, Do you have full bravo compatibility implementation anywhere?

For my particular case I was trying to create governance, but I didn't want to recreate the wheel so I reused uniswaps interface.

I noticed the compatibility doesn't really work. I needed to add a few properties and add an enumerator, since there's no way to determine the proposalid (other than using the graph or enumerating the blocks). I believe in the original proposalids we're sequential so you can predict them using the proposalCount

@frangio
Copy link
Contributor

frangio commented Dec 22, 2022

If I understand your comment correctly, you used Uniswap's interface connected to an instance of GovernorCompatibilityBravo, and it didn't work correctly. You've already said a few things but I'd appreciate if you can provide as much detail as possible.

So for example latestProposalIds is missing. What other properties did you need to add? And what enumerator?

Is the UI broken because proposal ids are not sequential?

This is very valuable so we can evaluate getting to 100% compatibility (sorry this wasn't caught earlier).

@RitzyDevBox
Copy link
Author

RitzyDevBox commented Dec 26, 2022 •
edited
Loading

Yeah the UI will break, because they use the proposalCount, to enumerate the previous proposals. In all fairness, the uniswap code there is kinda hacky, but I didn't feel like rewriting their UI so I just wrote more compatability for it and modified the UI in the slightest way possible.

latestPropsalIds was missing too, but TBH they were using for that, because they did it to prevent people from creating multiple active proposals or something but I didn't really care about it too much so I deleted their code

Here is the contract for reference:

`pragma solidity ^0.8.0;

import "@openzeppelin/contracts/governance/compatibility/GovernorCompatibilityBravo.sol";

abstract contract GovenorEnumerationCompatibilityBravo is GovernorCompatibilityBravo {

uint256 public proposalCount = 0;
mapping(uint256 => uint256) private _proposalIndexMap;

function getProposalIdFromIndex(uint256 index) public view returns (uint256) {
    return _proposalIndexMap[index];
}

// ============================================== Proposal lifecycle ==============================================
/**
 * @dev See {IGovernor-propose}.
 */
function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description
) public virtual override(GovernorCompatibilityBravo) returns (uint256) {
    return super.propose(targets, values, calldatas, description);
}

/**
 * @dev See {IGovernorCompatibilityBravo-propose}.
 */
function propose(
    address[] memory targets,
    uint256[] memory values,
    string[] memory signatures,
    bytes[] memory calldatas,
    string memory description
) public virtual override returns (uint256) {
    uint256 proposalId = super.propose(targets, values, signatures, calldatas, description);
    _storeProposalIndex(proposalId);
    return proposalId;
}


/**
 * @dev Store proposal metadata for later lookup
 */
function _storeProposalIndex(uint256 proposalId) private {
    _proposalIndexMap[proposalCount] = proposalId;
    proposalCount++;
}

}
`

@RitzyDevBox
Copy link
Author

I ignored proposals coming through the 4 parameter, which may be a bug.

I was debating on just using a closure around the propose function to store the original count e.g

uin256 originalCount = proposalCount;
propose...
proposalCount = originalCount+1.

This would ensure that the storage was correct but I'm still skeptical on whether the hashes would differ. I'm not in the production phase yet so that contract is just a rough workaround

@Amxx
Copy link
Collaborator

Amxx commented Jan 18, 2023 •
edited
Loading

Our governor inherits part of its interface (in particular the events and the voting ABI) from Compound's governor. Still, both have significant differences. The most significant is in the storage and representation of proposals:

  • Governor Bravo stores everything onchain, and numbers the proposal that are submitted sequentially. This is really expensive!
  • OZ's Governor does NOT store the proposal details by default. We publish them through event, which is enough for offchain indexing. A consequence of this design is that proposal need to be identified differently. Therefore, our proposal numbering system is not sequential but relies on the hash of the proposal.

The GovernorCompatibilityBravo module is here to provide some backward compatibility by storing the proposal details onchain, which can then be queried using the getActions(uint256) getter. This helps with some usecases, but the proposal numbering system remains hash-based.

Sequentiality of proposal in Compound's system allows for simpler (event-less) indexing of the proposal. You just query how much proposal there are, and you can then lookup the proposals, from 0 to N-1. I expect this is what uniswap's interface does.

Changing the governor to use sequential numbering, and thus achieve 100% compatibility, feels like a very difficult thing to do in a extension module. building an extension means that we continue supporting the "core" interface, which itself requires the hash-based proposal numbering.

We may have to reconsider the amount of compatibility we want to achieve. Event, and voting interface, compatibility is really a blessing, but we also inherited some poor design choice, made by compound years ago in a very different context. We hope that this debt, combined with the compatibility layer, would make everyone's work (expect ours) significantly easier... but if it turn out that this is not the case, maybe we should dial back and remove some (costly) extensions that don't achieve the expected goal.

Note: Similarly, compound includes rules about the each proposer's latestProposalIds and enforces a maximum of one live proposal per proposer (which we don't do, even in the compatibility module). Is this something we should have ?

@frangio
Copy link
Contributor

frangio commented Jan 19, 2023

Here is the full GovernorBravo interface:

pragma solidity ^0.5.16;
pragma experimental ABIEncoderV2;

interface GovernorBravo {
    struct Receipt {
        bool hasVoted;
        uint8 support;
        uint96 votes;
    }

    event NewAdmin(address oldAdmin, address newAdmin);
    event NewImplementation(
        address oldImplementation,
        address newImplementation
    );
    event NewPendingAdmin(address oldPendingAdmin, address newPendingAdmin);
    event ProposalCanceled(uint256 id);
    event ProposalCreated(
        uint256 id,
        address proposer,
        address[] targets,
        uint256[] values,
        string[] signatures,
        bytes[] calldatas,
        uint256 startBlock,
        uint256 endBlock,
        string description
    );
    event ProposalExecuted(uint256 id);
    event ProposalQueued(uint256 id, uint256 eta);
    event ProposalThresholdSet(
        uint256 oldProposalThreshold,
        uint256 newProposalThreshold
    );
    event VoteCast(
        address indexed voter,
        uint256 proposalId,
        uint8 support,
        uint256 votes,
        string reason
    );
    event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay);
    event VotingPeriodSet(uint256 oldVotingPeriod, uint256 newVotingPeriod);
    event WhitelistAccountExpirationSet(address account, uint256 expiration);
    event WhitelistGuardianSet(address oldGuardian, address newGuardian);

    function BALLOT_TYPEHASH() external view returns (bytes32);

    function DOMAIN_TYPEHASH() external view returns (bytes32);

    function MAX_PROPOSAL_THRESHOLD() external view returns (uint256);

    function MAX_VOTING_DELAY() external view returns (uint256);

    function MAX_VOTING_PERIOD() external view returns (uint256);

    function MIN_PROPOSAL_THRESHOLD() external view returns (uint256);

    function MIN_VOTING_DELAY() external view returns (uint256);

    function MIN_VOTING_PERIOD() external view returns (uint256);

    function _acceptAdmin() external;

    function _initiate(address governorAlpha) external;

    function _setPendingAdmin(address newPendingAdmin) external;

    function _setProposalThreshold(uint256 newProposalThreshold) external;

    function _setVotingDelay(uint256 newVotingDelay) external;

    function _setVotingPeriod(uint256 newVotingPeriod) external;

    function _setWhitelistAccountExpiration(address account, uint256 expiration)
        external;

    function _setWhitelistGuardian(address account) external;

    function admin() external view returns (address);

    function cancel(uint256 proposalId) external;

    function castVote(uint256 proposalId, uint8 support) external;

    function castVoteBySig(
        uint256 proposalId,
        uint8 support,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external;

    function castVoteWithReason(
        uint256 proposalId,
        uint8 support,
        string memory reason
    ) external;

    function comp() external view returns (address);

    function execute(uint256 proposalId) external;

    function getActions(uint256 proposalId)
        external
        view
        returns (
            address[] memory targets,
            uint256[] memory values,
            string[] memory signatures,
            bytes[] memory calldatas
        );

    function getReceipt(uint256 proposalId, address voter)
        external
        view
        returns (Receipt memory);

    function implementation() external view returns (address);

    function initialProposalId() external view returns (uint256);

    function initialize(
        address timelock_,
        address comp_,
        uint256 votingPeriod_,
        uint256 votingDelay_,
        uint256 proposalThreshold_
    ) external;

    function isWhitelisted(address account) external view returns (bool);

    function latestProposalIds(address) external view returns (uint256);

    function name() external view returns (string memory);

    function pendingAdmin() external view returns (address);

    function proposalCount() external view returns (uint256);

    function proposalMaxOperations() external view returns (uint256);

    function proposalThreshold() external view returns (uint256);

    function proposals(uint256)
        external
        view
        returns (
            uint256 id,
            address proposer,
            uint256 eta,
            uint256 startBlock,
            uint256 endBlock,
            uint256 forVotes,
            uint256 againstVotes,
            uint256 abstainVotes,
            bool canceled,
            bool executed
        );

    function propose(
        address[] memory targets,
        uint256[] memory values,
        string[] memory signatures,
        bytes[] memory calldatas,
        string memory description
    ) external returns (uint256);

    function queue(uint256 proposalId) external;

    function quorumVotes() external view returns (uint256);

    function state(uint256 proposalId) external view returns (uint8);

    function timelock() external view returns (address);

    function votingDelay() external view returns (uint256);

    function votingPeriod() external view returns (uint256);

    function whitelistAccountExpirations(address)
        external
        view
        returns (uint256);

    function whitelistGuardian() external view returns (address);
}

I don't think we want to support all of these, and like @Amxx says above in some cases it is not possible due to implementation differences. We should definitely be advertising better that compatibility is limited and what things are not supported.

@FriskyHamTitz
Copy link

FriskyHamTitz commented Jan 20, 2023 via email

@Amxx
Copy link
Collaborator

Amxx commented Jan 23, 2023

That would not be compatible though, because the proposalId given by the events is the hash one.

So when you have a proposalId, depending on if it is an index, of an actual hash based proposal id, you would have to workflows.

Also, if you get a count, take the last index, and start listening for the corresponding events you would get nothing.

@RitzyDevBox
Copy link
Author

RitzyDevBox commented Jan 23, 2023 •
edited
Loading

That would not be compatible though, because the proposalId given by the events is the hash one.

So when you have a proposalId, depending on if it is an index, of an actual hash based proposal id, you would have to workflows.

Also, if you get a count, take the last index, and start listening for the corresponding events you would get nothing.

My sugguestion for full compatibility is that, You override all the functions, e.g propose, execute, queue, to look up the HashId from the CountId passed in, and pass that to the base.

But my suggestion for "hybrid" I really meant "partial", with just the mapping you can easily enumerate without needing the graph.

@Amxx
Copy link
Collaborator

Amxx commented Jan 24, 2023

My sugguestion for full compatibility is that, You override all the functions, e.g propose, execute, queue, to look up the HashId from the CountId passed in, and pass that to the base.

The events are emitted by the core, using proposalId that are hashed by the core. I'm not sure how we would make sure the core doesn't use the proposalId it produces and use sequential ids instead.

It feels like the difference is so big that we would need a different codebase altogether

classDiagram
    class IGovernor{
        +function something() external virtual
    }
    class AbstractGovernor {
        +function something() public virtual override
    }
    class GovernorCore {
        +function something() public virtual override ...
    }
    class GovernorCompatibilityCore {
        +function something() public virtual override ...
    }


    IGovernor --|> AbstractGovernor
    AbstractGovernor --|> GovernorCore
    AbstractGovernor --|> GovernorCompatibilityCore
    AbstractGovernor --|> GovernorModuleXxx

    GovernorCore --> GovernorModuleXxx : possible composition
    GovernorCompatibilityCore --> GovernorModuleXxx : possible composition
Loading

I'm not sure would be a lot of development and maintenance, and also a lot of confuction for the users. Considering the compatibility mode is way more expensive to use (because of all the sload/sstore) I'm not sure we want to encourage that ... and then have users complain its expensive

@RitzyDevBox
Copy link
Author

I see your point, To be honest a separate base altogether may make sense.
The only downfalls there would be.

 1. More maintenance managing multiple contracts
 2. Updating the wizard so that it

Personally, I just added a BravoEnumeration to my project and added as a layer on top of the existing compatibility, Even though it doesn't follow the Bravo Spec, I'm able to enumerate just the same I just need to modify the front end interface. But the only issue is that if a contract is expecting a bravo interface it won't work.

I just finished writing a Multiple choice voting Governor, not sure what the process is but I would love to become a contributor here.

@Amxx
Copy link
Collaborator

Amxx commented Jan 25, 2023

I just finished writing a Multiple choice voting Governor, not sure what the process is but I would love to become a contributor here.

The process is documented here here

@Amxx
Copy link
Collaborator

Amxx commented Jan 25, 2023

Also note that we had the project of having a repository of "unofficial contracts" that would include things like governor modules. We are not yet sure how it would be maintained, but that is something we should focus some time on (after 5.0?)

@RitzyDevBox
Copy link
Author

RitzyDevBox commented Jan 25, 2023 via email

@ernestognw
Copy link
Member

Hey @RitzyDevBox, we've been reviewing this, and here are the missing functions:

function DOMAIN_TYPEHASH() external view returns (bytes32);
function MAX_PROPOSAL_THRESHOLD() external view returns (uint256);
function MAX_VOTING_DELAY() external view returns (uint256);
function MAX_VOTING_PERIOD() external view returns (uint256);
function MIN_PROPOSAL_THRESHOLD() external view returns (uint256);
function MIN_VOTING_DELAY() external view returns (uint256);
function MIN_VOTING_PERIOD() external view returns (uint256);
function _acceptAdmin() external;
function _setPendingAdmin(address newPendingAdmin) external;
function admin() external view returns (address);
function pendingAdmin() external view returns (address);
function _initiate(address governorAlpha) external; 
function _setProposalThreshold(uint256 newProposalThreshold) external;
function _setVotingDelay(uint256 newVotingDelay) external;
function _setVotingPeriod(uint256 newVotingPeriod) external;
function _setWhitelistAccountExpiration(address account, uint256 expiration) external;
function _setWhitelistGuardian(address account) external;
function isWhitelisted(address account) external view returns (bool);
function whitelistAccountExpirations(address) external view returns (uint256);
function whitelistGuardian() external view returns (address);
function comp() external view returns (address);
function initialProposalId() external view returns (uint256);
function latestProposalIds(address) external view returns (uint256);
function proposalCount() external view returns (uint256);
function proposalMaxOperations() external view returns (uint256);

// EIP-1967 and upgrades
function implementation() external view returns (address);
function initialize(
    address timelock_,
    address comp_,
    uint256 votingPeriod_,
    uint256 votingDelay_,
    uint256 proposalThreshold_
) external;

So far, we think not all of these functions need to be there, and most of them would be filled with placeholder values, take as a reference comp or whitelistGuardian, in which the former should return the address of an underlying token (which may not be ERC20, though), and there's no whitelist mechanism.

Aside from that, some of them can be implemented easily but are not critical (such as DOMAIN_TYPEHASH).

Regarding your use case, even if we add the missing proposalCount, it won't work for enumerability.
We'll document the limitations of the compatibility, including inner differences (like how proposalId works) though we'd like to hear your thoughts if you have reasons to support the addition of any of these functions.

@ernestognw ernestognw mentioned this issue Mar 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: governance
Projects
None yet
Milestone
4.9
Development

Successfully merging a pull request may close this issue.

Clarify Governor Bravo compatibility scope ernestognw/openzeppelin-contracts
6 participants
@frangio @Amxx @ernestognw @FriskyHamTitz @RitzyDevBox and others