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

Add platform airdrop functionality to SuperMinterV1_1 #298

Merged
merged 7 commits into from
Dec 15, 2023

Conversation

Vectorized
Copy link
Collaborator

@Vectorized Vectorized commented Dec 5, 2023

  • SuperMinterV1_1:

    • New "PLATFORM_AIRDROP" mode to
    • Removed artist-customizable signer. We don't need that flexibility. Simpler code.

Copy link

changeset-bot bot commented Dec 5, 2023

⚠️ No Changeset found

Latest commit: 802a7d7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vigneshka vigneshka changed the title Add presave functionality to SuperMinterV1_1 Add platform airdrop functionality to SuperMinterV1_1 Dec 6, 2023
Copy link

@mattg-a16z mattg-a16z left a comment

Choose a reason for hiding this comment

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

I did this in a slightly different way this time. I went through all the key changes to make sure I noted having reviewed the change or asked for verification that my understanding is correct. Overall, it seems like the main changes are:

  1. Removing the mint-specific signers in favor of platform signers
  2. Addition of another mint type
  3. A new minting function (airdrop) to support the new mint type that verifies a platform signature before airdropping to n many addresses
  4. Changes to the editions to allow the minter to call the airdrop function directly
  5. Changes to configuration functions to keep people from setting price or max mintable for an airdrop mint

If this matches your expectations, then we're probably good. I'll do another once-over tomorrow morning to make sure I'm not missing some strange edge case, but in the mean time, here is the review.

@@ -232,7 +232,7 @@ contract SoundEditionV2_1 is ISoundEditionV2_1, ERC721AQueryableUpgradeable, ERC
uint8 tier,
address[] calldata to,
uint256 quantity
) external payable onlyRolesOrOwner(ADMIN_ROLE) returns (uint256 fromTokenId) {
) external payable onlyRolesOrOwner(ADMIN_ROLE | MINTER_ROLE) returns (uint256 fromTokenId) {

Choose a reason for hiding this comment

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

Checked this in context. The change is to allow airdrop to be called by the minter to facilitate the minter's new airdrop functionality. This checks out no response needed


// Perform the sub workflows depending on the mint mode.
uint8 mode = d.mode;
if (mode == VERIFY_MERKLE) _verifyMerkle(d, p);
else if (mode == VERIFY_SIGNATURE) _verifyAndClaimSignature(d, p);
else if (mode == PLATFORM_AIRDROP) revert InvalidMode();

Choose a reason for hiding this comment

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

This is important, the main risk creating a new state involves running older functions in unexpected states. This keeps mintTo from being called for platform airdrops. This comment is to note down where the expected mitigations are, no response needed.

function computePlatformAirdropDigest(PlatformAirdrop calldata p) public view returns (bytes32) {
// prettier-ignore
return
_hashTypedData(keccak256(abi.encode(

Choose a reason for hiding this comment

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

EIP712 digest, addresses most signature gotchas. Good to see.

revert InvalidSignature();
if (block.timestamp > p.signedDeadline) revert SignatureExpired();
uint256 mintId = LibOps.packId(p.edition, p.tier, p.scheduleNum);
if (!_claimsBitmaps[mintId].toggle(p.signedClaimTicket)) revert SignatureAlreadyUsed();

Choose a reason for hiding this comment

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

So we have the EIP712 signature verification that uses the entire PlatformAirdrop object (except the signature). Then we verify the expiry, then check if it has been used before. Checked toggle to make sure that when the bit is unset it returns true, and that seems to be the case. Reversion should keep toggle from turning the bit off. Should work as intended.

@@ -285,16 +307,13 @@ contract SuperMinterV1_1 is ISuperMinterV1_1, EIP712 {
d.maxMintable = c.maxMintable;
d.affiliateFeeBPS = c.affiliateFeeBPS;
d.mode = c.mode;
d.flags = _MINT_CREATED_FLAG | LibOps.toFlag(usePlatformSigner, _USE_PLATFORM_SIGNER_FLAG);
d.flags = _MINT_CREATED_FLAG;

Choose a reason for hiding this comment

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

Sounds like everything is going to use the platform signer. Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's correct

_requireMintOpen(d);

if (d.mode != PLATFORM_AIRDROP) revert InvalidMode();
_verifyAndClaimPlatfromAidropSignature(d, p);

Choose a reason for hiding this comment

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

Signature check, expiry check, and replay check are all in this method.

if (d.mode != PLATFORM_AIRDROP) revert InvalidMode();
_verifyAndClaimPlatfromAidropSignature(d, p);

_incrementPlatformAirdropMinted(d, p);

Choose a reason for hiding this comment

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

quantity check is inside this method

/* ------------------------- MINT --------------------------- */

ISoundEditionV2_1 edition = ISoundEditionV2_1(p.edition);
fromTokenId = edition.airdrop(p.tier, p.to, p.signedQuantity);

Choose a reason for hiding this comment

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

Can only be reached if:

  1. the mint is a platform airdrop
  2. there is a valid platform signature by the platform specified during mint creation (createEditionMint sets the platform that is used to find the signer.
  3. the signature hasn't expired
  4. the claim ticket number hasn't been used yet
  5. the minting doesn't go over the max mintable

As long as this matches the intended restrictions, then this should be good to go.

/**
* @inheritdoc ISuperMinterV1_1
*/
function setSigner(

Choose a reason for hiding this comment

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

This seems to be a change where the only signer used going forward is going to be the platform signer for the platform declared as part of the mint creation. This means no more declaring a specific signer, instead for VERIFY_SIGNATURE mint you'll declare a platform and the platform will will use their enrolled signer. Is this correct? The change on line 1119 seems to support this being the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

correct

@vigneshka
Copy link
Contributor

I did this in a slightly different way this time. I went through all the key changes to make sure I noted having reviewed the change or asked for verification that my understanding is correct. Overall, it seems like the main changes are:

  1. Removing the mint-specific signers in favor of platform signers
  2. Addition of another mint type
  3. A new minting function (airdrop) to support the new mint type that verifies a platform signature before airdropping to n many addresses
  4. Changes to the editions to allow the minter to call the airdrop function directly
  5. Changes to configuration functions to keep people from setting price or max mintable for an airdrop mint

If this matches your expectations, then we're probably good. I'll do another once-over tomorrow morning to make sure I'm not missing some strange edge case, but in the mean time, here is the review.

Perfect summary, thank you Matt!

@vigneshka vigneshka merged commit 9d7530b into vectorized/affiliate-flat-fee Dec 15, 2023
4 checks passed
@vigneshka vigneshka deleted the vectorized/presaver branch December 15, 2023 17:04
vigneshka added a commit that referenced this pull request Dec 18, 2023
* Prep files

* Reinstall multicaller

* forge install: multicaller

v1.3.1

* Add LibMulticaller.senderOrSigner support

* Tidy

* freeMintIncentive -> cheapMintIncentive

* Add comment on the two types of affiliate fees

* Remove first collector incentives

* Add more comments and use checked math in mintTo

* Add finalAffiliateIncentive to Minted log. Change finalCheapMintFee -> finalCheapMintIncentive

* Add platform airdrop functionality to SuperMinterV1_1 (#298)

* Add presave functionality to SuperMinterV1_1

* Rename PRESAVE -> PLATFORM_AIRDROP

* Force PLATFORM_AIRDROP to only use platform signer

* Remove artist-customizable signer, everyone must use platform signer

* Make mintTo and platformAirdrop return fromTokenId, use airdrop in platformAirdrop

* Remove signer from MintData

* Update create2 deployment script

* Create modern-shirts-try.md

* pin version for typechain

* tweak ci

* more ci tweaks

* add back foundry

* bump

* bump typechain

* Implement fee changes (#299)

* Implement fee changes

* T

* Fix prices and add tests

* Add tests

* Simplified fees and rewards (#300)

* nit suggestion

* changes

* Fix code and fuzz test

* Tidy

* Nit comment

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>

* Update modern-shirts-try.md

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>
vigneshka added a commit that referenced this pull request Dec 18, 2023
* Clone files and bump versions

* SuperMinterV1_1 et al. (#296)

* Prep files

* Reinstall multicaller

* forge install: multicaller

v1.3.1

* Add LibMulticaller.senderOrSigner support

* Tidy

* freeMintIncentive -> cheapMintIncentive

* Add comment on the two types of affiliate fees

* Remove first collector incentives

* Add more comments and use checked math in mintTo

* Add finalAffiliateIncentive to Minted log. Change finalCheapMintFee -> finalCheapMintIncentive

* Add platform airdrop functionality to SuperMinterV1_1 (#298)

* Add presave functionality to SuperMinterV1_1

* Rename PRESAVE -> PLATFORM_AIRDROP

* Force PLATFORM_AIRDROP to only use platform signer

* Remove artist-customizable signer, everyone must use platform signer

* Make mintTo and platformAirdrop return fromTokenId, use airdrop in platformAirdrop

* Remove signer from MintData

* Update create2 deployment script

* Create modern-shirts-try.md

* pin version for typechain

* tweak ci

* more ci tweaks

* add back foundry

* bump

* bump typechain

* Implement fee changes (#299)

* Implement fee changes

* T

* Fix prices and add tests

* Add tests

* Simplified fees and rewards (#300)

* nit suggestion

* changes

* Fix code and fuzz test

* Tidy

* Nit comment

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>

* Update modern-shirts-try.md

---------

Co-authored-by: Vignesh Hirudayakanth <vigneshkanth@gmail.com>

* Update .github/workflows/canary.yml

---------

Co-authored-by: Vectorized <webby1111@hotmail.com>
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