Add event-based API to Client
#69
Conversation
rustaceanrob
force-pushed
the
event-10-14
branch
9 times, most recently
from
3ed5c60 to
8b85319
Compare
|
Updated and ready for review. My notes:
|
|
Concept ACK. I'm still in the process of reviewing |
rustaceanrob
force-pushed
the
event-10-14
branch
from
8b85319 to
164e4ee
Compare
| callbacks = [] | ||
| events = [] |
There was a problem hiding this comment.
Do these need to be feature flags or is it fine to have update and next_event available for Client out of the box?
There was a problem hiding this comment.
My intention was that developers should be able to choose the API surface as they please. I don't think there would be any downside to mixing the two calls, but it would be strange to do so.
rustaceanrob
force-pushed
the
event-10-14
branch
from
164e4ee to
09ad97d
Compare
| /// ## Note | ||
| /// | ||
| /// No action is required from the developer, as these events are already | ||
| /// handled within the [`Client`]. This event is to inform the user of | ||
| /// such an event. | ||
| BlocksDisconnected(Vec<DisconnectedHeader>), |
There was a problem hiding this comment.
To be clear, is the idea for this to be an event the caller will act on if we had something like bitcoindevkit/bdk#1655 ?
There was a problem hiding this comment.
It's actually more cosmetic. In the event a user is expecting their transaction has confirmed when it actually got reorged, I was thinking the developer should let them know. But we still maintain LocalChain event with the Event architecture, so it handled internally and conveyed by the FullScanResult.
My intention with that PR is more for ldk-node, where one Block or BlocksDisconnected event from kyoto might be handled by many structures, one being the Wallet. In that case, I am thinking they can just use kyoto directly, and that PR would enable some good synergy between Wallet and kyoto
There was a problem hiding this comment.
Looks good!
One more "meta" question I'd have is around the capture/drop of all events by the user-side code. There is currently (as far as I can tell) no way to retrieve events that have been emitted. If you missed one it's gone. ldk-node keeps them all in a queue and forces you to acknowledge receipt of the event before feeding you the next one, but honestly that feels pretty heavy-handed in the Kyoto case.
I was looking at all the event types and I don't think there are events that would create critical issues if you missed them, but I'm not 100% sure yet; just something to think about/confirm. The two that need confirming are Synced and BlockDisconnected. If you miss a Synced event, will you pick up the missing state on your next block/sync emitted event?
| recent_history.into_iter().for_each(|(height, header)| { | ||
| chain_changeset.insert(height, Some(header.block_hash())); | ||
| }); | ||
| self.chain |
There was a problem hiding this comment.
So the result of the sync is applied to the local chain here, but also passed to the user code so they can apply it too?
There was a problem hiding this comment.
Yeah, we maintain a copy of a LocalChain to apply updates, but it is only used to build wallet updates. That is why I opened this PR, so the Wallet may directly handle events from kyoto if we determine that is easier to work with
|
I'm not 100% sure of my reading of this yet but is it the case that the events-based approach here is basically sending back more "info-type" events letting the user know what happened internally, but the actual domain logic lives in bdk_kyoto (applying updates to the wallet, disconnecting blocks?). If that's the case it's a slight departure from the bdk_electrum and bdk_esplora clients, which simply return types you must use to update your wallet yourself. I can see there is logic applied to the localchain in the events, but also the example applies the update to the wallet. Just wanting to build a good mental model for this for myself. |
This isn't much of an option for use unless we want to serialize blocks. I think it makes more sense to curate the events, so, if there are changes to the wallet, they are just aggregated together with a
I am not sure what is meant by miss. The next event will always be held until |
rustaceanrob
force-pushed
the
event-10-14
branch
from
09ad97d to
121eff8
Compare
Ahead of the bindings chat I thought a little more about an event-based API as opposed to the callback style we currently have. In this draft I only have a few
Eventtypes, but there would be a handful more. I still don't quite know if this would be a developer experience improvement. Just looking for an opinion on this rough draft.