-
Notifications
You must be signed in to change notification settings - Fork 841
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
Small gas optimization for burn/transfers/mint #334
Small gas optimization for burn/transfers/mint #334
Conversation
I think the burn and transfer parts shouldn't be in assembly. Gas savings not really worth the extra function. Plus, it may make it harder to extend (e.g. for the storage hitchhiking PR #323 ). Nice catch on the gas savings in the Maybe we can consider using assembly for emitting the uint256 end = startTokenId + quantity;
// Use assembly to save around 30+ gas per iteration.
assembly {
// Emit the `Transfer` event.
// The event signature is given by:
// `keccak256(bytes("Transfer(address,address,uint256)"))`.
log4(
0, // Start of data. Zero, as no data.
0, // End of data. Zero, as no data.
0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, // Signature.
0, // `address(0)`.
to, // `to`.
startTokenId // `tokenId`.
)
for {
let tokenId := add(startTokenId, 1)
} iszero(eq(tokenId, end)) {
tokenId := add(tokenId, 1)
} {
log4(0, 0, 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, 0, to, tokenId)
}
}
_currentIndex = end; |
if we're thinking in terms of overall costs, offsetting the deployment cost would only require ~500 transfers, ~166 burns or a combination of the two. for most collections, 500 transfers is trivial and thus imo super worth it if we're thinking in terms of devex, imo it's much more readable to have the extra function describing the process
it's quite easy to extend actually and in the case of the storage hitchhiking PR the gas savings are even more significant (41 gas for burn, 6 gas for transfer)
+1 |
Renamed the PR to be more descriptive |
updated gas savings: https://www.diffchecker.com/BS9LslEP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
first commit:
second commit:
https://www.diffchecker.com/rYLkEpYd