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

Simplify chain event Called API #4664

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Oct 30, 2020

A couple of simplifications to the chain event Called API:

  1. The match function returns three parameters:
  • match once
    if true: if there are multiple listeners registered to listen for a particular message, only callback the first one
  • matched
    true if there was a match
  • error

In practice, everywhere in the code that Called is used, match once is always false. I can't think of a reason why you would only want the first registered listener to match a method to get a callback and ignore the other listeners, so this PR removes the match once return value.

  1. messageEvents has a map map[triggerID][]MsgMatchFunc
    But there is only ever one element in the array. So just map directly to a MsgMatchFunc instead of an array:
    map[triggerID]MsgMatchFunc

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Needed to dig a bit deeper to convince myself that this is not breaking anything we could have forgotten about

LGTM

Comment on lines -508 to -510
if once {
break
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If anyone is reviewing this PR later:

https://github.com/filecoin-project/lotus/pull/2246/files#diff-2c1843994ac72e7b0c9c419e81d77df0c3573f9858f2474632411b8bec005904R505-R507 introduced matchOne which made this break conditional, but really it just shouldn't be there.

@magik6k magik6k merged commit 0f8dada into master Oct 30, 2020
@magik6k magik6k deleted the refactor/events-api-call-matcher branch October 30, 2020 23:48
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