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

Add all uint/int/bytes primitive types for EIP-712 signing #10224

Merged
merged 5 commits into from
Mar 20, 2023

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Mar 16, 2023

Description

This adds all uint/int/bytes primitive types for EIP-712 signing.

Swapping on Uniswap with Valora was failing with the following error: Unrecognized type uint160 is not included in the EIP-712 type list

Uniswap started using Permit2 which requires signing a message containing uin160 and uint48 types which were missing in the implementation.

See also ethereum/go-ethereum#26770

Other changes

Also added support for all the added types for zeroValue

Tested

  • Update unit tests
  • Swapping on Uniswap with Valora works with the change

Related issues

N/A

Backwards compatibility

Yes

Documentation

N/A

@jeanregisser jeanregisser requested a review from a team as a code owner March 16, 2023 18:06
jeanregisser added a commit to valora-inc/wallet that referenced this pull request Mar 17, 2023
### Description

This fixes swaps always failing on Uniswap.

Uniswap started using
[Permit2](https://uniswap.org/blog/permit2-and-universal-router) for
their swaps and it requires a signed EIP-712 message with some types
which were unsupported by our implementation (specifically `uint160` and
`uint48`).

I've submitted a PR to support the needed types in
`sign-typed-data-utils` from `@celo/utils`:
celo-org/celo-monorepo#10224

This PR includes a patch, so we don't have to wait for a release of the
Celo SDK.

### Test plan

- Updated e2e tests


https://user-images.githubusercontent.com/57791/225853763-5a35d4ca-3e55-4005-9051-ba48b3d05be7.mp4


This shows someone who's never swapped on Uniswap:
- First "Allow" is a one time max approval of cUSD for
[Permit2](https://uniswap.org/blog/permit2-and-universal-router)
- Second "Allow" is the Permit2 signature
- Third "Allow" is the actual transaction signing

Subsequent swaps (of cUSD) won't need the one time approval.
And the Permit2 signature will only be needed if the existing one
expired.
Right now Uniswap Permit2 signed messages are valid for 1 month with max
approval.

The benefit is that if the user interacts with another app using
Permit2, they won't need the initial transaction for the approval. They
can just sign a Permit2 message which doesn't cost anything.

### Related issues

- Fixes RET-664

### Backwards compatibility

Yes
Copy link
Contributor

@eelanagaraj eelanagaraj left a comment

Choose a reason for hiding this comment

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

nice!

packages/sdk/utils/src/sign-typed-data-utils.ts Outdated Show resolved Hide resolved
packages/sdk/utils/src/sign-typed-data-utils.ts Outdated Show resolved Hide resolved
@eelanagaraj eelanagaraj enabled auto-merge (squash) March 20, 2023 15:33
@eelanagaraj eelanagaraj merged commit fc6f98a into celo-org:master Mar 20, 2023
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