OpenZeppelin
Redundant storeProposal in GovernorCompatibilityBravo
In the Bravo Compatibility Contract the proposal data is duplicated with different proposalIds The initial Bravo Contract does not have a proposal method which takes 4 parameters, why do we require storage for that proposalHash?
` // ============================================== Proposal lifecycle ============================================== /** * @dev See {IGovernor-propose}. */ function propose( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, string memory description ) public virtual override(IGovernor, Governor) returns (uint256) { _storeProposal(_msgSender(), targets, values, new string, calldatas, description); 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) {
_storeProposal(_msgSender(), targets, values, signatures, calldatas, description);
return propose(targets, values, _encodeCalldata(signatures, calldatas), description);
}
`
Why are you storing the proposal data for the 4 parameter propose?
Queue and execute, only work with proposalIds that come from the 5 parameter method
Yeah basically, the store proposal function is called twice when creating a bravo contract
1. The 5 parameter propose function calls the 4 parameter one.
which results in 2 calls to _storeProposal, also I believe they are using different hashes, since they pass in different call datas since one is encoded.
_storeProposal(_msgSender(), targets, values, signatures, calldatas, description);
_storeProposal(_msgSender(), targets, values, new string, calldatas, description);
The hash will be different, so duplicate storage, also a waste of gas. I figured this might be so the bravo contract could execute proposals using queue(hashId), but I don't think this would work because queue/execute. only lookup the hashId using _storeProposal(_msgSender(), targets, values, signatures, calldatas, description); and not the encoded one.
Maybe by chance the encoded calldatas will hash to the same id, if thats the case, calling store proposal the second time is a waste of gas.
Right. There does seem to be a redundancy. The hashes will not be different, though. The two calls that are being done are:
_storeProposal(_msgSender(), targets, values, signatures, calldatas, description); // (1)
_storeProposal(_msgSender(), targets, values, new string[](calldatas.length), _encodeCalldata(signatures, calldatas), description); // (2)
_storeProposal generates the hash by:
hashProposal(targets, values, _encodeCalldata(signatures, calldatas), descriptionHash);
_encodeCalldata has the property that if signatures[i] is the empty string, calldatas[i] is passed through as is in the return value.
Note that the second call to _storeProposal uses new string[](calldatas.length), an array of empty strings. As a result, in both cases hashProposal is called with the same arguments.
It is still an inefficiency that we should remove. I'm wondering if it has any weird interactions with inheritance that we might have to consider.
Wont this result in a different hash since the encoded calldata will be different?
uint256 proposalId = hashProposal(targets, values, _encodeCalldata(signatures, calldatas), descriptionHash);
If someone attempts to call Queue, or Execute with a hash that was created by the 4 parameter propose it would fail no? I think by default all bravo implementations are calling the 5 parameter method, so everything should work fine.
I only found this bug because I was creating a proposalCount and extended the bravo compatibility to increment on proposals and the count started doubling.
With the current implementation of governance, it relatively difficult to obtain the previous proposals without 1. using a graph provide, which will lag ~10 minutes behind proposals 2. enumerating the block on the RPC since the launch of contract (this is what uni does which is also an ugly solution IMO)
It would be nice is there was a simple way to query the past proposals especially if their being stored with bravo compatibility
I believed we did miss super. in the compatibility version of propose
Its important to notice that _storeProposal checks that the data is not written yet before doing the write, so we were not storing twice, and only the first store was recorded (that is the one with the signatures). Still, missing super. causes some unecessary operation (mostly one big hash and one sload) that we can easily avoid.
It is still an inefficiency that we should remove. I'm wondering if it has any weird interactions with inheritance that we might have to consider.
I think this is why we have this "inefficiency"
We want modules that override the core propose to have the override triggered by both version of the function. This means we need the "compatibility" version to hook into the "core" version. If we simplify this workflow (as proposed in #3989) we would possibly have an external propose method that bypasses some overrides.
After further discussion in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3989#pullrequestreview-1270311079, we're giving this less priority because it only results in a 1% gas optimization, but are still considering options to make this better.
The reason there are two calls to storeProposal in the Bravo module is that the first one takes care of storing the signatures strings and the second one doesn't know about the signatures (they are converted to function selectors by then). We need both because both functions can be used as entry points, and we need the redundancy to avoid other issues with inheritance.
One way I proposed to improve this is to keep storeProposal only in propose without signatures, and separately store the signatures in propose with signatures as in:
uint256 proposalId = propose(targets, values, _encodeCalldata(signatures, calldatas), description);
_proposalSignatures[proposalId] = signatures;
If we do this, in getActions we need to trim off the function selectors from calldatas where signatures are available.
@frangio cant this just be fixed by calling the super instead?
function propose( address[] memory targets, uint256[] memory values, string[] memory signatures, bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { _storeProposal(_msgSender(), targets, values, signatures, calldatas, description); return propose(targets, values, _encodeCalldata(signatures, calldatas), description); }
this line:
return propose(targets, values, _encodeCalldata(signatures, calldatas), description);
can change to this:
return super.propose(targets, values, _encodeCalldata(signatures, calldatas), description);
@RitzyDevBox That's what #3989 proposed. It fixes it at the cost of ignoring "downstream" overrides of propose. There may be important logic there that will not execute.
@frangio,
Can you clarify a bit for me, I haven't played with Diamond pattern inheritance in over a decade.
Won't this respect downstream overrides? its more of the upstream ones that are the problem?
e.g any situation where the caller does this:
function propose(address[] memory targets, uint256[] memory values, bytes[] memory calldatas, string memory description)
public
override(Governor, IGovernor) {
}
should be okay since its not overriding GovenorCompatibilityBravo any scenario where the user has to override
function propose(address[] memory targets, uint256[] memory values, bytes[] memory calldatas, string memory description)
public
override(**GovenorCompatibilityBravo,** Governor, IGovernor) {
}
Since the user explicity is specifying GovenorCompatibilityBravo Implies a few things to me
- We offload the responsibility for them to override both
- The users contract is using
Bravoso their initial override should be gear'd towards bravo which by default is the signature parameter one
Won't this respect downstream overrides? its more of the upstream ones that are the problem?
I think I meant the same as what you're saying, but upstream is a better word. "Downstream" should be defined as the direction of super.
any situation where the caller does this: [...] should be okay
No, it's more complicated. It depends on the order of inheritance, and in a potentially unintuitive way due to the linearization algorithm. We need to avoid designs that change semantics depending on order of inheritance because they are very subtle and easy to miss errors.
What I mean is that if an extension is defined as GovernorExt is Governor and it overrides propose, then behavior might change for the end user depending on if they write MyGovernor is GovernorExt, GovernorCompatibilityBravo or MyGovernor is GovernorCompatibilityBravo, GovernorExt.