Support multiple extension versions in V5 extrinsics #4
Conversation
src/decoding/extrinsic_decoder.rs
Outdated
| write!( | ||
| f, | ||
| "This extrinsic version type ({version_ty}) is not supported; only Bare, Signed and General types are supported." | ||
| "The extrinsic type {extrinsic_type:02b} is not supported (given extrinsic version {version})." |
There was a problem hiding this comment.
You think folks will understand binary representation of this better than decimal?
I would probably add "0b{extrinsic_type:02b}" to be sure that it isn't interpreted as decimal
There was a problem hiding this comment.
Yeah; it's only ever represented as binary in the code, so I think most likely people won't understand it, but we will be able to diagnose the issue given the error hopefully! I added the 0b for clarity :)
src/decoding/extrinsic_decoder.rs
Outdated
| @@ -634,7 +659,7 @@ where | |||
| } | |||
|
|
|||
| Ok::<_, ExtrinsicDecodeError>(ExtrinsicExtensions { | |||
| transaction_extensions_version, | |||
| transaction_extensions_version: transaction_extensions_version.unwrap_or(0), | |||
There was a problem hiding this comment.
So this doesn't exist for ExtrinsicType::Bare AFAIU, cool
A bit hard to understand with optio, maybe that could be improved with separate types from Bare, Signed and General to avoid this transaction_extensions_version: Option but fine for now
There was a problem hiding this comment.
I pondered separate types but didn't offhand because we don't know the extrinsic version until later so it'd end up with an Option anyways or getting messier converting from one type to another :)
src/decoding/extrinsic_type_info.rs
Outdated
| // Dev note: If we see a V5 GEneral extrinsic, it will have a byte for the version of the transaction extensions. | ||
| // In V15 or below metadata, we don't know which set of transaction extensions we're being told about though. Is it | ||
| // the set that corresponds to the version byte we see or not? | ||
| write!(f, "The extrinsic contains an extension version (here, {extension_version}), but in metadata <=V15 it's not clear if we can decode this version or not.") |
There was a problem hiding this comment.
I think it would useful with some info such as Note: Metadata <=V15 only supports transaction extension version 0
I find but in metadata <=V15 it's not clear if we can decode this version or not a bit tricky to understand as user
There was a problem hiding this comment.
Metadata V15 doesn't necessarily only support version 0 though. The actual issue is that in V15 metadata we are told about the current latest version of signed extensions, but we don't know what that version is.
So right now, there is only one version (0) so it's all good. When the code allows for more than one version, we'll actually have to disallow any version from decoding via V15 and below metadata becasue we don't know if the version aligns with what metadata is telling us or not.
(that's why it's ahrd to explain; more tricky than just "0 works in <V16" :D)
src/decoding/extrinsic_type_info.rs
Outdated
| write!(f, "Could not find the extrinsic signature type.") | ||
| } | ||
| ExtrinsicInfoError::ExtrinsicExtensionVersionNotSupported { extension_version } => { | ||
| // Dev note: If we see a V5 GEneral extrinsic, it will have a byte for the version of the transaction extensions. |
There was a problem hiding this comment.
You could clarify in the comment that is possible that metadata < v16 can have the transaction extension version == 0 which we know how to decode but metadata >= v16 i.e, "V5 General extrinsic" we don't know how to decode yet...
However, you added a similar comment in err_if_bad_extension_version which was easier for me to understand.
There was a problem hiding this comment.
I had a go at tweaking the wording, but yeah it's a bit tricky to word correctly :D
Split the trait to get extrinsic info apart a little so that we can separately get transaction extensions given a version.
Only allow extension version 0 for <V16 metadatas at the moment. When it's possible for another version to exist, we'll need to tighten up on this and require V16 metadata to decode any verioned extensions properly.
Also remove support for V5 signed extrinsics, since they no longer exist as of the PR merging.
Testing:
substrate-nodeas well as an older one.polkadot-historic-decoding-exampleto check that it could decode V5 txs from the new Substrate node as well as general old/new blocks from polkadot RPCs