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 serde feature - token-2022 #3291

Merged
merged 6 commits into from Jul 7, 2022

Conversation

omertxyz
Copy link
Contributor

@omertxyz omertxyz commented Jun 27, 2022

We would like to serialize TokenInstruction to json in order to have easier indexing, already implemented it for Metaplex token. This is still a draft I worked on with Lawrence Wu.

  • Add ser/deser traits to structs that are used when serde TokenInstruction
  • Use serde_as package to serialize Pubkey as string rather than bytes
  • Since COption doesn't implement serde, add manual serde
  • Tested roundtrip with some and none values

cc: @joncinque @CriesofCarrots @lwus

@mergify mergify bot added the community Community contribution label Jun 27, 2022
@mvines
Copy link
Member

mvines commented Jun 27, 2022

How about adding this to token-2022 instead, which should already be a superset of token

@omertxyz
Copy link
Contributor Author

@mvines thanks for the response, I'm not super familiar with token-2022, is that just the new version of token? As long as we can use it for indexing existing contracts that's fine on my side.

@mvines
Copy link
Member

mvines commented Jun 27, 2022

It's the next iteration of the token program, source is at https://github.com/solana-labs/solana-program-library/tree/master/token/program-2022

From a client perspective, using the spl-token-2022 should be equivalent to using the spl-token crate as long as you don't opt in to the new token 2022 features. My request has three motivations:

  1. Sneakily push users to start looking at token 2022 to drive interest/adoption
  2. We'd be more willing to land new features into token 2022, like this PR, because that code is under active development
  3. Anything that goes into the stable token crate also should go into token 2022, so you'd be asked to make these changes in token 2022 regardless!

@omertxyz omertxyz changed the title Add serde json main Add serde feature Jun 28, 2022
@omertxyz omertxyz changed the title Add serde feature Add serde feature - token-2022 Jun 28, 2022
@omertxyz
Copy link
Contributor Author

@mvines moved it to token-2022 as requested

@mvines
Copy link
Member

mvines commented Jun 28, 2022

It'd be nice to add a step in CI to run the new serde-feature tests in this PR, somewhere in here: https://github.com/solana-labs/solana-program-library/blob/master/.github/workflows/pull-request-token.yml#L16

@omertxyz
Copy link
Contributor Author

@mvines @lwus can you help me with the CI thing? I'm not familiar with github syntax

@jnwng
Copy link
Contributor

jnwng commented Jul 6, 2022

i might be mistaken, but the existing directive to "Build and test token" also recurses into the token/program-2022 folder to run tests there:

image

including the folder that the serialization.rs file is located in:
image

source

while i can't verify that the tests included in this PR will run successfully until the PR gets approved, just wanted to be sure we didn't miss anything—@mvines is there a different configuration you're expecting for this serialization test in particular that you want to be sure we run in CI?

@mvines
Copy link
Member

mvines commented Jul 6, 2022

@mvines is there a different configuration you're expecting for this serialization test in particular that you want to be sure we run in CI?

yeah, pretty sure these new tests wont be run by default because they're under the new "serde" feature, so we need a new step like I mentioned here: #3291 (comment)

run: |
cargo +"$RUST_STABLE" test-bpf \
--manifest-path=token/program-2022/Cargo.toml \
--features serde,serde_with \
Copy link
Contributor

Choose a reason for hiding this comment

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

@joncinque this looks suspicious—i had expected the serde-feature = ["serde", "serde_with"] in Cargo.toml to handle enabling this optional dependency, but cargo kept complaining to me whenever i omitted this feature. i have this here to make sure CI passes right now, but if you have some insight here that'd be useful!

image

@joncinque
Copy link
Contributor

I made a couple of little changes to get this to work on CI 🤞 , and some little cosmetic changes to make things clearer.

Unfortunately, we can't use the same feature name as a crate until we start using Rust 1.60, so I went with the name serde-traits, which seemed nice enough and didn't conflict with other crate names. We can update it to serde with 1.11 if the BPF compiler supports it, not sure what version it's on. https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies for more info

Also, there was an annoying deprecation warning because of rust-lang/rust#87454, so I just did #![allow(deprecated)] in the file, which was the only option unfortunately.

@joncinque
Copy link
Contributor

By the way, what do you think about implementing serde on all instructions the same way? It looks like only the mint initialization instructions are using the coption_serde for COption parameters.

@jnwng
Copy link
Contributor

jnwng commented Jul 7, 2022

By the way, what do you think about implementing serde on all instructions the same way? It looks like only the mint initialization instructions are using the coption_serde for COption parameters.

i think this makes sense! we had targeted just the instructions we were concerned with but shouldn't be a huge lift. looks like we probably only need to serialize the pubkeys; should this extend to all the extension instruction structs too? tactically, is there a way to validate this more efficiently or should we enumerate through all those instructions in the test to validate the serialization annotations?

@joncinque
Copy link
Contributor

Yep that was my thought! We don't have to do it in this PR though. If you create an issue outlining the rest of the work, then we can merge this one 😄

@joncinque
Copy link
Contributor

I rebased, so this should be good to go on my side once it passes CI

@jnwng
Copy link
Contributor

jnwng commented Jul 7, 2022

I rebased, so this should be good to go on my side once it passes CI

ready to go! outlined some of the information i know about in #3325

@joncinque joncinque merged commit f30ad1d into solana-labs:master Jul 7, 2022
@jnwng jnwng deleted the add-serde-json-main branch July 7, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants