-
Notifications
You must be signed in to change notification settings - Fork 402
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
Filter events before reply block #589
Conversation
Ready for review. Added a few tests to assert the "message" events were not returned from DispatchSubmessages, nor ever returned to the Reply call. Fixed/removed a few other tests that expected to find messages. |
Codecov Report
@@ Coverage Diff @@
## master #589 +/- ##
==========================================
+ Coverage 59.86% 59.92% +0.06%
==========================================
Files 45 45
Lines 5197 5200 +3
==========================================
+ Hits 3111 3116 +5
+ Misses 1862 1861 -1
+ Partials 224 223 -1
|
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.
👍 Looks good and works as expected 🌷
One note to reduce complexity in the filter
x/wasm/keeper/msg_dispatcher.go
Outdated
cap := 0 | ||
for _, evts := range events { | ||
cap += len(evts) | ||
} |
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 can reduce complexity of this method a lot by just accepting []sdk.Event
. On the caller side just do: filterEvents(append(em.Events(), events...))
. I would not expect thousands of events that require some optimization.
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, in Rust everytime you heap allocate to make vectors (slices), it is considered "expensive". I guess we do not micro-optimize so aggressively in Go.
I will simplify
Closes #587