Simple ERC721 Contract

Smart Contracts
1

Hi everyone,

I'm trying to implement a really simple contract to replace the functionality of uploading NFTs by hand on opensea. What i've come up with so far is:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

import "@openzeppelin/contracts@4.5.0/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts@4.5.0/access/Ownable.sol";
import "@openzeppelin/contracts@4.5.0/utils/Counters.sol";

contract Test is ERC721, Ownable {
    using Counters for Counters.Counter;

    Counters.Counter private _tokenIdCounter;

    constructor() ERC721("Test", "T") {
        _tokenIdCounter.increment();
        allMint(msg.sender);
    }

    function _baseURI() internal pure override returns (string memory) {
        return "ipfs://x/";
    }

    function allMint(address to) public onlyOwner {
        require(totalSupply()==0,"NFTs have already been minted");
        for(uint256 i; i <= 100; i++){
            uint256 tokenId = _tokenIdCounter.current();
            _tokenIdCounter.increment();
            _safeMint(to, tokenId);
        }
    }

    function totalSupply() public view returns (uint256) {
        return _tokenIdCounter.current()-1;
    }
}

The metadata is stored at _baseURI + tokenId so the tokenURI works fine, I will be using Polygon so minting costs will probably be ok aswell.
The question I have is if this contract is the correct way to do this? Since we assume that we mint all NFTs at once is there a way to improve the gas cost? Is there a completely different better way of doing this? Or are there just some good practices I can incoroporate?
Also I'm thinking of adding the OpenSea proxy, does that make sense here?

Thanks a lot!

2

There are a few issues I can see in the code snippet:

  1. Is there a total of 100 or 101 NFTs? The current code implies a total of 101.
  2. The for loop can be improved to be more efficient by declaring tokenId above the for loop.
  3. The function allMint does not seem to be necessarily public, since it's called in the constructor and it can be called only once. A private function would work as well and it can save some gas. In this case, the contract does not need to be Ownable. This can save gas at deployment as well.
1 Like
3

Thank you very much for your insights.

Yeah I messed the for loop up thanks for catching that!
If I declare tokenId above the for loop does it update with every call to increment()?
The allMint function being private makes sense. So removing Ownable and onlyOwner and making it private would be the best way to do that right?
Is minting with _safeMint the best option to mint multiple NFTs?

Thanks again!

Edit: Now that I think of it I'm not sure if ownership of the contract might be necessary to edit the collection on opensea

4

Yes.

I would say it's an efficient way, since the redundancy is removed.

This is a plausible approach if all NFTs are minted by the owner. If they are lazy minted, it would be a different design.

1 Like
5

Thank You very much!

1 Like
6

If anyone sees this in the future I can confirm you need to own the contract to be able to edit the collection on opensea.
About the tokenId: When I do tokenId = ...current() and then put tokenId in the safeMint I get an error that the token has been minted already :confused:

7

I think there's a misunderstanding of what I said. What I meant is put the declaration of tokenId above, i.e. uint256 tokenId is put above the for loop. This way, you don't have to declare it in every for loop run. This line tokenId = _tokenIdCounter.current(); is still in the for loop.

For ownership to edit the collection on opensea, yes, you need to be the owner of the contract, but I don't think Ownable is needed unless you can prove it is.

8

Ahh okay now I get what you meant. As for opensea I can't prove it but I tested both and could only edit the collection with Ownable but I honestly don't know

9

I see. Yes, Ownable is required. Check out the link here.

2 Likes
10

Perfect, thanks a lot!

1 Like