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

Add a compact flag to Field #42

Closed
wants to merge 17 commits into from
Closed

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Dec 17, 2020

Closes #8
Builds on #41

Adds a compact boolean to Field to signal that the type should be encoded/decoded as a parity_scale_codec::Compact type.

This is WIP and does not yet change the derive macro to use the new flag.

The code here takes the first approach outlined in #8, using a flag. The reasoning is to try the simplest thing that works and see where it takes us. The preferred approach outlined by @Robbepop, leveraging the existing machinery to derive Compact<T> is elegant but given the scary expansion of #[codec(compact)] (see comment below) I'm not sure if it is actually workable (tbh I haven't tried yet).

ascjones and others added 12 commits December 7, 2020 11:49
By default, parity-scale-codec does not provide Encode/Decode impls for an owned String.
This is only provided under the "full" feature which is not used by the substrate runtime,
because it should not be used for consensus critical code. So in order for the CompactForm
to be integrated into the substrate runtime, or wherever the "full" feature cannot be used,
then we must parameterize the `String` type so that it can be both an `&'static str` on the
runtime side where it is encoded, and a `String` in client/consuming code where it is decoded.
Add a `compact` member to `Field` to indicate it is `scale_codec::Compact`
src/build.rs Outdated Show resolved Hide resolved
@Robbepop
Copy link
Contributor

Robbepop commented Dec 17, 2020

Have you considered making use of the actual type metadata in order to infer types that are wrapped in a scale_codec::Compact type? I mean this library literally already supports this use case so the adding of the boolean flag is redundant if we are strict.

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 17, 2020

@Robbepop you mean that a field #[scale(compact)] a: i32, would become a: parity_scale_compact::Compact<i32>? Yes, I did try that route first (as you outlined that as your preferred way in #8) but when I saw what #[scale(compact)] actually expands to I thought I'd try this route instead to see where it leads.

For example:

#[derive(Encode)]
pub enum MultiAddress<AccountId, AccountIndex> {
    /// It's an account ID (pubkey).
    Id(AccountId),
    /// It's an account index.
    Index(#[codec(compact)] AccountIndex),
    /// It's some arbitrary raw bytes.
    Raw(Vec<u8>),
    /// It's a 32 byte representation.
    Address32([u8; 32]),
    /// Its a 20 byte representation.
    Address20([u8; 20]),
}

…expands to this:

impl<AccountId, AccountIndex> _parity_scale_codec::Encode for MultiAddress<AccountId, AccountIndex>
where
    AccountId: _parity_scale_codec::Encode,
    AccountId: _parity_scale_codec::Encode,
    AccountIndex: _parity_scale_codec::HasCompact,
{
    fn encode_to<__CodecOutputEdqy: _parity_scale_codec::Output>(
        &self,
        __codec_dest_edqy: &mut __CodecOutputEdqy,
    ) {
        match *self {
            MultiAddress::Id(ref aa) => {
                __codec_dest_edqy.push_byte(0usize as u8);
                __codec_dest_edqy.push(aa);
            }
            MultiAddress::Index(ref aa) => {
                __codec_dest_edqy.push_byte(1usize as u8);
                {
                    __codec_dest_edqy.push (&<<AccountIndex as _parity_scale_codec::HasCompact>::Type as _parity_scale_codec::EncodeAsRef< '_ , AccountIndex >>::RefType::from(aa));
                }
            }
            MultiAddress::Raw(ref aa) => {
                __codec_dest_edqy.push_byte(2usize as u8);
                __codec_dest_edqy.push(aa);
            }
            MultiAddress::Address32(ref aa) => {
                __codec_dest_edqy.push_byte(3usize as u8);
                __codec_dest_edqy.push(aa);
            }
            MultiAddress::Address20(ref aa) => {
                __codec_dest_edqy.push_byte(4usize as u8);
                __codec_dest_edqy.push(aa);
            }
            _ => (),
        }
    }
}

Comment on lines +89 to +92
/// This field should be encode/decoded as a
/// [`Compact`](parity_scale_codec::Compact) field
#[cfg_attr(feature = "serde", serde(skip_serializing_if = "is_false", default))]
compact: bool,
Copy link
Contributor

@Robbepop Robbepop Jan 5, 2021

Choose a reason for hiding this comment

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

I think an even more compact representation for the exact same use case would be to use yet another enum variant in order to represent compacted types. The idea is to add another variant Compact(TypeDefCompact<T>) to the TypeDef enum: https://docs.rs/scale-info/0.4.1/scale_info/enum.TypeDef.html
The TypeDefCompact type internally could look like this:

pub struct TypeDefCompact<T>
where
    T: Form = MetaForm
{
    #[serde(rename = "type")]
    type_param: T::Type,
}

Then instead of requiring each type to have this bool field we'd simply have a compact variant only for those type definitions that are actually compactly encoded. Since this is kind of a special case this design would suite much better. Also it would better reflect the encoding structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance I think this solution is more elegant than having the field, while still making it a first class citizen (unlike just using a type e.g. Compact<T>.

@dvdplm
Copy link
Contributor Author

dvdplm commented Jan 19, 2021

Closing in favour of #58

@dvdplm dvdplm closed this Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Compact types
3 participants