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 batchTransferFrom and ERC721ABatchTransferable extension #458

Merged
merged 86 commits into from
Aug 20, 2024

Conversation

jjranalli
Copy link
Contributor

This PR introduces _batchTransferFrom and safe variants, as well as the ERC721ABatchTransferable extension.

The logic was initially introduced in #444, then redesigned based on _batchBurn introduced 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.

@jjranalli
Copy link
Contributor Author

@Vectorized we should be good here

@Vectorized Vectorized changed the base branch from main to batchTransferable August 20, 2024 18:37
@Vectorized Vectorized merged commit ba80d01 into chiru-labs:batchTransferable Aug 20, 2024
Vectorized added a commit that referenced this pull request Sep 9, 2024
* Add `batchTransferFrom` and `ERC721ABatchTransferable` extension (#458)

* added comments on transfer hooks

* added sort

* added clearApprovalsAndEmitTransferEvent

* added tokenBatchTransfer hooks

* added _batchTransferFrom and safe variants

* added ERC721ABatchTransferable extension and interface

* formatting

* added interface and ERC721ABatchTransferableMock

* added ERC721ABatchTransferable tests (wip)

* added approvalCheck

* fixed duplicate call

* comment

* fixed next initialized

* refactored lastInitPackedOwnership to use prevPackedOwnership

* comments

* ensured correctness of nextInitialized in slots of transferred token Ids

* renamed variables

* reverted to leave nextInitialized unchanged

* comment

* replace sort -> insertion sort

* bump: prettier-plugin-solidity

* prettier

* added prettier-ignore

* fixed nextTokenId in last array element

* tests wip

* refactor

* updated BatchTransferable mock and extension

* updated tests

* add approval tests

* lint

* lint fix

* restore original .prettierrc

* fix

* comments and refactor

* added _batchBurn

* added ERC721ABatchBurnable extension, interfaces and mock

* fixed _batchBurn

* fixed update of last tokenId + 1

* batchBurnable tests wip

* refactor

* fix

* add auto-clearing of consecutive ids and set `nextInitialized` to false

* batchTransfer tests refactor

* tests wip

* tests wip

* comments

* added extraData logic to batch mocks

* updated batch tests

* refactored ERC721A to use _updateTokenId

* wip

* comment

* Add ERC721ABatchBurnableMock (#450)

* change tokenIds in ascending order in test

* removal of unneeded internal functions

* prettier

* removed batch transfer logic

* changed _updateTokenId

* fixed mock

* fixed extension and mock

* fixed tests and cleaned unused functions in mock

* removed _updateTokenId

* minor gas optimizations

* comment

* optimize: avoid potential double read from storage

* removed bulkBurn from mock

* optimization: reset _packedOwnerships for initialized sequential IDs

* added tests for sequential ID clearing

* added test for tokenIds in strictly ascending order

* comment

* optimize: keep track of prevTokenOwner to bypass duplicated logic

* revert: resetting _packedOwnerships in initialized sequential IDs

* cleanup

* optimize: avoid potential double read from storage

* refactor _batchTransfer logic

* optimized and stacked not too deep

* optimize: removed unneeded exists() via getApproved

* removed unneeded functions and batchBurn

* Tidy, optimize

* Tidy

* Add test

* Tidy

* Tidy

* Tidy, optimize

* Combine batch burnable and transferable

* Edit comment

* Tidy tests

* Tidy

* Edit comment

* Remove unnecessary internal functions, max out test coverage

---------

Co-authored-by: jacopo <39241410+jjranalli@users.noreply.github.com>
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.

2 participants