-
Notifications
You must be signed in to change notification settings - Fork 176
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
Refactor environment event emitter #3120
Conversation
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit ac1c5c7 The command Collapsed results for better readability
|
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.
LGTM. just a question out of curiosity
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. Just had some minor suggestion
1. move event.go & event_test.go from handler into environment 2. renamed (h *EventHandler) to (emitter *eventEmitter) 3. renamed (e *EventCollection) to (collection *EventCollection) 4. removed Child & Merge from EventCollection (dead code) 5. added EventEmitter interface and NoEventEmitter for scripts 6. merged TransactionEnv.EmitEvent's tracing / metering logic into eventEmitter.EmitEvent.
951df2a
to
6947fb0
Compare
bors merge |
The goal of this PR is to preserve the existing behavior. I'll let you
guys figure out what the desired behavior should be.
…On Tue, Aug 30, 2022 at 11:39 AM Deniz Mert Edincik < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In fvm/environment/event_emitter.go
<#3120 (comment)>:
> + // Cadence's runtime API. Note that the script variant will return
+ // OperationNotSupportedError.
+ EmitEvent(event cadence.Event) error
+
+ Events() []flow.Event
+ ServiceEvents() []flow.Event
+}
+
+var _ EventEmitter = NoEventEmitter{}
+
+// NoEventEmitter is usually used in the environment for script execution,
+// where no event should be emitted.
+type NoEventEmitter struct{}
+
+func (NoEventEmitter) EmitEvent(event cadence.Event) error {
+ return errors.NewOperationNotSupportedError("EmitEvent")
Any chance to make this non-error? This error is limiting a lot of good
stuff. ref: #2813 <#2813>
—
Reply to this email directly, view it on GitHub
<#3120 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABT6K3HHK7JW54I4KNCOTZDV3ZIOBANCNFSM577DAHIA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
eventEmitter.EmitEvent.