Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Fix a regression in decoding of dynamic structs! #2131

Merged
merged 1 commit into from Jun 26, 2019

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Jun 26, 2019

It seems I accidentally broke decoding of dynamic structs in calldata a while back. Probably when I factored out struct decoding; in combining the static and dynamic cases, I left out the startPosition argument in the call to decodeAbi, which is fine in the static case but is very wrong in the dynamic case. Oops.

(In the static case, you're always reading the member directly, not a pointer to it, so you don't need to know what base that pointer should be considered relative to, because it's not a pointer at all. In the dynamic case, you may be reading a relative pointer, so you need to know what it's relative to.)

As a check against this happening again, I added an additional (very simple) calldata decoding test to catch this.

Note that in order to make this work I ended up updating the Solidity version used in our debugger tests to 0.5.10. (It was on 0.5.4; dynamic structs in calldata weren't allowed yet in 0.5.4.)

@haltman-at haltman-at requested a review from gnidan June 26, 2019 00:51
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

hooray for regression test!

looks good - feel free to take or leave the suggestions

@haltman-at haltman-at merged commit 247e2ff into develop Jun 26, 2019
@haltman-at haltman-at deleted the fix-dynamic-struct-regression branch June 26, 2019 01:15
@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.16% when pulling 1700933 on fix-dynamic-struct-regression into ddc5216 on develop.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants