Add `ERC721ABatchBurnable` extension

[jjranalli]

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]

O.O Sort.

[Vectorized]

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]

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]

@jjranalli Yes!

[jjranalli]

@Vectorized approvalCheck flag added.

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

[jjranalli]

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

[Vectorized]

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]

Perfect, makes sense! Switched to insertion sort.

I had to bump prettier-plugin-solidity as it was causing an error during lint preventing checks to go through. However that also caused formatting to change across other contracts. Lmk how you want me to handle that

Will take care of tests now

I’m wondering if we can also do batch burning

Absolutely, we can use same concept on burn too. Happy to take care of that as well, maybe in a separate PR

[jjranalli]

@Vectorized Apologies for the long wait! This is now ready for review

Notes

• Fixed issue in _batchTransferFrom that could result in the tokenId subsequent to the last element in tokenIds not being initialized correctly
• Written tests for batch transfers and approvals

[jjranalli]

I had to bump prettier-plugin-solidity as it was causing an error during lint preventing checks to go through. However that also caused formatting to change across other contracts.

Also restored formatting to how it was. Hopefully checks will go through now.

[jjranalli]

@Vectorized It's done! 🫡 I ended up adding batch burns directly in this PR as it leverages common logic to that written for batch transfers. PR got a bit large but hope that's not an issue

Main changes

Batch transfers

• fixed a bug that prevented lastTokenId + 1 to be initialized depending on params passed
• previously initialized slots had the nextInitialized unchanged during batch transfers. Now they are set to false and (unless extraData is present) any consecutive tokenId is set to 0 to maximise gas savings without compromising functionality
• more tests

batchBurn

• shares similar logic of batchTransferFrom, with some differences related to params passed and intended behaviour
• ERC721ABatchBurnable extension, interfaces and mocks have been added
• tests

Notes

• I took the liberty to refactor some existing code using the added _updateTokenId to reduce bytecode size and improve readability.

[jjranalli]

@Vectorized should I make changes to this PR based on #450?

[jjranalli]

@Vectorized Managed to have a look at #450, awesome stuff over there. A few questions while I merge it in here and clean things up:

• What is the rationale behind the burner param instead of using msg.sender?
• _beforeTokenTransfers and _afterTokenTransfers are executed within the inner do-while loop, while storage variables are updated in the parent for loop during mini-batches. This can cause unexpected consequences if the hooks need to access _packedOwnerships, _packedAddressData or _burnCounter, and in general for a given tokenId they become basically interchangeable since the only difference between them is the moment when they are called in reference to the Transfer event. Is this acceptable? If so it needs to be properly documented. Otherwise the logic I proposed accounted for this by using two separate hooks, but I'm aware it can be inconvenient for different reasons.
• How about adding an internal _batchBurnUnsorted in the ERC721ABatchBurnable extension, that sorts the tokenIds before calling _batchBurn?

Also was trying to optimise gas in this commit by resetting _packedOwnerships for sequential IDs, but then realised that without additional ownership checks it would allow burner to burn any token basically. Might be a better way to handle it.

[Nlferu]

Hi @Vectorized

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