estarriolvetch / erc721psi Goto Github PK

Necessary for further improvements including providing tests for _startTokenId

If the current supply gets very high (say 50k+) then balanceOf call potentially run out of gas. Infura for instance caps eth_call to 60Millions gas.

What are your thoughts on this issue ?

The following changes improved the gas savings. gas differences are explained in the report diffs.

nidhhoggr@752fcd8

Would it be possible to have a mint function that randomizes the tokenID of the mint between a certain number interval (1-200, 200-400) depending on the function argument uint256 tier (would just be 1 or 2).

This is because of a tier system, and the metadata's are uploaded randomly, but also in a specific fashion to ensure specific images are matching their tier. So I'm trying to make Psi work with this, instead of just using ERC721 for the tier system.

I know that the tokenID currently can't be specified due to the batch minting and randomness, but if I could at least specify the interval in which the tokenID's may be randomized in between, depending on the tier chosen, this would work.

Oh.... and my current ERC721 implementation will be updating the baseURI at a later date for a reveal... but if I could integrate the RandomSeedReveal, while being able to choose the interval for the tokenID's based off the tier during mint, everything would work handsomely.

Thank you 谢谢 🤝

I'm going to attempt to start optimizing but there's no gas reporting built in? truffle has a gas reporter, not sure If you've been using something different.

The first thing that needs additional benchmarking is the _mint function so I would like to start adding those tests but a gas reporter is necessary.

Mint Benchmarks

I think it would be helpful to add a benchmark for safeMint and mint. right now there is only a benchmark for safeMint.

Here is how I handled that:

1. Change the existing mint benchmark to use the mint function
   https://github.com/nidhhoggr/ERC721Psi/blob/9b7a15fd2a0cf4c4392a26642370d5929ceadfa5/scripts/benchmark_mint.js
2. Add another benchmark script to use the safeMint function
   https://github.com/nidhhoggr/ERC721Psi/blob/9b7a15fd2a0cf4c4392a26642370d5929ceadfa5/scripts/benchmark_safemint.js

Missing Bitscan benchmark

Next, there is a benchmark for bitscan but it errors because no such Mock exists. As such we should simply delete it. The scanning benchmarking is already abstracted to functions that use it.

ownerOf Benchmark

Is it necessary to benchmark ownerOf in an iterative fashion? Currently the benchmark iterates 16 times but the result is the same. I found that however minting and calling the benchmark function in the same loop produces different results where ERC721A is no longer constant. Should we use this approach instead or remove iterations? Not sure what the intention is but its obvious that ERC721A is less efficient at finding the owner as the gas used becomes variable. I will provide the code and benchmark result below.

const hre = require("hardhat");

async function main() {
  const accounts = await hre.ethers.getSigners();
  const deployer = accounts[0];
  const user1 = accounts[1];

  let ERC721Psi = await hre.ethers.getContractFactory("ERC721PsiMock");
  ERC721Psi = await ERC721Psi.deploy("ERC721Psi", "ERC721Psi");
  ERC721Psi = await ERC721Psi.deployed();

  console.log("ERC721Psi deployed to:", ERC721Psi.address);

  let ERC721A = await hre.ethers.getContractFactory("ERC721AMock");
  ERC721A = await ERC721A.deploy("ERC721A", "ERC721A");
  ERC721A = await ERC721A.deployed();

  console.log("ERC721A deployed to:", ERC721A.address);

  let ERC721Enumerable = await hre.ethers.getContractFactory("ERC721EnumerableMock");
  ERC721Enumerable = await ERC721Enumerable.deploy("ERC721Enumerable", "ERC721Enumerable");
  ERC721Enumerable = await ERC721Enumerable.deployed();

  console.log("ERC721Enumerable deployed to:", ERC721Enumerable.address);


  for(let i = 1; i < 17; i++){
    console.log(i)
    let erc721Psi_mint = await ERC721Psi['safeMint(address,uint256)'](deployer.address, 64);
    console.log("ERC721Psi Mint", (await erc721Psi_mint.wait()).gasUsed.toString());
    let erc721a_mint = await ERC721A['safeMint(address,uint256)'](deployer.address, 64);
    console.log("ERC721A Mint", (await erc721a_mint.wait()).gasUsed.toString());

    for(let j = 64 * (i - 1); j< 64 * i; j++){
      await ERC721Enumerable['safeMint(address,uint256)'](deployer.address, j);
    }

    await ERC721Psi['benchmarkOwnerOf(uint256)']((64 * i) - i );
    await ERC721A['benchmarkOwnerOf(uint256)']((64 * i) - i);
    await ERC721Enumerable['benchmarkOwnerOf(uint256)']((64 * i) - i);
  }

}

// We recommend this pattern to be able to use async/await everywhere
// and properly handle errors.
main()
  .then(() => process.exit(0))
  .catch((error) => {
    console.error(error);
    process.exit(1);
  });

ERC721Psi deployed to: 0x5FbDB2315678afecb367f032d93F642f64180aa3
ERC721A deployed to: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
ERC721Enumerable deployed to: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
1
ERC721Psi Mint 213344
ERC721A Mint 214669
7150
143544
2256
2
ERC721Psi Mint 179144
ERC721A Mint 180469
7153
141336
2256
3
ERC721Psi Mint 179144
ERC721A Mint 180469
7153
139128
2256
4
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
136920
2256
5
ERC721Psi Mint 196244
ERC721A Mint 180469
7150
134712
2256
6
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
132504
2256
7
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
130296
2256
8
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
128088
2256
9
ERC721Psi Mint 196244
ERC721A Mint 180469
7150
125880
2256
10
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
123672
2256
11
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
121464
2256
12
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
119256
2256
13
ERC721Psi Mint 196244
ERC721A Mint 180469
7150
117048
2256
14
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
114840
2256
15
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
112632
2256
16
ERC721Psi Mint 179144
ERC721A Mint 180469
7150
110424
2256

Hello.
I am currently using PsiBurnable for some NFT contracts, but I am facing a problem with high mint cost.
Looking at the code, I noticed that Burnable's _burned() counts up the _burnedToken Bitmap and as the number of tokens increases, it accesses a lot of storage, which increases the gas cost.

Checking the latest ERC721A, we see that the gas cost of totalSupply() is constant regardless of the number of tokens because _burnCounter is incremented during the burn.

I want to set token id start at 1.
how to setting it.

Not sure if it is intentional or not, but _processRandomnessFulfillment in ERC721PsiRandomSeed can't be overwritten because it's missing the virtual keyword. Is there any sample code to show the expected use of the seed extension function? Since the seed function reverts if the randomness hasn't been fulfilled, how will we know when to call seed without the ability to access _processRandomnessFulfillment?

Example use case: User mints 2 NFTs, and once randomness is fulfilled the seed is used to assign a random trait on-chain.

From ERC721PsiUgradeable.sol

pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721MetadataUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721EnumerableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/StringsUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import "solidity-bits/contracts/BitMaps.sol";

Why refer to IERC721Receiver rather than the upgradable version?
Even if its no issue, it requires you to install both regular openzeppelin package as the opzeppelin upgradable package

nidhhoggr@9b7a15f

Random Seed Extension
The random seed extensions provide an easy way for NFT projects to create on-chain randomized metata at the individual token level.

Should read:

Random Seed Extension
The random seed extensions provide an easy way for NFT projects to create on-chain randomized metadata at the individual token level.

Shouldn't this be tokenId <= _minted instead of just tokenId < _minted ?

If I mint only 1 token, the function will return false for _exists(1).

Also breaks if I pass in tokenId of 0.

When investigating how to come up with a way to reduce log4 calls to only once per _mint call, I came up with the following code. The only problem here is that is winded up using ~16 more gas than before. Basically it emulates a do-while loop which is not supported un Yul yet.

        uint256 toMasked;
        uint256 end = nextTokenId + quantity;

        // Use assembly to loop and emit the `Transfer` event for gas savings.
        // The assembly, together with the surrounding Solidity code, have been
        // delicately arranged to nudge the compiler into producing optimized opcodes.
        assembly {
            // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean.
            toMasked := and(to, sub(shl(0xA0, 1), 1))

            let tokenId := nextTokenId
            // The duplicated do/while removes an extra check and reduces stack juggling
            for {} 1 {} {
                // Emit the `Transfer` event. Similar to above.
                log4(0, 0, 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, 0, toMasked, tokenId)
                tokenId := add(tokenId, 1)
                // The `eq(,))` check ensures that large values of `quantity`
                // that overflows uint256 will make the loop run out of gas.
                if eq(tokenId, end) { break }
            }
        }

I went back and checked ERC721As implementation and sure enough, they have also remedied this using a do while loop with native solidity. Implementing it this way result in aroun ~100 gas savings while providing more readability and still eliminating the constant declarations.
https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L773

        unchecked {
            // Mask `to` to the lower 160 bits, in case the upper bits somehow aren't clean.
            uint256 toMasked = uint256(uint160(to)) & (1 << 160) - 1;

            uint256 end = nextTokenId  + quantity;
            uint256 tokenId = nextTokenId ;

            do {
                assembly {
                    // Emit the `Transfer` event.
                    log4(
                        0, // Start of data (0, since no data).
                        0, // End of data (0, since no data).
                        0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, // Signature.
                        0, // `address(0)`.
                        toMasked, // `to`.
                        tokenId // `tokenId`.
                    )
                }
                // The `!=` check ensures that large values of `quantity`
                // that overflows uint256 will make the loop run out of gas.
            } while (++tokenId != end);
        }

At the end of the day constants are useful for code readability especially when the variable is used in multiple places. In our scenario, these variables are only used once in the code. Further, usage of constants in upgradable contracts introduce storage collision issues (as would any declared state variables) so the tradeoff of simply removing them are more advantageous. In scenarios where the constant would occur in multiple areas of the code, a library could be used with helper methods that contain the constants. e.g. a helper method for masking 160 bits. The commit below address the constants declared in our base contracts but there are still some declared in the Random extensions. It's up to you if you'd like to remedy those.