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

Add ERC721ABatchBurnable extension #444

Merged

Conversation

jjranalli
Copy link
Contributor

@jjranalli jjranalli commented Jan 18, 2023

This PR introduces the ERC721ABatchBurnable extension, leveraging the _batchBurn function introduced here and then refactored in #450.

Assumptions

Some assumptions had to be made:

  • Batch operations accept a uint256[] tokenIds param, instead of startTokenId + quantity. This was done to:
    • Allow the function to be used the same way as other common batch burns
    • Allow to operate on non-consecutive tokenIds, thus being less limiting and more practical for the caller
  • The tokenIds argument assumes ordered ids
    • It is delegated to the caller making sure that tokenIds are in ascending order.

Notes

  • Added minor gas optimizations in _batchBurn
  • Added tests for ERC721BatchBurnable

@Vectorized
Copy link
Collaborator

O.O Sort.

@Vectorized
Copy link
Collaborator

Oh, possible to add in an approvalCheck parameter to the function?

Similar to _burn(tokenId, approvalCheck).

In case someone has a use case for doing batch transfers internally.

@jjranalli
Copy link
Contributor Author

Oh, possible to add in an approvalCheck parameter to the function?

Absolutely, on it. Just to confirm, it would result in the following functions:

  • _batchTransferFrom(from, to, tokenIds)
  • _batchTransferFrom(from, to, tokenIds, approvalCheck)
  • _safeBatchTransferFrom(from, to, tokenIds)
  • _safeBatchTransferFrom(from, to, tokenIds, approvalCheck)
  • _safeBatchTransferFrom(from, to, tokenIds, _data)
  • _safeBatchTransferFrom(from, to, tokenIds, _data, approvalCheck)

@Vectorized
Copy link
Collaborator

@jjranalli Yes!

@jjranalli
Copy link
Contributor Author

jjranalli commented Jan 18, 2023

@Vectorized approvalCheck flag added.

Also fixing some issues with the logic. Will push a new version soon

contracts/ERC721A.sol Outdated Show resolved Hide resolved
@jjranalli
Copy link
Contributor Author

The commit above is an attempt to ensure correctness of nextInitialized for all token IDs, while minimizing gas impact.

@Vectorized
Copy link
Collaborator

Vectorized commented Jan 25, 2023

I think the sort can use insertion sort. Then we can add a comment on passing in sorted tokenIds for best performance.
The full intro sort costs several hundred bytes of bytecode.

Thanks so much for the PR.

I’m wondering if we can also do batch burning.

@jjranalli jjranalli changed the base branch from main to batchBurnTest March 25, 2023 19:35
@jjranalli jjranalli changed the title Add batchBurn and batchTransferFrom + extensions Add batchBurn Mar 25, 2023
@jjranalli jjranalli changed the title Add batchBurn Add ERC721ABatchBurnable extension Mar 25, 2023
@Nlferu
Copy link

Nlferu commented Aug 7, 2024

Hi @Vectorized

Are there any updates on batchTransferFrom and batchBurn extensions? Any chance those will be included into ERC721A soon?

@Vectorized Vectorized changed the base branch from batchBurnTest to batchBurnable August 21, 2024 03:18
@Vectorized Vectorized merged commit b9f9485 into chiru-labs:batchBurnable Aug 21, 2024
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 this pull request may close these issues.

3 participants