Skip to content
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

Support decoding signed extensions #1209

Merged
merged 16 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 201 additions & 45 deletions subxt/src/blocks/extrinsic_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ use crate::{
Metadata,
};

use crate::dynamic::{DecodedValue, DecodedValueThunk};
use crate::metadata::DecodeWithMetadata;
use crate::utils::strip_compact_prefix;
use codec::Decode;
use codec::{Compact, Decode};
use derivative::Derivative;
use scale_decode::{DecodeAsFields, DecodeAsType};

use std::sync::Arc;

/// Trait to uniquely identify the extrinsic's identity from the runtime metadata.
Expand Down Expand Up @@ -155,12 +158,8 @@ pub struct ExtrinsicDetails<T: Config, C> {
index: u32,
/// Extrinsic bytes.
bytes: Arc<[u8]>,
/// True if the extrinsic payload is signed.
is_signed: bool,
/// The start index in the `bytes` from which the address is encoded.
address_start_idx: usize,
/// The end index of the address in the encoded `bytes`.
address_end_idx: usize,
/// Some if the extrinsic payload is signed.
signed_details: Option<SignedExtrinsicDetails>,
/// The start index in the `bytes` from which the call is encoded.
call_start_idx: usize,
/// The pallet index.
Expand All @@ -178,6 +177,18 @@ pub struct ExtrinsicDetails<T: Config, C> {
_marker: std::marker::PhantomData<T>,
}

/// Details only available in signed extrinsics.
pub struct SignedExtrinsicDetails {
/// start index of the range in `bytes` of `ExtrinsicDetails` that encodes the address.
address_start_idx: usize,
/// end index of the range in `bytes` of `ExtrinsicDetails` that encodes the address. Equivalent to signature_start_idx.
address_end_idx: usize,
/// end index of the range in `bytes` of `ExtrinsicDetails` that encodes the signature. Equivalent to extra_start_idx.
signature_end_idx: usize,
/// end index of the range in `bytes` of `ExtrinsicDetails` that encodes the signature.
extra_end_idx: usize,
}

impl<T, C> ExtrinsicDetails<T, C>
where
T: Config,
Expand Down Expand Up @@ -217,38 +228,45 @@ where
// Skip over the first byte which denotes the version and signing.
let cursor = &mut &bytes[1..];

let mut address_start_idx = 0;
let mut address_end_idx = 0;

if is_signed {
address_start_idx = bytes.len() - cursor.len();

// Skip over the address, signature and extra fields.
scale_decode::visitor::decode_with_visitor(
cursor,
ids.address,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;
address_end_idx = bytes.len() - cursor.len();

scale_decode::visitor::decode_with_visitor(
cursor,
ids.signature,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;

scale_decode::visitor::decode_with_visitor(
cursor,
ids.extra,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;
}
let signed_details = is_signed
.then(|| -> Result<SignedExtrinsicDetails, Error> {
let address_start_idx = bytes.len() - cursor.len();
// Skip over the address, signature and extra fields.
scale_decode::visitor::decode_with_visitor(
cursor,
ids.address,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;
let address_end_idx = bytes.len() - cursor.len();

scale_decode::visitor::decode_with_visitor(
cursor,
ids.signature,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;
let signature_end_idx = bytes.len() - cursor.len();

scale_decode::visitor::decode_with_visitor(
cursor,
ids.extra,
metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(scale_decode::Error::from)?;
let extra_end_idx = bytes.len() - cursor.len();

Ok(SignedExtrinsicDetails {
address_start_idx,
address_end_idx,
signature_end_idx,
extra_end_idx,
})
})
.transpose()?;

let call_start_idx = bytes.len() - cursor.len();

Expand All @@ -261,9 +279,7 @@ where
Ok(ExtrinsicDetails {
index,
bytes,
is_signed,
address_start_idx,
address_end_idx,
signed_details,
call_start_idx,
pallet_index,
variant_index,
Expand All @@ -277,7 +293,7 @@ where

/// Is the extrinsic signed?
pub fn is_signed(&self) -> bool {
self.is_signed
self.signed_details.is_some()
}

/// The index of the extrinsic in the block.
Expand Down Expand Up @@ -326,8 +342,41 @@ where
///
/// Returns `None` if the extrinsic is not signed.
pub fn address_bytes(&self) -> Option<&[u8]> {
self.is_signed
.then(|| &self.bytes[self.address_start_idx..self.address_end_idx])
self.signed_details
.as_ref()
.map(|e| &self.bytes[e.address_start_idx..e.address_end_idx])
}

/// Return only the bytes of the signature for this signed extrinsic.
///
/// # Note
Copy link
Member

@niklasad1 niklasad1 Oct 30, 2023

Choose a reason for hiding this comment

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

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.

///
/// Returns `None` if the extrinsic is not signed.
pub fn signature_bytes(&self) -> Option<&[u8]> {
self.signed_details
.as_ref()
.map(|e| &self.bytes[e.address_end_idx..e.signature_end_idx])
}

/// Return only the bytes of the extra fields for this signed extrinsic.
///
/// # Note
///
/// Returns `None` if the extrinsic is not signed.
pub fn extra_bytes(&self) -> Option<&[u8]> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

self.signed_details
.as_ref()
.map(|e| &self.bytes[e.signature_end_idx..e.extra_end_idx])
}

/// Returns `None` if the extrinsic is not signed.
pub fn signed_extensions(&self) -> Option<ExtrinsicSignedExtensions> {
let signed = self.signed_details.as_ref()?;
let extra_bytes = &self.bytes[signed.signature_end_idx..signed.extra_end_idx];
Some(ExtrinsicSignedExtensions {
bytes: extra_bytes,
metadata: self.metadata.clone(),
})
}

/// The index of the pallet that the extrinsic originated from.
Expand Down Expand Up @@ -558,6 +607,113 @@ impl<T: Config> ExtrinsicEvents<T> {
}
}

#[derive(Debug, Clone)]
pub struct ExtrinsicSignedExtensions<'a> {
bytes: &'a [u8],
metadata: Metadata,
}

#[derive(Debug, Clone)]
pub struct ExtrinsicSignedExtension<'a> {
bytes: &'a [u8],
ty_id: u32,
identifier: &'a str,
metadata: Metadata,
}

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

@jsdw jsdw Oct 25, 2023

Choose a reason for hiding this comment

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

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).

self.iter()
.find_map(|e| e.ok().filter(|e| e.name() == signed_extension.as_ref()))
}

pub fn iter(&'a self) -> impl Iterator<Item = Result<ExtrinsicSignedExtension<'a>, Error>> {
let signed_extension_types = self.metadata.extrinsic().signed_extensions();
let num_signed_extensions = signed_extension_types.len();
let mut index = 0;
let mut byte_start_idx = 0;

std::iter::from_fn(move || {
if index == num_signed_extensions {
None
} else {
Copy link
Collaborator

@jsdw jsdw Oct 25, 2023

Choose a reason for hiding this comment

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

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

let extension = &signed_extension_types[index];
let ty_id = extension.extra_ty();
let cursor = &mut &self.bytes[byte_start_idx..];
if let Err(err) = scale_decode::visitor::decode_with_visitor(
cursor,
ty_id,
self.metadata.types(),
scale_decode::visitor::IgnoreVisitor,
)
.map_err(|e| Error::Decode(e.into()))
{
index = num_signed_extensions; // (such that None is returned in next iteration)
return Some(Err(err));
}
let byte_end_idx = self.bytes.len() - cursor.len();
let bytes = &self.bytes[byte_start_idx..byte_end_idx];
byte_start_idx = byte_end_idx;
index += 1;
Some(Ok(ExtrinsicSignedExtension {
bytes,
ty_id,
identifier: extension.identifier(),
metadata: self.metadata.clone(),
}))
}
})
}

/// The tip of an extrinsic, extracted from the ChargeTransactionPayment signed extension.
pub fn tip(&self) -> Option<u128> {
let tip = self.find("ChargeAssetTxPayment")?;
let tip = Compact::<u128>::decode(&mut tip.bytes()).ok()?.0;
Some(tip)
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

let nonce = self.find("CheckNonce")?;
let nonce = Compact::<u64>::decode(&mut nonce.bytes()).ok()?.0;
Some(nonce)
}
}

impl<'a> ExtrinsicSignedExtension<'a> {
/// The extra bytes associated with the signed extension.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

pub fn bytes(&self) -> &[u8] {
self.bytes
}

/// The name of the signed extension.
pub fn name(&self) -> &str {
self.identifier
}

/// The type id of the signed extension.
pub fn type_id(&self) -> u32 {
self.ty_id
}

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

@jsdw jsdw Oct 25, 2023

Choose a reason for hiding this comment

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

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(...)

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 = self.decoded()?.to_value()?;
Ok(value)
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
62 changes: 62 additions & 0 deletions testing/integration-tests/src/full_client/blocks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@
// This file is dual-licensed as Apache-2.0 or GPL-3.0.
// see LICENSE for license details.

use crate::utils::TestContext;
use crate::{test_context, utils::node_runtime};
use codec::{Compact, Encode};
use futures::StreamExt;
use std::future::Future;
use std::time::Duration;
use subxt::config::DefaultExtrinsicParamsBuilder;
use subxt_metadata::Metadata;
use subxt_signer::sr25519::dev;

Expand Down Expand Up @@ -227,3 +231,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() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

let ctx = test_context().await;
let api = ctx.client();
let alice = dev::alice();
let bob = dev::bob();

macro_rules! submit_transfer_extrinsic_and_get_it_back {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

($tip:expr) => {{
let tx = node_runtime::tx()
.balances()
.transfer_allow_death(bob.public_key().into(), 10_000);

let signed_extrinsic = api
.tx()
.create_signed(
&tx,
&alice,
DefaultExtrinsicParamsBuilder::new().tip($tip).build(),
)
.await
.unwrap();

let in_block = signed_extrinsic
.submit_and_watch()
.await
.unwrap()
.wait_for_finalized()
.await
.unwrap();

let block_hash = in_block.block_hash();
let block = api.blocks().at(block_hash).await.unwrap();
let extrinsics = block.extrinsics().await.unwrap();
let extrinsic_details = extrinsics
.iter()
.find_map(|e| e.ok().filter(|e| e.is_signed()))
.unwrap();
extrinsic_details
}};
}

let transaction1 = submit_transfer_extrinsic_and_get_it_back!(1234);
let extensions1 = transaction1.signed_extensions().unwrap();
let nonce1 = extensions1.nonce().unwrap();
let tip1 = extensions1.tip().unwrap();

let transaction2 = submit_transfer_extrinsic_and_get_it_back!(5678);
let extensions2 = transaction2.signed_extensions().unwrap();
let nonce2 = extensions2.nonce().unwrap();
let tip2 = extensions2.tip().unwrap();

assert_eq!(nonce1, 0);
assert_eq!(tip1, 1234);
assert_eq!(nonce2, 1);
assert_eq!(tip2, 5678);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}