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

Mint NFT Action #51

Merged

Conversation

kespinola
Copy link
Contributor

Changes

Bundle the available transaction instructions into a single action for creating a master edition NFT with some max supply based on a metadata JSON URL.

Questions

  • What are your opinions on the action parameters and response?
  • Should there be separate actions for generating metadata and master editions?
  • I pulled the convention of reading the metadata fields from a URL from the metaplex cli. Good or bad?

zaxozhu
zaxozhu previously approved these changes Nov 3, 2021
Copy link
Contributor

@zaxozhu zaxozhu left a comment

Choose a reason for hiding this comment

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

Looks good!

  • What are your opinions on the action parameters and response?
    I think that's how what we want it to be.

  • Should there be separate actions for generating metadata and master editions?
    I think, no. It's premature. That one, who will decide to have an action to generate the metadata, can just extract it from here and create a new action.

  • I pulled the convention of reading the metadata fields from a URL from the metaplex cli. Good or bad?
    Looks great

src/actions/mintNFT.ts Outdated Show resolved Hide resolved
{
mint: mint.publicKey,
dest: recipient,
amount: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess amount should be configurable

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not that aware of the flow, but can we mint a few items? And does it makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, let's stick to 1 for now

@kespinola kespinola force-pushed the feat/metadata-action-mint branch from 6e50249 to f191e74 Compare November 4, 2021 00:38
@kespinola kespinola marked this pull request as ready for review November 4, 2021 00:38
@kespinola kespinola force-pushed the feat/metadata-action-mint branch from f191e74 to 834fac6 Compare November 4, 2021 00:39
@kespinola
Copy link
Contributor Author

@m-sebastiyan and Metaplex team this is ready for final review.

Unfortunately, batching even 2 nft instruction bundles into a single transaction exceeded the max instruction size so going with the single mint action.

zaxozhu
zaxozhu previously approved these changes Nov 4, 2021
test/transactions/metadata.test.ts Outdated Show resolved Hide resolved
@kespinola
Copy link
Contributor Author

@b2kdaman that was an existing reference that I was looking to trim down to just the updateMetadata test since the mintNFT now bundles up the instructions but update is dependent on the create. I'm going to leave it in until an updateMetadata action is added.

Also in terms of the interface do what do you all think of a class method on something like metadata or master edition?

`const {txtId} = await MasterEdition.mint(params)

It doesn't return an instance of the class which is the normal output of such methods. I'm good either way just see this pattern used already in the sdk.

@kespinola
Copy link
Contributor Author

Can I get some more clarity on parameterizing the amount?

#51 (review)

@zaxozhu zaxozhu merged commit 122d08c into metaplex-foundation:main Nov 5, 2021
@zaxozhu
Copy link
Contributor

zaxozhu commented Nov 5, 2021

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monomadic
Copy link

FWIW there's a bug in this code when the creators array is missing or empty. The metaplex NFT standard supports this, but this code will throw this error:

TypeError: Cannot read properties of undefined (reading 'reduce')
    at /Users/nom/Workspaces/solana.workspace/solana-examples/node_modules/@metaplex/js/src/actions/mintNFT.ts:53:33

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.

5 participants