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
accounts/abi: fix event tupleUnpack errors + cosmetic cleaning #15452
Conversation
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
7d246c5
to
2dca219
Compare
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
2dca219
to
4dcada2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert your "cosmetic changes" as they clutter the patch. For the same reason, remove the dependency.
Also, explain exactly what tests panic when calling Elem()
. Last, is there an associated issue that describe how wrong the array size is? If not, it wouldn't hurt describing what goes wrong as well.
accounts/abi/method.go
Outdated
typ = value.Type() | ||
value = valueOf.Elem() | ||
typ = value.Type() | ||
isStruct = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make more sense to set isStruct
to false
and only set it to true if value.Kind()
is a reflect.Struct
. If that switch starts supporting more types, the programmer will have to set isStruct = false
on each entry (and might forget)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. The only additional type which we could support here is map, and this can be easily handled with enum type, which we will define once (performance, checks...)
accounts/abi/method.go
Outdated
) | ||
switch value.Kind() { | ||
case reflect.Struct: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason to initialize isStruct
to true
would be to not have an empty case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -85,7 +85,7 @@ func (abi ABI) Unpack(v interface{}, name string, output []byte) (err error) { | |||
} else if event, ok := abi.Events[name]; ok { | |||
unpack = event | |||
} else { | |||
return fmt.Errorf("abi: could not locate named method or event.") | |||
return fmt.Errorf("abi: could not locate named method or event") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that change really necessary? I can see the point (pun intended) of not displaying two dots if that error is subsequently printed as a %v
followed by another dot. However, it would be preferable not to add that dot over there, and not clutter the diff with unnecessary entries (more on that later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was reported by golinit
accounts/abi/event.go
Outdated
marshalledValue, err := toGoType((i+j)*32, input.Type, output) | ||
if err != nil { | ||
return err | ||
} | ||
reflectValue := reflect.ValueOf(marshalledValue) | ||
|
||
switch value.Kind() { | ||
case reflect.Struct: | ||
if isStruct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch made perfect sense here (especially since you are also merely moving it), as other types can be supported in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as explained below - we should check the type before unpacking the arguments in order to validate the array size once and for all. We can do the switch here, but since we already did it it's easier to reuse what we have learned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second switch will be optimized by the compiler, and we want to make adding new types easier. Revert back to it, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
accounts/abi/event.go
Outdated
return err | ||
} | ||
} | ||
} | ||
case reflect.Slice, reflect.Array: | ||
if value.Len() < i { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move the checks from line 68 right here, for a reduced diff and better understanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slice / array size should be done once, and at the beginning. Not every time in the loop. Also the error message was wrong, because it could fail before reaching the end (reporting the wrong expected size).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you could let the loop unroll with a i++
followed by a continue
and make the size check at the end. It would mean that you only scan the argument list once, and then that you would report other errors earlier than an error on the length of the array (which is unlikely to occur). Do let me know what you think of this, I might be missing something out here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you want to do the check at the end? We can do both things in fact: to use switch
before the unpacking to verify preconditions, and the use switch here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore if we will not change the size of slice before setting the value we will make a panic when we will go out of the slice / array.
BTW: the previous behaviour (few weeks ago) was a bit better in this case because it was specific to handle the slice/array use case (the tradeof was to have multiple copies of for loops for each destination value kind
case). Also it handled the case where the slice was nil
, and was appending the values.
"strings" | ||
"testing" | ||
|
||
"github.com/ethereum/go-ethereum/common" | ||
"github.com/ethereum/go-ethereum/crypto" | ||
"github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're pulling in two libraries that produce a big diff (and code) size, and you're the only one using it. I suggest that you create a different PR to start using those two libraries, so that the content of this PR is only about tupleUnpack
. If the community is interested in using testify, you will get upvotes and we'll then include it. This PR does too many different things and we can't include it whole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testify is a really convenient library when doing tests. I will extract it from this pull request and do another one then.
accounts/abi/method.go
Outdated
@@ -192,6 +194,8 @@ func (m Method) String() string { | |||
return fmt.Sprintf("function %v(%v) %sreturns(%v)", m.Name, strings.Join(inputs, ", "), constant, strings.Join(outputs, ", ")) | |||
} | |||
|
|||
// Id returns the canonical representation of the method's signature used by the | |||
// abi definition to identify method name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identify THE method name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks.
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
5a20f16
to
d6232b1
Compare
accounts/abi/event_test.go
Outdated
[3]byte{'u', 's', 'd'}}, | ||
jsonEventPledge, | ||
"Can unpack Pledge event into slice", | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the Slice
based tests were failing because of the Elem()
call in tupleUnpack
:
- Can unpack ERC20 Transfer event into slice
- Can unpack Pledge event into slice
@gballet Thanks for review. Please look at my answers above.
Please confirm everything is OK, and I can squash the commits before merging the PR. |
13d96af
to
e430273
Compare
@gballet I've squashed the review changes. Each commit has the description explaining the changes (eg the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're almost there. If you go back to switch and also add unit tests to cover your changes to methods.go, we should be good to go.
accounts/abi/reflect.go
Outdated
@@ -85,3 +85,10 @@ func set(dst, src reflect.Value, output Argument) error { | |||
} | |||
return nil | |||
} | |||
|
|||
func requireAssignable(dst, src reflect.Value) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to explain what that function does, it's obvious to everyone familiar with this PR but it won't be down the road.
accounts/abi/method.go
Outdated
@@ -24,7 +24,7 @@ import ( | |||
"github.com/ethereum/go-ethereum/crypto" | |||
) | |||
|
|||
// Callable method given a `Name` and whether the method is a constant. | |||
// Method represents method ABI entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some information being lost here, as well as an article missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment should start with the object name. The thing about Name and constant is covered in next lines of the comment.
Updating to Method represents a method ...
accounts/abi/event.go
Outdated
marshalledValue, err := toGoType((i+j)*32, input.Type, output) | ||
if err != nil { | ||
return err | ||
} | ||
reflectValue := reflect.ValueOf(marshalledValue) | ||
|
||
switch value.Kind() { | ||
case reflect.Struct: | ||
if isStruct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second switch will be optimized by the compiler, and we want to make adding new types easier. Revert back to it, please.
accounts/abi/event.go
Outdated
return err | ||
} | ||
} | ||
} | ||
case reflect.Slice, reflect.Array: | ||
if value.Len() < i { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you could let the loop unroll with a i++
followed by a continue
and make the size check at the end. It would mean that you only scan the argument list once, and then that you would report other errors earlier than an error on the length of the array (which is unlikely to occur). Do let me know what you think of this, I might be missing something out here.
accounts/abi/method.go
Outdated
v := value.Index(i) | ||
if v.Kind() != reflect.Ptr && v.Kind() != reflect.Interface { | ||
return fmt.Errorf("abi: cannot unmarshal %v in to %v", v.Type(), reflectValue.Type()) | ||
if err := requireAssignable(v, reflectValue); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're testing your changes in events.go, but not those in methods.go. The code is duplicated, and therefore in the future it could drift apart. So you need to cover that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot
accounts/abi/event_test.go
Outdated
expected interface{} | ||
jsonLog []byte | ||
name string | ||
}{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to ask in my previous review: are you going to test for errors as well? Like, the array boundary check that you made some changes for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add tests for that (my goal was to quickly fix the bugs in this package which I encountered when using it)
e430273
to
35801f9
Compare
@gballet @karalabe I've updated the repo according to the review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, we are using switch
again.
accounts/abi/event.go
Outdated
marshalledValue, err := toGoType((i+j)*32, input.Type, output) | ||
if err != nil { | ||
return err | ||
} | ||
reflectValue := reflect.ValueOf(marshalledValue) | ||
|
||
switch value.Kind() { | ||
case reflect.Struct: | ||
if isStruct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
accounts/abi/event.go
Outdated
return err | ||
} | ||
} | ||
} | ||
case reflect.Slice, reflect.Array: | ||
if value.Len() < i { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you want to do the check at the end? We can do both things in fact: to use switch
before the unpacking to verify preconditions, and the use switch here as well.
accounts/abi/method.go
Outdated
@@ -24,7 +24,7 @@ import ( | |||
"github.com/ethereum/go-ethereum/crypto" | |||
) | |||
|
|||
// Callable method given a `Name` and whether the method is a constant. | |||
// Method represents method ABI entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment should start with the object name. The thing about Name and constant is covered in next lines of the comment.
Updating to Method represents a method ...
accounts/abi/event_test.go
Outdated
expected interface{} | ||
jsonLog []byte | ||
name string | ||
}{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add tests for that (my goal was to quickly fix the bugs in this package which I encountered when using it)
accounts/abi/event.go
Outdated
return err | ||
} | ||
} | ||
} | ||
case reflect.Slice, reflect.Array: | ||
if value.Len() < i { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore if we will not change the size of slice before setting the value we will make a panic when we will go out of the slice / array.
BTW: the previous behaviour (few weeks ago) was a bit better in this case because it was specific to handle the slice/array use case (the tradeof was to have multiple copies of for loops for each destination value kind
case). Also it handled the case where the slice was nil
, and was appending the values.
Also, since |
Into the cleanning commit I've added one more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have this issue with the Travis build, but it doesn't seem to be directly related. Let's not block this PR any longer and merge it.
accounts/abi/method.go
Outdated
@@ -24,7 +24,7 @@ import ( | |||
"github.com/ethereum/go-ethereum/crypto" | |||
) | |||
|
|||
// Callable method given a `Name` and whether the method is a constant. | |||
// Method represents a method ABI entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment still drops information from the comment it replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. Look at the whole comment. I think it's more clear now. The Name
is part of it's public interface, and other part of the comment mentions about constant part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it definitely drops critical information regarding the ABI as the field "constant" is a core component of the current ABI. This will eventually be changed to "stateMutability" but for now it is needed, especially if the goal is to reduce breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the second line tells:
If the method is `Const` no transaction needs to be created for this
I don't want to insist here. I will update to:
Method represents a callable given a `Name` and whether the method is a constant.
accounts/abi/event.go
Outdated
j := 0 | ||
for i := 0; i < len(e.Inputs); i++ { | ||
input := e.Inputs[i] | ||
i, j := -1, 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is i -1 here? Is there any way to get a comment of some kind clarifying the logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we are incrementing at the beginning of the of the loop (if the argument is not Indexed) rather then at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to clarify things for me. So in the case of an event a(uint b, string indexed c, uint d), how does the -1 help read through b? Again, the logic could use some documentation is all I'm saying.
"indexed": false, "name": "currency", "type": "bytes3" | ||
}], | ||
"name": "Pledge", | ||
"type": "event" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test to take into account static array types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
accounts/abi/reflect.go
Outdated
@@ -85,3 +85,12 @@ func set(dst, src reflect.Value, output Argument) error { | |||
} | |||
return nil | |||
} | |||
|
|||
// requireAssignable verifies that `dest` and `src` types are compatible | |||
// and it's possible to safely assign `src` value to `dest`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not appear to be what it's actually doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, there is better way to document it. Howe about:
// requireAssignable verifies that dest
type is assignable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to:
// requireAssignable assures that `dest` is a pointer and it's not an interface.
accounts/abi/method.go
Outdated
for i := 0; i < len(method.Outputs); i++ { | ||
toUnpack := method.Outputs[i] | ||
if toUnpack.Type.T == ArrayTy { | ||
for i, o := range m.Outputs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please use a more descriptive name than o
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is used very often, and doesn't have a big context. But If you want I can change to something else.
accounts/abi/event.go
Outdated
continue | ||
} else if input.Type.T == ArrayTy { | ||
// need to move this up because they read sequentially | ||
j += input.Type.Size | ||
} | ||
i++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see the logic now, slightly...but this definitely needs documentation because currently it's a bit confusing as to why it's set up this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will add some comments.
@dshulyak any issues? |
no issues, i will make separate pr with that bugfix or include it into this one #15618 |
Great! I'm happy that we are finally moving forward. So
Or please advice if you want to do differently? |
293f550
to
b1eaf2b
Compare
@robert-zaremba i would prefer to merge both PRs separately (my change is this one #15618), doesn't matter for me in which order. |
b1eaf2b
to
a938796
Compare
@dshulyak OK. Removed. |
Ping. Any update here? |
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
Did you just rebase this ? |
yes I did, there seems to be a merge error though |
Let me know if you need some help with rebase or squashe. I didn't squash last commits because they were after the approved review. |
that should be fine now, if tests pass then we're good to merge |
It seams that there is an unrelated dependency build (installation) problem for GETH_ARCH=386, MSYS2_ARCH=i686, MSYS2_BITS=32, MSYSTEM=MINGW32 |
Closing due to #15731 |
The fix commits contain fixes for two bugs in event unpacker:
.Elem
method) - which caused panic in tests.The tests commit contains tests for all cases. For convenience I've added new dependency for doing assertions in tests (github.com/stretchr/testify).
Last commit contains cosmetic cleanup.