Static Decoding of Signed Extensions: Simple Approach #1235

tadeohepperle · 2023-10-30T17:59:39Z

Instead of introducing a new trait, like in #1226, this PR just adds an associated type Decoded: DecodeAsType to the SignedExtension trait.

Results should be the same as in #1226.

…nsions

…tensions-simple

subxt/examples/blocks_subscribing.rs

jsdw · 2023-10-31T15:28:54Z

subxt/src/blocks/extrinsic_types.rs

    /// Returns an iterator over each of the signed extension details of the extrinsic.
    /// If the decoding of any signed extension fails, an error item is yielded and the iterator stops.
-    pub fn iter(&self) -> impl Iterator<Item = Result<ExtrinsicSignedExtension<'a>, Error>> {
+    pub fn iter(&'a self) -> impl Iterator<Item = Result<ExtrinsicSignedExtension<'a, T>, Error>> {


I think you added this &'a self lifetime back because metadata is no longer referenced, but you should be able to get rid of it (esp since you self.metadata.clone() below still).

If Metadata needs to be owned in ExtrinsicSignedExtensions (even though other bits are borrowed) then you could:

let metadata = self.metadata.clone() at the top

then don't reference self in the iter::from_fn below.

But since everything else is borrowed, I'd borrow the Metadata in ExtrinsicSignedExtensions and then all of the data has the 'a lifetime and that lifetime flows into the iterated stuff too. (Basically, we should either fully commit to lifetimes or avoid them completely here I reckon, and fully committing seems like the good way to go unless we run into some case where they are a pain)

jsdw · 2023-10-31T15:30:10Z

subxt/src/blocks/extrinsic_types.rs

            }))
        })
    }

+    fn find_by_name(&self, name: impl AsRef<str>) -> Option<ExtrinsicSignedExtension<'_, T>> {


Could this take &str instead of impl AsRef<str>?

jsdw · 2023-10-31T15:31:37Z

subxt/src/blocks/extrinsic_types.rs

+    /// DecodedValueThunk representing the type of the extra of this signed extension.
+    fn decoded(&self) -> Result<DecodedValueThunk, Error> {
+        let decoded_value_thunk = DecodedValueThunk::decode_with_metadata(
+            &mut &self.bytes[..],
+            self.ty_id,
+            &self.metadata,
+        )?;
+        Ok(decoded_value_thunk)
+    }


It looks like some things, like this and the lifetime stuff on ExtrinsicSignedExtensions were lost again compared to the base PR that these build off; would be good to merge the underlying branch or rebase :)

You are right, some stuff was lost in the merge, sorry. But this decoded function I kept on purpose. It is just private now, but used in the public as_type() and value() functions.

In the other PR I'd updated to directly use Value::decode_as_type(..) which should be better than DecodedValueThunk::decode_with_metadata(..), and also I'd remove the value() function and just return a Value from this one (I'd have a look at the other PR because that was good here :))

jsdw · 2023-10-31T15:35:01Z

subxt/src/config/signed_extensions.rs

+    /// Tip
+    pub tip: Compact<u128>,
+    /// Asset Id
+    pub asset_id: Option<u32>,


here and below I think I wouldn't make thesepub and instead expose tip() and asset_id() getters where tip returns u128 and the Compact thing is hidden away (it's an implementation detail) :)

jsdw · 2023-10-31T15:35:52Z

subxt/src/config/signed_extensions.rs

-        (self.tip, self.asset_id).encode_to(v);
+        self.encode_to(v);


Personally I prefer the prev approach and not making the struct Encode (it feels like an implementation detail that's better to not expose). Same as below

jsdw · 2023-10-31T15:36:25Z

subxt/src/config/signed_extensions.rs

 pub struct ChargeTransactionPayment {
-    tip: Compact<u128>,
+    /// Tip
+    pub tip: Compact<u128>,


As above I'd expose a tip() getter that returns u128 to hide the Compact implementation detail, and wouldn't have this be Encode

jsdw · 2023-10-31T15:37:56Z

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

    assert_eq!(tip2, 5678);
-
+    assert_eq!(tip2, tip2_static);


it'd be really good I think to also test getting the mortality (just because that returns an enum and would be good to check that it decodes OK! I'm not sure off the top of my head whether the encoded impl is consistent with this or not))

I added a check at the end of the test to verify Mortality decodes and is Era::Immortal.

Nice, thank you! That's reassuring :)

jsdw · 2023-10-31T15:39:22Z

Nice! Yeah, I think I prefer this to the other way of doing things; one less trait, and the changes are confined to the SignedExtension logic and don't effect anybody that is using ExtrinsicParams directly for instance.

Also just generally less code and no weird indirection from the new trait to the ExtrinsicParamsEncoder trait :)

jsdw · 2023-11-02T16:51:50Z

subxt/src/blocks/extrinsic_types.rs

+    /// DecodedValueThunk representing the type of the extra of this signed extension.
+    fn decoded(&self) -> Result<DecodedValueThunk, Error> {
+        let decoded_value_thunk = DecodedValueThunk::decode_with_metadata(
+            &mut &self.bytes[..],
+            self.ty_id,
+            self.metadata,
+        )?;
+        Ok(decoded_value_thunk)
+    }
+
    /// Signed Extension as a [`scale_value::Value`]
    pub fn value(&self) -> Result<DecodedValue, Error> {
-        let value =
-            DecodedValue::decode_as_type(&mut &self.bytes[..], self.ty_id, self.metadata.types())?;
+        self.decoded()?.to_value()
+    }

-        Ok(value)
+    /// Decodes the `extra` bytes of this Signed Extension into a static type.
+    pub fn as_type<E: DecodeAsType>(&self) -> Result<E, Error> {
+        self.decoded()?.as_type::<E>().map_err(Into::into)


Please revert these changes here to master :)

jsdw · 2023-11-02T17:38:32Z

subxt/src/blocks/extrinsic_types.rs

+    pub fn as_type<E: DecodeAsType>(&self) -> Result<E, Error> {
+        let value = E::decode_as_type(&mut &self.bytes[..], self.ty_id, self.metadata.types())?;
        Ok(value)
    }


So for eg events we have pub fn as_event<E: StaticEvent>(&self) -> Result<Option<E>, Error>, and then for extrinsics we have pub fn as_extrinsic<E: StaticExtrinsic>(&self) -> Result<Option<E>, Error>.

I'd be inclined to add something here like:

pub fn as_signed_extra<S: SignedExtension>(&self) -> Result<Option<S::Decoded>, Error> { // Have sanity check that identifier lines up with `SignedExtension` given and then the // usual decode logic, `Ok(None)` if name doesn't match (like the others). }

This would be consistent with the above, and also be a little stricter than as_type, only allowing something that looks like the correct signed extension type to even be tried in the first place. I have no real objections to keeping as_type around too, but probably would remove it for now in favour of people defining and using more proper SignedExtension impls here :)

jsdw · 2023-11-02T17:51:19Z

All looks great now; just one comment to change as_type into as_signed_extra and add the extra check that the name lines up (see #1235 (comment)); this will be consistent then with the methods on events and extrinsics, and a little safer than allowing any as_type :)

jsdw · 2023-11-02T18:32:05Z

subxt/src/blocks/extrinsic_types.rs

@@ -669,7 +669,7 @@ impl<'a, T: Config> ExtrinsicSignedExtensions<'a, T> {
    /// If the Signed Extension is found, but decoding failed, `Some(Err(err))` is returned.
    pub fn find<S: SignedExtension<T>>(&self) -> Option<Result<S::Decoded, Error>> {


Nit: Maybe this should be Result<Option<..> instead, actually? I'm never sure; just looking at some of the other find methods we have around :)

lexnv · 2023-11-03T08:48:07Z

subxt/src/blocks/extrinsic_types.rs

-        // But both should start with a compact encoded u128, so this decoding is fine.
-        let tip = Compact::<u128>::decode(&mut tip.bytes()).ok()?.0;
-        Some(tip)
+        // Note: the overhead of iterating twice should be negligible.


We could probably modify the find_by_name function to take in a filter instead of the name, but yea we have a few extras anyway

lexnv

LGTM! Nice one 👍

tadeohepperle · 2023-11-03T17:12:03Z

Since #1227 merged, I am having some trouble implementing scale_decode::IntoVisitor for ChargeAssetTxPayment<T: Config> manually. Just deriving DecodeAsType on ChargeAssetTxPayment does not work anymore because of the Generics, even though I add a DecodeAsType trait bound to the AssetId type of the config.

It tells me, that Option<T::AssetId> also does not implement IntoVisitor and DecodeAsType which seems strange to me, because Option<G> should implement DecodeAsType and so should Option<T::AssetId> should do as well. Need to investigate this more next week.

jsdw · 2023-11-03T17:28:55Z

Just deriving DecodeAsType on ChargeAssetTxPayment does not work anymore because of the Generics

Maybe it's not documented (I didn't check) but I think DecodeAsType derive macro supports a top level trait_bounds attribute which you can set to "" or something to manually tell it that T: Config doesn't need to impl IntoVisitor!

(at least it looks like it supports this based on the scale-decode-derive code!)

Edit: ah here we go; see the bottom example:

https://docs.rs/scale-decode/latest/scale_decode/derive.DecodeAsType.html

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update lightclient Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * testing: Fix typo Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * testing: Update cargo.toml Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * lightclient: Add tracing logs to improve debugging Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * lightclient: Add socket buffers module for `PlatformRef` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * lightclient: Update `SubxtPlatform` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * cargo: Add lightclient dependencies Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Update cargo.lock of wasm tests Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * lightclient: Add constant for with-buffer module Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * lightclient: Replace rand crate with getrandom Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * example: Update cargo lock file Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * examples: Update deps Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Tadeo Hepperle <62739623+tadeohepperle@users.noreply.github.com>

…lt impl (#1227) * Asset Id in Config trait * add example configuring the config * fmt * fix Default trait bound * merge examples, fix default again * adjust config in examples * Update subxt/src/config/mod.rs Co-authored-by: James Wilson <james@jsdw.me> --------- Co-authored-by: James Wilson <james@jsdw.me>

…tensions-simple

jsdw · 2023-11-10T17:26:37Z

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

+    // check that era decodes:
+    for extensions in [&extensions1, &extensions2] {
+        let era: Era = extensions
+            .find::<CheckMortality<SubstrateConfig>>()


It's a shame to have to write the Config bound in like this, but unavoidable as it stands!

(We can probably come up with something to avoid this at some point, but all good for now!)

tadeohepperle and others added 16 commits October 9, 2023 11:02

skeleton commit

c0226dc

signed extension decoding

ffec0b1

fix some minor things

b0fb96a

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

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

fe708e7

…nsions

clippy

0eabab9

support both ChargeTransactionPayment and ChargeAssetTxPayment

58fd654

address PR comments

1dd8f53

Extend lifetimes, expose pub structs, remove as_type

64b5c90

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

7d8447e

…nsions

add signed extensions to block subscribing example

fe1db8c

add Decoded type

8c93d5b

tadeohepperle requested a review from a team as a code owner October 30, 2023 17:59

tadeohepperle added 3 commits October 31, 2023 10:49

Merge branch 'master' into tadeohepperle-static-decoding-of-signed-ex…

bba2c4b

…tensions-simple

fix merging bug and tests

2692882

add decoded type in CustomSignedExtension

0d52317

jsdw reviewed Oct 31, 2023

View reviewed changes

subxt/examples/blocks_subscribing.rs Outdated Show resolved Hide resolved

jsdw reviewed Oct 31, 2023

View reviewed changes

tadeohepperle mentioned this pull request Oct 31, 2023

Static decoding of signed extensions. #1226

Closed

tadeohepperle added 2 commits October 31, 2023 18:25

fix minor issues, extend test

7ff7552

cargo fmt differences

f200d0f

jsdw reviewed Nov 2, 2023

View reviewed changes

remove the decoded function

5afe150

jsdw reviewed Nov 2, 2023

View reviewed changes

new as_signed_extra fn, do not expose as_type anymore

42a0423

jsdw reviewed Nov 2, 2023

View reviewed changes

jsdw approved these changes Nov 2, 2023

View reviewed changes

lexnv reviewed Nov 3, 2023

View reviewed changes

lexnv approved these changes Nov 3, 2023

View reviewed changes

fix Result-Option order, simplify obtaining Nonce

b7d9411

lexnv and others added 4 commits November 9, 2023 13:17

tx: Remove wait_for_in_block helper method (#1237)

13e3f2d

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

generic AssetId

d50519d

jsdw mentioned this pull request Nov 10, 2023

Bump scale-decode and scale-value to latest #1252

Merged

tadeohepperle added 3 commits November 10, 2023 17:54

Merge branch 'master' into tadeohepperle-static-decoding-of-signed-ex…

1dc9976

…tensions-simple

fix generics

3a79e70

fmt

0876c28

jsdw reviewed Nov 10, 2023

View reviewed changes

tadeohepperle merged commit 56d0cda into master Nov 10, 2023
10 checks passed

tadeohepperle deleted the tadeohepperle-static-decoding-of-signed-extensions-simple branch November 10, 2023 17:32

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

Static Decoding of Signed Extensions: Simple Approach #1235

Static Decoding of Signed Extensions: Simple Approach #1235

tadeohepperle commented Oct 30, 2023

jsdw Oct 31, 2023

jsdw Oct 31, 2023

jsdw Oct 31, 2023

tadeohepperle Oct 31, 2023

jsdw Oct 31, 2023

jsdw Oct 31, 2023 •

edited

Loading

jsdw Oct 31, 2023

jsdw Oct 31, 2023 •

edited

Loading

jsdw Oct 31, 2023

tadeohepperle Nov 2, 2023

jsdw Nov 2, 2023 •

edited

Loading

jsdw commented Oct 31, 2023

jsdw Nov 2, 2023 •

edited

Loading

jsdw Nov 2, 2023

jsdw commented Nov 2, 2023 •

edited

Loading

jsdw Nov 2, 2023 •

edited

Loading

lexnv Nov 3, 2023

lexnv left a comment

tadeohepperle commented Nov 3, 2023

jsdw commented Nov 3, 2023 •

edited

Loading

jsdw Nov 10, 2023

tadeohepperle Nov 10, 2023

		@@ -669,7 +669,7 @@ impl<'a, T: Config> ExtrinsicSignedExtensions<'a, T> {
		/// If the Signed Extension is found, but decoding failed, `Some(Err(err))` is returned.
		pub fn find<S: SignedExtension<T>>(&self) -> Option<Result<S::Decoded, Error>> {

Static Decoding of Signed Extensions: Simple Approach #1235

Static Decoding of Signed Extensions: Simple Approach #1235

Conversation

tadeohepperle commented Oct 30, 2023

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 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 31, 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 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

jsdw commented Oct 31, 2023

jsdw Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw commented Nov 2, 2023 • edited Loading

jsdw Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

tadeohepperle commented Nov 3, 2023

jsdw commented Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 31, 2023 •

edited

Loading

jsdw Oct 31, 2023 •

edited

Loading

jsdw Nov 2, 2023 •

edited

Loading

jsdw Nov 2, 2023 •

edited

Loading

jsdw commented Nov 2, 2023 •

edited

Loading

jsdw Nov 2, 2023 •

edited

Loading

jsdw commented Nov 3, 2023 •

edited

Loading