openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

Support EIP-2309: ERC-721 Consecutive Transfer Extension

Open abcoathup opened this issue 5 years ago • 48 comments

🧐 Motivation Support EIP-2309: ERC-721 Consecutive Transfer Extension Enables minting any number of tokens in a single transaction

📝 Details

EIP is an extension to ERC-721, for a standardized event emitted when creating/transferring one, or many non-fungible tokens using consecutive token identifiers. https://github.com/ethereum/EIPs/blob/master/EIPS/eip-2309.md

EIP is final status.

Creating issue for discussion on how EIP could be supported

abcoathup avatar Sep 11 '20 02:09 abcoathup

It seems that the EIP only standardizes an event, we have to figure out what it means for us to support it. Should we add an ERC721 flavor with batch minting/transfering?

Also worth noting that our ERC721 preset uses consecutive ids so we may want it to emit this event.

frangio avatar Sep 11 '20 14:09 frangio

I'm interested to see an example implementation. I hope that Sean Papanikolas will be able to share one.

abcoathup avatar Sep 14 '20 02:09 abcoathup

maybe https://github.com/pizzarob can help on this? i am also interested in this Extension

arpu avatar Sep 24 '20 16:09 arpu

Yes, I too am very curious about how to actually implement this extension.

I've tried creating a quick Contract that is ERC721Full -I then added to it the ConsecutiveTransfer(...) event declaration (which the dev. said we're supposed to use): event ConsecutiveTransfer(uint256 indexed fromTokenId, uint256 toTokenId, address indexed fromAddress, address indexed toAddress); -I made a for loop in the constructor to mint 100 Tokens -but then... I realized I can't call the inherited super._mint function in the loop like we're used to doing, as follows: super._mint(msg.sender, tempColorCounter);

right? Because that mint function internally calls the Transfer event, which dev explicitly said we're NOT supposed to use anymore. Instead we're supposed to use his new ConsecutiveTransfer(...) event.

So... color me confused.

Also noticed none of dev's contracts have been verified on etherscan...

Soren74 avatar Sep 25 '20 00:09 Soren74

Hi @arpu & @Soren74,

I have reached out to Sean Papanikolas to ask if they can share an implementation.

@Soren74 my assumption is that you don't use any loops (as this would have limits on the amount of tokens that could be minted), instead you need to use additional state to capture the start and end of a series of tokens.

abcoathup avatar Sep 25 '20 04:09 abcoathup

Hi @abcoathup,

Can you elaborate on what you wrote: "you need to use additional state to capture the start and end of a series of tokens" - cause I'm not sure I follow. What would capturing this state do? Meaning how would it play into mass-minting tokens?

Also, in terms of using a for loop, you wrote: "this would have limits on the amount of tokens that could be minted" - but I'm thinking that the upper limit would be passed in as an argument. Meaning you don't do the looping in the Constructor but rather in another function (call it mintMany(uint256 howMany)), so that every time you call it you can pass in the number of tokens you want minted, giving you that flexibility.

Thoughts?

Soren74 avatar Sep 25 '20 14:09 Soren74

Hi @Soren74 ,

The EIP gives examples of minting, transfer and burning large quantities of tokens:

Batch token creation emit ConsecutiveTransfer(1, 100000, address(0), toAddress);

If you had a for loop doing individual mint/transfer/burns then I assume this function would exceed the gas limit for large quantities. That is why I assume that additional state would be required in the token to track this functionality.

I am interested to see an implementation of the EIP to see how this is being done.

abcoathup avatar Sep 28 '20 05:09 abcoathup

Hey @abcoathup,

I'm not sure I interpreted ConsecutiveTransfer the way you did. Put it this way: to me, merely emitting this ConsecutiveTransfer event doesn't actually mint anything. I think it announces that you've minted something, but it doesn't actually do any of that minting. At least that's the way I understand it.

So the EIP isn't really giving us any examples of anything - other than making announcements.

Check out Sean Papanikolas' post in the Docs section of his Cargo.build website (at: https://docs.cargo.build/advanced-information/batch-minting-non-fungible-tokens):

"with the Consecutive Transfer extension you could emit one event to track the creation of all 2^255 NFTs. It could look something like this:

function mint(uint amount, address from, address to) public {
  
  // Create NFTs and assign ownership
  // ...
  // Emit Consecutive Transfer Event
  emit ConsecutiveTransfer(lastMintedId + 1, lastMintedId + amount, from, to);
}

"The lastMintedId would be the last consecutive token that was created. For example lastMintedId would initially be 0 . After creating the first NFT it would become 1 . Then if you created five more NFTs the range of NFTs created would be 2 to 6 . "

So the way I read what he's saying there is that you can have your for loop, put all your minting code in it - meaning create your Token object/struct, add it to the various mappings or arrays you're using to keep track of ID's, Ownership, etc. - but don't emit any Transfer events in this loop! Instead, upon exiting the loop, emit the ConsecutiveTransfer event once - and only once, and that should take care of everything.

Again, I could be dead wrong about this, but that's what I think he's talking about.

What do you think?

Gotta say, I feel like if we put our minds together we could figure this out - cause Mr. Papanikolas sure doesn't seem to wanna share the knowledge at this point, which is quite disappointing - but also rather revelatory.

Soren74 avatar Sep 28 '20 13:09 Soren74

Hi @Soren74,

I assume that the contract needs to do some tracking of what it has minted and who owns what consecutive tokens, which is why I assume that additional state is needed. Ideally Sean and the team from Cargo will be able to share an implementation, rather than having to reinvent the wheel.

abcoathup avatar Sep 29 '20 10:09 abcoathup

@abcoathup I started on a new implementation that builds on top of openzeppelin ERC721 implementation.

  1. I added the event ConsecutiveTransfer() event to IERC721.sol
  2. Added a new function named _batchMint() to ERC721.sol which is similar to _mint() but I removed the Transfer event.
  3. Now I am testing a data structure that is able to keep track of a range of tokens to a single address. mapping(address => Tokens[]) hodlers; This the first thing that came to mind.
  function batchMint(address to) public virtual onlyAdmin {
    uint256 fromTokenId = _tokenIds.current();

    /// fancy data structure

    emit ConsecutiveTransfer(fromTokenId, _tokenIds.current(), address(0), to);
  }
  • We can loop and batch mint but you'll run out of gas pretty fast.
  • This might be a wrong approach, would be awesome if someone can share or discuss their data structure implementation.

b0xtch avatar Nov 10 '20 00:11 b0xtch

@Soren74 recently I got started with erc2309, so if you want to share ideas lmk!

b0xtch avatar Nov 10 '20 00:11 b0xtch

@BotchM Yeah, I was very excited to do this - but the guy who developed 2309 hasn't responded to any of us, so it's kinda stuck. I must say this really reflect very poorly on him as his refusal to share implementations completely goes against the very ethos of GitHub and what it's all about. Oh well.

I personally don't have any clue how to implement this - whatever ideas or questions I had I've already posted up above, so if any of those help you somehow, run with 'em! And of course I'd be happy to see what you're doing and comment on it, try to contribute to it, etc.

Thanks!

Soren74 avatar Nov 10 '20 15:11 Soren74

@Soren74 Sounds good! I am sure they have their reasons. Sean also created cargo which is a platform for batch minting, so I guess it's fair to be competitive and not share the whole sauce. It would be awesome to even just get an example or even the data structure used. I only started iterating over an implementation, so I will definitely share if I get anywhere.

b0xtch avatar Nov 10 '20 22:11 b0xtch

@BotchM @Soren74 Hi, In my view, EIP-2309 just send a special event and update id number like "from" and "to". for example: " now lastMintId is 100 and you want mint another 200 nft, so just change lastMintId to 300 and send a event". because it is not use for loop, so the gas is cheap also see this

poria-cat avatar Jan 24 '21 03:01 poria-cat

Is this EIP accepted to the point where Etherscan etc support the ConsecutiveTransfer event?

I think the only way to implement this is to have two layers of ownership: Base is mint-layer, which is a list of (owner, number_of_tokens) tuples. To check owner you need to loop through that list, accumulating until you get to tokenIdx you want to query ownership of. This list needs to be append only.

Next layer is transferance-layer, in which you have a map of [token-idx]->[owner] If you don't find token in this list you fall back to mint-layer for ownership.

veqtor avatar Feb 17 '21 15:02 veqtor

Hi @veqtor,

I haven't seen any details on Etherscan supporting EIP-2309.

abcoathup avatar Feb 19 '21 01:02 abcoathup

No matter how I twist and turn this around in my head I can't seem to arrive at an efficient implementation of this... Not if I'm also going to respect IERC721Enumerable.tokenOfOwnerByIndex The best idea I have so far is some kind of linked-list type thing with ranges.

veqtor avatar Feb 19 '21 12:02 veqtor

@veqtor What is the issue with tokenOfOwnerByIndex?

frangio avatar Feb 19 '21 17:02 frangio

@veqtor What is the issue with tokenOfOwnerByIndex?

Nothing really except with the approach I'm taking to implementing this I can't get around spending annoying amounts of gas.

veqtor avatar Feb 22 '21 07:02 veqtor

https://github.com/veqtor/ERC2309 <- super-experimental but maybe it can serve as an inspiration for a proper implementation

veqtor avatar Mar 09 '21 17:03 veqtor

@veqtor - that's really cool man, but the ownedRanges stuff is kinda complex and sorta changes a lot of the standard ERC-721 basics. I'm working on a solution that's a little different, but still doesn't feel right.

At the end of the day, we're still battling these basic limitations of the Solidity language.

superphly avatar Mar 13 '21 07:03 superphly

Has any NFT marketplace/platform implemented support for EIP-2309?

We have serious doubts about offering an implementation of this because we feel that an EIP-2309 token will not be well supported due to the lack of Transfer events. End users will not see their tokens listed in platforms that only look for those events.

frangio avatar Aug 09 '21 20:08 frangio

Hi @frangio, I tested this out and OpenSea does support this. However, Rarible and others didn't.

I created a ERC2309 implementation for this. It's a bit different than @veqtor's. There are a couple limitations, but it gives us constant gas fees and is pretty similar to ERC721.

zjpetersen avatar Sep 02 '21 02:09 zjpetersen

Hi @frangio, I tested this out and OpenSea does support this. However, Rarible and others didn't.

I created a ERC2309 implementation for this. It's a bit different than @veqtor's. There are a couple limitations, but it gives us constant gas fees and is pretty similar to ERC721.

Hey, @zjpetersen, read your implementation. The ownerOf function seems does not work?

gavinyue avatar Sep 07 '21 23:09 gavinyue

Hey @gavinyue thanks for taking a look, I appreciate it.

I believe ownerOf works. Only thing I see is that you can't use transferOwnership, so that method should be disabled.

Just to summarize the ownerOf logic:

  • If minting hasn't occurred the require statement will always fail, since _tokenCount is 0.
  • If we've minted, _tokenCount gets set, but _owners is still empty. So the if statement will be true and the contract owner (the minter) gets returned.
  • If we've minted and transferred the token, then we'll just return the token owner like before.

Let me know your thoughts, I could be missing something.

zjpetersen avatar Sep 08 '21 02:09 zjpetersen

Hey @gavinyue thanks for taking a look, I appreciate it.

I believe ownerOf works. Only thing I see is that you can't use transferOwnership, so that method should be disabled.

Just to summarize the ownerOf logic:

  • If minting hasn't occurred the require statement will always fail, since _tokenCount is 0.
  • If we've minted, _tokenCount gets set, but _owners is still empty. So the if statement will be true and the contract owner (the minter) gets returned.
  • If we've minted and transferred the token, then we'll just return the token owner like before.

Let me know your thoughts, I could be missing something.

Got it. It limits range mint can only happen when the total number is zero. It does simplify things, especially working well with the marketplace to have lazy-minting. While @veqtor's implementation support range-mint for a number of tokens to a given address at any time.

Wonder would be nice to borrow the _mintBatch concept from ERC1105 to further simplify things.

gavinyue avatar Sep 08 '21 16:09 gavinyue

Hi @frangio, I tested this out and OpenSea does support this. However, Rarible and others didn't.

I created a ERC2309 implementation for this. It's a bit different than @veqtor's. There are a couple limitations, but it gives us constant gas fees and is pretty similar to ERC721.

@zjpetersen How did you import your contract on OpenSea? Just tried and failed with the error - 'We couldn't find this contract. Please ensure that this is a valid ERC721 or ERC1155 contract deployed on Mumbai and that you have already minted items on the contract.' Also your example collection is not on OpenSea anymore.

Revinand avatar Nov 03 '21 16:11 Revinand

Hey @Revinand. Annoying it only worked for me on Rinkeby, when I tried later on Mumbai/Polygon main net it didn’t work. It’s pretty frustrating that each network has a different feature set…

Since I ended up deploying on Polygon I just went with standard minting since gas costs are pretty reasonable.

zjpetersen avatar Nov 03 '21 16:11 zjpetersen

I see, thanks for the answer.

Yeah, we decided to use Polygon too because of low transactions costs. It's really sad to know that no one (except Cargo) support this standard.

Revinand avatar Nov 03 '21 18:11 Revinand

Because of the lack of support we don't plan to implement this EIP.

frangio avatar Nov 14 '21 23:11 frangio