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

ERC777 Token Compatibility for TokenCallbackHandler #655

Closed
QYuQianchen opened this issue Sep 7, 2023 · 5 comments
Closed

ERC777 Token Compatibility for TokenCallbackHandler #655

QYuQianchen opened this issue Sep 7, 2023 · 5 comments

Comments

@QYuQianchen
Copy link

Context / issue

A Safe instance can configure one fallback hander on deployment or later by calling the setFallbackHandler(address) function by itself. Among all the cool fallback handlers developed by the Safe team, the TokenCallbackHandler stands out for its usefulness when dealing with token callbacks.

However, the current implementation of "TokenCallbackHandler" falls short when handling ERC777 token transactions, particularly mint, send and operatorSend calls.

In a scenario where a Safe sets "TokenCallbackHandler" as its fallback handler and receives ERC777 tokens through the functions mentioned above, it queries the ERC1820Registry contract to retrieve the implementer contract (see OpenZeppelin's ERC777 Implementation v4.9). If the Safe hasn't previously called setInterfaceImplementer on the ERC1820 contract, the returned implementer will be address(0), resulting in a revert of the ERC777 token transaction with the error message "ERC777: token recipient contract has no implementer for ERC777TokensRecipient."

Proposed solution

  1. Implement IERC1820Implementer in the TokenCallbackHandler contract.
contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ERC721TokenReceiver, IERC165, IERC1820Implementer {
   // ... other variables
    bytes32 private constant TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");
    bytes32 private constant ERC1820_ACCEPT_MAGIC = keccak256("ERC1820_ACCEPT_MAGIC");
   // ... other functions
    /**
     * override implementation check
     */
    function canImplementInterfaceForAddress(bytes32 interfaceHash, address account)
        public
        view
        virtual
        override
        returns (bytes32)
    {
        return interfaceHash == TOKENS_RECIPIENT_INTERFACE_HASH ? ERC1820_ACCEPT_MAGIC : bytes32(0x00);
    }
}
  1. Optionally, make an additional setInterfaceImplementer(safe_instance_address, TOKENS_RECIPIENT_INTERFACE_HASH, TokenCallbackHandler_deployment) call to the ERC1820Reigstry, when "TokenCallbackHandler" is set by internalSetFallbackHandler(). This step can also be left for Safe owners to decide to execute in a separate Safe transaction or not. However, it should be properly documented.
@mmv08
Copy link
Member

mmv08 commented Sep 7, 2023

It seems that openzeppelin deprecated ERC777 in their latest release: https://docs.openzeppelin.com/contracts/4.x/erc777 (top of the page)

We haven't previously received any requests for adding additional token handlers. Do you have any data about erc777 usage?

@SCBuergel
Copy link

Given that ERC777 has been within OZ releases for quite some time, it's safe to assume that many projects do use them. We (HOPR) did deploy $HOPR as an ERC777 and our token holders would significantly benefit from having this standard properly supported within Safe out of the box.

@QYuQianchen
Copy link
Author

Hey @mmv08! Indeed, it's accurate that OpenZeppelin (OZ) has deprecated ERC777, and it will be removed in the v5.0 release. However, it's worth noting that OZ has been supporting ERC777 for over four years, starting from v2.3.0. You can refer to their CHANGELOG for details.

There's still a substantial number of ERC777 tokens deployed on various EVM chains. A quick query on Dune here shows that there are at least 360 ERC777 tokens on the mainnet, around 250 on Polygon, and approximately 200 on Gnosis, see result here or below

deployed number of erc777 tokens

Moreover, I'd like to kindly emphasize that the goal isn't to add more token handlers but to address an issue within the existing code. Under the current implementation, for a Safe proxy to utilize ERC777TokensRecipient, it must undergo two transactions:

  1. Set "TokenCallbackHandler" as its callback handler
  2. Call setInterfaceImplementer to set the proxy itself as the interface implementer.

Additionally, it's worth noting that the proposed solution streamlines this process to just one step: calling setInterfaceImplementer to designate the "TokenCallbackHandler" deployment as the interface implementer.

Furthermore, the proposed solution offers the advantage of saving gas when receiving an ERC777 token upon minting, as it eliminates the need for the additional calldata and CALL operations involved in fallback().

@KapeR221
Copy link

great

@rmeissner
Copy link
Member

Should be considered together with #390

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

No branches or pull requests

5 participants