[Last Call] EIP-5216: ERC-1155 Allowance Extension

EIPs
,
1

Hi everyone,

After been discussed in this forum, I created an EIP PR for the initial idea of my draft (https://ethereum-magicians.org/t/working-draft-new-extension-for-erc1155).

I open this topic to discuss here said PR.

In case that I missed some step, feel free to comment and I’ll change it.

Thank you very much.

PR: https://github.com/ethereum/EIPs/pull/5216

1 Like
2

UPDATE: This EIP is now in Last Call.

I need the help of the community so feel free to check it, test the code and find bugs. Let’s discuss about the implementation and move it forward.

Thank you in advance!

EDIT.- This is the EIP: EIPs/eip-5216.md at master · ethereum/EIPs · GitHub

3

Hi @ivanmmurcia thanks for the EIP-5216

QQ: why is approve disallowing the caller to be the operator?

1 Like
4

Hi @xinbenlv, thanks for your feedback.

I have realized that reversing a call because of this check is silly, and also reviewing the OZ’s ERC-20 _approve function implementation, I have seen that what I should check instead, is that neither the owner nor the operator are zero address:

require(owner != address(0), "ERC20: approve from the zero address");
require(spender != address(0), "ERC20: approve to the zero address");

What do you think?

5

That sounds good. Can you create a new PR to update the spec accordingly?

1 Like
6

Of course, I’m on my way. Thanks dude!

EDIT: Update EIP-5216 with zero address control in _approve function like EIP-20 by ivanmmurciaua · Pull Request #5917 · ethereum/EIPs · GitHub here is the PR :slight_smile:

7

Hi @ivanmmurcia , love this EIP, the lack of a proper approval pattern is a big problem with ERC-1155, this EIP will definitely help to make it a more robust standard.

A couple things regarding the reference implementation:

1-The proposal states safeTransferFrom MUST Subtract the transferred amount of tokens from the approved amount if msg.sender is not approved with setApprovalForAll

But in the reference implementation, the allowance is decremented in all cases. In the case of an operator being approvedForAll, this means _allowances[from][msg.sender][id] can underflow and lead to an allowance being silently inflated.

You can add a check before decrementing:

        require(
            from == msg.sender || isApprovedForAll(from, msg.sender) || allowance(from, msg.sender, id) >= amount,
            "ERC1155: caller is not owner nor approved nor approved for amount"
        );
+       if (from != msg.sender && !isApprovedForAll(from, msg.sender)) {
        unchecked {
            _allowances[from][msg.sender][id] -= amount;
        }
+       }
        _safeTransferFrom(from, to, id, amount, data);

2-There is a tiny logic issue in safeBatchTransferFrom: The error string line 89 in safeBatchTransferFrom will never be reached, because_checkApprovalForBatch never returns false, it either returns true or reverts line 111 if one of the ids to be transferred does not have sufficient allowance.

You can amend the _checkApprovalForBatch so that it returns false instead of reverting if the allowance is not enough.

-            require(allowance(from, to, ids[i]) >= amounts[i], "ERC1155ApprovalByAmount: operator is not approved for that id or amount");
+           if (allowance(from, to, ids[i]) < amounts[i]) {
+               return false;
+           }
            unchecked { 
                _allowances[from][to][ids[i]] -= amounts[i];
                ++i; 
            }
2 Likes
8

Hey @joestakey, thanks for the great feedback.

1- Yes totally, and I’ve detected this but in my defense I have to say that I wrote the reference implementation and the testing suite in 0.8.15, i.e the compiler avoids the underflow you’ve said… however, this EIP could be implementated in any version of Solidity, so I’ll change.

2- Was my mistake, I only checked that everything worked fine and reverted with the appropiate message but was a tiny logic issue, changed :slightly_smiling_face:

Cheers!

9

What is the latest status on this EIP?

Can the title be simplified to ERC1155Allowance.
Imo it would be less verbose than ApprovalByAmount.

Anyone who knows the 1155 spec will know this is an extension for granular allowance.

10

Hi @dcota. This EIP has last call status. LGTM this title change, I could make a new PR to wake up the editors.

Thanks for your feedback.

EDIT: Official name changed: ERC-5216: ERC-1155 Allowance Extension

11

Has adding ERC20Permit style approvals been considered?
If you (very understandably) don’t want to include it in the base spec, it could also be added as an extension to the spec similar to ERC1155/ERC1155Metadata.

12

Hi @SanLeo461 thanks for your idea.

Maybe you can check this ERC-4494: Permit for ERC-721 NFTs to move forward your proposal. Would be interesting a permit extension for ERC-1155 but in this EIP we’re not going to implement it.

13

Conversation for permit style approvals can be found here, would love to hear your thoughts! Just started a PR for it

1 Like