V0.8

[estarriolvetch]

Related issues/PRs

• Adds _startTokenId overrideability #15
• Update test suite to improve coverage and code reusability #24
• using Solady LibBitmap speeds burn by several hunded (~500 avg) gas #17
• Upgrade openzeppelin to v5 and chainlink to v8 #45

[estarriolvetch]

• Properly cite ERC721A

[estarriolvetch]

@nidhhoggr We have everything for v0.8, right?

[nidhhoggr]

Aside from updating the benchmark scripts as discussed in #31 I don't have any other feature recommendations for this release. Perhaps testing and benchmark related stuff should be handled in a separate release including artifact generation for gas reports. The only other concern I have is the usage of constants in diamond storage contracts. My concern is that the constants (which pertains to the contract itself) are stored in a separate "slot" from those variables accounted for in the storage structs. The issue arrises when someone decides to add another constant to an extension where it could more easily result in storage collisions. Not sure what your thoughts on it are but here is an example:
https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/extension/ERC721PsiRandomSeedRevealUpgradeable.sol#L27

Maybe it might be better to abstract constant declaration for upgradable contracts to a library? Not sure, still something I have to research best practices on.

[nidhhoggr]

More importantly, the ERC721PsiUpgradeable contract also suffers from this concern. Specifically that two constants are declared. Is it possible to remove both of those constant declarations.
_BITMASK_ADDRESS is only used once.
_TRANSFER_EVENT_SIGNATURE is used twice but we can refactor the _mint to only use it once. For some reason the assembly calls log4 twice with the signature but we can change the looping index so that it only needs it once.
https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/ERC721PsiUpgradeable.sol

[nidhhoggr]

More importantly, the ERC721PsiUpgradeable contract also suffers from this concern. Specifically that two constants are declared. Is it possible to remove both of those constant declarations. _BITMASK_ADDRESS is only used once. _TRANSFER_EVENT_SIGNATURE is used twice but we can refactor the _mint to only use it once. For some reason the assembly calls log4 twice with the signature but we can change the looping index so that it only needs it once. https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/ERC721PsiUpgradeable.sol

I Address the problem here #35

[estarriolvetch]

My understanding is constants and immutable are in the bytecode, so they won't collide with the storage. Sent from Proton Mail mobile

…

-------- Original Message --------

On Jan 23, 2023, 7:28 PM, Joseph Persico wrote: Aside from updating the benchmark scripts as discussed in [#31](#31) I don't have any other feature recommendations for this release. Perhaps testing and benchmark related stuff should be handled in a separate release including artifact generation for gas reports. The only other concern I have is the usage of constants in with diamond storage contracts. My concern is that the constants (which pertains to the contract itself) are stored in a separate "slot" from those variables accounted for in the storage structs. The issue arrises when someone decides to add another constant to an extension where it could more easily result in storage collisions. Not sure what you're thoughts on it are but here is an example: https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/extension/ERC721PsiRandomSeedRevealUpgradeable.sol#L27 Maybe it might be better to abstract constant declaration for upgradable contracts to a library? Not sure, still something I have to research best practices on. — Reply to this email directly, [view it on GitHub](#27 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ASKF2QVQWIRLD2RNFEWID3DWT4OZNANCNFSM6AAAAAATJEHINI). You are receiving this because you authored the thread.Message ID: ***@***.***>

[estarriolvetch]

Not sure if etherscan or opensea supports it, but I think it may help reduce the event cost. https://eips.ethereum.org/EIPS/eip-2309 Sent from Proton Mail mobile

…

-------- Original Message --------

On Jan 23, 2023, 11:03 PM, Joseph Persico wrote: > More importantly, the ERC721PsiUpgradeable contract also suffers from this concern. Specifically that two constants are declared. Is it possible to remove both of those constant declarations. _BITMASK_ADDRESS is only used once. _TRANSFER_EVENT_SIGNATURE is used twice but we can refactor the _mint to only use it once. For some reason the assembly calls log4 twice with the signature but we can change the looping index so that it only needs it once. https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/ERC721PsiUpgradeable.sol I Address the problem here [#35](#35) — Reply to this email directly, [view it on GitHub](#27 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ASKF2QTRPLKPCNCVPR5UZPDWT5IBNANCNFSM6AAAAAATJEHINI). You are receiving this because you authored the thread.Message ID: ***@***.***>

[nidhhoggr]

My understanding is constants and immutable are in the bytecode, so they won't collide with the storage. Sent from Proton Mail mobile
…
-------- Original Message --------
On Jan 23, 2023, 7:28 PM, Joseph Persico wrote: Aside from updating the benchmark scripts as discussed in #31 I don't have any other feature recommendations for this release. Perhaps testing and benchmark related stuff should be handled in a separate release including artifact generation for gas reports. The only other concern I have is the usage of constants in with diamond storage contracts. My concern is that the constants (which pertains to the contract itself) are stored in a separate "slot" from those variables accounted for in the storage structs. The issue arrises when someone decides to add another constant to an extension where it could more easily result in storage collisions. Not sure what you're thoughts on it are but here is an example: https://github.com/estarriolvetch/ERC721Psi/blob/v0.8/contracts/extension/ERC721PsiRandomSeedRevealUpgradeable.sol#L27 Maybe it might be better to abstract constant declaration for upgradable contracts to a library? Not sure, still something I have to research best practices on. — Reply to this email directly, [view it on GitHub](#27 (comment)), or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

Interesting, you are probably correct. that seems to be the general consensus according to https://ethereum.stackexchange.com/questions/140628/where-are-the-smart-contract-constants-stored

With that being said, what #35 also achieves is about ~100+ in gas savings and it makes the _mint function DRYer by removing double calls to log4. It's up to you if you still want to merge it.

[nidhhoggr]

https://eips.ethereum.org/EIPS/eip-2309

That looks interesting. At the very least an extension could implement this by overriding the _mint function until it becomes fully supported.

[estarriolvetch]

I think if it works for etherscan and opensea. We can probably integrate it into the base contract.

[nidhhoggr]

https://eips.ethereum.org/EIPS/eip-2309

So far from my research, in order to remain ERC721 compliant this event can only be emitted on contract creation. And while marketplaces do support this event, they only do so with a limit. Also, safeMint must still emit the Transfer event because that's how it checks it was safely minted.

This is a repo that demonstrates the gas savings with ConsecutiveTransfer replaced in the mint function:
0xdeployer/ERC721a-w-2309@b938a6f

Further debate on the matter starting here:
OpenZeppelin/openzeppelin-contracts#2355 (comment)

[nidhhoggr]

Also, safeMint must still emit the Transfer event because that's how it checks it was safely minted.

Still need to research this.

[estarriolvetch]

Also, safeMint must still emit the Transfer event because that's how it checks it was safely minted.

Still need to research this.

I think transfer check (for safe operations) and events are separate things.

[estarriolvetch]

Since v0.8 is a huge change, we should use slither to do the analysis.

[nidhhoggr]

Also, safeMint must still emit the Transfer event because that's how it checks it was safely minted.

Still need to research this.

I think transfer check (for safe operations) and events are separate things.

After more research, it depends how the receiving contract implements onERC721Received. Of most all implementations I've seen, the function simply return a keccak-computed selector. But if the onERC721Received was implemented to check for a successful Transfer event emission, it would change things. That latter however is highly unlikely and in that case, not something we need to worry about.

Unrelated however, is if you take a look at the discussion in that thread I posted, there are issues with Etherscan returning an incorrect balanceOf due to usage of eip-2309. Another OZ contributor made the point that Transfer should always be emitted per mint because it's the expected behavior and that omission of this event will introduce bugs and/or unexpected behavior within the blockchain. One example is some libraries compute balanceOf by tallying transfer event emissions.

[nidhhoggr]

Since v0.8 is a huge change, we should use slither to do the analysis.

Yep, doesn't hurt to do static security analysis. I have a few tools that I use. The problem with these is they usually throw a ton of warnings about the code. Did you consider upgrading the compiler versions? right now hardhat is pointed to 0.8.11 and 0.4.16.

[nidhhoggr]

Looks like there are a couple repos using Slither (and others) in the CI pipeline.
https://github.com/utkusarioglu/workshop-ethereum/blob/main/.github/workflows/mythril.yml
https://github.com/ProjectTwelve/arcana-contracts/blob/main/.github/workflows/slither.yml
goldenrecursion/contracts@b5d2159

[niccolopetti]

On #45 I forked this branch to upgrade open zeppelin and chainlink, please take a look at it

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[github-advanced-security]

Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[nicolas-law]

Hey @estarriolvetch, what's the status on v0.8 ?