(WIP) Adding confirmation streaming in Event Stream #144
Conversation
This file contains hidden or 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
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
|
EnriqueL8
reviewed
Jul 9, 2025
There was a problem hiding this comment.
A few worries about API spelling of those functions, they seem to similar but do different things - @peterbroadhurst will have some opinions here
Sad that we just released 1.4.0 and this will break consumers
Thanks for fixing the typos!
Comment on lines
48
to
+49
| StartConfirmedBlockListener(ctx context.Context, id *fftypes.UUID, fromBlock string, checkpoint *ffcapi.BlockListenerCheckpoint, eventStream chan<- *ffcapi.ListenerEvent) error | ||
| StartBlockConfirmationsListener(ctx context.Context, id *fftypes.UUID, fromBlock string, checkpoint *ffcapi.BlockListenerCheckpoint, eventStream chan<- *apitypes.ConfirmationsForListenerEvent) error |
There was a problem hiding this comment.
Worth a comment to explain the difference, I'm still torn and think the confirmations context should be included in the ListenerEvent?
Comment on lines
+382
to
+384
| // ListenerEvent is an event+checkpoint for a particular listener, and is the object delivered over the event stream channel when | ||
| // a new event is detected for delivery to the confirmation manager. | ||
| type ConfirmationsForListenerEvent struct { |
There was a problem hiding this comment.
this comment needs updating
Signed-off-by: Enrique Lacal <[email protected]>
|
Okay there is a bug here I'm fixing It's dispatching the same block |
…xception Signed-off-by: Enrique Lacal <[email protected]>
Signed-off-by: Enrique Lacal <[email protected]>
|
Okay I think I managed to fix the above, was just returning instead of breaking when we don't dispatch anything else so wasn't clearing from the blocksSinceCheckpoint the ones dispatched |
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.
Adding support to stream confirmation events (the confirmation notification with a block event) for block listener only.
Unlike the built-in confirmation manager that's already used by the existing transaction handler, the confirmation events returned by the event stream ALWAYS return a full set of confirmations so that the consumer doesn't need to rebuild the full picture by accumulating them. This increases the size of the confirmation event, but hugely reduce the complexity of the client logic.