openzeppelin-contracts
openzeppelin-contracts copied to clipboard
OpenZeppelin
Use calldata to reduce gas costs in ERC1155
We can use calldata instead of memory in several places in ERC1155 (as well as the mocks/extensions). This will both save gas and also keep the implementation function signatures the same as the interface function signatures (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/IERC1155.sol). The EIP itself has the input parameters as calldata: https://eips.ethereum.org/EIPS/eip-1155.
I ran the full set of ERC1155 tests (env ENABLE_GAS_REPORT=true npx hardhat test --grep ERC1155) and got these gas savings: https://www.diffchecker.com/0Mac8iiV.
PR Checklist
- [x] Tests
- [ ] Documentation
- [x] Changelog entry
Thank you @cygaar for the PR and the gas saving analysis. Note that since last week we have a workflow that generates these reports automatically, just check the test summary for your commit, and its all there
Super convenient, looks like the gas went down for all the 1155 tests. For some reason ERC20VotesMock gas went up, but I'm guessing this might be some sort of transient result since I didn't touch anything outside of 1155.
The gas usage is good.
The thing is that is also limit some usages, such as an additional function (added by a dev) calling batchBalanceOf. Basically this change might be breaking some usecases.
We'll discuss that internally
I hear you, but my arguments for adding in this change are:
- I don't think anything should be breaking per se - calldata is allowed in internal function calls. At worst it'll increase gas costs if you call any of these functions internally. If you think about how often someone will be implementing custom functionality vs the default usecase of this contract, the savings seem worth it.
- The EIP itself has these arguments as
calldata. Devs should come in expecting these functions to already be calldata because at the end of the day, this library is implementing EIP1155.
About point 2:
The EIP writes calldata because the EIP describes an interface, with external functions ... and at the time external function only supported calldata. If you have a look at our repo, you'll see that the interface uses calldata, just like the EIP, and that its the implementation that "overrides" that to be memory.
Now about point 1:
It is not possible for a contract to "build" an array (or a struct) in calldata. Only in memory. That means that if you have a function that wants to create a list of tokensIds (and not get it as an argument) it will be memory, and you can't simply pass it to a calldata function. You can do this.functionName(myArrayInMemory) but that is more expensive and it changes the execution context.
I'm not too worried about balanceOfBatch because it's easy to call balanceOf in a loop, so creating an in memory array to then pass it to balanceOfBatch doesn't sound right anyway.
The _safeBatchTransferFrom is more concerning though. Not being to call it with internally built memory arrays is an issue IMO.
I'm not sure there is a benefit to making safeBatchTransferFrom calldata if _safeBatchTransferFrom is not
Can you elaborate more on
it changes the execution context.?
Lets say I have this contracts:
contract Test {
event SomeEvent(address caller);
function something() public {
emit SomeEvent(msg.sender);
}
function endpoint1() public {
something();
}
function endpoint2() public {
this.something();
}
}
If you call endpoint1, msg.sender used when emitting the event is your address. If you call endpoint2, the msg.sender is the address of the contract itself.
When calling safeTransferFrom this distinction is important.
I think the gas savings are too small (<1%) to justify the breaking change.
This PR has gotten old and is a breaking change because it limits the current functionality. We just released 5.0 and I agree the gas savings are too small.
Closing.
