estarriolvetch / ERC721Psi

MIT License
117 stars 29 forks source link

update benchmark scripts #31

Open nidhhoggr opened 1 year ago

nidhhoggr commented 1 year ago

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
nidhhoggr commented 1 year ago

nvm, for the ownerOf benchmark. I can now see that the reports for ERC721A are still variable with the existing benchmarks. The only difference however is that my implementation above demonstrates significantly higher gas usage when querying the ownerOf from ERC721A at a particular index. As such we can probably ignore that for now.