Skip to content

Fix decoding dynamic tuples and arrays with dynamic subtypes within tuples #440

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

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

jimmyneutront
Copy link
Contributor

This PR fixes #429 . When decoding a tuple that is not static or an array with a subtype that is not static, the second value in the tuple returned by decodeSignleType is a pointer to the next element, NOT the length of the consumed element. So when decoding such an element within a tuple, consumed should be set to consumedUnwrapped, NOT incremented by consumedUnwrapped.

@yaroslavyaroslav yaroslavyaroslav self-requested a review March 17, 2022 14:59
@yaroslavyaroslav yaroslavyaroslav merged commit 55c8c93 into web3swift-team:develop Mar 21, 2022
yaroslavyaroslav added a commit that referenced this pull request Mar 21, 2022
This reverts commit 55c8c93, reversing
changes made to cbaea38.
Copy link
Contributor

@mloit mloit left a comment

Choose a reason for hiding this comment

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

This review is post-merge, so I can't request changes. But the PR had to be backed out as it was resulting in not enough bits errors during decoding due to the fallthrough issue I outlined. Please correct and re-submit

if !subType.isStatic {
consumed = consumedUnwrapped
} else {
fallthrough
Copy link
Contributor

Choose a reason for hiding this comment

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

do not use fallthrough. in this case it will drop down to the next case, and it looks for !subTypes[i].isStatic which as it was an array will process as true, and do consumed = consumedUnwrapped instead of falling down to the default and doing consumed = consumed + consumedUnwrapped as intended.

As a general rule, don't ever use fallthrough unless absolutely necessary, and you fully understand all the potential side-effects

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.

ABIDecoder.decodeSignleType() [sic] fails to decode dynamic arrays in structs
3 participants