[events API 1/2]: Add new send and receive streaming RPCs #836
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
ffranr
reviewed
Mar 19, 2024
Fixed! |
jharveyb
reviewed
Mar 21, 2024
| WHERE addrs.taproot_output_key = $1 | ||
| ) | ||
| SELECT | ||
| addr_events.id, creation_time, status, asset_proof_id, asset_id, |
There was a problem hiding this comment.
Why expose addr_events.id here? I didn't spot a use of it later on (may be in some follow-on PR?)
There was a problem hiding this comment.
It's part of the address.Event struct. It looks like it's currently only used in unit tests... Didn't want to also refactor them, as the PR was already growing quite a bit. But it looks like we could probably un-export the address.Event.ID field.
jharveyb
approved these changes
Mar 21, 2024
guggero
changed the title
Roasbeef
requested changes
Mar 22, 2024
To avoid pending streams attempting to remove the same subscribers on stopping the daemon, we hold the corresponding mutex during the whole time and not just for each iteration.
This commit ensures that we only notify subscribers after we've actually updated the database to complete an event.
With this commit we skip inspecting on-chain addresses for inbound asset transfers that have already fully completed. Previously, if there was an incomplete inbound transfer for a certain block height, we would go through all transactions in blocks after that height and notify for all inbound transfers found, even if they already were fully complete.
With this commit we change the AssetReceiveCompleteEvent into a more generic AddressReceive event and publish more granular events. To not change the behavior of the developer RPC, we filter the events and only forward the completed event. All other events will be notified on the new RPC that is going to be added a couple of commits later.
With this commit we create "deep" copy methods for items we will send out as events. We need these to make sure the objects we notify about aren't changed by the main thread while we're serializing them to the RPC notification interface.
Because the loop that sends out the notifications for each send state terminates before reaching the SendStateComplete state, we manually need to send out that event once we've sent out all proofs to the receivers. We also want to unify how and when we send out notifications, so we move the event creation to _after_ executing an event.
With this commit we add a new RPC that allows users to subscribe to address receive events in general or filtered by a specific address. This RPC notifies more granularly about the different states an inbound asset goes through, but hides some of the implementation details the previous (now development only) RPC showed (namely the proof courier backoff events).
In this commit we slightly refactor the event stream subscription handling in the send integration test to make sure we correctly time out in case we don't receive the required events.
We want to make sure the new receive and send events RPC notifies on the correct events during the send process. We don't need to assert every single case, so we just add the new assertion to our main send test cases.
|
Re-reviewed after rebases, still LGTM. |
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.
Replaces #773.
Fixes #781.
Fixes #806.
Fixes #739.
This PR adds new RPCs for being notified about inbound and outbound asset transfers.
The existing
SubscribeSendAssetEventNtfnsandSubscribeReceiveAssetEventNtfnswere mostly built for asserting the internal state during integration tests and weren't super helpful for app builders. That's why these old RPCs were moved to thetapdevrpcpackage and are now only used in the integration tests.The new RPCs notify on all send and receive states instead of only a few of them. The events also contain way more information that's useful to map them to the user action. And both RPCs allow the events to be filtered (either by address for receive events and by script key for send events).
This is part 1 of the "events API" saga. The follow-up PRs will base on this one and address: