Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

first pass at new editions contract #80

Merged
merged 34 commits into from
Mar 11, 2024
Merged

first pass at new editions contract #80

merged 34 commits into from
Mar 11, 2024

Conversation

campionfellin
Copy link
Contributor

@campionfellin campionfellin commented Feb 8, 2024

This PR

This PR creates a new extension contract for minting Editions. This is used by the Series Uploader app (and maybe others). Essentially, it's a gas-efficient way to mint a bunch of tokens to yourself (or others).

The major changes from the old contract is that it takes in an instance ID for idempotency rather than a seriesIndex. This is more in-line with moving everything towards instances going forward.

Screenshots (if applicable)

N/A.

Ticket

https://linear.app/manifoldxyz/issue/CON-215/[batch-mint]-new-smart-contract

CR Notes

QA Steps

  • [ ]

Copy link

github-actions bot commented Feb 8, 2024

LCOV of commit 79b5ac6 during Test! #354

Summary coverage rate:
  lines......: 39.2% (858 of 2186 lines)
  functions..: 43.9% (161 of 367 functions)
  branches...: 30.1% (343 of 1140 branches)

Files changed coverage rate: n/a

Copy link

github-actions bot commented Feb 8, 2024

LCOV of commit 367d273 during Test! #321

Summary coverage rate:
  lines......: 37.1% (203 of 547 lines)
  functions..: 31.5% (29 of 92 functions)
  branches...: 30.7% (118 of 384 branches)

Files changed coverage rate: n/a

Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
Signed-off-by: Campion Fellin <[email protected]>
function mint(address creator, uint256 series, address recipient, uint16 count) external override nonReentrant creatorAdminRequired(creator) {
require(count > 0, "Invalid amount requested");
require(_totalSupply[creator][series]+count <= _maxSupply[creator][series], "Too many requested");
function mint(address creatorCore, uint256 instanceId, address recipient, uint16 count) external override nonReentrant creatorAdminRequired(creatorCore) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need nonReentrant here ? Curious cause we don't have this in our other contract mint function such as LazyClaims721 contract

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's because of the order of the end of the function here...

        uint256[] memory tokenIds = IERC721CreatorCore(creatorCore).mintExtensionBatch(recipient, count);
        _updateIndexRanges(creatorCore, instanceId, tokenIds[0], count);

If the recipient is a smart contract address, and has onERC721Received function that calls back in here, it could continue to loop this function before we call _updateIndexRanges to account for the total minted..

@wwhchung might know more

/**
* @dev See {IManifoldERC721Edition-createSeries}.
*/
function createSeries(address creatorCore, uint256 maxSupply_, string calldata prefix, uint256 instanceId, address[] memory recipients) external override creatorAdminRequired(creatorCore) returns(uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if I want to initialize a series and mint a bunch of tokens to my address only?

@@ -29,101 +29,108 @@ contract ManifoldERC721Edition is CreatorExtension, ICreatorExtensionTokenURI, I
mapping(address => mapping(uint256 => uint256)) _maxSupply;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the point of maxSupply if the mint function is gated by the creator only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I think it's helpful when they want say 300 tokens, so you can split into 2 transactions. maxSupply will be 300 and then totalSupply will be 200 after first tx and 300 after the next tx.

But yeah - not sure why we had this in the original contract..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yea make sense for multi transactions due to gas limit

Copy link
Contributor

@wwhchung wwhchung Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, with the v3 changes as discussed, this should be changed.

struct EditionInfo {
uint16 contractVersion;
uint24 totalSupply;
uint24 maxSupply;
string tokenPrefix;
}

mapping(address => mapping(uint256 => EditionInfo)) _editionInfo;

Now, we can store maxSupply, totalSupply and contract version into a single storage slot, further reducing memory usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also introduce:
uint256 internal constant MAX_UINT_56 = 0xffffffffffffff;

and ensure instanceId's <= uint56 (for data packing)

if (recipients.length == 0) revert("No recipients");

// Grab the start index by minting off a first token...
uint256 startIndex = IERC721CreatorCore(creatorCore).mintExtension(recipients[0].recipient);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong though. you need to call mintExtensionBatch with the count of the first recipient. Also you're double minting for the first recipient.

I'm pretty sure mintExtensionBatch will revert with count 0.

The logic is simpler now isn't it?

uint256[] memory tokenIdResults;
for (uint256 i; i < recipients.length;) {
       if (count == 0) revert InvalidCount();
        count += recipients[i].count;
        if (_totalSupply[creatorCore][instanceId]+count > _maxSupply[creatorCore][instanceId]) revert TooManyRequested();
        tokenIdResults = IERC721CreatorCore(creatorCore).mintExtensionBatch(recipients[i].recipient, recipients[i].count);
        if (i == 0) startIndex = tokenIdResults[0];
        unchecked{++i;}
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated to use this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hold on, I'm making some changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought of some stuff

}
_updateIndexRanges(creator, series, startIndex, recipients.length);
function createSeries(address creatorCore, uint256 maxSupply_, string calldata prefix, uint256 instanceId, Recipient[] memory recipients) external override creatorAdminRequired(creatorCore) returns(uint256) {
if (instanceId == 0 || maxSupply_ == 0 || _maxSupply[creatorCore][instanceId] != 0) revert("Invalid instance");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per above, change to (also no need to return anything anymore) (also argument ordering to be consistent with other extensions, creatorCore and instanceId first, because they are the keys)

    function createSeries(address creatorCore, uint256 instanceId, uint24 maxSupply, string calldata prefix, Recipient[] memory recipients) external override creatorAdminRequired(creatorCore) {
    if (instanceId > MAX_UINT_56 || maxSupply == 0 || _editionInfo[creatorCore][instanceId].maxSupply != 0) revert InvalidInput();
    uint8 creatorContractVersion;
        try IERC721CreatorCoreVersion(creatorContractAddress).VERSION() returns(uint256 version) {
            require(version <= 255, "Unsupported contract version");
            creatorContractVersion = uint8(version);
        } catch {}
    _editionInfo[creatorCore][instanceId] = EditionInfo({
         maxSupply: maxSupply,
         tokenPrefix: tokenPrefix,
         contractVersion: creatorContractVersion,
    })
        

@campionfellin campionfellin merged commit 66b794e into main Mar 11, 2024
7 checks passed
@campionfellin campionfellin deleted the newEditionContract branch March 11, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants