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

runtime API: Substitute UncheckedExtrinsic with custom encoding #1076

Merged
merged 20 commits into from
Jul 21, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
10 changes: 9 additions & 1 deletion codegen/src/types/substitutes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ impl TypeSubstitutes {
parse_quote!(#crate_path::utils::KeyedVec),
),
(path_segments!(BTreeSet), parse_quote!(::std::vec::Vec)),
// The `UncheckedExtrinsic(pub Vec<u8>)` is part of the runtime API calls.
// The inner bytes represent the encoded extrinsic, however when deriving the
// `EncodeAsType` the bytes would be re-encoded. This leads to the bytes
// being altered by adding the length prefix in front of them.
(
path_segments!(sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic),
parse_quote!(#crate_path::utils::UncheckedExtrinsic),
),
];

let default_substitutes = defaults
Expand Down Expand Up @@ -339,7 +347,7 @@ impl<T: scale_info::form::Form> From<&scale_info::Path<T>> for PathSegments {
/// to = ::subxt::utils::Static<::sp_runtime::MultiAddress<A, B>>
/// ```
///
/// And we encounter a `sp_runtime::MultiAddress<Foo, bar>`, then we will pass the `::sp_runtime::MultiAddress<A, B>`
/// And we encounter a `sp_runtime::MultiAddress<Foo, Bar>`, then we will pass the `::sp_runtime::MultiAddress<A, B>`
/// type param value into this call to turn it into `::sp_runtime::MultiAddress<Foo, Bar>`.
fn replace_path_params_recursively<I: Borrow<syn::Ident>, P: Borrow<TypePath>>(
path: &mut syn::Path,
Expand Down
4 changes: 3 additions & 1 deletion subxt/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pub mod bits;
mod multi_address;
mod multi_signature;
mod static_type;
mod unchecked_extrinsic;
mod wrapper_opaque;

use codec::{Compact, Decode, Encode};
Expand All @@ -18,6 +19,7 @@ pub use account_id::AccountId32;
pub use multi_address::MultiAddress;
pub use multi_signature::MultiSignature;
pub use static_type::Static;
pub use unchecked_extrinsic::UncheckedExtrinsic;
pub use wrapper_opaque::WrapperKeepOpaque;

// Used in codegen
Expand All @@ -26,7 +28,7 @@ pub use primitive_types::{H160, H256, H512};

/// Wraps an already encoded byte vector, prevents being encoded as a raw byte vector as part of
/// the transaction payload
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug, Eq, PartialEq, Decode)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah hmmm, I'm wondering about this automatic Decode, because:

  • decoding like this will first decode a Compact length for the vec length and then put the relevant number of butes into the vec.
  • encoding is not the opposite of this; it will just encode the bytes in the vec to the target; no compact length or whatever at the beginning.

Having a look, an extrinsic is actually just a compact encoded length and then that number of bytes.

So probably you were right in a previous iteration, and UncheckedExtrinsic should have a custom Decode impl that will decode the compact length and then the relevant number of bytes following it all into the Static<Encoded> struct, which will then encode (from the impl below) precisely those bytes back.

pub struct Encoded(pub Vec<u8>);

impl codec::Encode for Encoded {
Expand Down
107 changes: 107 additions & 0 deletions subxt/src/utils/unchecked_extrinsic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// Copyright 2019-2023 Parity Technologies (UK) Ltd.
// This file is dual-licensed as Apache-2.0 or GPL-3.0.
// see LICENSE for license details.

//! The "default" Substrate/Polkadot UncheckedExtrinsic.
//! This is used in codegen for runtime API calls.
//!
//! The inner bytes represent the encoded extrinsic expected by the
//! runtime APIs. Deriving `EncodeAsType` would lead to the inner
//! bytes to be re-encoded (length prefixed).

use std::marker::PhantomData;

use codec::Decode;
use scale_decode::{visitor::DecodeAsTypeResult, IntoVisitor, Visitor};

use super::{Encoded, Static};

/// The unchecked extrinsic from substrate.
#[derive(Clone, Debug, Eq, PartialEq, Decode)]
pub struct UncheckedExtrinsic<Address, Call, Signature, Extra>(
Static<Encoded>,
#[codec(skip)] PhantomData<(Address, Call, Signature, Extra)>,
);

impl<Address, Call, Signature, Extra> UncheckedExtrinsic<Address, Call, Signature, Extra> {
/// Construct a new [`UncheckedExtrinsic`].
pub fn new(bytes: Vec<u8>) -> Self {
Self(Static(Encoded(bytes)), PhantomData)
}

/// Get the bytes of the encoded extrinsic.
pub fn bytes(&self) -> &[u8] {
self.0 .0 .0.as_slice()
Copy link
Contributor

Choose a reason for hiding this comment

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

This formatting looks so weird, never seen that before :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same, I used to write it without spaces but I've changed my editor to run rustfmt for each file save which changed it like so :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think rustfmt is just being a bit naff here, but ah well :)

}
}

impl<Address, Call, Signature, Extra> From<Vec<u8>>
for UncheckedExtrinsic<Address, Call, Signature, Extra>
{
fn from(bytes: Vec<u8>) -> Self {
UncheckedExtrinsic::new(bytes)
}
}

impl<Address, Call, Signature, Extra> From<UncheckedExtrinsic<Address, Call, Signature, Extra>>
for Vec<u8>
{
fn from(bytes: UncheckedExtrinsic<Address, Call, Signature, Extra>) -> Self {
bytes.0 .0 .0
}
}

impl<Address, Call, Signature, Extra> codec::Encode
for UncheckedExtrinsic<Address, Call, Signature, Extra>
{
fn encode_to<T: codec::Output + ?Sized>(&self, dest: &mut T) {
dest.write(self.bytes())
lexnv marked this conversation as resolved.
Show resolved Hide resolved
}
}

impl<Address, Call, Signature, Extra> scale_encode::EncodeAsType
for UncheckedExtrinsic<Address, Call, Signature, Extra>
{
fn encode_as_type_to(
&self,
_type_id: u32,
_types: &scale_info::PortableRegistry,
out: &mut Vec<u8>,
) -> Result<(), scale_encode::Error> {
out.extend_from_slice(self.bytes());
lexnv marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}
}

pub struct UncheckedExtrinsicDecodeAsTypeVisitor<Address, Call, Signature, Extra>(
PhantomData<(Address, Call, Signature, Extra)>,
);

impl<Address, Call, Signature, Extra> Visitor
for UncheckedExtrinsicDecodeAsTypeVisitor<Address, Call, Signature, Extra>
{
type Value<'scale, 'info> = UncheckedExtrinsic<Address, Call, Signature, Extra>;
type Error = scale_decode::Error;

fn unchecked_decode_as_type<'scale, 'info>(
self,
input: &mut &'scale [u8],
_type_id: scale_decode::visitor::TypeId,
_types: &'info scale_info::PortableRegistry,
) -> DecodeAsTypeResult<Self, Result<Self::Value<'scale, 'info>, Self::Error>> {
use scale_decode::{visitor::DecodeError, Error};
let decoded = UncheckedExtrinsic::decode(input)
.map_err(|e| Error::new(DecodeError::CodecError(e).into()));
DecodeAsTypeResult::Decoded(decoded)
}
}
Copy link
Collaborator

@jsdw jsdw Jul 19, 2023

Choose a reason for hiding this comment

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

Aah, I was hoping that you could get rid of more of this by using Static<Encoded> instead! This visitor trait is quite verbose alas.

I wonder whether something like this would work at least, to lean on the inner impl better (while not making any assumptions about it):

impl<Address, Call, Signature, Extra> Visitor
    for UncheckedExtrinsicDecodeAsTypeVisitor<Address, Call, Signature, Extra>
{
    type Value<'scale, 'info> = UncheckedExtrinsic<Address, Call, Signature, Extra>;
    type Error = scale_decode::Error;

    fn unchecked_decode_as_type<'scale, 'info>(
        self,
        input: &mut &'scale [u8],
        type_id: scale_decode::visitor::TypeId,
        types: &'info scale_info::PortableRegistry,
    ) -> DecodeAsTypeResult<Self, Result<Self::Value<'scale, 'info>, Self::Error>> {
        DecodeAsTypeResult::Decoded(self.0.decode_as_type(input, type_id.0, types))
    }
}

Maybe this is a sign that we can do something better with the DecodeAsType macro to allow things to be decoded based on some inner type or something.


impl<Address, Call, Signature, Extra> IntoVisitor
for UncheckedExtrinsic<Address, Call, Signature, Extra>
{
type Visitor = UncheckedExtrinsicDecodeAsTypeVisitor<Address, Call, Signature, Extra>;

fn into_visitor() -> Self::Visitor {
UncheckedExtrinsicDecodeAsTypeVisitor(PhantomData)
}
}
55 changes: 55 additions & 0 deletions testing/integration-tests/src/full_client/runtime_api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// see LICENSE for license details.

use crate::{node_runtime, test_context};
use codec::Encode;
use subxt::utils::AccountId32;
use subxt_signer::sr25519::dev;

Expand Down Expand Up @@ -47,3 +48,57 @@ async fn account_nonce() -> Result<(), subxt::Error> {

Ok(())
}

#[tokio::test]
async fn unchecked_extrinsic_encoding() -> Result<(), subxt::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the test; great to check that this works :)

let ctx = test_context().await;
let api = ctx.client();

let alice = dev::alice();
let bob = dev::bob();
let bob_address = bob.public_key().to_address();

// Construct a tx from Alice to Bob.
let tx = node_runtime::tx().balances().transfer(bob_address, 10_000);

let signed_extrinsic = api
.tx()
.create_signed(&tx, &alice, Default::default())
.await
.unwrap();

let tx_bytes = signed_extrinsic.into_encoded();
let len = tx_bytes.len() as u32;

// Manually encode the runtime API call arguments to make a raw call.
let mut encoded = tx_bytes.clone();
encoded.extend(len.encode());

let expected_result: node_runtime::runtime_types::pallet_transaction_payment::types::FeeDetails<
::core::primitive::u128,
> = api
.runtime_api()
.at_latest()
.await?
.call_raw(
"TransactionPaymentApi_query_fee_details",
Some(encoded.as_ref()),
)
.await?;

// Use the generated API to confirm the result with the raw call.
let runtime_api_call = node_runtime::apis()
.transaction_payment_api()
.query_fee_details(tx_bytes.into(), len);

let result = api
.runtime_api()
.at_latest()
.await?
.call(runtime_api_call)
.await?;

assert_eq!(expected_result, result);

Ok(())
}