accounts/abi refactor by holiman · Pull Request #15731 · ethereum/go-ethereum

holiman · 2017-12-21T15:30:55Z

This PR changes the ABI-model, since ABI is an encoding of arguments, the unpack logic has been moved away from Method/ Event, and on to the new type Arguments (alias for []Argument).

This PR removes a lot of duplicated code and also makes it easier to unpack method inputs as well as outputs, and also simplifies the logic in the abi-package.

This PR also incorporates (and supersedes) #15452 , with some extra testcases taken from #15568 .

This is a large PR, but I believe the abi-package to be fairly well covered by testcases at this point.

Event.tupleUnpack doesn't handle correctly Indexed arguments, hence it can't unpack an event with indexed arguments.

+ The event slice unpacker doesn't correctly extract element from the slice. The indexed arguments are not ignored as they should be (the data offset should not include the indexed arguments). + The `Elem()` call in the slice unpack doesn't work. The Slice related tests fails because of that. + the check in the loop are suboptimal and have been extracted out of the loop. + extracted common code from event and method tupleUnpack

+ adding missing comments + small cleanups which won't significantly change function body. + unify Method receiver name

+ Reworked Method Unpack tests into more readable components + Added Method Unpack into slice test

holiman · 2017-12-21T15:32:11Z

cc @robert-zaremba @dshulyak @gballet @yondonfu

holiman · 2017-12-21T17:21:59Z

To the people on cc: I have done my best to ensure that the functionality of the preceding PR:s in this package has been kept intact, but if I've messed with anything, please yell.

robert-zaremba · 2017-12-22T10:39:57Z

Why we are redoing this changes instead of merging long hanging PR (around 2 months already) and then updating?

gballet · 2017-12-22T11:00:34Z

@robert-zaremba it's not "redone", if you look at it your PR has been integrated into this one.

holiman · 2017-12-22T11:22:58Z

instead of merging long hanging PR (around 2 months already) and then updating?

The reason is that we started merging a couple of PR:s in that section, which made your PR conflict. So we had to fix it up. Since I did not want to get stuck waiting for someone to fix it, I did it myself (at the same time that @gballet was also doing it), and then rebased my pr on top of your commits.

So I've spent quite some time to ensure that everyone gets credit (commits) and that all functionality is intact.

Now, in hindsight, I should've/could've the rebased version of you PR back into your PR... But as I noticed halfway through that @gballet was also rebasing that one, it seemed like quite a tangle.

robert-zaremba · 2017-12-22T14:11:27Z

@holiman
OK, Thanks for handling it. It's a pity that at the end this takes so much work.
Let me know if you need some help.

robert-zaremba

I had a quick overview of the code and left few comments.

robert-zaremba · 2017-12-22T14:18:35Z

Do we need to return a pointer? It's a small structure. How about returning (Method, bool)?

Well, that would work too... Don't know why that would be better really...

robert-zaremba · 2017-12-22T15:10:57Z

How about using testify?

6 lines above could be substituted with:

require.True require.Equal

Also this solves another issue: if the first if fails then the second one shouldn't be executed.

I haven't gotten used to testify yet, that's the reason :)
I'll keep it in mind for future reference

robert-zaremba · 2017-12-22T15:11:06Z

same as above.

robert-zaremba · 2017-12-22T15:12:48Z

you can use testify:

require.NoError

robert-zaremba · 2017-12-22T15:13:07Z

comment should be removed

robert-zaremba · 2017-12-22T15:15:06Z

you don't need this if. for will handle empty / nil slice

gballet

Minor nitpicks. Great cleanup!

gballet · 2017-12-22T15:33:05Z

Couldn't there be a lookup table for method.Id() <=> method ?

There could be, but I don't think this will be extensively used; I'll use it from the signer, but it's a one-off operation. So might as well do it lazily.

gballet · 2017-12-22T15:42:27Z

Some linters will complain if there is no consistency when implementing methods for a type. I advise that you call all methods arguments instead of a

holiman · 2017-12-22T18:08:26Z

Travis failures unrelated:
Go 1.8.x:

?   	github.com/ethereum/go-ethereum/dashboard	[no test files]
ok  	github.com/ethereum/go-ethereum/eth	4.733s	coverage: 25.0% of statements
--- FAIL: TestFastCriticalRestarts (0.00s)
    --- FAIL: TestFastCriticalRestarts/protocol_63_progress_true (4.70s)
    	downloader_test.go:1818: pivot block not locked in after critical section failure
FAIL
coverage: 79.3% of statements

Go 1.7.x:

ok  	github.com/ethereum/go-ethereum/node	0.902s	coverage: 43.5% of statements
# cover github.com/ethereum/go-ethereum/p2p
cover: internal error: block 17 overlaps block 19
	/home/travis/gopath/src/github.com/ethereum/go-ethereum/p2p/server.go:#8021,#8156 /home/travis/gopath/src/github.com/ethereum/go-ethereum/p2p/server.go:#8078,#8106
cover: internal error: block 23 overlaps block 25
	/home/travis/gopath/src/github.com/ethereum/go-ethereum/p2p/server.go:#8307,#8400 /home/travis/gopath/src/github.com/ethereum/go-ethereum/p2p/server.go:#8361,#8380
ok  	github.com/ethereum/go-ethereum/p2p	0.213s	coverage: 73.8% of statements

holiman · 2017-12-22T18:21:09Z

I pushed some fixes to the comments, thanks for reviewing!

holiman · 2017-12-22T18:27:30Z

Squashed for niceness

robert-zaremba added 6 commits December 21, 2017 15:14

accounts/abi: adding event unpacker tests

3511904

accounts/abi: fix event tupleUnpack

9becba5

Event.tupleUnpack doesn't handle correctly Indexed arguments, hence it can't unpack an event with indexed arguments.

accounts/abi: satisfy most of the linter warnings

95461e8

+ adding missing comments + small cleanups which won't significantly change function body. + unify Method receiver name

accounts/abi: add Method Unpack tests

1afca33

+ Reworked Method Unpack tests into more readable components + Added Method Unpack into slice test

accounts/abi: add unpack into array test

81d4caf

holiman mentioned this pull request Dec 22, 2017

abi.tupleUnpack() fails to match event (or method?) parameters with underscores #15710

Closed

holiman requested a review from gballet December 22, 2017 09:29

robert-zaremba reviewed Dec 22, 2017

View reviewed changes

gballet approved these changes Dec 22, 2017

View reviewed changes

holiman added 2 commits December 22, 2017 19:26

acounts/abi: refactor abi, generalize abi pack/unpack to Arguments

73d4a57

accounts/abi: merging of ethereum#15452 + lookup by id

c095c87

holiman force-pushed the revamp_abi branch from 49559db to c095c87 Compare December 22, 2017 18:27

holiman merged commit 9d187f0 into ethereum:master Dec 22, 2017

karalabe added this to the 1.8.0 milestone Dec 22, 2017

holiman mentioned this pull request Dec 22, 2017

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

Closed

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

Conversation

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

Uh oh!

Uh oh!

holiman commented Dec 21, 2017

Uh oh!

holiman commented Dec 21, 2017

Uh oh!

robert-zaremba commented Dec 22, 2017

Uh oh!

gballet commented Dec 22, 2017

Uh oh!

holiman commented Dec 22, 2017

Uh oh!

robert-zaremba commented Dec 22, 2017

Uh oh!

robert-zaremba left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gballet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holiman commented Dec 22, 2017

Uh oh!

holiman commented Dec 22, 2017

Uh oh!

holiman commented Dec 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holiman commented Dec 21, 2017 •

edited

Loading

robert-zaremba left a comment •

edited

Loading