Adds _startTokenId overrideability

[nidhhoggr]

Makes _startTokenId overrideable with overrided methods added to the mocks for code coverage for gh issue #7

[estarriolvetch]

Hi @nidhhoggr ,

Thanks for your contribution! I've been thinking of making it overrideable for a while. The reason I haven't done it is because there is no test cases for it.

I think the best way to added these test cases is through porting ERC721A's new test cases. They now have much more test cases than the time I created this project.

[nidhhoggr]

@estarriolvetch Yes, I was considering adding a test but it would require creating a mock just for a contract with a different startTokenId. I was getting ready to implement it but, I figured I'd hold off and figure out how you wanted to test for it. Looking at the ERC721A tests, it looks like they are loading in the startTokenId as a constructor argument to avoid code-duplication in the tests:
https://github.com/chiru-labs/ERC721A/blob/main/test/ERC721A.test.js#L716

Where the test cases would need to focus on is the totalSupply methods.

[estarriolvetch]

@nidhhoggr

I think removing the old test cases and porting the ERC721A's test cases is the way to go. (and add the test cases that are specific to the 721Psi back). ERC721A's new test cases are more modular and easier to maintain.

Really appreciate your help!

[nidhhoggr]

@estarriolvetch

porting the ERC721A tests over to the new contract require significant modifications. I was however able to get the the two tests added to the folder with all tests passing. You can review all of the changes necessary to get it compatible with the ERC721A test suite. Let me know if you want to proceed. Next step is to get ERC721PsiUpgradeable passing.

main...nidhhoggr:ERC721Psi:feature/update_test

1. Requires reentrant detection on safeMint
2. All testing for burned removed
3. All testing for ownershipData removed
4. Extending our own IERC721Psi interface
5. Replacing all requires with custom errors
6. Making transfer methods payable
7. Adding necessary mock methods to get tests passing: totalMinted, numberMinted, nextTokenId, mint, directApprove
8. Updating NPM dependencies

[estarriolvetch]

Wow this is quick!

Tools for custom errors is more mature now. I think switching to it is a good idea.

As for getting rid of OZ's ERC165 and context, I saw people misuse it a lot in ERC721A

Even with the document, people still misuse it.
https://chiru-labs.github.io/ERC721A/#/migration?id=supportsinterface

What's your thought on this?

Overall, lgtm.

[nidhhoggr]

@estarriolvetch Yep, I agree about custom errors.

I don't know the issue with dropping Context, the only methods it gives us are _msgSender and _msgData. we only use _msgSender. We can add it back but I'm indifferent, don't know how it would be properly misused without it.

Regarding the supportsInterface diamond problem, from what I understand about it is that it can still be overridden incorrectly despite inheriting ERC165 or not. it depends on the inheritance hierarchy and which base classes they decide to override in the supportsInterface method. So if they were to use ERC2189 they would have to specify || conditionals for multiple superclass support.

Incorrect

  function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721Psi, ERC2981, AccessControl)
    returns (bool) {
    return super.supportsInterface(interfaceId);
  }

Correct

  function supportsInterface(bytes4 interfaceId)
    public
    view
    override(ERC721Psi, ERC2981, AccessControl)
    returns (bool) {
        ERC721Psi.supportsInterface(interfaceId) || 
        ERC2981.supportsInterface(interfaceId) ||
        AccessControl.supportsInterface(interfaceId) 
  }

This is true regardless of how we implement supportsInterface if I'm not mistaken but It's something I'd have to look into and test a bit more. The advantage of ditching those interface extensions and appending them all into our own interface is that it reduces the bytecode size of the contract for spurious dragon limits and reduces the deployment costs.

One Idea I had was to provide a PSI2981 that does the overriding but I'm not sure if it's worth it considering other extensions would have the same problem and the impracticality of providing extensions for everything.

Related links

Cygaar explains in tweet
Stack Exchange article

[estarriolvetch]

Cool! Let's go with what you have now.

[nidhhoggr]

The problem with ERC721PsiUpgradeable is that the interfaceId's are subject to change when the contract signature changes. For this reason first I want to try moving forward with keeping all of the OZ Upgradeable extensions. The problem is that transferFrom methods are not payable so the tests will behave different. This is probably why ERC721A didn't implement Upgradeable. Technically we can just use IERC721Psi and only extend Initializable and IERC721Psi as such:

contract ERC721PsiUpgradeable is Initializable, IERC721Psi {

But the problem is that the computed interfaceIds are subject to change upon upgrades. Let me know you thoughts but I think we should continue with OZ extensions.

    /**
     * @dev See {IERC165-supportsInterface}.
     */
    function supportsInterface(bytes4 interfaceId)
        public
        view
        virtual
        override(ERC165Upgradeable, IERC165Upgradeable)
        returns (bool)
    {
        return
            interfaceId == type(IERC721Upgradeable).interfaceId ||
            interfaceId == type(IERC721MetadataUpgradeable).interfaceId ||
            interfaceId == type(IERC721EnumerableUpgradeable).interfaceId ||
            super.supportsInterface(interfaceId);
    }

[estarriolvetch]

Actually IERC721EnumerableUpgradeable should be dropped. I must forget to remove it.

[estarriolvetch]

ERC721A upgradable contracts are in this repo instead.
https://github.com/chiru-labs/ERC721A-Upgradeable

[estarriolvetch]

I am fine either keeping the OZ extensions or removing it completely, but I think it should be consistent between upgradeable and non-upgradeable contracts.

[nidhhoggr]

Still need to review thoroughly but this get ERC721PsiUpgradeable implemented with passing tests.

nidhhoggr@062dbec

[nidhhoggr]

@estarriolvetch

This is the convention for adding storage slots to upgradeable extensions. Before I proceed with the rest of the extensions do you have any concerns with this approach?

nidhhoggr@c70fa16

[estarriolvetch]

I think the way you deal with the extension is good.

Usually using unstructured storage layout would increase the bytecode size of the contract, just wondering do you have a comparison between using / not using unstructured storage.

What's your thought on it? Do you prefer unstructured or structured storage?

[nidhhoggr]

Good question. I'm not sure of the contract bytesize implications as a result of this approach. The advanatge of this approach is that instead of relying on Solidity to decide the storage slot, it's done using a fixed storage position that's computed by a hash. This prevents the possibility of storage collision upon upgrades but all of the state variables must be stored in there. I'm not familiar with "structured approach" I have heard the approach ERC721A is using as being called the Diamond Storage pattern and App Storage pattern. It's been advocated by OpenZeppelin:
https://blog.openzeppelin.com/upgradeability-using-unstructured-storage/

I'm not familiar with other patterns but we can benchmark an extension or two against another pattern if you like. One thing we'll have to consider is refactoring extension state variable to perform more optimally using struct packed Diamond Storage.

[estarriolvetch]

@nidhhoggr I personally prefer not to use diamond storage layout unless the performance and bytecode size is similar.

For the project that requires diamond storage, they can use ERC721A. I would think this project is more focusing on the performance side ( but not sacrificing the readability of the code).

Though they have an article for that, Openzeppelin doesn't use diamond in there upgradeable contracts..

Also, there are some case that requires initializer but doesn't need upgradeablity, for example, minimal proxy.

What do you think?

[nidhhoggr]

@estarriolvetch

Of all that I've read about upgradeability patterns and tradeoffs, performance was never a concern with regard to diamond storage. The reason why Open Zeppelin has not yet implemented this in their contracts yet (they plan on it) is because of reverse compatibility reasons. Introducing this new pattern obviously results in breaking changes. Entire thread here;
OpenZeppelin/openzeppelin-contracts#2964

Further, with your current implementation, you can't add any state variable without having collision issues. OpenZeppelin currently uses gaps to allow updating contracts with additional state variables but they'll soon be switching it out with their own implementation of diamond storage.
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC721/ERC721Upgradeable.sol#L514

Would you like to address your performance concern and run a benchmark?

[nidhhoggr]

@estarriolvetch Here is a feature branch I created for benchmarking diamond storage against your existing implementation.

main...nidhhoggr:ERC721Psi:feature/benchmarkingDiamondStorage

What I found is the following:

1. most runtime performance except for transfers only increased by 100 gas.
2. the transfer benchmark script written reports that the diamond implementation incurs about ~600 more gas. I was however able to optimize the transfer function to use only ~100 more gas. (commit below)
3. The bytesize of the code decreased by 10,000 with diamond storage. This is because the new diamond storage contract uses its own interface.

Commit that optimizes transfer function on the Diamond contract:
e137d2a

Commands:

1. generate reports:

npx hardhat run scripts/upgradeable/benchmark_mint.js > reports/.upgradeable_benchmark_mint
npx hardhat run scripts/upgradeable/benchmark_transfer.js > reports/.upgradeable_benchmark_transfer
npx hardhat run scripts/upgradeable/benchmark_ownerOf.js > reports/.upgradeable_benchmark_ownerOf
REPORT_GAS=true npx hardhat test test/ERC721PsiUpgradeable.js --bail > reports/.hardhat_gas_report_upgradeable
REPORT_GAS=true npx hardhat test test/ERC721PsiDiamondUpgradeable.js --bail > reports/.hardhat_gas_report_diamond_upgradeable

2. view hardhat gas reports:

cat reports/.hardhat_gas_report*up* | grep '|'

3. view benchmark reports:

cat reports/.upgradeable_benchmark_*

Conclusion

All-in-all these change only introduce about ~100 gas penalty on runtime performance at the cost of providing more resilient storage slots that are more immune to storage collisions and provide the ability to upgrade state variable with ease. Feel free to take a look at these and let me know if I'm looking at something wrong.

[estarriolvetch]

I expect the runtime performance to be almost the same. I was more worrying about the bytecode size.

I could be wrong, but I think the bytecode reduction mainly comes from the custom error.

Anyway, I think that is pretty good improvement. Le's stick with diamond for now.

Now we have the tests, I think we can start making the pr.

Again, many thanks!

[nidhhoggr]

@estarriolvetch Alright, so the next test needs to be written for AddressDataStorage. Below is the diamond storage contract that uses a nested struct:

library ERC721PsiAddressDataStorage {

    // Compiler will pack this into a single 256bit word.
    struct AddressData {
        // Realistically, 2**64-1 is more than enough.
        uint64 balance;
        // Keeps track of mint count with minimal overhead for tokenomics.
        uint64 numberMinted;
        // Keeps track of burn count with minimal overhead for tokenomics.
        uint64 numberBurned;
        // For miscellaneous variable(s) pertaining to the address
        // (e.g. number of whitelist mint slots used).
        // If there are multiple variables, please pack them into a uint64.
        uint64 aux;
    }

    struct Layout {
        // Mapping owner address to address data
        mapping(address => AddressData) _addressData;
    }

    bytes32 internal constant STORAGE_SLOT = keccak256('ERC721Psi.contracts.storage.AddressData');

    function layout() internal pure returns (Layout storage l) {
        bytes32 slot = STORAGE_SLOT;
        assembly {
            l.slot := slot
        }
    }
}

With diamond storage it's recommended not to use nested structs:

Do not put structs directly in structs unless you don’t plan on ever adding more state variables to the inner structs. You won't be able to add new state variables to inner structs in upgrades.

While we can only ever reserve 256bits for this word we still have the ability to repurpose aux which isn't used anywhere else in the codebase. We should allow the remaining 64 bits to be used for arbitrary purposes instead of naming it aux. In this manner is will serve a storage "gap". Moving AddressData to uint256 not only makes it more flexible for diamond storage upgrades but also "Manual unpacking allows us to achieve a much lower overhead". I think we should benchmark to confirm and isolate this as a separate issue called optimize address data storage. Let me know your thoughts before proceeding.

Related:
chiru-labs/ERC721A@3783cc4
chiru-labs/ERC721A#272
https://github.com/0xPhaze/ERC721M/blob/master/src/ERC721MLibrary.sol#L6

[nidhhoggr]

@estarriolvetch Can you explain to me the purpose of BatchMetaData? I see that is sets a bitmap in the safeMint function of as the nextTokenId(). It also provides an internal function to retrieve this batchMetaDataHead. Where I'm confused is that the base class already does this in the _mint function (BatchMetaData does it again in safeMint) and also provides a method to get the batchMetaDataHead. What is the purpose of storing this data twice? and it's not clear how it has anything to do with metadata.

[estarriolvetch]

@nidhhoggr batchMetaDataHead It's for on-chain metadata that associate with the batch.

The reason why I don't use the batch head for the ownership is because the ownership will change after transfer.

On the other hand, the metadata remains the same after minting.

It will make more sense if you look into the randomseed extension.

[estarriolvetch]

@estarriolvetch Alright, so the next test needs to be written for AddressDataStorage. Below is the diamond storage contract that uses a nested struct:

library ERC721PsiAddressDataStorage {

    // Compiler will pack this into a single 256bit word.
    struct AddressData {
        // Realistically, 2**64-1 is more than enough.
        uint64 balance;
        // Keeps track of mint count with minimal overhead for tokenomics.
        uint64 numberMinted;
        // Keeps track of burn count with minimal overhead for tokenomics.
        uint64 numberBurned;
        // For miscellaneous variable(s) pertaining to the address
        // (e.g. number of whitelist mint slots used).
        // If there are multiple variables, please pack them into a uint64.
        uint64 aux;
    }

    struct Layout {
        // Mapping owner address to address data
        mapping(address => AddressData) _addressData;
    }

    bytes32 internal constant STORAGE_SLOT = keccak256('ERC721Psi.contracts.storage.AddressData');

    function layout() internal pure returns (Layout storage l) {
        bytes32 slot = STORAGE_SLOT;
        assembly {
            l.slot := slot
        }
    }
}

With diamond storage it's recommended not to use nested structs:

Do not put structs directly in structs unless you don’t plan on ever adding more state variables to the inner structs. You won't be able to add new state variables to inner structs in upgrades.

While we can only ever reserve 256bits for this word we still have the ability to repurpose aux which isn't used anywhere else in the codebase. We should allow the remaining 64 bits to be used for arbitrary purposes instead of naming it aux. In this manner is will serve a storage "gap". Moving AddressData to uint256 not only makes it more flexible for diamond storage upgrades but also "Manual unpacking allows us to achieve a much lower overhead". I think we should benchmark to confirm and isolate this as a separate issue called optimize address data storage. Let me know your thoughts before proceeding.

Related: chiru-labs/ERC721A@3783cc4 chiru-labs/ERC721A#272 https://github.com/0xPhaze/ERC721M/blob/master/src/ERC721MLibrary.sol#L6

I think it's better to keep it as aux for compatibility with ERC721A.

[nidhhoggr]

@estarriolvetch Okay that makes sense about the BatchMetaData. Basically it seems to be a "base class" for the RandomSeed extensions. It's a shame there isn't a DRYer way to add Upgradeability without having to duplicate all the code from the non-upgradeable contracts, are you aware of any pattern that accomplishes this?

With that being said would you like to move forward with converting AddressData to a unit256 bytemap? If so I can create a separate issue for it. With regard to testing all of this. Since all of these extensions extend the base ERC721Psi, I think it's unnecessary to duplicate all of the tests for the extensions and instead only test the methods defined in the extensions. i.e. for ERCAddressData we should only test balanceOf method and triggering the transfer hook to test AddressData persistence. This will result in DRYer tests without neglecting code coverage. Thoughts?

[estarriolvetch]

The AddressData also affects the mint and burn, so they should also be tested.

We should be able to repeat all the tests easily with the new test suite.

I preferred the way you've already implemented. I don't think there is a nested struct issue.

The size of mapping is fixed (1 storage slot).

It's a bigger issue if the code looks like this.

struct Layout {
  SOME_STRUCT foo;
  uint256 a;
  uint256 b;
}

[nidhhoggr]

Yep, both mint and burn trigger the transfer hook. I understand your logic regarding storage expectations with mappings but apparently Solidity introduces overhead that can be avoided by replacing structs with bytemaps. I'll provide a separate branch for that to see if the benchmarks introduce considerable gas savings.

[nidhhoggr]

@estarriolvetch I noticed ERC721PsiAddressData doesn't have a burn function so how are we supposed to test burning on it? For now, I can add the following method to the mock in order to get the test passing:

    function burn(
        uint256 tokenId
    ) public {
        if (!_exists(tokenId)) revert OwnerQueryForNonexistentToken();
        if (!_isApprovedOrOwner(_msgSenderERC721Psi(), tokenId)) {
             revert TransferCallerNotOwnerNorApproved();
        }
        address from = ownerOf(tokenId);
        _beforeTokenTransfers(from, address(0), tokenId, 1);        
        emit Transfer(from, address(0), tokenId);
        _afterTokenTransfers(from, address(0), tokenId, 1);
    }

[estarriolvetch]

@nidhhoggr You can create a mock with both burnable and addressdata extensions.

[nidhhoggr]

Also, I noticed that after a token is "burned" (effectively transferred to address zero), the tokenId still belongs to the burning owner.

          it('after a burn', async function () {
            // Burn tokens
            const tokenIdToBurn = [offsetted(7)];
            await this.erc721PsiAddressData.burn(tokenIdToBurn[0]);
            //passes
            expect(await this.erc721PsiAddressData.ownerOf(tokenIdToBurn[0])).to.equal(this.owner.address);
          });

We can leave that and create another test to test the above assertion is not owned by the burning address. The problem is that the Burnable extension needs to override the ownerOf function to check that it exists (not burned). Otherwise we'd have to do it in the mock to get a test passing expecting the burned token to be zero address.
For example:

Burnable Extension

abstract contract ERC721PsiBurnable {
    ...
    function ownerOf(uint256 tokenId)
        public
        view
        virtual
        override
        returns (address)
    {
        if (_burnedToken.get(tokenId)) {
            return address(0);
        }
        else {
            return super.ownerOf(tokenId);
        }
    }

Mock:

contract ERC721PsiAddressDataBurnableMock is ERC721PsiAddressData,  ERC721PsiBurnable {

[estarriolvetch]

ERC721Psi/contracts/extension/ERC721PsiBurnable.sol

Line 51 in 91bf2ec

if(_burnedToken.get(tokenId)) {

Hmm... The exists is override, so it should revert when querying the owner.

Yeah you need to override it to use the one from burnable.

[nidhhoggr]

I figured that override was better than the following since this results in duplicate code and duplicate calls.

abstract contract ERC721PsiBurnable is ERC721Psi {
    ...
    function ownerOf(uint256 tokenId)
        public
        view
        virtual
        override
        returns (address)
    {
        if (_exists(tokenId)) {
            return super.ownerOf(tokenId);
        }
        else if(_burnedToken.get(tokenId)) {//burned 
            return address(0);
        }
        else {//not yet minted
            revert OwnerQueryForNonexistentToken();
        }
    }

[nidhhoggr]

Last I'll be adding back the tests for the RandomSeed extensions. Here are the test for the AddressData mocks:
nidhhoggr@d131380

[nidhhoggr]

@estarriolvetch alright, I added the last tests for the RandomSeed extensions. Let me know if it's good to submit a PR.
main...nidhhoggr:ERC721Psi:feature/update_test

[estarriolvetch]

Yeah I think it's ready for the PR.

[estarriolvetch]

merged in #30