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

feat: added new actions for metadata program #56

Merged
merged 3 commits into from
Nov 10, 2021

Conversation

ilmoi
Copy link
Contributor

@ilmoi ilmoi commented Nov 8, 2021

I've been using metaplex/js for a project and ended up writing some of the actions that were missing.

A few things to point out:

  • Unfortunately tests need pretty big delays in them to succeed. This is because some actions are sequential and devent can be slow to propagate them. From trial and error, 20s delay seems to do it. This brings up the total test time from <30s to just over a minute. If that's too much of a pain, most waiting periods can be disabled (they're really mainly needed for deserializing accounts and asserting changes were successfully made - but we can also take a successfully completed tx as a proxy-indication.
  • I found an issue in UpdateMetadata, where because the below 2 lines were set to = null, borsh wasn't serializing data properly.
  data: MetadataDataData | null;
  updateAuthority: string | null;
  • I don't know enough about borsh to tell why the = null was needed. Through trial and error removing it seems to fix the issue and didn't break any other tests.

Finally, I've left a few comments in the code to clarify parts I found confusing when coding this. I noticed the repo is basically comment-free - if that's an intentional decision, more than happy to remove. But I do think they're useful.

Hope this is useful!

src/actions/mintEditionFromMaster.ts Outdated Show resolved Hide resolved
src/actions/signMetadata.ts Outdated Show resolved Hide resolved
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.

Awesome work! Thanks for doing that, have no objections.

We need to think about tests speed.

@ilmoi
Copy link
Contributor Author

ilmoi commented Nov 9, 2021

No prob - one team one dream 🙏

Re tests - if the delays bother you, I would honestly run them once (now) and comment out the deserialization & state checking parts that take extra time. The fact that the transaction goes through without errors should give you 95% comfort that it's doing what it must. Program endpoints don't change - when new ones are added the old are simply renamed "deprecated", so if it worked once, in theory should work forever.

Another much more involved option is to do a local validator for tests. Tbh that's the right way to do it, but I get the dev overhead associated with that, and that it might not be a priority right now.

Just my 2c

@zaxozhu zaxozhu merged commit a9208c9 into metaplex-foundation:main Nov 10, 2021
@zaxozhu
Copy link
Contributor

zaxozhu commented Nov 10, 2021

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants