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

requireReceptionAck parameter is required when _mint for ERC20 compatible #2271

Closed
bruce-eljovist opened this issue Jun 8, 2020 · 7 comments · Fixed by #2552
Closed

requireReceptionAck parameter is required when _mint for ERC20 compatible #2271

bruce-eljovist opened this issue Jun 8, 2020 · 7 comments · Fixed by #2552
Labels
breaking change Changes that break backwards compatibility of the public API. feature New contracts, functions, or helpers.

Comments

@bruce-eljovist
Copy link

I’m developing ERC20 and ERC777 compatible token.
I’m experiencing a hard time implementing mint for ERC20.
In order to be fully compatible with ERC20 in ERC777, an account that does not implement IERC777Recipient must be able to mint.

But, in the _mint methods, _callTokensReceived’s last parameter is hardcoded to true.
I can’t make ERC20 compatible mint method.

What about changing
_callTokensReceived(operator, address(0), account, amount, userData, operatorData, true);
to
_callTokensReceived(operator, address(0), account, amount, userData, operatorData, requireReceptionAck);
and take requireReceptionAck as a parameter.

@nventuro
Copy link
Contributor

nventuro commented Jun 8, 2020

Hello @bruce-eljovist, thank your for raising this!

This is an interesting point, I did not find explicit guidelines on the specification as to how this situation should be handled.

I'd be wary of introducing this additional complexity to _mint and have the user make such a decision. It is also possible to sidestep this check by first minting the tokens and then transferring them using ERC20 functions to the intended recipient.

Is this a pattern you can follow in your application? Can you share more information about your use case, and how the recipient checks are causing you trouble? Thanks!

@bruce-eljovist
Copy link
Author

Thank you.
I rewrote code like below.

contract MintHelperForERC20 is Context, Ownable, IERC777Recipient {
    IERC1820Registry private _erc1820 = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);
    IERC20 private token;
    bytes32 constant private _TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");

    constructor() public
    {
        _erc1820.setInterfaceImplementer(address(this), _TOKENS_RECIPIENT_INTERFACE_HASH, address(this));
        token = IERC20(_msgSender());
    }

    function tokensReceived(address operator, address from, address to, uint256 amount, bytes calldata userData, bytes calldata operatorData)
        external override
    {
    }

    function transfer(address to, uint256 amount) external onlyOwner {
        token.transfer(to, amount);
    }
}
contract Token777_2 is ERC777 {
    MintHelperForERC20 mintHelper;

    constructor(string memory name, string memory symbol, address[] memory defaultOperators)
        public
        ERC777(name, symbol, defaultOperators)
    {
        mintHelper = new MintHelperForERC20();
    }

    function mint777(address account, uint256 amount, bytes calldata userData)
        external
    {
        _mint(account, amount, userData, "");
    }

    function mintErc20(address account, uint256 amount)
        external
    {
        _mint(address(mintHelper), amount, "", "");
        mintHelper.transfer(account, amount);
    }
}

The new version can mint when recipient is no IERC777Recipient implemented contract.
But there remains a small event log problem. Minted eventlog’s to field is not recipient.

[ Minted 0x4CfE78BD9EaEfE7D042855568C9b0531862a72FD 0x78168DD3E10ABA3c42E07E417fd88ea43F6d1E43 1000000000000000000 {null} {null} ]
[ Transfer 0x0000000000000000000000000000000000000000 0x78168DD3E10ABA3c42E07E417fd88ea43F6d1E43 1000000000000000000 ]
[ Sent 0x78168DD3E10ABA3c42E07E417fd88ea43F6d1E43 0x78168DD3E10ABA3c42E07E417fd88ea43F6d1E43 0x873f0eE6A4Dfd4c48163Fe493551A912a66a6979 1000000000000000000 {null} {null} ]
[ Transfer 0x78168DD3E10ABA3c42E07E417fd88ea43F6d1E43 0x873f0eE6A4Dfd4c48163Fe493551A912a66a6979 1000000000000000000 ]

Do you have better ERC20 compatible mint implementation idea?

@nventuro
Copy link
Contributor

nventuro commented Jun 9, 2020

Do you need the Minted event to have that address? I find that a bit strange, since Minted is ERC777 specific - if working with the contract as an ERC20, you should use Transfer instead.

It'd be great if you could share more details of what you're trying to achieve. Perhaps it makes sense for your contract to initially mint supply for itself, and then transfer it to recipients when you're now calling _mint?

@bruce-eljovist
Copy link
Author

Our native token has an underlying token.
The native token’s initial totalSupply will be zero because it has no underlying value.
TotalSupply changes when someone mints and redeems the native token in underlying token.
I think Mint and Redeem event is crucial for this use case and it would be better to define it our own Mint event

@nventuro
Copy link
Contributor

I see, that makes sense. Is there a particular reason why you chose to implement this using ERC777? Part of the idea behind it is preventing recipient contracts that are not aware of the protocol from holding tokens that would be forever locked in that account. If you're bypassing that mechanism, it might make sense to just use ERC20 directly.

Alternatively, you could go ahead with the ERC777 checks and make sure the recipients implement the ERC777Recipient interface. Is there a reason why that is not feasible?

@bruce-eljovist
Copy link
Author

Of course, ERC777 has a recipient checking function, but it is applicable only to the contract. For the EOA, locking is not prevented. I think it is not a core function but a mistake reducing set up.
Currently, openzepplin’s ERC777 can also be used to send to a contract through erc20 compatible transfer, so I don’t think our erc20 compatible mint will increase the risk compared to not having one.
Because our token will be used in multiple DApps in the future, it should be fully operational as ERC20 and ERC777 as well.
I think the biggest potential for ERC777 is that it calls the tokensReceived of the contract that received the token.
If there is a DApp that uses ERC777, the customer of this DApp can just send a token so that the DApp can take an appropriate action.
It means that user does not need to sign two transactions, one for approving and the other one for transfer. In terms of UI, ERC777 is better.
In terms of transaction atomicity, ERC777 is also superior.
In the current web application, the approve is often set to max, but if the user approves it to max in this way, security becomes a serious risk.
I think making users to approve it like this is a big problem. Therefore, in the future, I think this purpose of approve will be less widely used.
That’s why we want to use ERC777. At the same time, we consider ERC20 mint compatible function is important. In many cases, existing contract-based wallets do not implement IERC777Recipient. And there is a probability that ERC777 is not going to become popularized despite our expectations. So in order to prepare for both scenarios, our token should be fully compatible to ERC20.

@Amxx Amxx added breaking change Changes that break backwards compatibility of the public API. feature New contracts, functions, or helpers. labels Feb 15, 2021
@frangio
Copy link
Contributor

frangio commented Mar 3, 2021

We want to resolve this before the stable 4.0 release in about a week.

@SCBuergel suggested as an alternative solution to disable requireReceptionAck by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants