Refactor environment event emitter #3120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pattyshack
requested review from
SaveTheRbtz,
zhangchiqing,
Tonix517 and
fxamacker
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit ac1c5c7 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Tonix517
reviewed
Aug 29, 2022
Tonix517
approved these changes
Aug 29, 2022
zhangchiqing
approved these changes
Aug 29, 2022
janezpodhostnik
approved these changes
Aug 30, 2022
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.
pattyshack
force-pushed
the
patrick/event-emitter
branch
from
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:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
eventEmitter.EmitEvent.