Skip to content

accounts/abi: allow interface as the destination#18490

Merged
gballet merged 1 commit intoethereum:masterfrom
rjl493456442:interface-destination
Jan 23, 2019
Merged

accounts/abi: allow interface as the destination#18490
gballet merged 1 commit intoethereum:masterfrom
rjl493456442:interface-destination

Conversation

@rjl493456442
Copy link
Copy Markdown
Member

@rjl493456442 rjl493456442 commented Jan 22, 2019

In this PR, interface as the abi unpack destination is allowed.

It means user could pass a pointer to an empty interface (v) to the abi.Unpack(v interface{}, data []byte) method in order to unpack values returned from contract method calls without needing to know the type of the returned values beforehand(Copy from #18480 issue description).

Also this PR fix #18480

Copy link
Copy Markdown
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Those changes make sense. Have a look at the few nitpicks that I have.

Comment thread accounts/abi/unpack_test.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're chaning what is being tested here. See https://play.golang.org/p/jVgSuTeE1Fr

I suggest that you add a separate test with []byte{} and in this case check that you are indeed getting the error that occurs when dst.Elem().IsValid() == false

Comment thread accounts/abi/unpack_test.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's nice, and yet I would also like to see a test for what happens when the interface doesn't correspond do the types in def

@gballet gballet self-assigned this Jan 22, 2019
@i-norden
Copy link
Copy Markdown
Contributor

This change fixes issue #18480 :)

i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 22, 2019
FetchContractData method as it can no longer take a pointer to an empty
interface which was allowing to act completely generically in our Poller
methods... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 22, 2019
FetchContractData method as it can no longer take a pointer to an empty
interface which was allowing to act completely generically in our Poller
methods... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
@rjl493456442 rjl493456442 force-pushed the interface-destination branch from bc70a8a to 9c4a95e Compare January 23, 2019 04:44
@rjl493456442
Copy link
Copy Markdown
Member Author

@gballet I moved the test cases into TestMethodMultiReturn function since in that I can specify the unpack destination manually.

Since in the TestUnpack the unpack destination is created by reflect.New, that means for the slice type it will only create a nil slice without length information and could fail for the multi-return scenario.

Please take another look.

Copy link
Copy Markdown
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@gballet gballet added this to the 1.9.0 milestone Jan 23, 2019
@gballet gballet merged commit a50b471 into ethereum:master Jan 23, 2019
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 23, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
i-norden added a commit to vulcanize/vulcanizedb that referenced this pull request Jan 24, 2019
FetchContractData method in the context of our Poller as it can no longer take a pointer to an empty
interface which allowed for generic handling of any return type... fix provided in PR
[#18490](ethereum/go-ethereum#18490)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025
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.

accounts/abi.Unpack method no longer accepts empty interface as destination

3 participants