Add collection plus and merkle minters #116
Conversation
| | settings | struct AuctionTypesV1.Settings | 4 | 0 | 64 | src/auction/Auction.sol:Auction | | ||
| | token | contract IBaseToken | 6 | 0 | 20 | src/auction/Auction.sol:Auction | | ||
| | auction | struct AuctionTypesV1.Auction | 7 | 0 | 96 | src/auction/Auction.sol:Auction | | ||
| | currentBidReferral | address | 10 | 0 | 20 | src/auction/Auction.sol:Auction | |
There was a problem hiding this comment.
be careful when adding slots, looks fine here.
src/minters/CollectionPlusMinter.sol
Outdated
| /// @notice Allows the manager admin to set the ERC6551 implementation address | ||
| /// @param _erc6551Impl Address of the ERC6551 implementation | ||
| function setERC6551Implementation(address _erc6551Impl) external { | ||
| if (msg.sender != manager.owner()) { |
There was a problem hiding this comment.
Can this break registry entries and provide a potential for attack with a bad erc6551 impl?
There was a problem hiding this comment.
If this is an immutable registry would setting it as an instance (immutable) variable on deploy make sense and if there is a reason to upgrade changing it during an upgrade to a known registry with added functionality etc.
There was a problem hiding this comment.
agreed will change to immutable
src/minters/CollectionPlusMinter.sol
Outdated
|
|
||
| /// @notice mints a token from reserve using the collection plus strategy | ||
| /// @param params Mint parameters | ||
| function mintFromReserve(MintParams calldata params) public payable { |
There was a problem hiding this comment.
Would it make sense to break out MintParams as args or are they used elsewhere as well?
There was a problem hiding this comment.
used this to work around stack too deep issue. open to changing this if you think there is a better solution @iainnash
There was a problem hiding this comment.
seems like this was fixed when I moved validation to an internal function actually. will break out the params here
| } | ||
|
|
||
| // Transfer funds to treasury | ||
| if (settings.pricePerToken > 0) { |
There was a problem hiding this comment.
nit: what happens if claimCount is 0? This function will still be called. Would it make sense to either 1) check on uint256 desiredAmount = claimCount * settings.pricePerToken and desiredAmount > 0 here rather than pricePerToken or disallow mints with 0 as a claimCount?
There was a problem hiding this comment.
disallowing mints with 0 as claimCount thanks for flagging
src/minters/MerkleReserveMinter.sol
Outdated
| /// @param tokenContract Token contract to set settings for | ||
| /// @param merkleMinterSettings Settings to set | ||
| function setSettings(address tokenContract, MerkleMinterSettings memory merkleMinterSettings) external { | ||
| if (IOwnable(tokenContract).owner() != msg.sender) { |
There was a problem hiding this comment.
nit: could make for a useful modifier onlyContractOwner(tokenContract)
src/minters/MerkleReserveMinter.sol
Outdated
| /// @notice Resets the minter settings for a token | ||
| /// @param tokenContract Token contract to reset settings for | ||
| function resetSettings(address tokenContract) external { | ||
| if (IOwnable(tokenContract).owner() != msg.sender) { |
Adds collection plus minter:
Adds merkle minter:
Changes ERC721Votes batch delegate with sig function:
Code review: