Inconsistent IERC1155 calldata vs. memory usage

[engn33r]

🧐 Motivation

ERC1155 receiver functions onERC1155Received() and onERC1155BatchReceived() are declared with calldata args in some places and memory args in other places. I would expect consistency across this library, unless there is a clear explanation to be inconsistent.

📝 Details

Use calldata or memory for all function args for onERC1155Received() and onERC1155BatchReceived() throughout the library. For reference, the EIP1155 spec uses calldata.

Calldata arg usage:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a72c9561b9c200bac87f14ffd43a8c719fd6fa5a/contracts/token/ERC1155/IERC1155Receiver.sol#L52-L57
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a72c9561b9c200bac87f14ffd43a8c719fd6fa5a/contracts/mocks/token/ERC1155ReceiverMock.sol#L53-L58

Memory arg usage:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a72c9561b9c200bac87f14ffd43a8c719fd6fa5a/contracts/token/ERC1155/utils/ERC1155Holder.sol#L33-L38
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a72c9561b9c200bac87f14ffd43a8c719fd6fa5a/contracts/governance/Governor.sol#L697-L702

[ernestognw]

Thanks for reporting @engn33r.

The reason why we use memory across our implementations is because calldata can't be constructed from within the contract. Public functions can't be called internally if they don't use calldata. This explains mismatches between the interfaces defined with calldata and the implementations with memory. Note that the implementations are still compliant because the function selector remains the same.

A good example of this is ERC1155-balanceOfBatch.

However, I agree onERC1155BatchReceived is likely to be called only externally, but I'm not sure about that. We've previously had discussions considering using calldata, but it's a matter of analyzing every single use of memory to evaluate whether it makes sense to use calldata instead.