CLI subxt explore commands #950
Conversation
Bumps [serde](https://github.com/serde-rs/serde) from 1.0.160 to 1.0.162. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.160...1.0.162) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update polkadot.scale Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics: Add extrinsics client Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics: Decode extrinsics Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * subxt: Add extrinsic error Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * blocks: Expose extrinsics Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * examples: Fetch and decode block extrinsics Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Fix clippy Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics: Fetch pallet and variant index Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * subxt: Move extrinsics on the subxt::blocks Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * example: Adjust example Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * metadata: Collect ExtrinsicMetadata Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * subxt: Implement StaticExtrinsic for the calls Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Adjust examples Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * codegen: Add root level Call enum Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Adjust testing Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * subxt: Add new decode interface Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * subxt: Merge ExtrinsicError with BlockError Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * examples: Find first extrinsic Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Move code to extrinsic_types Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Add Extrinsic struct Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Adjust examples Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * test: Decode extinsics Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/test: Add fake metadata for static decoding Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/test: Decode from insufficient bytes Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/test: Check unsupported versions Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/test: Statically decode to root and pallet enums Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/tests: Remove clones Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * blocks: Fetch block body inline Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * blocks: Rename ExtrinsicIds to ExtrinsicPartTypeIds Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics/test: Check decode as_extrinsic Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * blocks: Remove InsufficientData error Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * blocks: Return error from extrinsic_metadata Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * extrinsics: Postpone decoding of call bytes Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * metadata_type: Rename variables Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Adjust calls path for example and tests Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * examples: Remove traces Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * book: Add extrinsics documentation Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * book: Improve extrinsics docs Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: James Wilson <james@jsdw.me>
|
|
tadeohepperle
changed the title
|
This has been awesome to play with; great work! Some notes and nitpicks (mainly design based) from playing with the CLI tool to follow! When we first explore, I'd suggest: ie no When selecting a pallet, could we support case insensitive pallet names too? When you select a pallet, we can follow this same "Usage:" pattern, and provide a little info on each with clear indent like this: When we select ie more indenting and less '-'s to make it a little prettier (imo) :) When we select a specific call: I don't think we need the type name offhand, but in any case fewer '-'s and clear shape and type printout for prettiness :) Try updating Finally, when we provide a valid call (Just suggesting a slight indent here to match the style above): When you ask for constants I'd also suggest breaking them down into individual constants and following a similar design to above. Why; because some constant outputs are more verbose (try And then: And then for storage I'd go with: And then, if we are connected to a node, it'd be awesome if we could actually let the user provide values and fetch the entry Ie a similar display to submitting extrinsics. Some storage entries will take multiple keys (maybe you've thought about this already) so they will either need to be one VALUE that takes each arg as you did for extrinsics, or a separate arg per value if that's not possible. (we could use a I think that about covers it! Summary of thoughts so far:
|
|
@jsdw Thanks for the detailed feedback! I will go ahead and implement the design suggestions, and I think things like case insensitive input should be possible.
This is already working (e.g. see last example in PR description), multiple keys are represented as an unnamed composite scale value. I have to try some calls with multiple keys to see if the encoding works properly there. |
|
I can't dislike the functionnality :) |
| let mut strings: Vec<_> = pallet_metadata.constants.iter().map(|c| &c.name).collect(); | ||
| strings.sort(); | ||
| for constant in strings { | ||
| write!(output, "\n {}", constant).unwrap(); |
There was a problem hiding this comment.
write! is a bit clunky and having to unwrap I guess you could do:
output.push_str("\n ");
ouput.push_str(&constant);
cli/src/commands/explore/calls.rs
Outdated
| let mut strings: Vec<_> = pallet_calls.variants.iter().map(|c| &c.name).collect(); | ||
| strings.sort(); | ||
| for variant in strings { | ||
| write!(output, "\n {}", variant).unwrap(); |
There was a problem hiding this comment.
This should infallible but could re-written with push_str ^^
cli/src/commands/explore/calls.rs
Outdated
| Ok((calls_enum_type_def, calls_enum_type)) | ||
| } | ||
|
|
||
| fn new_offline_client(metadata: Metadata) -> OfflineClient<SubstrateConfig> { |
There was a problem hiding this comment.
I don't follow why these hardcoded values are needed
There was a problem hiding this comment.
I figured they are needed to create an OfflineClient. Is there a quicker way to just create an OfflineClient? We don't need an OnlineClient here yet. I just took the hardcoded values from an example.
There was a problem hiding this comment.
It's a bit hard to follow why creating an offline_client with hard coded values here which is not obvious to me at least :)
I would prefer if you could rename this function to mocked_offline_client and add a comment why/where it's used.
@niklasad1 Thanks for your review! We could do that, but it comes at the cost that every time we change a minor thing in formatting the style of the CLI output we would have to adjust all the tests. I could do that in a separate PR after this is merged. |
|
It is an interesting question how to test this; one approach would be to split the CLI crate into a lib and bin (living in the same folder for simplicity) and then we could integration test the lib by having each test create the CLI Opts struct (probably often by pointing at our The tests could also cover a couple of the existing commands if this restructuring was done, and the actual parsing that Tests would involve a bunch of searching for things in strings and such though, which is the less ideal bit. I suppose it's not awful if we just check that the output string contains a specific value and ignore the rest of it though, so that the formatting that we go with isn't really being tested. This could all be a separate PR, but curious to know what you guys think to the idea? Is it sane? |
|
@jsdw I think that is a good idea. What is the splitting between bin and lib about? Just so that we can have the lib expose a |
|
From my side, I just want some basic unit tests for the custom stuff introduced in this PR i.e, not relying on the Thus, some basic unit tests for the helpers would do it I think + that the basic Manually parsing strings/CLI output should be avoidable if possible :) |
| /// ## Pallets | ||
| /// | ||
| /// Show the pallets that are available: | ||
| /// ``` |
There was a problem hiding this comment.
Nit: All doc examples with "" are assumed to be rust doc comments; I'd do "text".
I guess rustdoc isn't trying to interpret these or something because it's a binary and not library project; I'd have expected to see CI others ordinarily as it tried to run them.
I don't expect anybody will ever see these comments, so they are only really useful to us.
| write!(output, "Description:\n{docs_string}")?; | ||
| } | ||
| write!(output, "Usage:")?; | ||
| write!(output, "\n subxt explore {pallet_name} calls\n explore the calls that can be made into this pallet")?; |
There was a problem hiding this comment.
Nit: as a general tidiness thing for everywhere you've printed things, I'd probably go with:
writeln!(output, "Usage:")?;
writeln!(output, " subxt explore {pallet_name} calls");
writeln!(output, " explore the calls that can be made into this pallet")?;ie separate lines for each printed line, so you can see super clearly in the code what it'll look like when printed.
| // if no storage entry specified, show user the calls to choose from: | ||
| let Some(entry_name) = command.storage_entry else { | ||
| let storage_entries = print_available_storage_entries(storage_metadata, pallet_name); | ||
| println!("Usage:\n subxt explore {pallet_name} storage <STORAGE_ENTRY>\n view details for a specific storage entry\n\n{storage_entries}"); |
There was a problem hiding this comment.
nit:
println!("Usage:");
println!(" subxt explore {pallet_name} storage <STORAGE_ENTRY>");
println!(" view details for a specific storage entry\n\n{storage_entries}");
just an example of another place multiple lines makes sense for me
| pub(crate) url: Option<Uri>, | ||
| /// The path to the encoded metadata file. | ||
| #[clap(long, value_parser)] | ||
| file: Option<PathBuf>, | ||
| pub(crate) file: Option<PathBuf>, |
There was a problem hiding this comment.
Nit: probably fine just being pub, ie no (crate) (this is a binary anyway atm and even if we make a bunch of it a lib for testing nobody should depend on it.
| @@ -71,3 +75,22 @@ impl FileOrUrl { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn print_docs_with_indent(docs: &[String], indent: usize) -> String { | |||
| // take at most the first paragraph of documentation, such that it does not get too long. | |||
There was a problem hiding this comment.
I feel like this hsould be reflected in the function name, since it would be easy to be confused at why docs weren't all being printed otherwise, eg print_first_paragraph_with_indent
| with_indent(docs_str, indent) | ||
| } | ||
|
|
||
| pub(crate) fn with_indent(s: String, indent: usize) -> String { |
There was a problem hiding this comment.
No need for the (crate)'s :)
| const MIN_VARIANT_COUNT_FOR_TRAILING_COMMA: usize = 100; | ||
| let add_trailing_comma = self.variants.len() >= MIN_VARIANT_COUNT_FOR_TRAILING_COMMA; |
There was a problem hiding this comment.
Why do we explicitly add a trailing comma if 100+ variants?
| if iter.peek().is_some() || add_trailing_comma { | ||
| variants_string.push(','); | ||
| } |
There was a problem hiding this comment.
Maybe this is a hint to tweak the parsing of scale-value's to allow trailing commas :)
There was a problem hiding this comment.
Oh, I was not aware of the handling of trailing commas in scale-value, I just looked at the rust formatter for inspiration.
| fn type_description(&self, registry: &PortableRegistry) -> color_eyre::Result<String>; | ||
| } | ||
|
|
||
| impl TypeDescription for u32 { |
There was a problem hiding this comment.
this impl is a bit strange; the others are all about generating a description of the themselves, but this one uses self as the lookup into a type registry.
Personally I'd just make this be a standalong function so people can either print_type_description(typedef, registry) or print_type_description_of(u32, registry), but not a big deal really :)
| } | ||
| } | ||
|
|
||
| fn format_type_description(input: &str) -> String { |
There was a problem hiding this comment.
Normally I'd suggest indentation being a part of the TypeDescription trait method, so that things can say how to indent children etc, and the formatting logic for each item is all in one place, but I've no problem with this for now since it seems to work!
| } | ||
|
|
||
| /// a trait for producing scale value examples for a type. | ||
| pub trait TypeExample { |
There was a problem hiding this comment.
I think this is ok for now, but I'm not super convinced that a trait is necessary here (or above). I'd think that in general if you want to give an example of some type, you could expose a single function, type_example(type_id: u32, registry: &PortableRegistry, indent: 32) -> String that would be the entire interface to this stuff, which would make it quite easy to change around the internals (eg stop relying on scale-value) or whatever without worrying about other code breaking. Then, you'd just have internal functions to handle the different types rather than trait impls.
There was a problem hiding this comment.
That is true, maybe I don't need to expose the trait. It just made it easier to reason about the examples for me. I can clean this up.
There was a problem hiding this comment.
Love the new output; looks great :)
I left various comments on the code but nothing blocking approval and things we can iterate on over time.
I do think we should add some unit tests for some of the different internal functions, but I also think we'll want to evolve some of the code anyway over time, like not using scale-value to print output (or improving how scale-value prints output), handling hex and ss58 addresses in the output etc. I don't mind merging this though as a good first step!
fixes #583
fixes #621
fixes #620
The CLI tool has now a command explore which has a few functionalities to explore pallets, calls, call parameters, storage entries and constants of a node. You can also create (unsigned) extrinsics from a SCALE-value from the command line.
The
subxt explorecommand has a tree like structure that guides you through the process of accessing different pallets and their calls, constants and storage entries.Here are some commands that can get you started:
subxt exploreorsubxt explore --file=./artifacts/polkadot_metadata.scaleThe --file or --url flag is optional, if no is submitted it looks for a local node at localhost://9933 as a default
subxt explore Balances(should also show docs for the pallet to help people understand what the pallet does. But currently there are no docs in the metadata)
CALLS
subxt explore Balances callssubxt explore Grandpa calls note_stalledsubxt explore Grandpa calls note_stalled { "delay": 99000, "best_finalized_block_number": 99000 }CONSTANTS
subxt explore Balances constantssubxt explore Balances constants ExistentialDepositSTORAGE
subxt explore Grandpa storagesubxt explore System storage Digestthis is a storage entry that does not need a key, so the value is fetched directly:
subxt explore System storage AccountThis storage call needs an account id as a key, the output tells you that as well:
I guess here I still need to add the custom stringify for account adresses in the type example, such that it is not such an inconvenient byte array.
If we know the Account address of Alice, we can get here account details:
subxt explore System storage Account "((212,53,147,199,21,253,211,28,97,20,26,189,4,169,159,214,130,44,133,88,133,76,205,227,154,86,132,231,165,109,162,125))"