Feat/nft collection #1568
Feat/nft collection #1568
Conversation
Feat/nft collection
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
adjisb
force-pushed
the
feat/nft-collection
branch
4 times, most recently
from
658133a to
201b6e8
Compare
adjisb
force-pushed
the
feat/nft-collection
branch
from
1e9d2d9 to
f8cd396
Compare
adjisb
force-pushed
the
feat/nft-collection
branch
2 times, most recently
from
b44d0d7 to
2259967
Compare
adjisb
force-pushed
the
feat/nft-collection
branch
from
684024e to
17c49f2
Compare
adjisb
force-pushed
the
feat/nft-collection
branch
2 times, most recently
from
212ca09 to
a268b9b
Compare
Non gas optimized version of the transfer methods
fix the tests because there where using the wrong contract
Initialize is stil too big, we need to rewiew it.
let the contract initialize without a trusted forwarder improve emited events during initialization
Also some small improvementes to the contract
adjisb
force-pushed
the
feat/nft-collection
branch
from
2d2404c to
4291ff9
Compare
- Use fully qualified names for contracts - Add constructor arguments for verification
| * @dev based on: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.6.0/contracts/metatx/ERC2771Context.sol | ||
| * with an initializer for proxies and a mutable forwarder | ||
| */ | ||
| abstract contract ERC2771HandlerUpgradeable is ContextUpgradeable { |
There was a problem hiding this comment.
Should this file be in this folder or in dependency-metatx?
We may want to re-use this for other contracts right?
There was a problem hiding this comment.
This package has a collection of different implementations of groups of tokens and each has it own code, compiler version and style.
I think that to reuse code we must do a cleanup first (the problem is that most of the contracts are already deployed, so we must be careful about how we manage the versions). I tried to keep NFTCollection free of dependencies.
| * @param allowedToExecuteMint token address that is used for payments and that is allowed to execute mint | ||
| * @param maxSupply max supply of tokens to be allowed to be minted per contract | ||
| */ | ||
| event ContractInitialized( |
There was a problem hiding this comment.
Whats the point of this event when its emitted only once?
There was a problem hiding this comment.
It was already there in AvatarCollection, this contract is used with a factory and maybe somebody wants the arguments used for the creation of a new collection.
| event WaveSetup( | ||
| address indexed operator, | ||
| uint256 waveMaxTokens, | ||
| uint256 waveMaxTokensToBuy, |
There was a problem hiding this comment.
waveMaxTokensToBuy is not very clear, can we change to waveMaxTokensPerWallet?
Also update the comment.
There was a problem hiding this comment.
It was already there in AvatarCollection, we can ask.
| - ERC2981 compliant | ||
| - ERC4906 compliant | ||
| - ERC165 compliant | ||
| - supports ERC2771 for services like Biconomy |
There was a problem hiding this comment.
Biconomy no more! Maybe just say for meta transactions
| * @param waveIndex the index of the wave used to mint | ||
| * @param wallets list of destination wallets and amounts | ||
| */ | ||
| function batchMint(uint256 waveIndex, BatchMintingData[] calldata wallets) external whenNotPaused onlyOwner { |
There was a problem hiding this comment.
I feel like the owner should be able to batch mint tokens without setting up a wave.
If we want to just mint internally we need to setup a wave for 1000 tokens to mint 1000 tokens all for ourselves.
Is there a reason why we would do it that way?
There was a problem hiding this comment.
I also think batch minting should not be so restrictive, we may want to be able to mint all to same wallet etc.
I think we should just check if we are not minting more than the total supply of the collection.
| import {ZeroAddress} from 'ethers'; | ||
|
|
||
| describe('NFTCollection batch transfer', function () { | ||
| function check(method, data) { |
Description
NFT Collection