Add apply_block_events and apply_block_connected_to_events
#336
Conversation
Pull Request Test Coverage Report for Build 18936234986Details
💛 - Coveralls |
| block: &Block, | ||
| height: u32, | ||
| ) -> Result<Vec<WalletEvent>, CannotConnectError> { | ||
| let connected_to = match height.checked_sub(1) { |
There was a problem hiding this comment.
Thanks for working on this. I noticed apply_block_events seems to duplicate logic from apply_block. Could we move the event handling from apply_block_connected_to_events into apply_block_events, then have it call apply_block instead of apply_block_connected_to? Would be more consistent with how apply_update_event works and avoid the duplication.
There was a problem hiding this comment.
No, I intentionally added variants for apply_block as well as for apply_block_connected_to as we may also want to use apply_block_connected_to_events at some point.
There was a problem hiding this comment.
You could have apply_block call the new apply_block_events and map the return value to ().
There was a problem hiding this comment.
You could have
apply_blockcall the newapply_block_eventsand map the return value to().
Hmm, but that would run the (possibly costly) delta calculation for everybody, even if they wouldn't make use of the events. I believe this is why @notmandatory added separate _event variants of the methods in the first place.
|
PR needs a rebase on the |
Previously, we added a new `Wallet::apply_update_events` method that returned `WalletEvent`s. Unfortunately, no corresponding APIs were added for the `apply_block` counterparts. Here we fix this omission.
tnull
force-pushed
the
2025-10-add-apply-block-events
branch
from
3039c1b to
df444d0
Compare
Rebased on |
5 participants
Description
Previously, we added a new
Wallet::apply_update_eventsmethod that returnedWalletEvents. Unfortunately, no corresponding APIs were added for theapply_blockcounterparts. Here we fix this omission.Notes to the reviewers
I opened this towards the
release-2.2branch, but it would probably need another release branch. Or let me know if you prefer to open it against master (which seems to be lackingapply_update_eventscurrently though).I also added no test coverage given that none seems to exist for
Wallet::apply_blockin the first place. Let me know if I should add something here.Checklists
All Submissions:
just pbefore pushingNew Features:
cc @notmandatory