EIP-6059: Parent-Governed Nestable Non-Fungible Tokens

Okay, that clarifies things; the intended audience for this standard seems to be primarily aimed at new creations, and ones that won’t be interacting with ERC721 tokens as children.

I reread the “Backwards Compatibility” section and realized it does have a more full description of what the intention here is (EIP6059 works with older tooling), but to me the crux of the issue is I don’t think that is the definition of “backwards compatible”, which lead to my confusion.

I most commonly have seen that term used to indicate “old things can work in the new tool” (e.g. What is Backward Compatible (Backward Compatibility)? “software system that can successfully use interfaces and data from earlier versions of the system or with other systems”). Therefore the intention of “ERC6059 tokens can masquerade as ERC721 tokens for tools that don’t know about ERC6059” is a fine goal, but is going the “wrong way around” (“new thing works in the old tool”) so I believe should be labeled something different than “Backwards Compatible”.

I think this is a very valid point. We will update that section to be more clear about compatibility. Thanks!

1 Like

How does this compare/relate to:

  1. EIP-998
  2. EIP-6551

I guess it is orthogonal to EIP-6551 since it has to specifically with nested NFTs, but your proposal does seem to overlap substantially with EIP-998.

As someone who builds on-chain games, simple ownership is not enough to convey semantic information about how an NFT owns other NFTs. Instead, our solution (still in early stages, but one gaining adoption) is to use an external registry contract which takes ownership of children/items: contracts/InventoryFacet.sol at develop · G7DAO/contracts · GitHub

In terms of actionable critique - I would like to understand if you looked at ERC-998 and why that fell short of your needs.

EIP998 is abandoned, and the author of EIP6059 did already comment on it here; they did read it and draw inspiration from it, but didn’t directly mirror its structure.

EIP6551 is another way to achieve a similar result, but EIP6551 aims to be completely external to the “parent” token (and hence even existing tokens or non-ERC721-tokens could implement), while EIP6059 aims for new token collections, as a way to have child-token-ownership “baked in” (aiming to have EIP6059 be a new “base standard” like ERC721 that wallets and apps would implement, expecting that additional functionality beyond basic ownership/transfer functionality).

1 Like

Exactly.

@stoicdev0 took some time, to prepare a really nice comparison on Twitter. Let me share it here:
https://twitter.com/stoicdev0/status/1657236277891067904

We use ERC-6059 in the ERC-6220, which seems to solve some issues that you are solving in the InventoryFacet. I didn’t take too deep of a dive, but you might find it useful :thinking:

1 Like

Thank you @MidnightLightning and @ThunderDeliverer .

I didn’t realize that ERC998 was abandoned but it has certainly been under draft for a long time. Thanks for clarifying.

Didn’t know about ERC-6220, either. Thank you for pointing it out.

1 Like

PEEPanEIP #109: ERC-6059: Parent-Governed Nestable NFT with @stoicdev0 & @ThunderDeliverer

2 Likes

The function nestTransferFrom() declared in EIP and in attached implementation mismatch. The parameter data is missing.

The EIP6059’s function ownerOf() returns the root owner actually. It is not just a design decision. Third party tools, indexers, that track only ERC721 events will find a different owner than is returned by ownerOf(). Other platforms, marketplaces, wallets, can track ERC721 events and synchronize with blockchain by calling ownerOf() and balanceOf() occasionally. This provides a clear inconsistency. The test that fails.

    it('Transfer event does not point to the owner', async function () {
      // preparation
      await parent.mint(alice.address, 1);
      await child.mint(alice.address, 2);
      await child.connect(alice).nestTransferFrom(alice.address, parent.address, 2, 1, '0x');

      // verification
      expect(await child.ownerOf(2)).to.be.eq(alice.address);
      const aliceAddressSerialized = ethers.utils.hexZeroPad(alice.address.toLowerCase(), 32);
      const transferTopic = ethers.utils.id('Transfer(address,address,uint256)');
      const tokenIdSerialized = ethers.utils.hexZeroPad('0x02', 32);
      const logs = await ethers.provider.getLogs({address: child.address, fromBlock: 1, toBlock: 'latest', topics: [transferTopic, null, null, tokenIdSerialized]});
      const lastOwner = logs[logs.length - 1].topics[2];
      expect(lastOwner).to.be.eq(aliceAddressSerialized);
    });

There is another problem with the function balanceOf(). An address that is an owner of a token in terms of ownerOf() should have positive balance, from ERC721 perspective. The test that fails.

    it('the owner has a balance', async function () {
      // preparation
      await parent.mint(alice.address, 1);
      await child.mint(alice.address, 2);
      await child.connect(alice).nestTransferFrom(alice.address, parent.address, 2, 1, '0x');
      await parent.connect(alice).acceptChild(1, 0, child.address, 2);

      // verification
      expect(await child.ownerOf(2)).to.be.eq(alice.address);
      expect(await child.balanceOf(alice.address)).to.be.gt(0);
    });

So EIP6059 is not 100% compatible with ERC721. Some 3rd party tools can run into inconsistency.

And please check the function burn(). It puts the root owner instead of the direct owner in the Transfer event, this is different than in other places.

@SamWilsn can you advise on how to proceed here? The interface ID in the proposal takes the missing parameter into account, but the parameter itself is not there in the specification. What would be the best way to address this?
Can we open another PR to fix this issue or is there another way of going about it since the proposal is in Final status?

The least contentious approach would be to make a new EIP with a new number. This has the downside that EIP-6059 will stick around in Final forever, but would end up with the most clear document in the end.

You could also make a small change to EIP-6059 to fix the issue in such a way that contracts that would’ve been valid today are still valid. From my understanding, calling a Solidity function with extra calldata is fine, so the backwards compatible change would be to add the parameter and something like this to the comment:

@param data Additional data with no specified format, which should be sent in call to `to`

The key bit is the “should”, which means implementations are free to ignore data.

2 Likes

Thank you for your suggestion!

We decided to open a PR to update the current proposal. We feel like this is the best course of action since having two virtually the same proposals makes little sense to us. This way, we end up with a complete proposal and allow for implementers to continue using a standardized way of nesting non-fungible tokens.

1 Like

Hi I see EIP-7401 and EIP-6059 are both in the repository now. Will it be confusing?

Hi,

We are in contact with EIP editors in order to add a warning to use EIP-7401 instead of ERC-6059, much like ERC-820 and ERC-1820 (which fixes ERC-820) have.

We hope this will eliminate any possible confusion.

1 Like

A client asked me to review ERC-7401 and whether it is suitable for their project. I’m sharing those notes here in case they will be helpful to others considering to use this.

Thank you for sharing ERC-7401, the motivation and the work on a spec and implementation. Some of its ideas are helpful, but as-is the specification is unusable. Problems include undefined behavior, inaccurate assertions, unnecessary complexity, security problems, and unfitness of the design to meet its own stated objectives.

It doesn’t actually allow token bundling

The motivation talks about “bundling” different kinds of NFTs under effective ownership of a ERC-7401 NFT. However nowhere in the spec does it actually illustrate how to take a Bored Ape Yacht Club NFT and subordinate in under a ERC-7401 token.

Inconsistent naming

The words “direct owner” and “parent” are used interchangeably. Instead, only one name should be used for this concept.

The pagination functions childOf and pendingChildOf use pagination matching how ERC721Enumerable works but they are named differently than function naming pattern established in that spec.

EOA, smart contract owners and ERC721Receiver are used incorrectly

In ERC-721, it is possible for a token to be owned by an externally-owned account (“EOA” and “EoA”), an ERC721Receiver smart contract or by a smart contract that is not an ERC721Receiver (when using transferFrom or when minting). It is also possible for a token to be owned by an account that is not know yet to be an externally owned account or a smart contract. (This is helpful when using counterfactuals, bridging and zero-knowledge proofs.)

However, the spec uses some subset of these three types of owners variously at different times. It appears that this is written this way unintentionally.

Does not implement ERC-721

The specification claims to “extend” ERC-721, however it does not.

  1. In the interface definition, it is missing is ERC721.
  2. The balanceOf function does not satisfy requriments of ERC-721. It does not count nested tokens (which do have an ownerOf of the parent).
  3. The transferFrom function and Transfer event do not satisfy requirements of ERC-721. Specificly, ERC-721 requires “This emits when ownership of any NFT changes by any mechanism.” It does not emit the Transfer event for those nested token ownership changes.
  4. The transfers do not support payable transfers.

Accepted and rejected children is unnecessary

There is no actual benefit of tracking accepted and rejected children. Spam NFT token are everywhere and nobody is hurt by them. Preventing spam NFT tokens in the “accepted” part, when they can barely be controlled in the “pending” does not merit the additional complexity this feature requires.

Tokens can get stuck

If a token is nested too deeply, they can get stuck. This means you could transfer a token to somebody else and they will be forever unable to transfer it out.

Test case:

  1. Assume a maximum gas per block of 8 million (not on Ethereum Mainnet).
  2. Create a ERC-7401 token, named evil.
  3. Put a cat token into that token, nested deeply. So deep that it requires 7.99 million gas to find the owner.
  4. Sell that token using a marketplace that requires you to transfer the token into the marketplace.
  5. When the token is sold it will be transferred out by the marketplace, but the marketplace will also require some gas. This puts the total over 8 million. And therefore the marketplace cannot look up the owner of the token. Now the token is stuck.

Burning is out of scope

Burning tokens does not provide any useful purpose and it costs Ether. Therefore nobody should ever do it.

This function does not meet any of the other identified requirements of the spec.

Therefore this function is useless and should not be part of the spec. Perhaps it is useful for other business reasons in some application. In that case it is an application-specific detail and not appropriate for a specification.

Undefined behavior

The spec has many instances of important functionality that result in undefined behavior. Some of the examples include:

  1. In the addChild function, it is specified that “The destination token MUST NOT be a child token of the token being transferred or one of its downstream child tokens.”. First, the words “destination token,” “token being transferred,” and “downstream child tokens” are all not defined. Second, in this context, the specification is made to the caller not to call the function in this certain way. Therefore if the caller does in fact do that thing that is not allowed, no statement at all is made about what the addChild function is to do.
    Compare this to ERC-721 which states “Throws if _from is not the current owner.” which is an implementable specification.
  2. “MUST not be full” has the same problem.
  3. “MUST NOT be called” is the same.
  4. In the rationale, it is specified when clearing the array SHOULD resullt in a reverted transaction.

There is no access control

All the tokens in ERC-7401 can be stolen by anybody.

In the specification of transferChild, there is no limitation on who can call this function. So if you have tokens, I can take them and then you can take them back from me.

The main applications of NFTs are ones that have a concept of some exclusivity of who can control them. If the intention of ERC-7401 is that anybody can steal anything, it could have limited applications.

Reordering attacks with indexes

In the spec, children are tracked using an ordering index. Tokens can be added by anybody (“spam” pending children) and they can be accepted (“acceptChild”) apparently by anybody.

This means when you expect to send one token you may in fact be sending a different one.

Test case:

  1. You have pending tokens 1=A, 2=B, 3=C
  2. You deny token 3=C
  3. Somebody else transfers to you 3=D
  4. You transfer out 3=D

Your expectation is that you transferred out 3=D. However if the transactions are played to the blockchain in a different order you actually would have transferred out 3=C.

There are mitigations possible for this. But they are specified in the spec.

Inaccurate statements

The spec motivation states that using indexes is necessary because “if the token ID was used to find which token to accept or reject, iteration over arrays would be required…”. This is false. For a demonstration where this is not the case, please see ERC-721, enumerable extension and its implementations.

Comparison to existing

A mention of ERC-998 would be appropriate and is missing.

Does not allow selling batch tokens

A motivation for ERC-7401 is that it will allow you to bundle sale of tokens. However the security considerations section notes that actually this use case is not supported because you can sell a bundle and rugpull the components out while you are selling.

Too complicated

A different spec meets all the motivations of ERC-7401, achieves compatibility with ERC-721 and does nothing more. It is:

inferface XXXX /* is ERC165, ERC721 */ {
    // Throws if no parent is in this contract.
    function parentOf(ERC721 childContract, uint256 childTokenId) external view returns (uint256 parentTokenId);
    // Throws if parentTokenId does not exist.
    function balanceOfParent(uint256 parentTokenId) external view returns (uint256 balance);
    // Throws if parentTokenId does not exist or index is >= balance.
    function childOfParentByIndex(uint256 parentTokenId, uint256 index) external view returns (ERC721 childContract, uint256 childTokenId);
    // Throws if tokenId does not exist.
    function lockedUntil(uint256 tokenId) external view returns (uint256 endTime);

    // Throws unless childContract.ownerOf(childTokenId) == this.ownerOf(toParentTokenId) == msg.sender. Throws if locked.
    function transferChildIn(uint256 toParentTokenId, ERC721 childContract, uint256 childTokenId) payable;
    // Throws unless this.ownerOf(fromParentTokenId) == msg.sender (this is not a recursive check). Throws if locked.
    function transferChildOutFrom(uint256 fromParentTokenId, address to, ERC721 childContract, uint256 childTokenId) payable;
    // Throws unless this.ownerOf(fromParentTokenId) == msg.sender (this is not a recursive check). Throws if locked.
    function transferChildAcrossFrom(uint256 fromParentTokenId, uint256 toParentTokenId, ERC721 childContract, uint256 childTokenId) payable;
    // Throws unless this.ownerOf(parentTokenId) == msg.sender. Implementations MAY limit seconds, e.g. to 30 days. Hint: useful for selling. Makes all ERC721 transfers and XXXX transfers with this token throw until endTime. Throws if that token is currently locked.
    function lockToken(uint256 tokenId, uint256 endTime) external;
}
1 Like

Hey there, thanks for sharing your findings! I’ll address some of your points.

Some of its ideas are helpful, but as-is the specification is unusable

I have to disagree here, dozens of collections are already using this for a wide variety of applications, including art, gaming and reputation.

It doesn’t actually allow token bundling.

A bundle is “a group of things fastened together for convenient handling”. By nesting tokens into another you are effectively creating a bundle. If you look at the proposed implementation and tests, it can be done through nestTransferFrom or with an implemention of nestMint, we provide the internal method. Parents and children must implement 7401, so you cannot do it with a BAYC unless you wrap it.

Inconsistent naming

Direct owner can be an EOA or an NFT. Parent is only an NFT.

EOA, smart contract owners and ERC721Receiver are used incorrectly

Not sure I understand the problem you are pointing out. The sample implementation does this check only on sending. If you want to have an implementation which is also allowed to receive NFTs to no particular token, you can add the IERC721Receiver interface and that’s it. I’ve actually done it a few times. What we do is to actually check when sending a token to another one via transfer or mint, that the destination contract implements IERC7401, otherwise it would get trapped.

Does not implement ERC-721

  1. In the interface definition, it is missing is ERC721.

If I remember correctly, we did this on purpose to avoid linearization issues.

  1. The balanceOf function does not satisfy requriments of ERC-721.

We added this clarification on 7401, it was missing on 6059.

  1. The transferFrom function and Transfer event do not satisfy requirements of ERC-721.

If you are referring to emitting for children when a parent changes ownership, this is absolutely impractical and expensive. This is the same reason why we have a different behavior of balanceOf when tokens are nested.

  1. The transfers do not support payable transfers.

You are right, we missed this.

Accepted and rejected children is unnecessary

We simply disagree here, by design this was an important part of the standard for us proposers. It can easily be bypassed by using a hook after a child is added, but we wanted to have this mechanism by default as it proved to be very useful on the initial protocol implementation on Kusama, which ran for about 2 years before we took it to EVM world.

Tokens can get stuck
If a token is nested too deeply, they can get stuck

In our test implemetation we have very simple way to prevent this. We check for inheritance loops up to a certain number of levels. If there are too many nesting levels we simply revert.

Burning is out of scope

Besides the reasoning of burning being useless, which I don’t really agree as it can be used for different mechanics, it was our intention initially not to have it, but it presented a problem. If you wanted to burn a token with children, we considered the right thing to do to also burn recursively its children. This was not possible if some of them could be not burnable, so we opted to include it. I’m not fan of having it here but I think it was the lesser evil.

Undefined behavior

I agree here in most points, we could have been clearer.

There is no access control

I agree here, we probably felt it was obvious, but we should have clarified in some of the methods, that they should only be done by an approved party or the token owner.

Reordering attacks with indexes

We don’t expect the acceptChild to be called by anybody but token owner or approved, another case were we just assumed it but would have been much better to clarify. That solves the reordering attack.

Inaccurate statements

You are right, it is not an accurate statement as there is an alternative, however it adds cost to every transfer so we would not include it here. This was another hard choice but requiring child indices in some operations was the cheapest solution in terms of gas, so we went for it.

Comparison to existing

If you mention another EIP in your proposal, you cannot finalize it until the other one does, so mentioning it was not an option, at that moment ERC-998 was stagnant even. A quick mention would be that ERC998 splits ability to own and ability to be owned into 2 interfaces. With our proposal there’s no need for this. Furthermore, while developing this, we found that unless the parent-child relationships are done in a parent-governed style, there are security issues which an evil intended child could do, a failure that 998 probably has.

Does not allow selling batch tokens

It can be achieved by either escrowing the parent token, or by storing the expected children on listing. We clarify this in the security considerations.

Too complicated

Unless you provide a sample implementation with good test coverage of this interface you propose, I will be doubtful that this can be achieved in a simpler way. We spent about a year going back and forth with the implications of every step, and then finally we stripped all the non absolutely required methods.

If you still have some interest I invite you to take a look at our implementation which addresses most of your concerns since it implements reasonable gating and checks. Here is an NPM package which contains core, non-opinionated, implementations and also ready to use implementations for this and other EIPs. The package is open source, here is the core implementation of this ERC. We also have a convenient wizard here, OZ style, to easily customize your implementations/

Finally, there are over a hundred collections deployed in our marketplace which are taking advantage of this and other standards we’ve proposed, currently we support Ethereum, Polygon, Base, Moonbeam and Astar. I invite you to take a look and reconsider your position after these clarifications.