Parse signed payload and expose call data type information #66

jsdw · 2021-11-03T11:20:06Z

This PR builds on @Noch3's work to add decoding for the signer payload. It also exposes the Variant type in the call data, so that you can access the name and details about each field, and it exposes scale_info more directly, and a means of getting hold of the type registry from our metadata.

Noch3's PR exposed the cursor approach (using input like &mut &[u8]); I felt like this made it easier to decide how to handle remaining bytes, and decode bits from a stream, so I consistified the rest of the decode API on this approach too.

I'm also experimenting here with returning references wherever possible when decoding to save some allocations when they may not be needed.

A couple of thoughts/questions:

Is it worth having the Owned variants of these structs around or should it be left to users to clone/own whatever they want from them?
I'm exposing the Variant type in the call data info. I'm not super confortable about mingling types and values in the output (at least, w.r.t converting to owned structs). Curious how others feel on this!

jsdw · 2021-11-03T11:20:57Z

@Noch3 I'd be keen on gathering your feedback on this one, since it builds on your previous PRs and hopefully exposes the pieces of information you wanted :)

elliot-u410 · 2021-11-03T12:46:24Z

Wow thank you so much! I'll take a look today!

elliot-u410

This is awesome! Thank you!

I think this helps #57 but #60 would require keeping the call data in each layer of Value. That would be a larger refactor and can be saved for later but I think that would be amazing for nested call-data things like batch/proxy.

I'm gonna do a bit of testing with this.

core_v14/src/decoder/mod.rs

jsdw · 2021-11-03T15:31:06Z

I think this helps #57 but #60 would require keeping the call data in each layer of Value. That would be a larger refactor and can be saved for later but I think that would be amazing for nested call-data things like batch/proxy.

I wonder whether this PR providing a separate tree of type information that can be aligned to the Value types that come back would be sufficient for #60 (it's a little more work perhaps to line up and compare the type tree with the values, but I believe still possible?

elliot-u410 · 2021-11-03T15:32:10Z

Yes the design space there is large so it makes sense to solve that separately.

core_v14/src/metadata/mod.rs

TarikGul · 2021-11-03T17:21:17Z

core_v14/src/metadata/mod.rs

-	pub fn args(&self) -> &[TypeId] {
-		&self.args
-	}
+struct MetadataCalls {


TarikGul · 2021-11-03T17:22:49Z

core_v14/tests/decode_extrinsics.rs

+	unnamed_value(vec![])
+}
+
+fn singleton_value(x: Value) -> Value {


Whats the idea behind storing prim_u<int>_value() in a singleton for the tests?

had the same question!

A singleton here is just a Composite value with only a single member. Some of the metadata types decode into such a thing (eg newtype wrapper structs), and so we need to do the same wrapping when comparing the Value we get back.

TarikGul · 2021-11-03T17:27:36Z

core_v14/src/lib.rs

@@ -21,3 +21,6 @@ pub mod value;
 pub use decoder::Decoder;
 pub use metadata::Metadata;
 pub use value::Value;
+
+/// A re-export of the [`scale_info`] crate, since we delegate much of the type inspection to it.


What does re-export mean, as in where else is this getting exported? Trying to get more familiar with whats going on here :)

It's common for libraries that manipulate data expressed in types stemming from another crate (like here, much of what is going on in desub relies on scale-info types) to "adopt" or "forward" that other crate's types as if they were its own.
One instance of this is how the rust standard library re-exports types and code modules from the core:: namespace as std::. The core:: set of types and modules do not require a std environment but that's not very important to std-using applications, so for convenience the core::* modules are also available through std::*.
In this case James is re-exporting all of scale-info, allowing users to access them through the desub path, e.g. the use desub::TypeInfo, rather than use scale_info::TypeInfo. This is convenient and perfectly fine because it is not a feature of desub to have "swappable" scale type info backends.

Clear as mud? :)

(Thank you! Nit: they'd have access via desub::scale_info::TypeInfo as it stands. The main benefit just being that you don't need to also add/keep-up-to-date scale_info to yoru Cargo.toml; you can just access the version used by desub)

TarikGul · 2021-11-03T17:34:46Z

core_v14/src/metadata/mod.rs

-	calls: HashMap<u8, MetadataCall>,
+	/// Metadata may not contain call information. If it does,
+	/// it'll be here.
+	calls: Option<MetadataCalls>,


Might be a silly dumb question, but do you know any cases off the top of your head where this would unwrap to None?

Just guessing here, but maybe something like the shell parachain could have zero calls? (Well, hmm, that one has exactly one call right?). It'd be something pretty exotic, but unless it's illegal to have a pallet with zero calls we need something here that can represent such a thing.

I wasn't sure either, but since the frame-metadata type allows for None I thought that instead of erroring immediately, I'd allow for the possibility and, if needed, handle errors closer to the calls that would be broken by it.

TarikGul

lgtm. I really like the inline docs as it allowed me to follow along pretty seamlessly.

@Noch3 Thanks for bringing this to light!

core_v14/src/decoder/extrinsic_bytes.rs

core_v14/src/decoder/mod.rs

dvdplm · 2021-11-03T19:45:22Z

core_v14/src/decoder/mod.rs

@@ -127,17 +129,14 @@ pub enum DecodeError {
 	DecodeTypeError(#[from] DecodeTypeError),
 	#[error("Failed to decode: expected more data")]
 	EarlyEof(&'static str),
+	#[error("Failed to decode extrinsics: expected {0} more bytes to be consumed from the input")]
+	DidntConsumeBytes(usize),


UndrainedBytes maybe?

I had trouble with this one! Maybe BytesNotConsumed or ExcessBytes?

I like ExcessBytes.

ExcessBytes it is!

dvdplm · 2021-11-03T19:45:59Z

core_v14/src/decoder/mod.rs

-	/// of the extrinsic we think we decoded, and the number of bytes left in the slice provided (which we
-	/// expected to entirely consume, but did not).
-	#[error("Decoding an extrinsic should consume all bytes, but {0} bytes remain")]
-	LateEof(usize, Extrinsic),


good riddance! That "late end of file" really messed with my brain. :)

dvdplm · 2021-11-03T20:27:05Z

core_v14/src/metadata/version_14.rs

+				for (idx, variant) in calls_variant.variants().iter().enumerate() {
+					call_variant_indexes.insert(variant.index(), idx);
+				}


Can't we use FromIterator here somehow, dunno, call_variant_indexes = calls_variant.variants().iter().enumerate().collect()?

random thought: perhaps call_variant_indexes should be an IndexMap? I guess these collections are small enough that it doesn't matter.

Ah, yup we can now collect straight into a HashMap; good call!

I looked at IndexMap briefly; it's all about preserving insertion order by the sounds of it. What was your thinking around using it?

(since the index is a u8 I could see that a [usize; 256] or something could be used if we wanted)

What was your thinking around using it?

It was a random thought, something like this: given the purpose of this hashmap is to map from one index to another index, we could sort of use insertion order (idx here) instead of a hashed key. So IndexMap's IndexSet type. Or, as you suggest, a fixed sized array.
I don't think it's a good optimization to make at this point though so, "no action required"!

core_v14/src/metadata/version_14.rs

core_v14/src/decoder/mod.rs

dvdplm · 2021-11-03T20:58:53Z

core_v14/tests/decode_extrinsics.rs

+	unnamed_value(vec![])
+}
+
+fn singleton_value(x: Value) -> Value {


had the same question!

core_v14/src/decoder/mod.rs

Co-authored-by: David <[email protected]> Co-authored-by: Tarik Gul <[email protected]>

insipx

lgtm!

jsdw added 5 commits November 2, 2021 19:14

WIP decode signer payload

a4c4b4e

owned and borrowed, fix and import test from Noch3's PR

cfb3e05

Cargo fmt

82857a5

fix clippy lints

5950e98

expose a way to resolve types, and expose scale_info at root

e04db26

jsdw requested a review from insipx November 3, 2021 11:23

jsdw mentioned this pull request Nov 3, 2021

Add parsing for signed payload #63

Closed

Fix binaries

4f78ad9

Fix space in doc comment issue more aggressively

09d8743

elliot-u410 reviewed Nov 3, 2021

View reviewed changes

core_v14/src/decoder/mod.rs Outdated Show resolved Hide resolved

core_v14/src/decoder/mod.rs Show resolved Hide resolved

map rather than for loop

db07552

jsdw mentioned this pull request Nov 3, 2021

Keep more verbose information about each extrinsic #60

Closed

TarikGul reviewed Nov 3, 2021

View reviewed changes

core_v14/src/metadata/mod.rs Outdated Show resolved Hide resolved

TarikGul reviewed Nov 3, 2021

View reviewed changes

core_v14/src/metadata/mod.rs

pub fn args(&self) -> &[TypeId] {

&self.args

}

struct MetadataCalls {

Copy link

Member

TarikGul Nov 3, 2021

Choose a reason for hiding this comment

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

👍

TarikGul reviewed Nov 3, 2021

View reviewed changes

TarikGul approved these changes Nov 3, 2021

View reviewed changes

dvdplm reviewed Nov 3, 2021

View reviewed changes

dvdplm reviewed Nov 4, 2021

View reviewed changes

core_v14/src/decoder/mod.rs Outdated Show resolved Hide resolved

jsdw and others added 3 commits November 4, 2021 10:22

Cargo fmt

7923b5c

A couple of tidyups from review suggestions

b930215

Apply doc comment suggestions from code review

9c12e08

Co-authored-by: David <[email protected]> Co-authored-by: Tarik Gul <[email protected]>

insipx approved these changes Nov 4, 2021

View reviewed changes

Add a couple of tests around not consuming extra bytes

3bde9e8

dvdplm approved these changes Nov 4, 2021

View reviewed changes

ExcessBytes

0fbd194

jsdw merged commit 87377e7 into master Nov 4, 2021

jsdw deleted the jsdw-decode-flexibility branch November 4, 2021 13:55

elliot-u410 mentioned this pull request Nov 4, 2021

Support decoding unsigned signer payload extrinsics #61

Closed

jsdw mentioned this pull request Nov 8, 2021

Parse and retain metadata call argument names #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse signed payload and expose call data type information #66

Parse signed payload and expose call data type information #66

jsdw commented Nov 3, 2021 •

edited

Loading

jsdw commented Nov 3, 2021

elliot-u410 commented Nov 3, 2021

elliot-u410 left a comment •

edited

Loading

jsdw commented Nov 3, 2021

elliot-u410 commented Nov 3, 2021

TarikGul Nov 3, 2021

TarikGul Nov 3, 2021

dvdplm Nov 3, 2021

jsdw Nov 4, 2021

TarikGul Nov 3, 2021

dvdplm Nov 3, 2021

jsdw Nov 4, 2021 •

edited

Loading

TarikGul Nov 3, 2021

dvdplm Nov 3, 2021

jsdw Nov 4, 2021

TarikGul left a comment

dvdplm Nov 3, 2021

jsdw Nov 4, 2021

dvdplm Nov 4, 2021

jsdw Nov 4, 2021

dvdplm Nov 3, 2021

dvdplm Nov 3, 2021

dvdplm Nov 3, 2021

jsdw Nov 4, 2021

dvdplm Nov 4, 2021

dvdplm Nov 3, 2021

insipx left a comment

Parse signed payload and expose call data type information #66

Parse signed payload and expose call data type information #66

Conversation

jsdw commented Nov 3, 2021 • edited Loading

jsdw commented Nov 3, 2021

elliot-u410 commented Nov 3, 2021

elliot-u410 left a comment • edited Loading

Choose a reason for hiding this comment

jsdw commented Nov 3, 2021

elliot-u410 commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TarikGul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insipx left a comment

Choose a reason for hiding this comment

jsdw commented Nov 3, 2021 •

edited

Loading

elliot-u410 left a comment •

edited

Loading

jsdw Nov 4, 2021 •

edited

Loading