Consider removing MinimalForwarder

[frangio]

Even though the documentation states that this contract is not suitable for production use, that message isn't getting across and I believe we should just remove the contract entirely.

openzeppelin-contracts/contracts/metatx/MinimalForwarder.sol

Lines 12 to 15 in 3d7a938

	* MinimalForwarder is mainly meant for testing, as it is missing features to be a good production-ready forwarder. This
	* contract does not intend to have all the properties that are needed for a sound forwarding system. A fully
	* functioning forwarding system with good properties requires more complexity. We suggest you look at other projects
	* such as the GSN which do have the goal of building a system like that.

[pcaversaccio]

This might be a silly idea, but why not rename the contract & file name to something more obvious (it would be a breaking change, yes, but I don't think this matters in that case)? Maybe something like MinimalForwarderTestContract or similar... or moving the contract to the Mocks folder would be another solution. I just feel like that OZ should preserve such contracts that might be useful for some people at one point.

[frangio]

I just feel like that OZ should preserve such contracts that might be useful for some people at one point.

It could be documentation though, like "here's what a proto forwarder contract might look like".

[pcaversaccio]

It could be documentation though, like "here's what a proto forwarder contract might look like".

Speaking of myself, I prefer reading the code directly at first. So if I would want to get started with meta-transactions, I would search for code, issues, and PRs about that topic in the OZ repo. In any case - I agree that you should reflect it in the documentation.

[grp06]

If minimalforwarder is not suitable for production, what should I use as a replacement?

[pcaversaccio]

@grp06 maybe look at the Forwarder from GSN https://github.com/opengsn/gsn/blob/master/packages/contracts/src/forwarder/Forwarder.sol. They got 2 audits on their contracts, but I think this version was not included back then FYI.

[ernestognw]

@pcaversaccio my understanding from Fran's message was that the contract itself can be documentation, not that is a documentation issue.

The idea of having it in mocks makes sense since it's its only purpose.

might be useful for some people at one point.

Agree, but we don't keep things just because they might be useful, I believe there has to be a community-driven reason.

If minimalforwarder is not suitable for production, what should I use as a replacement?

I'd better ask what you want to achieve. Using meta-transactions with EIP-712 signatures as with ERC20's permit could be enough for most of the use cases I'd say.

[pcaversaccio]

not that is a documentation issue

Well, this somehow contradicts @frangio's message here, no?

Agree, but we don't keep things just because they might be useful, I believe there has to be a community-driven reason.

Fully agreed, that's why I share my opinion here :) - my preferred solution for this issue is to move it into mocks.

[ernestognw]

Well, this somehow contradicts @frangio's message here, no?

Somehow, yes.
But let me phrase this clearly.

We do think that there's a problem with multiple users asking about this (as @grp06) and it's a legitimate question.
In order to make clear that is not suitable for production purposes, there are a few options:

1. Improve the documentation by adding a big warning (this doesn't imply it's a documentation issue, people might still ignore it)
2. Move it to mocks
3. Completely remove it
4. Keep it in the docs somewhere else written in the docs as an example

My intuition makes me feel that number 1 won't solve the issue long-term. So I'd advocate for not keeping it in the /contracts folder, which may be 2, 3 or 4

Fully agreed, that's why I share my opinion here :) - my preferred solution for this issue is to move it into mocks.

Sure, we always appreciate your contributions! Do you know any relevant system based on EIP-2771? Although some people in the community might want to keep it in mocks (maybe including me, not sure yet), it wouldn't make any sense if there's no ecosystem support for it.

Aside from GSN, do we know other players relying on a forwarder implementation? 👈 That's a question for everyone.

[pcaversaccio]

Do you know any relevant system based on EIP-2771?
Aside from GSN, do we know other players relying on a forwarder implementation?

Ad-hoc, I know that Reddit relied on meta-transactions for their community tokens called "Moon". Here is the old Relay Hub contract (I think it's from GSN) on Rinkeby they used for gasless transactions: https://rinkeby.etherscan.io/address/0xd216153c06e857cd7f72665e0af1d7d82172f494. I don't know about the current setup tho.

In case I encounter a further ecosystem besides GSN, I would let you know here.

[frangio]

There is a 5th option which is to make MinimalForwarder suitable for production so we can recommend it to people. I think the only thing that it's missing is an expiration parameter. Is there anything else?

[lokman928]

For production, MinimalForwarder should become more easily to customize (eg let the function become virtual so we can override them)

[nftchance]

There is a 5th option which is to make MinimalForwarder suitable for production so we can recommend it to people. I think the only thing that it's missing is an expiration parameter. Is there anything else?

In the current state, money can also be lost due to the lack of refund; ref: #3664

Although I don't believe that should be a detractor to finalize a prod-ready implementation of MinimalForwarder as a trusted foundation for forwarders is not readily available in another trusted place that I am aware of.

[ernestognw]

We're proposing to make the MinimalForwarder production ready, also adding a deadline parameter in ForwardRequest.

MinimalForwarder should become more easily to customize

What do people think should fall into this category from the MinimalForwarder? One could argue both verify and execute should be virtual but a few questions arise to me:

1. Is it a use case for overriding verify? First sight, EIP-712 _TYPE_HASH could be public virtual although properties in ForwardRequest might be missing (if you know one aside from deadline, please share)
2. How good is it to make execute virtual? For reference, some use cases may include not checking success, take Safe as a reference. This affects gas estimations and assumptions that can be made (like gas estimations).

money can also be lost due to the lack of refund; ref: #3664

Yes, we're definitely taking care of that if no major issues pop up during implementation 👍🏻

I'll send a PR to continue the discussion there.