-
Notifications
You must be signed in to change notification settings - Fork 13
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
Keep argument names in for extrinsic calls in v14 #58
Keep argument names in for extrinsic calls in v14 #58
Conversation
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.
LGTM
@@ -48,7 +48,7 @@ pub fn decode(meta: RuntimeMetadataV14) -> Result<Metadata, MetadataError> { | |||
for variant in call_variant.variants() { | |||
// Allow case insensitive matching; lowercase the name: | |||
let name = variant.name().to_ascii_lowercase(); | |||
let args = variant.fields().iter().map(|field| *field.ty()).collect(); | |||
let args = variant.fields().iter().map(|field| (*field.ty(), field.name().map(|x| x.clone()))).collect(); |
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.
let args = variant.fields().iter().map(|field| (*field.ty(), field.name().map(|x| x.clone()))).collect(); | |
let args = variant.fields().iter().map(|field| (*field.ty(), field.name().cloned()))).collect(); |
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.
Heh I suspected this would work but forgot to try. Thanks.
Now I'm wondering if there's a way we can just keep all of the |
@@ -151,7 +151,7 @@ impl MetadataCall { | |||
/// The types expected to be provided as arguments to this call. | |||
/// [`TypeId`]'s can be resolved into [`Type`]'s using | |||
/// [`Metadata::resolve_type`] |
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.
sorry, not part of your PR but this comment should be:
/// [`Metadata::resolve_type`] | |
/// [`Metadata::types`] |
resolve_type
fn does not exist on metadata, instead resolve
is accessed by first calling types()
Yeah I think this is definitely possible. I'm not totally clear on what information might be missing, though. Extrinsic is a list of arguments that were executed in a fn call (with a signature/signed extension attached if it's signed) Maybe you're talking about doc comments and stuff? |
Yes I'm imagining a way to show the tree of calls along with argument names, type names, and docs for each argument. The calls can be nested (if you use batch or proxy) so that the full tree might be rather large. But the goal is to basically give someone a lot of confidence that the thing they plan to sign is in fact what they intended to do. |
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.
Would it make sense to include the docs here as well? Code lgtm modulo the clippy complaints.
I also think that we should look to expose as best as possible all of the information one might need. To avoid copying too much data out of the type registry, I'd lean towards exposing the type information fairly directly, possibly starting with the call variant information. Thst way, you can dig through the types as much as you like to get out the information you need. So far, the metadata struct has been kept deliberately fairly opaque (see the pub crate fns), and the scale-info types kept hidden away, but I definitely see the value in exposing access to the type information stored in the metadata! |
I'll close this in favor of a better design that keeps everything! |
My plan is to work on that quite shortly; there is some other refactoring I might do as part of such a change :) |
Fixes #57