Skip to content
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

abi: adding the method EventById and its test #19359

Merged
merged 6 commits into from
Jun 24, 2019
Merged

abi: adding the method EventById and its test #19359

merged 6 commits into from
Jun 24, 2019

Conversation

salanfe
Copy link
Contributor

@salanfe salanfe commented Apr 1, 2019

adding the method EventById to the ABI, that allows to retrieve the event given a topic hash. Use case is similar to the method MethodById: given a transactoionReceipt with a log slice and the contract ABI, EventById returns the corresponding event "object" matching the log's topic field.

@salanfe salanfe requested a review from gballet as a code owner April 1, 2019 07:17
Copy link
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.

See inline comments, it would be better to return a (possibly empty) slice rather than an error


// EventById looks up a event by the topic hash
// returns nil if none found
func (abi *ABI) EventById(topic common.Hash) (*Event, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I see two issues with this approach:

  • The only kind of error that can arise is that there are no event with this topic, which is not an error a priori. It's only an error if the calling code considers no event to be an error, but we shouldn't be making this assumption at this stage.
  • If there are more than one event with a given topic, you will only be returning one.

As a result, I recommend returning a slice of pointers, i.e. :

func (abi *ABI) EventById(topic common.Hash) []*Event {

Copy link
Contributor Author

@salanfe salanfe May 15, 2019

Choose a reason for hiding this comment

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

thx you for the review. First point makes complete sense to me.

However, on the second point, I think we can have at most one event per topic. The topic is the hash of the event name and arg types. Example from the test file, the topic is:

crypto.Keccak256Hash([]byte("received(address,uint256,bytes)"))

therefore shouldn't be the mapping topic -> event unique ? I can't see how we could get multiple Event struct for a single topic (but having a collision).

{"indexed":false,"name":"amount","type":"uint256"},
{"indexed":false,"name":"memo","type":"bytes"}
]
}]`
Copy link
Member

Choose a reason for hiding this comment

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

so yeah please test with 0 event, 1 event and more than one event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I said in my previous comment, a topic has a single event and an event has a single topic (hash is unique).

My test already includes a case for

  • event can be found
  • event cannot be found

@gballet gballet self-assigned this May 15, 2019
@gballet gballet added this to the 1.9.0 milestone May 15, 2019
JFO and others added 2 commits May 22, 2019 17:45
Copy link
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.

@salanfe good point, so in that case let me backtrack and let's keep the error field - I don't want to have a nil returned without some sort of warning.

There was a PR from @tranvictor who did the same thing, I have merged both in this one, and you are both attributed as an author of course. Thanks again for your work!

@gballet gballet changed the title adding the method EventById and its test abi: adding the method EventById and its test Jun 24, 2019
@gballet gballet merged commit e4a1488 into ethereum:master Jun 24, 2019
@salanfe
Copy link
Contributor Author

salanfe commented Jul 5, 2019

👍 great thx

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 2, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 3, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 4, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 6, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 7, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 7, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 7, 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.

2 participants