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.

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

Enable ERC-1271 signature checks in Governor castVoteBySig #4418

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Jul 3, 2023

Fixes LIB-954

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2023

🦋 Changeset detected

Latest commit: 3b9dcd2

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 Major

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

@ernestognw ernestognw requested review from Amxx and frangio and removed request for Amxx July 3, 2023 22:41
@ernestognw ernestognw marked this pull request as ready for review July 3, 2023 22:42
@ernestognw ernestognw requested a review from Amxx July 3, 2023 22:42
.changeset/dry-crews-sell.md Outdated Show resolved Hide resolved
'openzeppelin-solidity': major
---

`Governor`: Use `bytes memory` signature instead of `r`, `s` and `v` parameters in the `castVoteBySig` and `castVoteWithReasonAndParamsBySig` functions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not merge the two changesets into one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought both required their own entry. Although the main reason of this PR is to introduce 1271 compatibility, it requires this change that should be communicated on its own.

We can think of an individual changeset if you think it's needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intuition is that all of the changes to castVoteBySig should be together in the same changeset, including also #4378 currently in .changeset/sixty-numbers-reply.md.

If not that, I do think we should at least merge the 2 in this PR.

Copy link
Member Author

@ernestognw ernestognw Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer merging these two into a single one because otherwise, we'll be breaking the PR reference when processing the changesets into the changelog.

Not sure if all the changes should be in a single entry. But I'd be worried about the order in the Changelog once they get processed.

test/governance/Governor.test.js Outdated Show resolved Hide resolved
test/governance/extensions/GovernorWithParams.test.js Outdated Show resolved Hide resolved
test/governance/extensions/GovernorWithParams.test.js Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Jul 4, 2023

Can we quickly look at how much it increases deployment cost to add an overload of delegateBySig in Votes using SignatureChecker? I mean adding, without removing the existing function.

ernestognw and others added 2 commits July 4, 2023 10:26
Co-authored-by: Hadrien Croubois <[email protected]>
Co-authored-by: Francisco <[email protected]>
Co-authored-by: Francisco <[email protected]>
@ernestognw
Copy link
Member Author

Can we quickly look at how much it increases deployment cost to add an overload of delegateBySig in Votes using SignatureChecker? I mean adding, without removing the existing function.

Sure, these are the results (a is the current state and b is this gist):

diff --git a/.gas b/.gas
index cd6f167d..42f6269f 100644
--- a/.gas
+++ b/.gas
@@ -9,25 +9,25 @@
 ························|································································|·············|·············|·············|···············|··············
 |  $VotesMock           ·  $_mint(address,uint256)                                       ·      62741  ·     141566  ·      85268  ·           30  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesMock           ·  delegate(address,address)                                     ·      81745  ·      96110  ·      92513  ·            4  ·          -  │
+|  $VotesMock           ·  delegate(address,address)                                     ·      81723  ·      96088  ·      92491  ·            4  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesMock           ·  delegate(address)                                             ·      48371  ·     111409  ·      71138  ·            9  ·          -  │
+|  $VotesMock           ·  delegate(address)                                             ·      48349  ·     111387  ·      71116  ·            9  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesMock           ·  delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32)  ·      76778  ·     124056  ·      92537  ·            3  ·          -  │
+|  $VotesMock           ·  delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32)  ·      76745  ·     124035  ·      92512  ·            3  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
 |  $VotesTimestampMock  ·  $_burn(address,uint256)                                       ·      95539  ·      95623  ·      95567  ·            3  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
 |  $VotesTimestampMock  ·  $_mint(address,uint256)                                       ·      62650  ·     141384  ·      85164  ·           30  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesTimestampMock  ·  delegate(address,address)                                     ·      81654  ·      96019  ·      92422  ·            4  ·          -  │
+|  $VotesTimestampMock  ·  delegate(address,address)                                     ·      81632  ·      95997  ·      92400  ·            4  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesTimestampMock  ·  delegate(address)                                             ·      48371  ·     111227  ·      71088  ·            9  ·          -  │
+|  $VotesTimestampMock  ·  delegate(address)                                             ·      48349  ·     111205  ·      71066  ·            9  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
-|  $VotesTimestampMock  ·  delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32)  ·      76778  ·     123965  ·      92507  ·            3  ·          -  │
+|  $VotesTimestampMock  ·  delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32)  ·      76757  ·     123944  ·      92486  ·            3  ·          -  │
 ························|································································|·············|·············|·············|···············|··············
 |  Deployments                                                                           ·                                         ·  % of limit   ·             │
 ·························································································|·············|·············|·············|···············|··············
-|  $VotesMock                                                                            ·          -  ·          -  ·    1571033  ·       15.7 %  ·          -  │
+|  $VotesMock                                                                            ·          -  ·          -  ·    1783636  ·       17.8 %  ·          -  │
 ·························································································|·············|·············|·············|···············|··············
-|  $VotesTimestampMock                                                                   ·          -  ·          -  ·    1534941  ·       15.3 %  ·          -  │
+|  $VotesTimestampMock                                                                   ·          -  ·          -  ·    1747531  ·       17.5 %  ·          -  │
 ·----------------------------------------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·

@ernestognw ernestognw requested review from frangio and Amxx July 4, 2023 17:21
@frangio
Copy link
Contributor

frangio commented Jul 5, 2023

LGTM. We can discuss delegateBySig separately.

This was referenced Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants