V0.8 #27
V0.8 #27
Conversation
Co-authored-by: Nidhhoggr <[email protected]>
Use assembly to emit transfer events (#23)
|
* update deps * Gets tests working for ERC721Psi * removing ununsed custom errors * Adds ERC721PsiUpgradeable with passing tests * Getting upgradeable mocks working * Adding and implementing ERC721PsiAddressDataStorage * Implement remaining storage slots * Adding test for Burnable and replacing Mock initialize functions with WithInit bootstrapper * Adding tests for AddressData and AddressDataBurnable * Adds tests for RandomSeed extensions * fixing override compiler error and swaping out require string with custom error * Adds tests for RandomSeedReveal extensions * Moving RandomSeed test to appropriate extensions folder Co-authored-by: Nidhhoggr <[email protected]>
Co-authored-by: nidhhoggr <[email protected]>
* Using Solady BitMaps instead of solidity-bits * using bytemap in place of the AddressData struct for further optimizations #33 * Ensures once-only writes to mapping on transfers for additional savings #33 Co-authored-by: nidhhoggr <[email protected]>
|
@nidhhoggr We have everything for v0.8, right? |
|
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: 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. |
|
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. |
I Address the problem here #35 |
|
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: ***@***.***>
|
|
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: ***@***.***>
|
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. |
|
That looks interesting. At the very least an extension could implement this by overriding the _mint function until it becomes fully supported. |
|
I think if it works for etherscan and opensea. We can probably integrate it into the base contract. |
|
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: Further debate on the matter starting here: |
Still need to research this. |
I think transfer check (for safe operations) and events are separate things. |
|
Since v0.8 is a huge change, we should use slither to do the analysis. |
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. |
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. |
|
Looks like there are a couple repos using Slither (and others) in the CI pipeline. |
|
On #45 I forked this branch to upgrade open zeppelin and chainlink, please take a look at it |
* Readme updates * Update hardhat.config.js updated spelling * Update README.md fixed a spelling issue * migrated to openzeppelin v5 and minor fixes -upgraded to oz v5 -upgraded to chainlink v8 -uniformed heterogeneous revert error messages -removed unused dependencies -bumped solc version to 0.8.22 -added solidity-coverage dependency * removed unused Address library * migrated to ethers v6 -also removed unused dependencies --------- Co-authored-by: Daniel Gruesso <[email protected]> Co-authored-by: Player1Taco <[email protected]>
Added slither github action to v8 and minor changes
|
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[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
|
Hey @estarriolvetch, what's the status on v0.8 ? |
Related issues/PRs