search

OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.94k stars 11.8k forks source link

Expiration support for MinimalForwarder #2566

Closed gorgos closed 3 years ago

gorgos commented 3 years ago

Currently the MinimalForwarder does not seem to support expiration times. In my opinion this should be even part of a 'minimal' forwarder, otherwise forwarders could wait as long as they want.

Amxx commented 3 years ago

Hello @gorgos.

As the name states, the MinimalForwarder really is a minimal forwarder. It is just here for showcasing how to build a forwarder, and for testing purposes. It is not intended to be a production ready forwarder by any means.

I do have an OutOfOrderForwarder, which support expiry dates as well as both ordered and ordered meta-tx here. Is it something you think we should add in contracts ?

gorgos commented 3 years ago

As the name states, the MinimalForwarder really is a minimal forwarder. It is just here for showcasing how to build a forwarder, and for testing purposes. It is not intended to be a production ready forwarder by any means.

This is not clear yet. It's currently the only forwarder contract inside metatx and I assume most people would just use that.

I do have an OutOfOrderForwarder, which support expiry dates as well as both ordered and ordered meta-tx here. Is it something you think we should add in contracts ?

This seems like a good addition. But why not add the deadlines to the minimal forwarder, is there anything else missing to be production ready?

Amxx commented 3 years ago

Deadline are just one feature on top of the minimal forwarder ... but many more might make sens (out-of-order being on of these). My position is to wait for a standard to reach consensus and provide an implementation for it. I would hate to push a partial solution (like a minimal + deadline), and have it become widely adopted without proper prior community discussion.

gorgos commented 3 years ago

I think a deadline should become widely adopted. And in your example people can even set the deadline to 0 if they really want to opt-out. If anything, the question for me would be deadlines with or without opt-out option.

Amxx commented 3 years ago

You might be focused on deadline only, but I can assure deadlines are not the only thing possible. And again, pushing a with-deadline forwarder for adoption means pushing a without-the-other-stuff forwarder. Also I believe that having multiple forwarder is a bad idea. IMO, there should be only one standard forwarder, and it should be available on all networks at the same address (just like 1820 registry). This means that it must be standardise, and while we are willing to participate, this standardisation is beyond our prerogatives.

As you seem very motivated, I encourage you to participate to discussion with the GSN team and the community (on https://ethereum-magicians.org/) to have a standard move forward.

frangio commented 3 years ago

We agree that deadlines are very important for a working forwarder, but the MinimalForwarder is not meant to be a final forwarder contract. Maybe we should be more clear about this in our documentation: that a fully functioning forwarding system with good properties requires more complexity, and that people should look at other projects such as the GSN which do have the goal of building a system like that.