-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow multiple transaction extension version in UncheckedExtrinsic type.
#7035
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
base: master
Are you sure you want to change the base?
Conversation
…ension-multiple-version
…ension-multiple-version
…ension-multiple-version
franciscoaguirre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
| } | ||
|
|
||
| #[test] | ||
| fn dispatch_of_invalid_extrinsic_fails() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining why this is invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I changed the inner comments to make it more clear. Also the previous inner comment was wrong.
Fixed: 0071ee2
| /// | ||
| /// This trait is part of [`VersTxExtLine`]. It is defined independently to allow implementation to | ||
| /// rely only on it without bounding the whole trait [`VersTxExtLine`]. | ||
| pub trait VersTxExtLineWeight<Call: Dispatchable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the Line in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is short for pipeline. I couldn't find good names for this. The main issue is that we often conflate a singular transaction extension, and a pipeline of transaction extension. They are both represented in a single trait TransactionExtension where the associated const IDENTIFIER is only meaningful for the singular transaction extension, and the associated function fn metadata() -> Vec<TransactionExtensionMetadata> has a type signatured designed for the pipeline, and the automatic implementation is wrong for the pipeline and must be override.
So for those new types I tried to explicitly make a difference by adding this Line in the name, which means it is only meaningful for a pipeline.
The version we give here is only a version for the pipeline, not an individual transaction extension.
I added the reason for the name in the doc: 0071ee2
substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs
Outdated
Show resolved
Hide resolved
carlosala
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good!
| signature_ty: ir.signature_ty, | ||
| extra_ty: ir.extra_ty, | ||
| signed_extensions: ir.extensions.into_iter().map(Into::into).collect(), | ||
| signed_extensions: ir.extensions_v0.into_iter().map(Into::into).collect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just passing by, checking this change was made, and we were not adding lots of wrong extensions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and for metadata v16, if a user still want to use a signed transaction instead of a general transaction, the version for the the extensions is 0.
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
| /// The transaction extensions version 0 attached to this `Extrinsic`. | ||
| // We could remove this associated type and let user retrieve it from | ||
| // `TransactionExtensionsVersions`. | ||
| type TransactionExtensionsV0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should probably merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I now removed TransactionExtensionsV0: a49294c
| type TransactionExtensionsV0; | ||
|
|
||
| /// All version of transaction extensions attached to this `Extrinsic`. | ||
| type TransactionExtensionsVersions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming could be improved, because these are the transaction extensions and not just their versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use TransactionExtensionPipelines I think
| // NOTE: We avoid adding `ExtensionOtherVersions` in the type parameter type info | ||
| // because they break tools like subxt that hard-coded the number of type | ||
| // parameters. Also in general type parameters are not useful, information related | ||
| // to transaction extension are available in the metadata like for v16 the fields | ||
| // `transaction_extensions_by_version` and `transaction_extensions`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is subxt using internally to build/verify the extrinsic? The dedicated extrinsic metadata or just this type somehow extracted from the metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading the type info, they replace some types in primitives to their own primitives written here: https://github.com/paritytech/subxt/blob/a2080bf1f7414e3e55429984b3d53f3828fe7897/core/src/utils/unchecked_extrinsic.rs#L20-L25
And the default substitutes are written here: https://github.com/paritytech/subxt/blob/a2080bf1f7414e3e55429984b3d53f3828fe7897/codegen/src/lib.rs#L428-L432
| /// The transaction extensions in the order they appear in the extrinsic for the version 0. | ||
| pub extensions_v0: Vec<TransactionExtensionMetadataIR<T>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should remove this and have the invariant that 0 always exists in extensions_by_version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I now removed TransactionExtensionsV0: a9942a8
| /// The transaction extensions in the order they appear in the extrinsic for the version 0. | ||
| pub extensions_v0: Vec<TransactionExtensionMetadataIR<T>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would remove this and always assume that extensions_by_version contains 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I now removed TransactionExtensionsV0: a9942a8
| // The type cannot be instantiated so this method is never called. | ||
| Err(TransactionValidityError::Invalid(InvalidTransaction::Custom(0))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then just use unimplemented!()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fixed here e071249
|
|
||
| /// A type implements [`DecodeWithVersion`] where inner decoding is implementing | ||
| /// [`codec::DecodeWithMemTracking`]. | ||
| pub trait DecodeWithVersionWithMemTracking: DecodeWithVersion {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah why do we need this?
| /// This defines multiple version of a transaction extensions pipeline. | ||
| /// | ||
| /// (type name is short for versioned (Vers) transaction (Tx) Extension (Ext) pipeline (Line)). | ||
| pub trait VersTxExtLine<Call: Dispatchable>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need 5 different traits? Can this not just be one trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For VersTxExtLineWeight, we could merge it in, I avoided because it required to change some bounds here and there that weren't bound before. But also it looks ok to force them as I don't think anybody use the genericity offered here. https://github.com/paritytech/polkadot-sdk/compare/gui-transaction-extension-multiple-version...gui-transaction-extension-multiple-version-remove-tx-weight?expand=1
For VersTxExtLineVersion, I don't think we can merge it in. It it required to derive Encode for many types such as Preamble but VersTxExtLine is generic over the call, and Preamble doesn't have a call.
Then the question is why VersTxExtLine is generic over the call? IIRC because TransactionExtension is generic over the call. Why is TransactionExtension generic over the call? actually I think for no reason. Usually the transaction extensions are defined generically over the runtime config, which defines the call. So TransactionExtension could be not generic and instead have an associated type name Call but this is a bit late. Or Maybe I miss something.
For DecodeWithVersion, same issue as VersTxExtLineVersion but for implementing Decode on Preamble.
(Note that maybe both issue could be fixed with having Preamble use a phantom data for the call. not sure).
For DecodeWithVersionWithMemTracking, for consistency and hopefully less errors, but also I agree we can merge into DecodeWithVersion.
| /// ``` | ||
| #[allow(private_interfaces)] | ||
| #[derive(PartialEq, Eq, Clone, Debug, TypeInfo)] | ||
| pub enum MultiVersion< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire type can just be replaced by a tuple of TxExtLineAtVers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace it with a tuple of something I guess but not TxExtLineAtVers. We need to implement Encode and Decode, so the resulting type is not a tuple of all possible pipelines but it is the variant for the version.
If we want to define this aggregation of versioned pipeline with a tuple I guess we need a new trait that decodes into a trait object?
trait PipelineDefinition {
fn decode() -> Result<Box<impl VersTxExtLine>, ()>;
fn metadata() -> Metadata;
}
Then we can define all the supported version with a tuple of PipelineDefinition.
If we want this we also need to move the type info into this PipelineDefinition because it would make the VersTxExtLine incompatible with trait object usage.
Maybe MultiVersions is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we can make a tuple that defines an associated type and the associated type is just MultiVersion of each element of the tuple's associated type.
| // We could remove this associated type and let user retrieve it from | ||
| // `TransactionExtensionsVersions`. | ||
| type TransactionExtensionsV0; | ||
|
|
||
| /// All version of transaction extensions attached to this `Extrinsic`. | ||
| type TransactionExtensionsVersions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably also not worth the split here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I now removed TransactionExtensionsV0: a49294c
|
I currently renamed into:
|
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
This PR enhance
UncheckedExtrinsictype with a new optional generic:ExtensionOtherVersion.This generic defaults to
InvalidVersionmeaning there is not other version than the regular version 0. This is the same behavior as before this PR.New feature
To use this new feature, you can use the new types
TxExtLineAtVersandMultiVersionto define a transaction extension with multiple version:Breaking change
The types
Preamble,CheckedExtrinsicandExtrinsicFormatalso have this new optional generic. Their type definition also have changed a bit, theGeneralvariant was 2 fields, the version and the extension, it is now only one field, the extension, and the version can be retrieve by callingextension.version()The type inference for those types may fail because of this PR, to update the code, write some partial type:
UncheckedExtrinsic<_, _, _, _>,Preamble<_, _, _>,ExtrinsicFormat<_, _> andCheckedExtrinsic<_, _, _>`.Alternative implementation
This PR breaks the types a bit, I think it is very minimal and fine, but if this is annoying we can still keep the old types and write new types such as
UncheckedExtrinsicV2,CheckedExtrinsicV2etc..