-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Draft for review, once I get the go ahead, will finish deserialization for |
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!( |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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?
Added enum deserialization and docs! |
@kklas is attempting to deploy a commit to the 200ms Team on Vercel. A member of the Team first needs to authorize it. |
Hmm I didnt author this |
1264181
to
340a495
Compare
340a495
to
08c76b7
Compare
Whoops, my bad. Some issues while I was rebasing. |
@armaniferrante can I have another round of review, thanks |
The syntax does seem a bit awkward in terms of needing the program id + the account type name as part of the fetch command. |
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 ( |
Briefly looking over the code, it seems odd to add the IDL impl in the Though perhaps I'm just not familiar enough with the IDL parsing code in general to know whether it's actually a bad idea. |
I had a similar concern as well! However, I saw fit to add it to the |
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. |
Okay, will move it over to 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 |
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? |
Pretty-printed JSON is still valid JSON so I guess I'll just go ahead with that then. Also, since support for |
I think we actually added support for them in a recent PR.
#2260
…On Tue, Dec 6, 2022 at 7:10 PM Adithya Narayan ***@***.***> wrote:
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
<solana-idl-foundation/solana-idl-spec#6>), is
it alright if I mark that as a todo when deserialising?
—
Reply to this email directly, view it on GitHub
<#1923 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADAHMGHEIT6L3FVLHBTW5V3WL6FQPANCNFSM5XE32VGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
The rust side of things hasn't been implemented, correct? So, you still cannot create an |
ah i guess you're right. That PR is all pure ts. |
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 |
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. |
There was a problem hiding this 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)
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! |
There was a problem hiding this 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.
Fixes #1
Features
account
subcommand that can deserialize an on-chain account and output it--idl
that can be used to feed anyidl
to use to deserialize the accountExample Usage
The above command will deserialize the given account pubkey with the
Balance
struct fromprog1
program. Output as show below.Notes
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 theidl
in order to deserialize the account. Hence, we need to deserialize the account without the use of concretestruct
/enum
definitions.serde_json
library was chosen for this.JsonValue
and then printed out