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

encode: Encode nested tuples properly #31

Closed
wants to merge 2 commits into from
Closed

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented May 29, 2023

This PR ensures that nested tuples inside sequences can be encoded properly.

Prior to this encoding a Vec<(u8, u16)> would not work properly with dynamic values of

Value::unnamed_composite(
        vec![
            Value::u128(3u8.into()),
            Value::u128(1u16.into()),
        ]
    );

This is because find_sequence_candidate function did assume that the last type of the value is the "end-type" of the sequence.

Instead, if we deal with tuple/composite inside a sequence (sequence or array otherwise), then recurse back to encode_composite with proper parameters (unflattened).

Detected by: paritytech/subxt#982.

// @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from jsdw May 29, 2023 15:34
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a 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 but obviously not super familiar with the code

@jsdw
Copy link
Collaborator

jsdw commented May 30, 2023

I propose closing this in favour of #32; I had a look for a while and wanted to come up with an approach to simplify the method we use to encode things slightly since it was already quite complicated and had a bunch of other edge cases where it would fail to encode things properly :)

@lexnv
Copy link
Contributor Author

lexnv commented May 30, 2023

Yep, closing this in favor of #32 👍

@lexnv lexnv closed this May 30, 2023
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.

None yet

3 participants