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

Gas Abstraction #301

Merged
merged 9 commits into from
Jun 10, 2020
Merged

Gas Abstraction #301

merged 9 commits into from
Jun 10, 2020

Conversation

petejkim
Copy link
Contributor

@petejkim petejkim commented Jun 1, 2020

Technical Design Doc

  • Adds increaseAllowance and decreaseAllowance to mitigate ERC20 multi-withdrawal attack vulnerability approve has.
  • Adds transferWithAuthorization, approveWithAuthorization, increaseAllowanceWithAuthorization, decreaseAllowanceWithAuthorization to enable ETH-less transactions.
  • Adds permit – an alternative to approveWithAuthorization, provided for compatibility with the draft EIP2612 proposed by Uniswap. Differences are laid out here.

@petejkim petejkim changed the base branch from master to 2.0 June 1, 2020 20:03
@petejkim petejkim changed the base branch from 2.0 to v2 June 1, 2020 20:05
@petejkim petejkim requested a review from eztierney June 2, 2020 17:28
petejkim added 8 commits June 2, 2020 19:35
Add internal functions _approve and _transfer for code reuse
* Implement EIP712 helper functions
* Implement transferWithAuthorization
* Implement approveWithAuthorization
* Implement cancelAuthorization
* Apply all V1 an V1.1 tests to V2
These alternative methods mitigate ERC20 multiple withdrawal attack.
* Implement increaseAllowanceWithAuthorization
* Implement decreaseAllowanceWithAuthorization
@petejkim petejkim changed the base branch from v2 to master June 2, 2020 19:38
@petejkim petejkim changed the base branch from master to v2 June 2, 2020 19:38
@eztierney
Copy link
Contributor

Slowly going through this, but in general looks good so far.

One thing that would be helpful is adding a doc in the docs directory explaining the new v2 features - including things like how to sign approvals, why we we have the *WithAuthorization methods and permit, etc.

I can puzzle most of it out from the code/comments but would be nice to have it all in 1 place for future integrators who are trying to build on these features.

Copy link
Contributor

@eztierney eztierney left a comment

Choose a reason for hiding this comment

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

This looks really good - I noticed maybe 1 missing test, but code coverage indicates maybe a few more.
I think we should try to get ECRecover.sol and the v2 contracts to 100% for this PR. There are some 1.1 and upgradeablility issues in the coverage too, but those don't need to block this PR - and we should probably fix those in master anyways.

The code coverage report I got:

File                           |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-------------------------------|----------|----------|----------|----------|----------------|
 upgradeability/               |      100 |    78.57 |      100 |      100 |                |
  AdminUpgradeabilityProxy.sol |      100 |       80 |      100 |      100 |                |
  Proxy.sol                    |      100 |      100 |      100 |      100 |                |
  UpgradeabilityProxy.sol      |      100 |       75 |      100 |      100 |                |
 util/                         |    81.82 |     62.5 |      100 |    83.33 |                |
  ECRecover.sol                |    71.43 |       50 |      100 |    71.43 |          63,67 |
  EIP712.sol                   |      100 |      100 |      100 |      100 |                |
 v1.1/                         |      100 |       75 |      100 |      100 |                |
  FiatTokenV1_1.sol            |      100 |      100 |      100 |      100 |                |
  Rescuable.sol                |      100 |       75 |      100 |      100 |                |
 v1/                           |      100 |     96.3 |      100 |      100 |                |
  AbstractFiatTokenV1.sol      |      100 |      100 |      100 |      100 |                |
  Blacklistable.sol            |      100 |      100 |      100 |      100 |                |
  FiatTokenProxy.sol           |      100 |      100 |      100 |      100 |                |
  FiatTokenV1.sol              |      100 |    94.74 |      100 |      100 |                |
  Ownable.sol                  |      100 |      100 |      100 |      100 |                |
  Pausable.sol                 |      100 |      100 |      100 |      100 |                |
 v2/                           |    96.23 |       90 |    90.91 |    96.23 |                |
  AbstractFiatTokenV2.sol      |      100 |      100 |      100 |      100 |                |
  EIP712Domain.sol             |      100 |      100 |      100 |      100 |                |
  FiatTokenV2.sol              |      100 |       50 |      100 |      100 |                |
  GasAbstraction.sol           |    96.88 |      100 |    88.89 |    96.88 |             71 |
  Permit.sol                   |       80 |      100 |       50 |       80 |             53 |
-------------------------------|----------|----------|----------|----------|----------------|
All files                      |    97.89 |    88.89 |    97.56 |    98.05 |                |
-------------------------------|----------|----------|----------|----------|----------------|```

test/v2/safeAllowance.behavior.ts Show resolved Hide resolved
@petejkim petejkim requested a review from eztierney June 9, 2020 21:22
@petejkim petejkim marked this pull request as draft June 9, 2020 21:30
@petejkim
Copy link
Contributor Author

petejkim commented Jun 9, 2020

Improved test coverage. New code is all at 100% now.

-------------------------------|----------|----------|----------|----------|----------------|
File                           |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-------------------------------|----------|----------|----------|----------|----------------|
 upgradeability/               |      100 |    78.57 |      100 |      100 |                |
  AdminUpgradeabilityProxy.sol |      100 |       80 |      100 |      100 |                |
  Proxy.sol                    |      100 |      100 |      100 |      100 |                |
  UpgradeabilityProxy.sol      |      100 |       75 |      100 |      100 |                |
 util/                         |      100 |      100 |      100 |      100 |                |
  ECRecover.sol                |      100 |      100 |      100 |      100 |                |
  EIP712.sol                   |      100 |      100 |      100 |      100 |                |
 v1.1/                         |      100 |      100 |      100 |      100 |                |
  FiatTokenV1_1.sol            |      100 |      100 |      100 |      100 |                |
  Rescuable.sol                |      100 |      100 |      100 |      100 |                |
 v1/                           |      100 |     96.3 |      100 |      100 |                |
  AbstractFiatTokenV1.sol      |      100 |      100 |      100 |      100 |                |
  Blacklistable.sol            |      100 |      100 |      100 |      100 |                |
  FiatTokenProxy.sol           |      100 |      100 |      100 |      100 |                |
  FiatTokenV1.sol              |      100 |    94.74 |      100 |      100 |                |
  Ownable.sol                  |      100 |      100 |      100 |      100 |                |
  Pausable.sol                 |      100 |      100 |      100 |      100 |                |
 v2/                           |      100 |      100 |      100 |      100 |                |
  AbstractFiatTokenV2.sol      |      100 |      100 |      100 |      100 |                |
  EIP712Domain.sol             |      100 |      100 |      100 |      100 |                |
  FiatTokenV2.sol              |      100 |      100 |      100 |      100 |                |
  GasAbstraction.sol           |      100 |      100 |      100 |      100 |                |
  Permit.sol                   |      100 |      100 |      100 |      100 |                |
-------------------------------|----------|----------|----------|----------|----------------|
All files                      |      100 |    94.44 |      100 |      100 |                |
-------------------------------|----------|----------|----------|----------|----------------|

@petejkim petejkim marked this pull request as ready for review June 9, 2020 22:55
Copy link
Contributor

@eztierney eztierney left a comment

Choose a reason for hiding this comment

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

Thanks, looks good

@eztierney eztierney merged commit 1023371 into circlefin:v2 Jun 10, 2020
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.

2 participants