vittominacori / erc1363-payable-token

Reference implementation for the ERC-1363 Payable Token
https://vittominacori.github.io/erc1363-payable-token/
MIT License
132 stars 52 forks source link

Add `safeMint` or Public Method to Trigger `onTransferReceived` After Minting in ERC1363 #34

Closed Gaussianer closed 2 months ago

Gaussianer commented 2 months ago

The current ERC1363 implementation provides functionality to invoke the onTransferReceived callback after a transfer or transferFrom operation, ensuring that smart contracts can handle incoming tokens correctly. However, there is no built-in public method to trigger the onTransferReceived callback directly after minting tokens with the _mint function.

In scenarios where tokens are minted directly to a contract, the tokens are immediately available on the contract's balance, but the contract does not automatically receive the onTransferReceived callback. Developers are forced to manually call the callback or implement custom logic, which introduces unnecessary complexity and potential errors.

Current Behavior:

Proposed Solutions:

  1. Introduce a safeMint function:

    • Similar to the existing safeTransfer in ERC721, safeMint would mint tokens to an address and, if the recipient is a contract, automatically trigger the onTransferReceived callback.
    • This would streamline the process of minting tokens to contracts and reduce manual callback invocations by developers.

    Example Implementation:

    function safeMint(address to, uint256 amount, bytes memory data) external onlyOwner nonReentrant {
        if (to == address(0)) revert InvalidAddress();
    
        // Mint tokens to the recipient
        _mint(to, amount);
    
        // Check if the recipient is a contract
        if (to.code.length > 0) {
            // Invoke the onTransferReceived function for contracts
            try IERC1363Receiver(to).onTransferReceived(_msgSender(), address(0), amount, data) returns (bytes4 retval) {
                if (retval != IERC1363Receiver.onTransferReceived.selector) {
                    revert ERC1363InvalidReceiver(to);
                }
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert ERC1363InvalidReceiver(to);
                } else {
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        }
    }
  2. Make _checkOnERC1363TransferReceived a protected method or add a mintAndCall function:

    • By making _checkOnERC1363TransferReceived protected, it would allow derived contracts to trigger the onTransferReceived callback after minting, following similar logic to the transferAndCall function.
    • Alternatively, introduce a mintAndCall function, similar to transferAndCall, which combines minting and callback logic without requiring a redundant transfer.

    Proposed Code for mintAndCall:

    function mintAndCall(address to, uint256 amount, bytes memory data) external onlyOwner nonReentrant {
        if (to == address(0)) revert InvalidAddress();
    
        // Mint tokens to the recipient
        _mint(to, amount);
    
        // Check if the recipient is a contract
        if (to.code.length > 0) {
            // Invoke the onTransferReceived function for contracts
            try IERC1363Receiver(to).onTransferReceived(_msgSender(), address(0), amount, data) returns (bytes4 retval) {
                if (retval != IERC1363Receiver.onTransferReceived.selector) {
                    revert ERC1363InvalidReceiver(to);
                }
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert ERC1363InvalidReceiver(to);
                } else {
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        }
    }

Benefits:

vittominacori commented 2 months ago

Hello, thanks for pointing this, but the mint or safeMint method is not part of ERC1363 or of the derived ERC20 contract.

As per your implementation the safeMint method requires privileges like ownership so it needs to extend the Ownable contract or in any case add a role manager. This is not a standard behavior and cannot be inserted in an abstract class.

It is simply implementable by extending ERC1363 contract and adding this in your derived contract.

vittominacori commented 2 months ago

Will try to add extensions with an internal _mintAndCall method.

Gaussianer commented 2 months ago

Thank you, I think that's a very useful extension :)

vittominacori commented 2 months ago

@Gaussianer I think that adding the ERC1363Mintable contract will solve your issue https://github.com/vittominacori/erc1363-payable-token/pull/35/files#diff-8066f898b0ecc371d2ac435998daeb3166440803a7e7830a08ba4878a52519be

In your contract you could use it as follows

pragma solidity ^0.8.20;

// other imports
import "erc-payable-token/contracts/token/ERC1363/extensions/ERC1363Mintable.sol";

contract MyToken is ERC1363Mintable, Ownable {
    // your stuff

    function safeMint(address account, uint256 value, bytes memory data) public onlyOwner {
        if (account.code.length == 0) {
            _mint(account, value);
        } else {
            _mintAndCall(account, value, data);
        }
    }

    // your stuff
}
Gaussianer commented 2 months ago

Hey there,

Thank you for implementing the _mintAndCall method. It addresses the issue effectively.

In your previous comment, you mentioned that developers could handle the check for externally owned accounts (EOAs) manually in their own implementation, as shown below:

function safeMint(address account, uint256 value, bytes memory data) public onlyOwner {
    if (account.code.length == 0) {
        _mint(account, value);
    } else {
        _mintAndCall(account, value, data);
    }
}

While this approach works, I believe the ERC1363Mintable contract could handle this check internally, similar to how ERC721 handles it in the safeMint method with the implementation of the checkOnERC721Received

In ERC721, when using safeMint, the following occurs:

  1. The token is minted and transferred to the recipient.
  2. The contract checks whether the recipient is an EOA or a smart contract by inspecting to.code.length.
  3. If the recipient is a smart contract, the onERC721Received callback is triggered. If it’s an EOA, no callback is triggered, and the transfer proceeds.

Proposed implementation (similar to ERC721 logic):

function _mintAndCall(address account, uint256 value, bytes memory data) internal {
        if (account.code.length == 0) {
            _mint(account, value);
        } else {
            _mintAndCall(account, value, data);
        }

This logic is seamless and user-friendly, removing the need for developers to handle EOAs vs. contracts manually. I suggest adopting a similar pattern in ERC1363Mintable by modifying _mintAndCall to handle this internally.

Why this change is beneficial:

This change would make the library more developer-friendly, following well-established patterns like those in ERC721.

Thank you again for your hard work and for considering this suggestion!

vittominacori commented 2 months ago

Note that the ERC721 standard expects that there is a safeTransfer behavior so you don't have to worry about whether the receiver can handle the tokens. By using the safe* behavior it may happen that tokens will be stuck into contracts that are not able to recover them.

ERC1363 Standard, instead, doesn't allow this behavior because the main purpose of this EIP is to allow developers/users to have control over the receiver's callbacks. I mean if you send tokens (or mint them) to pay an invoice and you expect the receiver to set the invoice ID as paid, you want to be sure that onTransferReceived has been executed. Using safeMint or (similarly a safeTransfer) you won't be sure that receiver executed the callback, resulting in token loss.

This has been discussed several times (check links in that thread) and it was finally chosen to not to allow the safe behavior.

Anyway, any developer, can develop his contract by extending the standard behavior and introducing the safeMint (or a safeTransfer), but this will not be part of the reference implementation.