Skip to content

Token Claims EIP712 signature verification (and ethereum primitives switch)#431

Merged
MOZGIII merged 4 commits intomasterfrom
claims-eip712
Aug 9, 2022
Merged

Token Claims EIP712 signature verification (and ethereum primitives switch)#431
MOZGIII merged 4 commits intomasterfrom
claims-eip712

Conversation

@MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Aug 3, 2022

This PR adds eip712-token-claim, refactors the eip712 code to enable more reuse, and switches the codebase to ethereum primitives crate.

It also resolves the incorrect use of "EVM address" wording. The terms we have are EVM account and Ethereum address. Using the term "EVM address" is discouraged as confusing.

See #430.


eip712-token-claim

To do

  • implementation
  • test that recovery returns the expected eth address with a valid signature
  • test that recovery doesn't return the expected eth address with an invalid signature

@MOZGIII MOZGIII marked this pull request as ready for review August 3, 2022 23:12
@MOZGIII MOZGIII mentioned this pull request Aug 3, 2022
Copy link
Contributor

@dmitrylavrenov dmitrylavrenov left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments!

@MOZGIII MOZGIII enabled auto-merge (squash) August 8, 2022 11:09
@MOZGIII MOZGIII disabled auto-merge August 8, 2022 11:14
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Aug 8, 2022

I decided we'd want to change the trait at token claims to only do the signer recovering instead of full verification, and move the verification out of the trait. It makes a lot of sense to do it like that, as the verification is not something one should be able to override. But we'll address it alter, I'll create an issue for this, and we can merge this PR.

@MOZGIII MOZGIII merged commit d7d6689 into master Aug 9, 2022
@MOZGIII MOZGIII deleted the claims-eip712 branch August 9, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants