estarriolvetch / ERC721Psi

MIT License
117 stars 29 forks source link

use bytemap in place of the AddressData struct for further optimizations #33

Closed nidhhoggr closed 1 year ago

nidhhoggr commented 1 year ago

Depsite the compiler being able to handle this under the hood as the AddressData struct was 256 bits, apprently there is overhead indeed as others have found. The feature branch above saves ~700 gas on mints, burns and transfers and about ~6000 gas on deployments for the AddressData extension.

Associated benchmark: https://github.com/nidhhoggr/ERC721PsiReports/commit/63160b5c99246bee581f5808fcb0740562563ec4

For ease of merging, this branch should only be merged into v0.8 only after #32 is merged. Let me know if and when to submit the PR.

nidhhoggr commented 1 year ago

Note: Without the usage of unchecked, the gas savings are ~500 on mints, burns and transfers. I felt using unchecked to be safe because the require statement asserts the quantity is less than 64 bits. Another assumption with unchecked is that it's impossible to undeflow e.g. transfering or burning a token you don't own on a zero balance

nidhhoggr commented 1 year ago

The next commit above saves an additional ~300 gas on burns and mints, ~50 gas on transfers and ~2000 gas on deployments. It's able to achieve this by restructuring from/to conditionals resulting in three cases where a particular mapping slot is written to only once using bitmasking. This additional commit would total ~1000 savings on mints and burns.

Associated isolated benchmark: https://github.com/nidhhoggr/ERC721PsiReports/commit/de8f5afb5c5bf8f1eb93c401ffb6592272f6b57e

estarriolvetch commented 1 year ago

@nidhhoggr Looks pretty good to me!

estarriolvetch commented 1 year ago

merged