openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

Consider removing Context

Open frangio opened this issue 3 years ago • 8 comments

Context provides _msgSender() and the reason it exists and is used across the codebase is so that the logic for getting the function caller can be replaced via inheritance, for example using ERC2771Context. It is necessary if we want to support overriding that logic for any piece of code in the library. While Context is fairly simple, it is pervasive and the vast majority of use cases do not need the functionality at all, so it's a great candidate for removal to simplify the code.

We would like to understand first if Context alternatives such as ERC2771Context are actively used, as well as what are the alternative recommendations we can make for those use cases.

frangio avatar Jan 04 '23 16:01 frangio

Please correct me if I'm wrong

Context is used for transactions signed by one wallet (private key) and executed on behalf of another wallet. The most common thing I can refer to is OpenSea which makes user sign ERC20 transfer and ERC721 transfer and then both transactions are executed by the server. This is where ERC2771Context can help

But basically, any transaction can be executed by 3rd party. It is mostly a problem of security to validate transaction content. For example, https://opengsn.org/ pays gas instead of the end user. So it should be a good practice to always use Context

TrejGun avatar Jan 04 '23 23:01 TrejGun

OpenSea which makes user sign ERC20 transfer and ERC721 transfer and then both transactions are executed by the server

Can you share a reference to documentation or where we can see this being used?

frangio avatar Jan 05 '23 03:01 frangio

I can't point to documentation because it does not exist, unfortunately( but can point to tests

wyvern protocol https://github.com/wyvernprotocol/wyvern-v3/blob/master/test/5-wyvern-exchange-matching.js#L197

\eaport protocol https://github.com/ProjectOpenSea/seaport/blob/main/test/basic.spec.ts#L1308

TrejGun avatar Jan 05 '23 11:01 TrejGun

Neither the WyvernExchangeWithBulkCancellations nor Seaport use Context. They both rely on msg.sender to get the caller.

In both cases they support "signed operation" using either EIP-191 or EIP-712, which are independent of Context.

Amxx avatar Jan 05 '23 13:01 Amxx

@Amxx right and your implementation of ERC721 does

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L168

so safeTransferFrom invoked by Seaport Zone will resolve _msgSender to the signer and not to Zone. the same will work if I call _msgSender in onERC721Received

it means I always want to use it

TrejGun avatar Jan 05 '23 13:01 TrejGun

so safeTransferFrom invoked by Seaport Zone will resolve _msgSender to the signer and not to Zone. the same will work if I call _msgSender in onERC721Received

When seaport calls safeTransferFrom, it does not pack a sender address using EIP-2771. Does it? AFAIK, seaport works with any ERC721, and will not expect the token contract to support EIP-2771 message comming from seaport.

Said otherwise, seaport is not an EIP-2771 relay.

We are not planning to stop using msg.sender in our contract, that would be foolish. We plan on using msg.sender without using Context._msgSender() as an alias. I don't see how this would affect seaport. The only apps that it would affect are the one that override _msgSender() in order to add support for some relaying mechanism.

Amxx avatar Jan 05 '23 13:01 Amxx

Thanks, I see my mistake. I thought it would work for any signed transaction executed by someone else

TrejGun avatar Jan 05 '23 13:01 TrejGun

Yeah, it's important to point out that the removal of Context doesn't imply that "meta transactions" of all kinds are no longer supported. What wouldn't be supported is using meta transactions to "override" the msg.sender of any piece of code in the library. But meta transaction patterns can still be implemented via signatures.

frangio avatar Jan 05 '23 14:01 frangio

We have decided to keep Context for v5 based on the following:

  • We've found that ERC2771Context has a reasonable amount of usage (see image below). This requires the use of Context throughout the library.
  • We've confirmed that the use of Context and _msgSender() instead of msg.sender do not cause additional gas usage.
  • The same pattern is used by ERC721A, a gas-focused implementation.

We will reconsider this for v6. The growth of ERC-4337 infrastructure and adoption would be a contributing factor towards removal, as it can be an alternative to ERC-2771 meta-transactions.


Looking at Ethereum and Polygon networks:

frangio avatar Jun 15 '23 02:06 frangio