js icon indicating copy to clipboard operation
js copied to clipboard

Design issues with the new metaplex js library.

Open Jac0xb opened this issue 2 years ago • 6 comments

I think there are some design issues with the new metaplex js library.

  1. Why is it trying to re-invent the wheel? Why not just use anchor libraries? It adds a lot of complexity to learning and some of the patterns don't mesh well with other solana web3 libraries. There are lots of new random types such as accounts, assets, Pda, etc and conversion functions along with these new types with no way of constructing them without fetching from the solana RPC.

  2. Needing to rpc fetch, parse, and pass in accounts to the builders is very cumbersome:

  • Needing to pass in the entire parsed auctionHouse account just to build instructions adds unnecessary fetching to solana RPC when much of the data in the auction house account won't be used in the instruciton builder.

  • The need to pass in the listing account into executeSale builder, and bid account into executeSale builder for example means it expects bid to be done in a transaction before executeSale when you should definitely be allowed to bundle the instructions into one transaction.

  • Why pass in these new extraneous types and not just the pubkeys / field values to the builders (the things the instructions need).

  • The need to pass in assets into various builder functions is cumbersome, if bid only needs the metadata/mint addresses why are we expected to fetch the entire asset w/ token?

  • Usually instruction relevant data is stored in databases, why require a refetching of accounts to build instructions?

  1. Being expected to use the task system takes away control. toTransaction doesn't make a working tx, its missing signers, etc. The anchor libraries do a really good job at allowing you to convert their method builders into instructions/transactions.

Jac0xb avatar Aug 08 '22 22:08 Jac0xb

Hi there 👋

Thank you for your feedback. I’ll try and address all of your points here in a structured manner.

Before I’ll start, I’d like to note that some of the operations and modules of the SDK are still a work in progress and are being frequently refactored to provide a better developer experience as we implement them. The Auction House module in particular is not ready to be used yet.

Anchor Libraries vs Autogenerated librairies

Please correct me if I’m wrong but, as far as I’m aware, the Anchor libraries are not designed to work on Vanilla Solana programs.

At Metaplex, we maintain both Anchor and Vanilla programs and therefore, we created a few tools such as Shank, Beet and Solita to unify them and autogenerate low-level JavaScript libraries that advanced users can use. The JS SDK aims to add layers of abstractions on top of these autogenerated librairies.

If you have any specific examples of libraries (Anchor or not) that could benefit this SDK I’d love to hear them.

On re-inventing the wheel

Whilst it encapsulate the autogenerated librairies, this SDK aims to keep control on its types and interfaces to have a more coherent API.

That means, owning its own data representation at the account layer AND at the application layer. For instance, an NFT is composed of at least 4 on-chain accounts yet our mental model of an NFT is one singular asset.

I do think these bring value but I also see your point that too many data structures can be overwhelming and confusing. Currently, most of these are not documented on purpose because they are meant to be internal types.

On constructing data within fetching

In the SDK, data models are simple typed object so constructing one from scratch is as easy as defining an object with the right attributes.

When parsing models, it is a bit more tricky because, again, models and accounts are not equivalent. In order to create an NFT model, you need more than just one account. Therefore, you need to be able to parse raw bytes of data into account data before you can pass those into a method that knows how to transform account data into a user friendly model. If you want to do that wiring yourself it’s honestly just a few lines of codes.

Internally, we even have methods that convert high-level data representation into program data that instructions can understand.

On not passing minimum valuable input

I agree with you that requiring a whole data model when only some of its attributes are being used is not an optimal solution and I’d like to change that.

The SDK relies a lot on these high-level data models and, when the user as an instance of a model, it does make sense for them to pass them around like database records to achieve various operations on them.

That being said, the SDK should at the very least enforce minimum valuable input on its transaction builders and allow syntactic sugar on the main operations (that return tasks).

Also, requiring the Bid and Listing models on the transaction builder is a very good catch and I’ll take note of that so it can be fixed before the Auction House module is released.

On models that live in a database

Just a side note here to say that you can create your own operation handlers and register them to override some default behaviour. This is useful when fetching data which defaults to fetching the cluster instead of a database or an indexed you might have.

This is not yet documented but I’m planning on documenting all of that asap.

Tasks versus transaction builders

The main API that return tasks is a great way to get started with the SDK but I agree that any slightly advanced application will need to compose these transactions manually which is why the transaction builder pattern was implemented.

I’d love to hear some of your ideas on how to make the process of composing transactions more developer friendly.

Regarding the toTransaction method that generates a transaction without signers, I think there is a misunderstanding here as well. The Signers that the builders accepts are different than the Signers accepted by web3.js because they also accept Metaplex identities. Therefore you have to pass the builder to the RPC module to sign and send the transaction. Again, this will be much more clear when documented but the fact that this was a frustration for you is definitely a manifestation that the devex could be improved and I’d love your opinion on that.

Conclusion

Thanks again for taking the time to share your thoughts on this new SDK.

Here are some of my key takeaways:

  • The lack of documentation on how this SDK works creates confusion right now and I think a lot of your frustrations will be cleared after everything is properly documented and released.
  • Enforcing minimum valuable input on operations and transaction builders is important.
  • We should especially not ask for full data models within transaction builders as they might not already exist at this point.

I’d love more of your input on:

  • What Anchor librairies we should learn from to improve the SDK and the tools that autogenerate the libraries it depends on.
  • How to improve the process of composing and sending more complex transactions.
  • Any other frustrations you might have with the SDK with code snippets of how it is right now versus how you think it’d be better.

lorisleiva avatar Aug 09 '22 00:08 lorisleiva

Hi there 👋

Thank you for your feedback. I’ll try and address all of your points here in a structured manner. ....

Thank you for the very detailed response, I will try to compose some constructive thoughts/feedback this week. I think the NFT module is really solid btw, not trying to fud the entire library.

Jac0xb avatar Aug 09 '22 02:08 Jac0xb

Unfortunately I agree with OP. This package feels like reinventing the wheel: it adds a wrapper around most of the @solana/web3 or spl-token functions. Why do we need transactionBuilders? Here's another problem I'm struggling with right now: metaplex.nfts().create() fails because it can't confirm the tx (even though it ends up being processed), so I'm trying to do same operation but using my own send & confirm logic. However, there is no way to customize this at all, I'm reading the sources and jumping through hoops so I could avoid re-implementing the whole method from scratch. Ideally, the package API should give me more control over send & confirm logic: I need to be able to get web3.transaction object or instructions + signers.

knivets avatar Oct 09 '22 05:10 knivets

The docs are also not clear, how the metaplex.nfts().create() method relates to the old "create candy machine & mint a token" approach. If I create an NFT through the create() method, is there a corresponding candy machine account created? Or do we bypass the candy machine minting process and communicate with token metadata program directly. If that's the case would there be any issues for that NFT when trying to sell it on marketplaces (I assume some tooling relies on NFT being minted from candy machine)? Also, how would one create a collection rather than a single NFT with this method? I noticed there's a maxSupply parameter but it seems to be related to Editions (not sure if that's the same thing as the NFTs being minted from a candy machine)

Apologies if this is not the right thread to ask this question

knivets avatar Oct 09 '22 05:10 knivets

FWIW, you can generate anchor IDLs for non-anchor programs and plug them in to anchor to get an anchor sdk, complete with has_one, pda, and custom resolvers.

ChewingGlass avatar Mar 22 '23 20:03 ChewingGlass

I am not sure if there is something still forthcoming but as it stands, I find it a bit jarring when the SDK provides you with a bag of functions to work with and no clear "entry point". The docs can certainly help but a sign of good interface design is its relative intuitiveness. For all its faults, the *Client pattern provided some of this.

sohrab- avatar Mar 28 '23 00:03 sohrab-