-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
Add GovernorCountingFractional #5045
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
* * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far:
- The reference implementation is in OpenZeppelin Contracts v4
- It includes a
nonce
in theparams
object given v4 had no nonce protection - The
COUNTING_MODDE
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
00e425b
to
3f1c13b
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, we agreed that this double assignment is clear enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LFG 🚀
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
)