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

cli: anchor account subcommand to read program account #1923

Merged
merged 20 commits into from Dec 12, 2022

Conversation

AdithyaNarayan
Copy link
Contributor

@AdithyaNarayan AdithyaNarayan commented May 27, 2022

Fixes #1

Features

  • account subcommand that can deserialize an on-chain account and output it
  • --idl that can be used to feed any idl to use to deserialize the account

Example Usage

anchor account prog1.Balance DiuUL66WXc6cQFYSvAKmSxZ4izwsDJbm7yxY6DDtAdBm

The above command will deserialize the given account pubkey with the Balance struct from prog1 program. Output as show below.

{
  "balance": 9
}

Notes

  • As the anchor_client in rust requires the user to import concrete type definitions from the program itself, we cannot use it since we only have access to the idl in order to deserialize the account. Hence, we need to deserialize the account without the use of concrete struct/enum definitions.
  • This means, we need to fetch and deserialize the accounts manually into a struct that can hold any type. serde_json library was chosen for this.
  • The account will first be converted into a JsonValue and then printed out

@AdithyaNarayan
Copy link
Contributor Author

Draft for review, once I get the go ahead, will finish deserialization for enum types and do up documentation!

@armaniferrante
Copy link
Member

Would love to see this finished. This is an extremely useful feature.

cli/src/lib.rs Outdated
address: Pubkey,
idl_filepath: Option<String>,
) -> Result<()> {
let (program_name, account_type_name) = account_type.split_once('.').ok_or(anyhow!(
Copy link
Member

Choose a reason for hiding this comment

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

Any idea for alternative syntax here? I don't love this but I also don't have a better idea. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of alternate syntax, I've added a check to make sure that this is the only . in the name. Would that be alright?

@AdithyaNarayan
Copy link
Contributor Author

Added enum deserialization and docs!

@AdithyaNarayan AdithyaNarayan marked this pull request as ready for review June 2, 2022 16:16
@vercel
Copy link

vercel bot commented Jun 29, 2022

@kklas is attempting to deploy a commit to the 200ms Team on Vercel.

A member of the Team first needs to authorize it.

@kklas
Copy link
Contributor

kklas commented Jun 29, 2022

Hmm I didnt author this

@AdithyaNarayan
Copy link
Contributor Author

Hmm I didnt author this

Whoops, my bad. Some issues while I was rebasing.

@AdithyaNarayan
Copy link
Contributor Author

@armaniferrante can I have another round of review, thanks

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022

The syntax does seem a bit awkward in terms of needing the program id + the account type name as part of the fetch command.

@AdithyaNarayan
Copy link
Contributor Author

If a workspace has multiple programs, then there is a need to explicitly state the program and the account, correct? Also, I thought it would be somewhat similar in syntax to the current TypeScript client (<programName>.account.<accountType>) that people are used to.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022

Briefly looking over the code, it seems odd to add the IDL impl in the syn crate and not just add it in the CLI crate. This is partly because I'd like to avoid adding borsh and solana dependencies to the syn crate but also the implementation doesn't quite belong there.

Though perhaps I'm just not familiar enough with the IDL parsing code in general to know whether it's actually a bad idea.

@AdithyaNarayan
Copy link
Contributor Author

I had a similar concern as well! However, I saw fit to add it to the syn crate since it would also add a feature for rust clients to be able to dynamically read an IDL and use that IDL to deserialise to an account to serde_json::Value, instead of first generating structs from the IDL and then using those in a client. If you feel like this use case is something we can ignore, then I can move the code to the cli crate

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022

Maybe it's a dumb call by me but it seems like a use case we can ignore for now and maybe revisit it later.

The rust client hasn't had a lot of love yet but i know some people will be looking into it in the near future.

I posted about the PR on twitter as well to see if someone else would have feedback. Generally speaking though, do you consider the PR ready to go / feature complete? It looks like there's still some clippy lints to fix on the PR.

@AdithyaNarayan
Copy link
Contributor Author

AdithyaNarayan commented Dec 6, 2022

Okay, will move it over to cli.

I'd say it's rough around the edges since it hasn't been touched in a while but finishing touches should be quick and straightforward to implement after the feedback!

Another question: Should it output pretty printed JSON or just JSON? It pretty-prints right now but now that I think about it, printing out normal JSON is probably more useful for the output to be piped to jq or something else.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022

Hard to say. I like the idea of it being pretty printed but in terms of the use case it's for does it make sense to still be pretty printed? Can people still copy paste and have it be valid json even if it's pretty printed?

@AdithyaNarayan
Copy link
Contributor Author

AdithyaNarayan commented Dec 6, 2022

Pretty-printed JSON is still valid JSON so I guess I'll just go ahead with that then.

Also, since support for u256/i256 has not been implemented for Anchor Rust programs (see: solana-idl-foundation/solana-idl-spec#6), is it alright if I mark that as a todo when deserialising?

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022 via email

@AdithyaNarayan
Copy link
Contributor Author

The rust side of things hasn't been implemented, correct? So, you still cannot create an Account struct with a u256/i256 in a Anchor Rust program? Or am I mistaken?

@Henry-E
Copy link
Contributor

Henry-E commented Dec 6, 2022

ah i guess you're right. That PR is all pure ts.

@AdithyaNarayan
Copy link
Contributor Author

Should be ready for review now. I've tried to keep it similar to the TypeScript deserialization and so enums are deserialized as follows:

#[derive(Clone, AnchorDeserialize, AnchorSerialize)]
pub enum EnumExample {
    One,
    Two { x: u32, y: u32 },
}

#[account]
pub struct MyAccount {
    pub data: EnumExample,
}

would appear as

{
  "data":  {
    "Two": {
      "x": 10,
      "y": 20
    }
  }
}

in the CLI which is similar to the TypeScript version expect for the case sensitive Two

@Henry-E Henry-E requested review from Henry-E and removed request for armaniferrante December 7, 2022 16:48
@Henry-E
Copy link
Contributor

Henry-E commented Dec 7, 2022

Ok, read through. It looks straightforward enough. One thing though, is there some way to add a test? Maybe the test could piggyback off of one of the existing CLI or rust client tests that creates a program + IDL + program account.

Copy link
Contributor

@Henry-E Henry-E left a comment

Choose a reason for hiding this comment

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

Needs a test. The meat of the code is in deserializing the data which looks to be robust enough although it's not something i'm super familiar with. Probably want an actual example as well in the docs of the usage. For example <my_program> is that a public key or is it a string, where can the <my_program> name be gotten from to use in the command. Same with type_name, although that's probably easier. Although I don't think <type_name> is correct it's more like<account_type>, where it's the struct name in capitalized letters?

In the examples / docs it just needs to be very clear to users

  • what exactly it is they're supposed to be using as an argument
  • where they can find their own value for the arg
  • what format the arg is supposed to come in (e.g. anchor messes around with all sort of different formatting and camel casing for struct names between what shows up in rust vs. what's used in ts)

@Henry-E
Copy link
Contributor

Henry-E commented Dec 9, 2022

Thanks for all the hard work on this.

This PR has definitely made me realise that I need to spend more time looking at the IDL related code. It's surprising that rust client doesn't already have the ability to deserialize account from the IDL built into it, I suppose this is because when you have the crate for a particular type it's much easier to deserialize than when you have just the IDL. Plus there's probably no way to dynamically load a program's crate if it's just a CLI command. I see you mention a lot of this already in your opening post.

I think it looks good to merge. Hopefully some people can start testing it out and we can see what the feedback is like!

Copy link
Contributor

@Henry-E Henry-E 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, thanks very much!

In the future anchor needs to sort out how it handles IDLs and dynamically importing types from potentially generated crates.

Let's see how people get with this for now.

@Henry-E Henry-E merged commit b662ff1 into coral-xyz:master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI subcommands for reading any program account in a workspace
4 participants