Support decoding signed extensions #1209

tadeohepperle · 2023-10-16T08:11:46Z

Fixes #1124
This PR adds an option to dynamically decode signed extensions from extrinsics.
Until now we just skipped over any signed extensions (Signed Extra bytes).
There are two convenience functions for tip() and nonce(). When you have ExtrinsicDetails you can not do the following:

let extrinsic: ExtrinsicDetails;
let signed_extensions = extrinsic.signed_extensions()?;
// tip and nonce
let tip: u128 = signed_extensions.tip()?;
let nonce: u64 = signed_extensions.nonce()?;
// iterate over signed extensions
for s in signed_extensions.iter() {
     let s = s.unwrap(); // error if decoding failed. Ends iteration.
     let name = s.name();    
     let scale_value = s.value();
     let bytes = s.bytes();
}

// or find a specific one:
let mortality = signed_extensions.find("CheckMortality")?;
...

subxt/src/blocks/extrinsic_types.rs

subxt/src/config/polkadot.rs

subxt/src/config/substrate.rs

subxt/src/blocks/extrinsic_types.rs

…nsions

niklasad1 · 2023-10-25T09:05:18Z

subxt/src/blocks/extrinsic_types.rs

+    /// # Note
+    ///
+    /// Returns `None` if the extrinsic is not signed.
+    pub fn extra_bytes(&self) -> Option<&[u8]> {


dq: does this include just extra or extra | additional_signed?

See https://docs.rs/sp-runtime/latest/src/sp_runtime/generic/unchecked_extrinsic.rs.html#197 for context

Regardless, I think it would be good to document that

Also, since we have signed_extensions below to get the signed extensions, let's call this signed_extensions_bytes so it's clear it's getting the bytes for the same thing.

subxt/src/blocks/extrinsic_types.rs

niklasad1 · 2023-10-25T09:47:07Z

subxt/src/blocks/extrinsic_types.rs

+    /// The tip of an extrinsic, extracted from the ChargeTransactionPayment signed extension.
+    pub fn tip(&self) -> Option<u128> {
+        let tip = self
+            .find("ChargeTransactionPayment")


Would be cool have these in the metadata, I'm not sure whether some chains overrides the transaction_payment pallet and add their own impl probably not.

Yeah; the idea of these "quick" helpers is that they work in the overwhelmingly common case, but may not always work (and should fail gracefully eg returning None here if the extension isn't available)

niklasad1 · 2023-10-25T09:50:56Z

subxt/src/blocks/extrinsic_types.rs

+    }
+
+    /// The nonce of the account that submitted the extrinsic, extracted from the CheckNonce signed extension.
+    pub fn nonce(&self) -> Option<u64> {


This may fail on certain runtimes if the CheckNonce isn't part of the SignedExtra.

Might worth a comment for it and advise to use the dynamic API then.

Should be nice to have for the general usecase...

Yeah, it's just for a common case that will most likely work, and should fail gracefully and return None if the relevant extension isn't found :)

niklasad1 · 2023-10-25T09:55:11Z

testing/integration-tests/src/full_client/blocks/mod.rs

@@ -227,3 +229,61 @@ async fn fetch_block_and_decode_extrinsic_details() {
    assert_eq!(ext.value, 10_000);
    assert!(tx.is_signed());
 }
+
+#[tokio::test]
+async fn decode_signed_extensions_from_blocks() {


Would be cool to test with different runtimes so both ChargeAssetTxPayment and ChargeTransactionPayment works but probably annoying to support here.

Could be done in another PR

Yeah; I pondered this too but not sure how I'd test this?! It'd be a pain introducing the ability to spawn up a different node just for this

jsdw · 2023-10-25T10:47:26Z

subxt/src/blocks/extrinsic_types.rs

+            if index == num_signed_extensions {
+                None
+            } else {


Nit; perhaps avoid the indentation in the else by doing return None here instead; an early exit when done :)

jsdw · 2023-10-25T15:20:29Z

subxt/src/blocks/extrinsic_types.rs

+
+impl<'a> ExtrinsicSignedExtensions<'a> {
+    /// Get a certain signed extension by its name.
+    pub fn find(&self, signed_extension: impl AsRef<str>) -> Option<ExtrinsicSignedExtension> {


To be consistent with how Extrinsics/Events are decoded, I think find should look more like:

fn find<S: SignedExtension>() -> Result<Option<S>, Error>

For "dynamically finding things", one can .iter().find(..) anyway.

I'd suggest just removing this for now until we have a nice way to find and decode statically based on the subxt::tx::signed_extensions::* types (which should def be a separate PR).

jsdw · 2023-10-25T15:21:17Z

subxt/src/blocks/extrinsic_types.rs

+}
+
+impl<'a> ExtrinsicSignedExtension<'a> {
+    /// The extra bytes associated with the signed extension.


Suggested change

/// The extra bytes associated with the signed extension.

/// The bytes representing this signed extension.

jsdw · 2023-10-25T15:23:15Z

subxt/src/blocks/extrinsic_types.rs

+    }
+
+    /// DecodedValueThunk representing the type of the extra of this signed extension.
+    pub fn decoded(&self) -> Result<DecodedValueThunk, Error> {


Let's remove this decoded fn; it doesn't add much on top of value() below imo :)

The value fn can do a more "direct" decoding into a Value type them too; something like Value::decode_as_type(...)

jsdw · 2023-10-25T15:37:53Z

testing/integration-tests/src/full_client/blocks/mod.rs

+    assert_eq!(nonce1, 0);
+    assert_eq!(tip1, 1234);
+    assert_eq!(nonce2, 1);
+    assert_eq!(tip2, 5678);


I'd like us to also test that iterating over signed extensions works; right now we have no way to really know if it's actually decoding them as we expect :)

The set of signed extensions on a node is fairly fixed, so we can just sanity test that each of the expected ones is decoded and that the names are right or something like that (in addition to the above).

I added code for testing this to the end of the test :)

jsdw · 2023-10-25T15:42:27Z

Looking good; I resolved all of the comments from myself that you've addressed so just worth looking over the others; nothing major from me now! Will be lovely to have this!

On testing: one way to unit test this would be crafting some custom metadata with whatever signed extensions we want, and then creating an OfflineClient (note the offline), setting the metadata on it to be what we want, and then building an extrinsic (could be dynamically constructed). Then, one can manually make the bytes into an ExtrinsicDetails and try to decode each of the signed extensions from it.

This way, we could test a few different combinations of signed extensions (eg if Nonce/Tip ones don't exist, or if both the tip ones exist etc).

But yes, I don't mind this being a follow-up PR because it'll take a little work to do nicely!

jsdw

Great job, looks good to me!

I wondered why missing docs on a function weren't being picked up in CI, and noticed that the new structs weren't exposed in the module, so I did that and extended a few lifetimes while in the area :)

lexnv · 2023-10-27T15:27:23Z

testing/integration-tests/src/full_client/blocks/mod.rs

+    let alice = dev::alice();
+    let bob = dev::bob();
+
+    macro_rules! submit_transfer_extrinsic_and_get_it_back {


dq: What are the benefits of this macro over a plain function? Looks good otherwise!

There were just some really stupid lifetime issues when using a Closure.
And I cannot use a normal function easily because then I need to specify all types.
And the type of api here depends on the unstable-light-client feature flag. And the type of api would be a generic type of the return value of that function as well. So I would have to provide two implementations of such a function depending on the feature flag. Also I cannot put the api a trait object easily, because OnlineClientT is not object safe. So in the end I just went with this macro.

subxt/src/blocks/extrinsic_types.rs

lexnv · 2023-10-27T15:40:43Z

LGTM!

What do you think about extending the blocks_subscribing to also decode the extensions? 🤔

…nsions

jsdw · 2023-10-30T12:11:12Z

What do you think about extending the blocks_subscribing to also decode the extensions? 🤔

I don't think that this is a blocker for merging, but would be nice to show this stuff in an example somewhere!

subxt/src/blocks/extrinsic_types.rs

niklasad1 · 2023-10-30T12:34:37Z

subxt/src/blocks/extrinsic_types.rs

+
+    /// Return only the bytes of the signature for this signed extrinsic.
+    ///
+    /// # Note


This note can be removed and change the documentation to something like:

Returns Some(signature_bytes) if the extrinsic was signed otherwise None is returned.

niklasad1

nice

jsdw · 2023-10-30T16:31:12Z

subxt/examples/blocks_subscribing.rs

+            if let Some(signed_extensions) = ext.signed_extensions() {
+                println!("      Signed Extensions:");
+                for signed_extension in signed_extensions.iter() {
+                    let signed_extension = signed_extension?;
+                    let name = signed_extension.name();
+                    let value = signed_extension.value()?.to_string();
+                    println!("        {name}: {value}");
+                }
+            } else {
+                println!("      No Signed Extensions");
+            }


Just because this is an example, I wonder whether we could make this a bit less ugly, ie removing the else branch and moving it after the other println's or something?

tadeohepperle added 3 commits October 9, 2023 11:02

skeleton commit

c0226dc

signed extension decoding

ffec0b1

fix some minor things

b0fb96a

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/config/polkadot.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/config/substrate.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 17, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

tadeohepperle added 5 commits October 23, 2023 10:50

make api more similar to Extrinsics

c87f165

defer decoding of signed extensions

4ed554d

fix byte slices

460bfaf

add test for nonce signed extension

ea59e6c

adjust test and extend for tip

3dad997

jsdw reviewed Oct 24, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

tadeohepperle added 3 commits October 24, 2023 13:07

Merge branch 'master' into tadeohepperle/support-decoding-signed-exte…

fe708e7

…nsions

clippy

0eabab9

support both ChargeTransactionPayment and ChargeAssetTxPayment

58fd654

tadeohepperle marked this pull request as ready for review October 24, 2023 12:07

tadeohepperle requested a review from a team as a code owner October 24, 2023 12:07

niklasad1 reviewed Oct 25, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 25, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Show resolved Hide resolved

niklasad1 reviewed Oct 25, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 25, 2023

View reviewed changes

jsdw reviewed Oct 25, 2023

View reviewed changes

address PR comments

1dd8f53

tadeohepperle mentioned this pull request Oct 26, 2023

Static decoding of signed extensions. #1226

Closed

Extend lifetimes, expose pub structs, remove as_type

64b5c90

jsdw approved these changes Oct 27, 2023

View reviewed changes

lexnv reviewed Oct 27, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Show resolved Hide resolved

Merge branch 'master' into tadeohepperle/support-decoding-signed-exte…

7d8447e

…nsions

niklasad1 reviewed Oct 30, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 30, 2023

View reviewed changes

subxt/src/blocks/extrinsic_types.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 30, 2023

View reviewed changes

niklasad1 approved these changes Oct 30, 2023

View reviewed changes

add signed extensions to block subscribing example

fe1db8c

jsdw reviewed Oct 30, 2023

View reviewed changes

lexnv approved these changes Oct 30, 2023

View reviewed changes

make example less ugly

d354b1a

tadeohepperle merged commit 1b88ebf into master Oct 31, 2023
9 checks passed

tadeohepperle deleted the tadeohepperle/support-decoding-signed-extensions branch October 31, 2023 09:40

jsdw mentioned this pull request Dec 7, 2023

Prep for v0.33.0 release #1311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support decoding signed extensions #1209

Support decoding signed extensions #1209

tadeohepperle commented Oct 16, 2023 •

edited

Loading

niklasad1 Oct 25, 2023

jsdw Oct 25, 2023

niklasad1 Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023

niklasad1 Oct 25, 2023

niklasad1 Oct 25, 2023

jsdw Oct 25, 2023

niklasad1 Oct 25, 2023

jsdw Oct 25, 2023

jsdw Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023

jsdw Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023

tadeohepperle Oct 26, 2023

jsdw commented Oct 25, 2023

jsdw left a comment

lexnv Oct 27, 2023

tadeohepperle Oct 27, 2023

lexnv commented Oct 27, 2023

jsdw commented Oct 30, 2023

niklasad1 Oct 30, 2023 •

edited

Loading

niklasad1 left a comment

jsdw Oct 30, 2023

	/// The extra bytes associated with the signed extension.
	/// The bytes representing this signed extension.

Support decoding signed extensions #1209

Support decoding signed extensions #1209

Conversation

tadeohepperle commented Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Oct 25, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

jsdw Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Oct 25, 2023

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv commented Oct 27, 2023

jsdw commented Oct 30, 2023

niklasad1 Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeohepperle commented Oct 16, 2023 •

edited

Loading

niklasad1 Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023 •

edited

Loading

jsdw Oct 25, 2023 •

edited

Loading

niklasad1 Oct 30, 2023 •

edited

Loading