Move events and subscriptions out of RawExecutionOutcome.
#3309
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
deuszx
reviewed
Feb 12, 2025
| @@ -1156,6 +1156,34 @@ impl<'a> Deserialize<'a> for Blob { | |||
| } | |||
| } | |||
|
|
|||
| /// An event recorded in an executed block. | |||
| #[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize, SimpleObject)] | |||
| pub struct EventRecord { | |||
There was a problem hiding this comment.
Right… the original reason (from the original PR) was that we already had an Event type with a different meaning. But now I can rename it…
deuszx
reviewed
Feb 12, 2025
Comment on lines
+46
to
+49
| /// Subscribe chains to channels. | ||
| pub subscribe: Vec<(ChannelFullName, ChainId)>, | ||
| /// Unsubscribe chains from channels. | ||
| pub unsubscribe: Vec<(ChannelFullName, ChainId)>, |
There was a problem hiding this comment.
What happens if there are multiple actions to subscribe/unsubscribe for the same channel<>chainId? Will we try to "cancel them out" before handling?
There was a problem hiding this comment.
No, we just handle all of them.
These will go away anyway when we finally have event streams and remove pub-sub channels.
There was a problem hiding this comment.
It's wasteful to handle if they will cancel out each other... If you say we'll remove them then it doesn't matter though.
deuszx
approved these changes
Feb 12, 2025
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.
Motivation
The fact that events are in
RawExecutionOutcomemakes #365 more complicated. They also conceptually don't need to be in the structure whose purpose is mainly to associate messages with the correct application ID and authenticated signer.Proposal
Remove
eventsfromRawExecutionOutcome. Also removesubscribeandunsubscribe; the same argument applies to them.Test Plan
Refactoring; CI should catch regressions.
Release Plan
Links