Add GovernorCountingFractional #5045
Conversation
🦋 Changeset detectedLatest commit: 149fe65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
12123b0 fixes the events, but is a breaking change. I don't think its a big deal (it wont be silent, compiler fail) ... but its not ideal. @ernestognw @frangio WDYT? |
|
I don't see a way to fix this without a breaking change. Another possible fix would be to add a new internal function. But I think just adding a return value is a better option. |
Yeah same, it's one of those breaking changes that are easy to fix imo. |
| * * Voting from an L2 with tokens held by a bridge | ||
| * * Voting privately from a shielded pool using zero knowledge proofs. | ||
| * | ||
| * Based on ScopeLift's GovernorCountingFractional[https://github.com/ScopeLift/flexible-voting/blob/e5de2efd1368387b840931f19f3c184c85842761/src/GovernorCountingFractional.sol] |
There was a problem hiding this comment.
Are there any important differences that people should consider if they migrate from ScopeLift's implementation to this one, or if an interface has to support both? I think it may be worth listing them here.
There was a problem hiding this comment.
So far:
- The reference implementation is in OpenZeppelin Contracts v4
- It includes a
noncein theparamsobject given v4 had no nonce protection - The
COUNTING_MODDEis different
The voting format may vary after the reviews. I think it's currently fine since it reads it's "based on", so no compatibility should be assumed right away
| return usedWeight; | ||
| } | ||
|
|
||
| function _extractUint128(bytes memory data, uint256 pos) private pure returns (uint128 result) { |
There was a problem hiding this comment.
This function concerns me because the length of data is ignored and pos may be reading out of bounds. The assembly block correctly omits the memory-safe annotation, but like I said in a recent issue this globally disables optimizations so it should be avoided.
Is it possible to just check the validity of pos against the data length? Sadly I do see it would make the code less efficient by checking the length three times... Perhaps the helper should directly unpack all three parameters.
There was a problem hiding this comment.
I think having the helper to unpack all three parameters is the easiest to ensure memory safetyness.
I'll update with this alternative
There was a problem hiding this comment.
This is private, and everywhere we call it we first validate the length. Is that not good enough?
There was a problem hiding this comment.
GIven how frequent it is for people to copy-paste from our code I think we should try to make functions self-contained and well documented even if they're private.
In this case, not only that I think this is the cleanest way of making the assembly block memory-safe to avoid disabling optimizations, but also I share the concern with @frangio; although the function is unreachable with invalid parameters, I don't feel comfortable having such a function around.
Co-authored-by: Francisco <fg@frang.io>
|
I honestly feel e0f5b71 reduces redeability ne consistency (instead of checking length in one place for both paths, we do it in two different places). I don't understand what the issue is with having a private function that doesn't check the input validity if it is called with input that was already validated. |
|
I'd be open to change it as long as we either remove the assembly or make the block I agree it's not an issue itself given the function is private and can't be reached with invalid arguments, it's just that I wouldn't feel comfortable given how people might use the code. |
Amxx
force-pushed
the
feature/governor-counting-fractional
branch
from
00e425b
to
3f1c13b
Compare
Amxx
force-pushed
the
feature/governor-counting-fractional
branch
from
3f1c13b
to
37391ce
Compare
|
I think it would be ok to leave the previous |
| * potentially large number of calls to cast all their votes. The voter has the possibility to cast all the | ||
| * remaining votes in a single operation using the traditional "bravo" vote. | ||
| */ | ||
| // slither-disable-next-line cyclomatic-complexity |
There was a problem hiding this comment.
For the record, we both feel fine by keeping this function self contained
| // - "Fractional" voting: `support = 255`, with 48 bytes params. | ||
| if (support == uint8(GovernorCountingSimple.VoteType.Against)) { | ||
| if (params.length != 0) revert GovernorInvalidVoteParams(); | ||
| usedWeight = againstVotes = remainingWeight; |
There was a problem hiding this comment.
Similarly, we agreed that this double assignment is clear enough
|
Seeing as the main benefit of this is to allow for proxy voting of locked tokens, are there any plans on adding a FlexVotingClient like the one in ScopeLift that users can use with this governor? At the moment, I want to use this, but it seems like I will have to adapt the client from ScopeLift in order to make it work (after reviewing the diff from this and theirs). I am unsure if I should open an issue for this. https://github.com/ScopeLift/flexible-voting/blob/master/src/FlexVotingClient.sol |
Yes please, open an issue describing the behavior desired as precisely as possible. |
Fixes #4958
PR Checklist
npx changeset add)