WIP: SPL Token Metadata JS Library

[mistersimon]

JavaScript library for SPL Token Metadata

#5228

Took the base scaffolding from token/js

[buffalojoec]

This is on the right track, thanks!

I left a few comments, and I also have a couple of review thoughts I'll put here:

• I think TokenMetadata should be in a separate module from the interface instructions, namely state.ts.
• Same with Field, just to keep with the Rust layout.
• I also think we need to include the 8-byte discriminator for token metadata, since we're going to need to use the TLV library to read it from some slab of bytes.
• There's a number of implemented helpers and traits on state::TokenMetadata that allow consumers to easily interact with serialized metadata. We should provide similar support in the JS library for the interface. We won't have all of the robust Rust TLV goodness in JS (yet), so we'll have to lean on the JS TLV library and make any suggestions for furthering its development in order to provide similar helpers for metadata here in this library.

[mistersimon]

Hey @buffalojoec - thanks for the feedback and review.

I've most of the changes, left some comments to clarify things and have not implemented the pack/unpack functions as i'm still trying to figure out how to do them.

Do you have any sample data i can create a test case around for the pack/unpack data?

Thanks!

[buffalojoec]

Thanks for the continued effort. I left some comments hopefully clearing up a few of your questions!

[mistersimon]

This appears to arise from the fact that field is of type "Field" rather than "string" in the rust code. If i convert it to a string argument, the 3 no longer appears.
https://github.com/solana-labs/solana-program-library/blob/master/token-metadata/interface/src/instruction.rs#L255

My guess is it is some additional encoding borsh is doing, but can't figure it out. Any insights?

I think it should be safe, because it will just be treated as a string instead - but not confident.

[buffalojoec]

Hm, maybe because a string that doesn't match the first 3 values would be the type at index 3 of the union?

'name' | 'symbol' | 'uri' | string

[mistersimon]

Yeah you are right. Didn't realise that Field::Key("Name".to_string()) is different from Field::Name (still a rust noobie)

Managed to extract the buffer when you use Name into a test case (but its failing).
30ed1be

[mistersimon]

I haven't successfully serialised this yet - but i'll take this in another thread.

But it raises the question as to what should be matched against the rust enum "Name"/"Symbol"/"Uri"
I think there are two solutions, using an enum or string matching

1. Enum
Define a typescript enum, and encode as the enum only if the enum is provided

enum RequiredField {
  Name = 0 ,
  Symbol = 1,
  Uri = 2,
}
type Field = RequiredField | "string"

2. string matching

   if(field == "Name" || field == "name" ) {
    // encode as enum Field::name
  } else {
     // encode as user defined key
  }

Solution 1 more closely matches the rust implementation and allows there to both keys Field::Name and Field::Key("Name".to_string()) (which i think can co-exist based on my limited understanding?). However Enums is a typescript feature only, and any javascript users would be out of like. I think they would need to pass in 0 as the field to get the correct behaviour'

I'm more inclined to go with Solution 1 - if its really true that Field::Name and Field::Key("Name".to_string()) can co-exist

[buffalojoec]

This is looking great, thanks for a continued effort! Almost ready to go

I left a few comments, many of them around some of the comments you left. Keep in mind comments are super helpful for downstream users to understand the code, so we want to make sure they are accurate, clean, and make sense!

[buffalojoec]

Hm, maybe because a string that doesn't match the first 3 values would be the type at index 3 of the union?

'name' | 'symbol' | 'uri' | string

[mistersimon]

Hey @buffalojoec thanks for your review and comments - really helpful.

I think the only real outstanding issue is serialisation/deserialisation code

[buffalojoec]

Do you mind adding this library to CI? Similar to Token JS and Type-Length-Value JS

[mistersimon]

Do you mind adding this library to CI? Similar to Token JS and Type-Length-Value JS

That should be working now, but need your help to trigger it! Don't have the required permissions

[buffalojoec]

Hey @mistersimon thanks for the awesome work on this library.

I pushed up a commit to integrate @solana/codecs-* - which are a few experimental libraries we're developing in-house at Labs for serializing Rust counterparts in JavaScript. This should solve your concerns with the serialization.

Take a look at my changes and let me know what you think!

[buffalojoec]

This is super close to being done! Just have some cleanup to do and have to get CI working, then we should be cruisin'!

[mistersimon]

Hey @mistersimon thanks for the awesome work on this library.

I pushed up a commit to integrate @solana/codecs-* - which are a few experimental libraries we're developing in-house at Labs for serializing Rust counterparts in JavaScript. This should solve your concerns with the serialization.

Take a look at my changes and let me know what you think!

New library looks great - definitely the right choice.

Only change i would make would be to export the type for the argument object for each of the functions like this:

export interface InitializeInstructionArgs {
{
    programId: PublicKey;
    metadata: PublicKey;
    updateAuthority: PublicKey;
    mint: PublicKey;
    mintAuthority: PublicKey;
    name: string;
    symbol: string;
    uri: string;
}
export function createInitializeInstruction(args: InitializeInstructionArgs): TransactionInstruction {
...
}

More of a convenience for library users, to give them more flexibility. They can create the object typed and then pass it into the function, something like this:

const arg: InitializeInstructionArgs = {
  ....
}
createInitializeInstruction(arg)

Not sure if this change was intentional on your part - happy to make it

[buffalojoec]

Not sure if this change was intentional on your part - happy to make it

Yeah it was intentional - I didn't really think exporting them was necessary. But if you think it's more useful to downstream users go for it! Then I think we're good to go.

[mistersimon]

Not sure if this change was intentional on your part - happy to make it

Yeah it was intentional - I didn't really think exporting them was necessary. But if you think it's more useful to downstream users go for it! Then I think we're good to go.

@buffalojoec made that change. Agreed, think we are good to go!

Do you take it from here to get it merged + deployed or is there anything else I need to do?

[buffalojoec]

Awesome work, thanks again!!

If you feel like also tackling #5229 as well, using your new library, let me know!

I'll publish this package later today.