OpenZeppelin
GovernorCountingFractional
Fixes #4958
PR Checklist
- [x] Tests
- [x] Documentation
- [x] Changeset entry (run
npx changeset add)
🦋 Changeset detected
Latest commit: 149fe651bdcdd7f5aa3bfdda6b7301a7a01461fc
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| openzeppelin-solidity | Minor |
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
![changeset-bot[bot] avatar](https://avatars.githubusercontent.com/in/28616?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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.
I honestly feel e0f5b71f24f1228affb5caea90e5469a07115bf7 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 memory-safe.
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.
I think it would be ok to leave the previous _extractUint128 if we mark it as memory-safe, along with a comment warning that the caller must validate pos.
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
I am unsure if I should open an issue for this.
Yes please, open an issue describing the behavior desired as precisely as possible.