V14 Decoding #45
V14 Decoding #45
Conversation
… Don't need a wrapper. Implement some decoding
| @@ -0,0 +1,18 @@ | |||
| [package] | |||
| name = "core_v14" | |||
There was a problem hiding this comment.
Might not be the best name if this crate could one day be used on its own? desub-metadata-v14?
There was a problem hiding this comment.
Definitely; the plan is to merge it with a crap name, and then do a PR to rename soem crates.. probably promoting this in place of the <=V13 logic, and moving that logic to be under something with legacy in the name (we'll need to work out the naming though when we get to that!)
core_v14/src/substrate_type.rs
Outdated
| Tuple(TupleType), | ||
| Primitive(PrimitiveType), | ||
| Compact(CompactType), | ||
| BitSequence(BitSequenceType), |
There was a problem hiding this comment.
Isn't this one feature gated in scale-info?
There was a problem hiding this comment.
It is! Honestly I've no idea whether or not it's necessary in the decoding, but the <=V13 logic handled decoding to bitvec, so I did too. I did more recently run into some sporadic errors that meant I had to add a feature flag, though...
core_v14/src/decoder/decoder.rs
Outdated
| CannotFindCall(u8, u8), | ||
| #[error("Failed to decode extrinsic: {0}")] | ||
| CannotFindType(#[from] MetadataError), |
…as I believe it can vary per node
I second this. "Extrinsic" was chosen very carefully to reflect its "anything really, as long as it comes from the outside"-nature, so piling on "generic" on top of it doesn't add much, semantically. |
|
|
…lly unclear when one or the other is to be used
|
|
||
| impl<'a> AllExtrinsicBytes<'a> { | ||
| /// How many extrinsics are there? Note that this is simply the reported number of extrinsics, | ||
| /// and if the extrinsic bytes are malformed, it may not equal the actual number of extrinsics |
There was a problem hiding this comment.
Could you elaborate on "extrinsic bytes are malformed"?
There was a problem hiding this comment.
Sure! The length is calculated assuming that you've passing a scale encoded version of Vec<Vec<u8>> (so we have the vector length and then the bytes for each extrinsic also have a length prefixed).
But the actual iteration doesn't rely on that reported length (it can't, really), so it may be that we run out of bytes before we've actually decoded as many extrinsics as the length reported we'd have.
There was a problem hiding this comment.
(In general, I've tried to make sure that this iterator won't panic even if you give completely nonsense bytes, but that hasn't been thoroughly vetted!)
There was a problem hiding this comment.
so it may be that we run out of bytes before we've actually decoded as many extrinsics as the length reported we'd have.
Ahh, this puts it together for me, that clears up some confusion I was having when reading through the impl<'a> Iterator.
Thanks for the explanation
thae -> that typo Co-authored-by: Tarik Gul <[email protected]>
|
|
||
| ```rust | ||
| use hex; | ||
| use core_v14::{ Metadata, Decoder }; |
There was a problem hiding this comment.
If the name of the crate changes, just a reminder that this would update as well, based on the comment above?
There was a problem hiding this comment.
Yeahh, the name is almost certain to change, but at least it should break this example and make it obvious that we need to fix it (I should probably confirm that CI is checking the examples..) :)
| } | ||
|
|
||
| /// Get the decoded metadata version. At some point `RuntimeMetadataPrefixed` will end up | ||
| /// with a `.version()` method to return the version, and then this can be removed. |
There was a problem hiding this comment.
Let's make an issue for this so we don't forget
| let val = match ty { | ||
| TypeDefPrimitive::Bool => Primitive::Bool(bool::decode(data)?), | ||
| TypeDefPrimitive::Char => { | ||
| // [jsdw] TODO: There isn't a `char::decode`. Why? Is it wrong to use u32 or is there a more "proper" way? |
There was a problem hiding this comment.
https://docs.substrate.io/v3/advanced/scale-codec/
Strings are just encoded as an array of u8 so it is possible there is no notion of char. I'm guessing this is something that scale-info added but which is not present in scale-codec
There was a problem hiding this comment.
It seems that way; I want to handle the scale info type properly, but since scale-codec isn't implemented for char I'm not actually sure what the expectation is (so I'm just assuming that char will be a 32bit value like in Rust!)
There was a problem hiding this comment.
that make sense, i think that's the most sensible route
| _ty: &TypeDefBitSequence<PortableForm>, | ||
| _types: &PortableRegistry, | ||
| ) -> Result<BitSequence, DecodeTypeError> { | ||
| // [jsdw] TODO: might be worth checking the bit_store and bit_order types |
There was a problem hiding this comment.
i think the Lsb0/u8 is definition in polkadot-related runtimes, but i parity-scale codec impl Decode/Encode directly on O/T generics. Not sure any other project is using anything other than u8/lsb0, and it might significantly complicate bitsequence decoding if we were to make it generic?
There was a problem hiding this comment.
Yeah; I can see that fairly arbitrary ordering and storage format choices could be made which might be somewhat hard to handle! Offhand it looks like scale-codec just encodes it as the number of bits and then a slice of whatever the underlying storage type is. I think that means that whatever we assume we'll at least decode the correct number of bits, even if we mess up the ordering of them.
There was a problem hiding this comment.
(as a random side note: when encoding the bitvec type via serde, an additional piece of information is encoded (which may describe format details), It might be that the way scale-codec encodes/decodes it means that it must always be a u8 backed lsb0 ordered thing)
Co-authored-by: Andrew Plaza <[email protected]>
|
@insipx The clippy lints are txdecoder related I think; what's the best way to handle this? If you've been working on an update for it, should we merge the update into this PR, or should we merge this PR in full knowledge that it has these errors and then your work can be merged on top to fix them, orrr should we just exclude the txdecoder bin again in this PR so that the related errors go away (like how it was)? :) |
|
Ah sorry somehow I saw the clippy lints and forgot about that (i think in my head I confused clippy lints with the compile errors from before) Anyway we can just merge this then, I'll just fix the clippy lints in the txdecoder branch before merging that, since I have to change the core14 ref anyway |
This PR lays the foundation for decoding using V14 metadata.
Currently supported is decoding (v4) extrinsics (multiple or single) based on V14 metadata. Decoding other things to follow in subsequent work.