Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support EIP-2309: ERC-721 Consecutive Transfer Extension #2355

Closed
abcoathup opened this issue Sep 11, 2020 · 50 comments · Fixed by #3311
Closed

Support EIP-2309: ERC-721 Consecutive Transfer Extension #2355

abcoathup opened this issue Sep 11, 2020 · 50 comments · Fixed by #3311
Milestone

Comments

@abcoathup
Copy link
Contributor

abcoathup commented Sep 11, 2020

🧐 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

@frangio
Copy link
Contributor

frangio commented Sep 11, 2020

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.

@abcoathup
Copy link
Contributor Author

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

@abcoathup abcoathup changed the title Support ERC-721 Consecutive Transfer Extension Support EIP-2309: ERC-721 Consecutive Transfer Extension Sep 14, 2020
@arpu
Copy link

arpu commented Sep 24, 2020

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

@Soren74
Copy link

Soren74 commented Sep 25, 2020

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...

@abcoathup
Copy link
Contributor Author

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.

@Soren74
Copy link

Soren74 commented Sep 25, 2020

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?

@abcoathup
Copy link
Contributor Author

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.

@Soren74
Copy link

Soren74 commented Sep 28, 2020

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.

@abcoathup
Copy link
Contributor Author

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.

@b0xtch
Copy link

b0xtch commented Nov 10, 2020

@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
Copy link

b0xtch commented Nov 10, 2020

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

@Soren74
Copy link

Soren74 commented Nov 10, 2020

@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!

@b0xtch
Copy link

b0xtch commented Nov 10, 2020

@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.

@poria-cat
Copy link

@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

@veqtor
Copy link

veqtor commented Feb 17, 2021

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.

@abcoathup
Copy link
Contributor Author

Hi @veqtor,

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

@veqtor
Copy link

veqtor commented Feb 19, 2021

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.

@frangio
Copy link
Contributor

frangio commented Feb 19, 2021

@veqtor What is the issue with tokenOfOwnerByIndex?

@veqtor
Copy link

veqtor commented Feb 22, 2021

@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
Copy link

veqtor commented Mar 9, 2021

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

@superphly
Copy link

@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.

@frangio
Copy link
Contributor

frangio commented Aug 9, 2021

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.

@zjpetersen
Copy link

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.

@gavinyue
Copy link

gavinyue commented Sep 7, 2021

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?

@zjpetersen
Copy link

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.

@gavinyue
Copy link

gavinyue commented Sep 8, 2021

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.

@Revinand
Copy link

Revinand commented Nov 3, 2021

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.

@zjpetersen
Copy link

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.

@Revinand
Copy link

Revinand commented Nov 3, 2021

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.

@frangio
Copy link
Contributor

frangio commented Nov 14, 2021

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

@frangio frangio closed this as completed Nov 14, 2021
@bc-1998
Copy link

bc-1998 commented Dec 19, 2021

ERC1105

Hi @zjpetersen. Do you know what would be the cap for number of NFTs can be minted in a batch? Polygon's block gas limit is 20Million. And what would be a feasible implementation to mint 500,000-1,000,000 NFTs at a time?

@0xdeployer
Copy link

Can we re-open this? Here's a great implementation of ERC-2309 which is added to the ERC-721a contract chiru-labs/ERC721A#311

OpenSea and LooksRare support this on mainnet apparently. Pushing for wider support of this standard given the constant gas price for batch mints. Most of the top collections on opensea use consecutive token IDs during minting and include batch minting functionality. If this standard becomes widely used it would save users tens of millions of dollars in gas fees over time. Savings for contract w/ 2309 is represented as ERC-721ab below

image

@frangio
Copy link
Contributor

frangio commented Jun 8, 2022

Contrary to what I expressed earlier in this thread, it seems that EIP-2309 is compatible with ERC-721 because the latter specifies Transfer events are not necessarily emitted in the constructor. So as long as EIP-2309 mints are done exclusively in the constructor they are fine. We can reopen this and will be considering it for the next release.

@frangio frangio reopened this Jun 8, 2022
@frangio frangio added this to the 4.8 milestone Jun 8, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jun 9, 2022

ERC-721 specifies Transfer events are not necessarily emitted in the constructor.

I will personally STRONGLY oppose any code that mints a token without emitting the Transfer event!

It being done in the constructor should not change anything

(Just like I will oppose any ERC721 contract that mints a batch without emitting event for each single token)

@0xdeployer
Copy link

@Amxx why? People want to pay less when minting multiple NFTs. This accomplishes that.

@Amxx
Copy link
Collaborator

Amxx commented Jun 9, 2022

It breaks a lot of the indexing services.

There are ways to reduce minting cost by greatly reducing storage without changing the observable behavior. Breaking the events pattern goes one step to far to me.

@0xdeployer
Copy link

Got it. Thanks for explaining. I think ERC721A made by the Azuki devs optimizes the current erc721 spec as far as it will go. You can see the constant gas price 2309 brings in the image above. With O(n) transfer events it will never be as cheap as what 2309 would allow. Opensea and LooksRare both support the standard. If indexing services is the only reason not to support we should add it so indexing services have to update their systems.

@frangio
Copy link
Contributor

frangio commented Jun 9, 2022

It being done in the constructor should not change anything

It's allowed by the spec though. In a sense, ERC-2309 is even an improvement over pure ERC-721, because it adds an event (ConsecutiveTransfer) to complement the lack of event allowed by ERC-721.

@Amxx
Copy link
Collaborator

Amxx commented Jun 10, 2022

IMO it's similar to some ERC20 not emitting a transfer event when minted. That is tolerated by the ERC...

My point is that an ERC having special cases should not be a reason for leveraging the special cases if that leads to behaviour most people would not expect.

@Amxx
Copy link
Collaborator

Amxx commented Jun 10, 2022

This contract on rinkeby

  • minted tokens 0, 17 and 42 without an event (during construction)
  • minted token 1 with an event (after construction)
  • minted token 100 to 110 (included) with a ConsecutiveTransfer event

You can check that balanceOf(0xF037353a9B47f453d89E9163F21a2f6e1000B07d) is 15 and that the owner if defined for all these tokenIds

Result on explorers (feel free to add more)

@Amxx
Copy link
Collaborator

Amxx commented Jun 10, 2022

In thing it that, even if we could possibly not emit the ERC721 event, we would still have to call the _beforeTokenTransfer and _afterTokenTransfer hooks on all tokens, or would we add _beforeConsecutiveTokenTransfer and _afterConsecutiveTokenTransfer hooks?

This would be a significant breaking change if people start using this without properly refactoring their hooks.

One last thing is, when I think about smart contract security & composability of systems, it goes beyond what happens onchain. Some changes might be ok if you forget the event parts, but IMO they should not be seen that way. Things as simple as event reordering could possibly break indexing services, and I would argue this is critical. Its not just about having the "ownerOf" and "balanceOf" function return the right values, its also about how other systems (beyond contracts) are going to interact with it.

@Amxx
Copy link
Collaborator

Amxx commented Jun 10, 2022

In 2309, the sequence is never properly defined, is it [fromTokenId, toTokenId[ or [fromTokenId, toTokenId]

In other words, is it
for (uin256 tokenId = fromTokenId; tokenId < toTokenId; ++tokenId) { ... }
or
for (uin256 tokenId = fromTokenId; tokenId <= toTokenId; ++tokenId) { ... }

@frangio
Copy link
Contributor

frangio commented Jun 10, 2022

Thanks for putting together that test on Rinkeby, it's very informative. These are pretty strong arguments against using the ERC.

As usual though it's a chicken and egg problem... If all platforms supported EIP-2309 this would be easier to consider.

@0xdeployer
Copy link

I've talked to Etherscan about supporting this standard and they needed an implemented, verified contract to look at. Now that that exists I think it's worth reaching out again. Platforms will add support if there's community support. Community support happens once there's an open-sourced contract that easy to use. It seems that this will be added to the 721a contract. May be worth seeing how that goes if there's some hesitation to adding to OZ?

@frangio
Copy link
Contributor

frangio commented Jun 10, 2022

Sure, keep us updated.

@TimTinkers
Copy link

TimTinkers commented Jul 30, 2022

@Amxx thank you for putting together that test. I'd also like to just chime in as someone building an NFT indexing service that I am also generally against minting multiple NFTs without emitting at least one event per NFT. I'm mostly concerned with having to truncate ConsecutiveTransfer ranges to prevent denial of service.

It looks like the Chiru Labs implementation has a limit of 5,000 on the size of an EIP-2309 range. I'm not sure why though and have asked @Vectorized to help me understand. That is the OpenSea limit on the size of an EIP-2309 range.

If OpenZeppelin adds support for EIP-2309 I think the default implementation should limit EIP-2309 batch sizes to 5,000.

@Amxx
Copy link
Collaborator

Amxx commented Jul 30, 2022

Should it be limited to 5000 per event, or 5000 per contract ?

@Vectorized
Copy link
Contributor

We restricted to 5000 per event for two reasons:

  • Through empirical testing, we have determined that OpenSea supports a maximum of 5000 per transaction. Not ideal from a purist POV, but for practical reasons.
  • Prevent overflows.

@xinbenlv
Copy link
Contributor

xinbenlv commented Nov 28, 2022

FYI, compatibility issue of EIP-2309 with ERC-721 have been raised per this discussion and this discussion

@frangio
Copy link
Contributor

frangio commented Nov 29, 2022

There is no compatibility issue. Please open a new issue if you still believe otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.