-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix compact encoding on structs #32
fix compact encoding on structs #32
Conversation
scale-decode/src/visitor/decode.rs
Outdated
// guard against invalid composite types: only `U8`, `U16`, `U32`, `U64` and `U128` can be compact encoded | ||
if is_compact | ||
&& !matches!( | ||
ty, | ||
TypeDefPrimitive::U8 | ||
| TypeDefPrimitive::U16 | ||
| TypeDefPrimitive::U32 | ||
| TypeDefPrimitive::U64 | ||
| TypeDefPrimitive::U128 | ||
) | ||
{ | ||
return Err(DecodeError::CannotDecodeCompactIntoType(ty.clone().into()).into()); | ||
} |
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.
Personally for this and the matches!
above, I'd be tempted to do something like define this here:
macro_rules err_if_compact! {
() => {
if is_compact {
return Err(DecodeError::CannotDecodeCompactIntoType(ty.clone().into()).into());
}
}
}
And then in each branch wehre it applies:
match ty {
// ...
TypeDefPrimitive::Bool => {
err_if_compact!();
// usual logic here
}
// ...
}
The reason for this is so that it's very clear that all of the branches that don't want/have any special compact logic are guarded right there, rather than having the check a little bit away from where it's protecting :)
(also it saves us a match but that's not a big deal)
The macro could be defined to take in the ty it'll clone and put in the error too, and then used both here and above perhaps.
/// not used in the course of decoding are still pointed to after decoding is complete. | ||
/// | ||
/// If is_compact=true, it is assumed the value is compact encoded (only works for some types). | ||
fn decode_as_type_maybe_compact( |
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.
let's add #[doc(hidden)]
to this, because I hope that it's only really needed for internal use, and it'd be nice if people only saw/used decode_as_type
:)
); | ||
} | ||
|
||
#[test] | ||
fn decode_nested_compact_structs() { |
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.
Cool! It's good to see this works :)
#[derive(CompactAs, Decode, DecodeAsType)] | ||
pub struct Perbill(pub u32); | ||
|
||
#[derive(Decode, DecodeAsType)] | ||
pub struct ValidatorPrefs { | ||
#[codec(compact)] | ||
pub commission: Perbill, | ||
pub blocked: ::core::primitive::bool, | ||
} |
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.
Nice to see this tested too!
A couple of tiny things but overall this looks great; nice work! |
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.
Nice!
In subxt we had issues decoding structs that have compact encoded fields, which are other struct (see Issue 1034).
This PR changes the way compact decoding works by passing an
is_compact
flag around recursively while decoding data (entry point:decode_with_visitor_maybe_compact(..)
indecode.rs
).This code snippet (see Issue 1034) did not work previously, but now runs without issue: