accounts/abi: Index inputs are not present in output by dshulyak · Pull Request #15568 · ethereum/go-ethereum

dshulyak · 2017-11-28T11:38:10Z

GitCop · 2017-11-28T11:38:11Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 1095a29e6a7f2b46531890c3503578694d5dc521
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

holiman · 2017-11-29T10:26:26Z

@VoR0220 Would you mind taking a look at this? I find that code a bit difficult to follow, and don't understand why the fix works, and if there are any other sideeffects of it.

dshulyak · 2017-11-29T10:47:24Z

Because of this
https://github.com/ethereum/go-ethereum/pull/15568/files#diff-7af5eed2bdda4c04119f5dba36a637b4R80

if an event has 3 fields, and 2 of them are indexed - following function will expect that data for 3rd field will be in 2*32:96 range of the output. But the encoded output doesn't contain fields that are indexed and following error is returned:

2017/11/28 13:34:28 abi: cannot marshal in to go type: length insufficient 32 require 96

maybe it would be better to have all the fields in log.Data, but then this condition won't be needed https://github.com/ethereum/go-ethereum/pull/15568/files#diff-7af5eed2bdda4c04119f5dba36a637b4R71

VoR0220 · 2017-11-29T21:36:18Z

Yup, this is my bad. While I took into account that indexed types should be skipped, it doesn't appear I did so in terms of how long the bytecode should be. This appears to correct that, however the tests are a little confusing to read.

VoR0220 · 2017-11-29T21:40:11Z

 		input := e.Inputs[i]
 		if input.Indexed {
+			// indexed inputs are not available in log output
+			j--


This seems fairly elegant. Works for me.

Second thought, I'm not sure this plays nicely with a static array element insofar as indexing is concerned.

thanks for review, i added required test, but i think that i spotted another issue with array types. please see my comment below

VoR0220

Needs to take into account static array indexing and how that interacts with the changes prescribed.

VoR0220 · 2017-11-29T21:42:51Z

+		{
+			definition: `[{"anonymous":false,"inputs":[{"indexed":false,"name":"value1","type":"uint256"},{"indexed":true,"name":"value2","type":"uint256"}],"name":"transfer","type":"event"}]`,
+			want:       testResult{Value1: big.NewInt(100)},
+		},


A test that should be added for the sake of sanity would be in the case of a indexed static array type. Say for example I'm passing in an indexed uint[2], and a non indexed string, this should take into account the length of the static array and ignore it...perhaps this should be handled by another variable in the events.go file above to best handle this.

GitCop · 2017-11-30T09:59:45Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 362fb00fd6e03810242b032c7d37a42ad4c2054b
Your commit message body contains a line that is longer than 80 characters

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

dshulyak · 2017-11-30T10:05:19Z

In current codebase array fields are counted before unpacking output.
So, if we have array with 2 fields: toGoType function will try to unpack
values from 64 till 64 + 2*32 which is not correct in my opinion.

Counting will be done after processing array, so in case array is the first
field, and it has 2 elements: we will read it from 0 till 2*32, and next element will start at 64.

I was using this contract for test:

contract Test { event Echo(uint8[2] message, address from); function emit() { uint8[2] memory r = [1,2]; Echo(r, msg.sender); } }

Error message:
2017/11/30 12:37:46 abi: cannot marshal in to go array: offset 96 would go over slice boundary (len=128)

Why would that be incorrect? Sounds right at the beginning.

how is that correct? example that i provided actually fails on current master

let's say we will try to unpack an event with ([7,8], 9) all simple integers,
it will be encoded as

0000000000000000000000000000000000000000000000000000000000000007 0000000000000000000000000000000000000000000000000000000000000008 0000000000000000000000000000000000000000000000000000000000000009

without this change we will try to read array from 64 till 128, and integer from 96 till 128, and actually will fail with this error 2017/12/07 16:04:29 abi: cannot marshal in to go array: offset 96 would go over slice boundary (len=128)

with this change first read will be done from 0 till 64 (array), and for integer from 64 till 96

Ah. Okay. Yea I'm sorry for the wait, I've reviewed the code, it looks fairly good, but I would just add a comment about the j += input.Type.Size - 1...specifically why you're subtracting the 1. Just leave a comment on that and I'll approve this...for whatever that's worth.

@VoR0220 added a comment

dshulyak · 2017-12-04T08:07:01Z

@VoR0220 would you mind taking a look again?

In current codebase array fields are counted before unpacking output. So, if we have array with 2 fields: toGoType function will try to unpack values from 64 till 64 + 2*32 which is not correct in my opinion. I shifted counting to be done after processing array, so in case array is the first field, and it has 2 elements: we will read it from 0 till 2*32. Additionally array doesnt require length prefix, so field that follows array should be read from 64 byte. This is why i made additional substitution in this change.

dshulyak · 2017-12-07T11:51:33Z

@gballet perhaps you can help with review?

GitCop · 2017-12-08T10:39:56Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 9b8e32023bf26405933ca3640d38310dc05bbccb
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

dshulyak · 2017-12-08T10:45:55Z

@VoR0220 @gballet I improved tests for fixes, i believe they do better job at explaining the problem now

VoR0220

This is GTM from my end @gballet @holiman

dshulyak changed the title ~~Index inputs are not present in output~~ accounts/abi: Index inputs are not present in output Nov 28, 2017

accounts/abi: Indexed inputs are not present in output

e3bca64

dshulyak force-pushed the indexed_inputs branch from 1095a29 to e3bca64 Compare November 28, 2017 11:47

VoR0220 reviewed Nov 29, 2017

View reviewed changes

VoR0220 suggested changes Nov 29, 2017

View reviewed changes

dshulyak force-pushed the indexed_inputs branch from 362fb00 to 3fd0350 Compare November 30, 2017 10:02

dshulyak commented Nov 30, 2017

View reviewed changes

dshulyak force-pushed the indexed_inputs branch from 3fd0350 to b11eecb Compare December 1, 2017 09:10

gballet self-requested a review December 1, 2017 13:29

dshulyak force-pushed the indexed_inputs branch from b11eecb to 4fc2aa3 Compare December 6, 2017 13:38

dshulyak force-pushed the indexed_inputs branch from 9b8e320 to 6a9ed4c Compare December 8, 2017 10:41

dshulyak force-pushed the indexed_inputs branch 2 times, most recently from b959674 to 10c4590 Compare December 8, 2017 11:39

accounts/abi: Improve tests for provided fixes

2191706

dshulyak force-pushed the indexed_inputs branch from 10c4590 to 2191706 Compare December 11, 2017 14:16

VoR0220 approved these changes Dec 11, 2017

View reviewed changes

VoR0220 mentioned this pull request Dec 11, 2017

accounts/abi: fix event tupleUnpack errors + cosmetic cleaning #15452

Closed

holiman mentioned this pull request Dec 19, 2017

accounts/abi: add array length after parsing array #15618

Merged

dshulyak closed this Dec 19, 2017

holiman mentioned this pull request Dec 21, 2017

accounts/abi refactor #15731

Merged

Conversation

dshulyak commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GitCop commented Nov 28, 2017

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

dshulyak commented Nov 29, 2017

Uh oh!

VoR0220 commented Nov 29, 2017

Uh oh!

VoR0220 Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

VoR0220 Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

dshulyak Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

VoR0220 left a comment

Choose a reason for hiding this comment

Uh oh!

VoR0220 Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

GitCop commented Nov 30, 2017

Uh oh!

dshulyak Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dshulyak Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VoR0220 Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

dshulyak Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VoR0220 Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

dshulyak Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

dshulyak commented Dec 4, 2017

Uh oh!

dshulyak commented Dec 7, 2017

Uh oh!

GitCop commented Dec 8, 2017

Uh oh!

dshulyak commented Dec 8, 2017

Uh oh!

VoR0220 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dshulyak commented Nov 28, 2017 •

edited

Loading

dshulyak Nov 30, 2017 •

edited

Loading

dshulyak Nov 30, 2017 •

edited

Loading

dshulyak Dec 7, 2017 •

edited

Loading