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

Minimal support for ERC2771 (GSNv2) #2508

Merged
merged 31 commits into from
Feb 19, 2021
Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 4, 2021

This PR includes a ERC2771 compatible BaseRelayRecipient and a compatible, ERC712 based, relayer.

Fixes #2258

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx changed the title Support for GSNv2 Minimal support for GSNv2 Feb 4, 2021
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

I don't know how to evaluate whether this TrustedForwarder implementation is compliant with the GSNv2 ERCs. I could only find a PR for an EIP that has been closed due to inactivity: ethereum/EIPs#2770. In this draft there is something weird about extending the ForwardRequest type that I don't understand where it fits in the GSNv2 picture. The signature for the verify and execute functions are different from what we have here.

Do you know if these two functions got changed at some point?

contracts/GSNv2/BaseRelayRecipient.sol Outdated Show resolved Hide resolved
contracts/GSNv2/BaseRelayRecipient.sol Outdated Show resolved Hide resolved
contracts/GSNv2/BaseRelayRecipient.sol Outdated Show resolved Hide resolved
contracts/GSNv2/MinimalForwarder.sol Outdated Show resolved Hide resolved
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
@Amxx
Copy link
Collaborator Author

Amxx commented Feb 8, 2021

I don't know how to evaluate whether this TrustedForwarder implementation is compliant with the GSNv2 ERCs. I could only find a PR for an EIP that has been closed due to inactivity: ethereum/EIPs#2770. In this draft there is something weird about extending the ForwardRequest type that I don't understand where it fits in the GSNv2 picture. The signature for the verify and execute functions are different from what we have here.

Do you know if these two functions got changed at some point?

AFAIK, there is no requierements about what the forwarder should or should not do. I went with a ERC712 metatx relayer, but anything is possible. My understanding is that the standard focus on the data encoding between the forwarder and the recipient. How the forwarder works, and how it verifies ownership before encoding data is not specified.

@frangio
Copy link
Contributor

frangio commented Feb 9, 2021

There is at least a requirement for the TrustedForwarder to follow an interface that the RelayHub will use, which if I understand correctly consists of verify and execute. However, these two functions are not specified in the EIP that was merged. We should report this to the OpenGSN team to at least get some clarity, because otherwise we would be merging a feature that we can't be sure is spec-compliant.

@frangio
Copy link
Contributor

frangio commented Feb 9, 2021

Actually, there is no EIP but I suppose the current de facto spec is the OpenGSN repo and it seems we're implementing a different Forwarder interface. Take a look at IForwarder.sol.

@forshtat
Copy link

forshtat commented Feb 9, 2021

We will get a verification audit of GSN v2 smart contracts, including Forwarder, in a very short time. I don't expect Forwarder to be affected, but still, after that I will update the ERCs with current interfaces.

In the meanwhile we accepted a proposal to add optional expiration date to the base ForwardRequest to prevent eternally valid signed messages (not on master yet):
opengsn/gsn@5d562f8

@frangio
Copy link
Contributor

frangio commented Feb 9, 2021

Thanks for the update @forshtat!


@Amxx Given that ERC2771 can be used independently of the GSN we should consider renaming the GSNv2 directory. Perhaps something associated to meta-transactions would be a good name. One option is metatx.

@Amxx Amxx marked this pull request as ready for review February 17, 2021 17:55
contracts/metatx/BaseRelayRecipient.sol Outdated Show resolved Hide resolved
contracts/metatx/MinimalForwarder.sol Outdated Show resolved Hide resolved
contracts/metatx/MinimalForwarder.sol Show resolved Hide resolved
Amxx and others added 2 commits February 18, 2021 10:35
@frangio frangio changed the title Minimal support for GSNv2 Minimal support for ERC2771 (GSNv2) Feb 19, 2021
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

GSNv2 support
3 participants