Minimalistic transferable interface

SamWilsn · May 2, 2023, 2:11pm

Agreed! You’d call the one-argument overload when deciding if you should show the transfer UI at all, and the three-argument overload for a particular transfer. I guess.

stoicdev0 · May 2, 2023, 2:45pm

I think this explains it clearly. Some NTFs are not meant to be transferred at all, others might be only transferred with certain from-to combination. This is our attempt to address both cases since it will be easier for dApps to adopt a single EIP instead of 2.

We agree on this implication:
isTransferable(uint256):false -> isTransferable(address,address,uint256):false

But all these are incorrect.
isTransferable(uint256):true -> isTransferable(address,address,uint256):true
isTransferable(address,address,uint256):false -> isTransferable(uint256):false
isTransferable(address,address,uint256):true -> isTransferable(uint256):true

So it’s not really redundant.

stoicdev0 · May 4, 2023, 6:47pm

@wwhchung @SamWilsn @tbergmueller what do you think of the latest version? We think we addressed all concerns.

wwhchung · May 4, 2023, 7:01pm

Not sure why you don’t just clone the onERC721Received interface then.

The updated proposal doesn’t seem to take into account the operator, which is also used in certain mechanics.

If you’re going with multiple signatures then I think the spec needs to specify what they actually mean.

Eg if you give it a token only, if true it means it’s transferable for at least one sender/receiver. Etc.

I don’t know if you need a yes/no/maybe. Maybe yes/no/conditional is better. And if that’s the convention adopted, conditional means you have to check the complete interface.

stoicdev0 · May 4, 2023, 7:38pm

That’s exactly the idea. We’ll make it clearer on the proposal.
If the method with only tokenId returns false, it means it cannot be transferred under any combination.
If it returns true, then you need to check for the from/to combination.

We are actually doing one of the things you proposed.

sullof · May 4, 2023, 10:12pm

In some old comments I suggested a 4 parameters function to cover all scenarios.

Sometimes the token is transferable by the owner but not by a marketplace, or, at the opposite, it is transferable by a lending pool but not by the owner.

I am for the single function approach, but since we are going with the two-functions approach as said above, I think that it would be better to have

isTransferable (
  uint tokenId
) external returns (bool);

isTransferable (
  uint256 tokenId, 
  address from, 
  address to, 
  address operator
) external returns (bool);

When there is no operator in the equation, we may pass address(0).

If we just had the second function, we may call it like

isTransferable(tokenId, address(0), address(0), address(0))

to obtain what the first function expects. The problem that I see is that the implementer must implement both functions, despite which one makes sense. Having a single one able to manage all the scenario seems more powerful to me, and potentially consumes less gas.

Similarly, if I want to check if a token is transferable to someone, despite the from, I could call it like

isTransferable(tokenId, address(0), recipient, address(0))

In other words, a similar function would be very versatile and efficient.

tbergmueller · May 5, 2023, 8:21am

If we go with that, I think in the EIP it would need to say say “any parameter that should be ignored is should be address(0)”.
With this definition we run into severe problems, because burning and minting is a transfer to or from address(0).
So going by your suggestion isTransferable(tokenId, address(0), recipient, address(0)), this could mean that mints are on the one hand or that the from address is ignored on the other hand. Minting may not be our biggest issue though, you could add a “must throw when tokenId does not exist”.

Burning is a real issue. isTransferable(tokenId, sender, address(0), address(0)) can have multiple meanings

returns whether a sender can transfer tokenId to address(0) ==> can I burn?
returns whether a sender can transfer tokenId to ANY address ==> can sender transfer the token w/o restrictions?

So I would argue using address(0) for from and to is not a good idea, for the operator it may work (as long as nobody finds address(0) private key ;))

sullof · May 5, 2023, 5:32pm

You are right.

Let me clarify that my goal here is to discuss any possible development so we end up with a strong, robust proposal that does not need to be upgraded two months later because some issue comes out.

That said, we may define a magic address to be used as a zero address. Something like
0xF0F0F0f0f0F0F0f0f0F0f0f0f0f0F0F0f0f0f0F0. But it would make it too complex and gas-consuming. So, forgot about it.

The big doubt is about keeping it minimalistic, with a single function with just the tokenId as a parameter, or to cover most scenarios. I liked the minimalistic version, but I see the need for a broader coverage.

I think that one way to go is to keep ERC6454 as simple as possible, with just isTransferable(tokenId) and add a second interface, that we may call ERC6454Extended that extends ERC6454 and adds

isTransferable(uint, address, address)
isTransferable(uint, address, address, address)

Whoever wants/needs to give more details can implement the extended version.

stoicdev0 · May 8, 2023, 8:04pm

After discussing with @ThunderDeliverer, @YuriNondual and @sullof I think we have a final version.
We will only have a single method, receiving tokenId, from and to. The behavior should be as follows:

from and to are zero → Is transferable at all? (This would replace the single argument version)
Only from is zero → Can mint to to? (Can be used for allow lists)
Only to is zero → Can from burn? (Up to the implementation to allow or not).
If the call with address 0 for both from and to is false, any other combination where from and to are not zero, MUST return false as well.

Optionally, implementers may add the single argument version which SHOULD call the 3 arguments one with both addresses as 0. This is similar to what is done in many 721 implementations where they include a transfer method using the message sender as the from.

We opted for not including the operator as it would make behavior more complex and no longer be minimalistic.

@tbergmueller I hope this approach is useful to you.

stoicdev0 · May 9, 2023, 11:55pm

The proposal has been updated: ERC-6454: Minimal Transferable NFT detection interface

tbergmueller · May 10, 2023, 2:51pm

Hi,

appreciate the changes and wording in EIP is clearer than in @stoicdev0 last post; In your post above I see potential for misunderstanding

Only from is zero → Can mint to to? (Can be used for allow lists)

Not so sure for allow-lists; often you put the minting wallet on the allowlist, not the receiving wallet. For security reasons, people often like to have a minting wallet but then mint the tokens e.g. to their ledger.
If minting wallet == to, no issue of course.

Only to is zero → Can from burn? (Up to the implementation to allow or not).

I think here we have problematic edge cases. Assume token 3 is owned by 0xaaa. 0xaaa approved 0xbbb to burn. In that case; should isTransferable(3, 0xaaa, address(0)) naturally returns true, but must isTransferable(3, 0xbbb, address(0)) also return true? In this case, 0xbbb is approved and can actually burn, but on the other hand your goal was to reflect with the parameters the parameters of the ERC721.TransferFrom I believe?
If we want to be compliant ERC721 parameter, I would maybe mention this example in rationale and suggest that - if we want to know whether 0xbbb can burn a token owned by 0xaaa the correct check would be isTransferable(3, 0xaaa, address(0)) && isApproved(0xbbb, 3)

Looking for clarification here, thanks!

Minor comment on wording of the EIP

If the general transferability of token is false, any kind of transfer of the token, save minting and burning, MUST revert execution.

instead of save minting and burning I would use except minting and burning, otherwise it’s too close to safeMint function naming.

With a bit more of clarification / examples of those edge-cases, I’d recommend to move this forward to final Would or will implement it in my contracts

stoicdev0 · May 10, 2023, 3:01pm

@ThunderDeliverer is better with words

Regarding the burn, it is a great point. I was thinking that the burn check should also check for permissions and return false if the from is not owner or approved. But I like your suggestion better: isTransferable(3, 0xaaa, address(0)) && isApproved(0xbbb, 3). We should add that to the EIP.

Also agree with this suggestion. We will apply them ASAP and request move to final now that we all agree. Thanks for your suggestions!

sullof · May 10, 2023, 11:36pm

I think that it is out of the scope of this EIP to tell the dev how to manage that scenario.

Let’s say that 0xbbb has not been approved yet but it wants to know if it can transfer the token if it gets approved. That suggestion would return false all the time, but as soon as 0xbbb gets approved may turn true. As you can see it can become very complicated — which is why we talked of not including spenders in the proposal.

tbergmueller · May 11, 2023, 1:06pm

Thank you for clarification - as I said, I completely agree with the wording of the EIP and I also see (now) that mentioning those special cases may be out of scope.

Sorry for being picky, but I think there may be more potential in the interface through very simple rewording and you cannot change once you move to final;

IRC6454 suggests the following:

@param from Address from which the token is being transferred

I would argue, that in this case you cannot pass an approved address, as this is strictly speaking not the address, the token is transferred from (it is always transferred from the owner address). I would suggest to consider rewriting this as

@param from Address from which the token transfer is being initiated

By specifiying the iniating address, this would allow a very simple implementation of

isTransferable(tokenId, from, to) {
  // May add a throw here when tokenId does not exist.
   if (from == address(0) && to == address(0))
         // Indicate the token can be transferred in general
        return true;

   return ERC721.isApprovedOrOwner(from);  
}

Also, I want to empathize that I think it’s brilliant that you do not specify when isTransferable()==false. This provides enough wiggle-room to implement also more exotic use cases, like we have for example a.t.m with our ERC-6959 (oracle-based transfers, where neither approved nor owner can transfer per default, yet there is an extension, where certain tokens that are marked floatable can also be transferred by owners or approved - easy right?).

Just as an example on how I would / will implement this for ERC6956 extending IERC6956, to demonstrate the flexibility your proposed interface has imho:

isTransferable(tokenId, from, to) {
  // May add a throw here when tokenId does not exist.
   if (from == address(0) && to == address(0))
         // Indicate the token can be transferred in general, as tokens can be oracle-transferred anytime

        return true;

   return IERC6956.isFloating(anchorByToken(tokenId)) && ERC721.isApprovedOrOwner(from);  
}

sullof · May 11, 2023, 9:52pm

Clever. i like it.
@stoicdev0 What do you think?

ThunderDeliverer · May 12, 2023, 3:43pm

I took some time and talked to @stoicdev0 about this suggestion.

We see the value of it, but it might also limit the usability of the proposal.

If we were to modify it to enforce checking for approvals, this would force implementers to add additional code that would in turn require more gas when executed in the before-transfer hook.

We are trying to make this proposal as minimal and unopinionated as possible and this could be considered an opinionated implementation in the sense that it differs from the common transferable interfaces (that usually include the tokenId, from and to values).

Our suggestion would be to make this behavior optional. So not enforce it, but allow it. The smart contracts and UIs interacting with the smart contract should not expect it in this case, but custom UIs could make use of it.

An additional argument for this is that approvals are already standardized and can be combined with the check that this proposal brings to the table in some form of the isConditionallyTransferable custom method.

Would this suffice for your implementation?

tbergmueller · May 12, 2023, 8:26pm

Yes. And optional sounds like a good compromise!

Note I’m mainly sharing my opinions in the general case, not in particular tailored for my projects.

In most of my current projects the isTransferable() interface is of no central functional importance. I’m rather looking for the current or future mass-adapted way on how to tell an external application (mostly marketplaces) that it will not be able to transfer some tokens by “standard ERC-721 ways”.

Motivation for this is I just find it very frustrating for users (see earlier post of how market places react a.t.m.) if they think they can sell something and others that they can buy it and only after all decisions have been made, the transfer reverts. I want to spare our users from this frustration and spending unneccessary gas on approvals etc If we’re very realistic, the typical NFT holder doesn’t know what a smart contract even looks like let alone how to read them. So we market-places will need provide better usability here in the long term, and I see that this EIP can help them to check whether a token can or shall be listed or not.

ThunderDeliverer · May 12, 2023, 11:26pm

Thank you for the feedback and all of the constructive comments! I think the note about approvals we suggested might help with the transferability issue you described.

I’m really happy as I think this is the proposal’s last amendment!

ThunderDeliverer · May 13, 2023, 9:27pm

We published the amended proposal with the optional check for approvals. Please take a look and let us know if you agree with finalizing the proposal @sullof @tbergmueller . We want to bring it up in the next EIP editors’ call.

tbergmueller · May 14, 2023, 3:52am

Looks very good to me - appreciate moving it to final!