Add apply_unconfirmed_txs_events and apply_evicted_txs_events#388
Conversation
|
Per discussion in #374 I'm happy to deprecate these _events functions and replace them with some sort of If we agree to go this direction for 3.0 then I'll finish adding tests and cherry-pick these commits back to the release/2.x branch. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
+ Coverage 79.17% 79.77% +0.60%
==========================================
Files 24 24
Lines 5311 5266 -45
Branches 242 241 -1
==========================================
- Hits 4205 4201 -4
+ Misses 1029 988 -41
Partials 77 77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4883f54 to
c845417
Compare
|
Added a couple simple tests. Ready for review. |
c845417 to
01b1559
Compare
|
Updated with a couple changes suggested by @ValuedMammal (and also by Claude, will get it to review patches before pushing next time 🤖 ).
|
01b1559 to
c4084ab
Compare
|
LGTM From bdk-ffi perspective, |
…events - refactor _events functions to use new map_transactions helper
08ffbae to
ab82463
Compare
This can be used to generate `WalletEvent`s after executing a function defined by the caller. Refactored the `*_events` functions to use the events helper internally. test: Test that no events are returned by applying an empty Update and applying the same Update twice.
oleonardolima
left a comment
There was a problem hiding this comment.
utACK 12c453f
It looks way better with the events_helper fn 🚀. I left a single nit (non-blocker), and a fix (blocker) suggestion.
a61a41d to
45f4e22
Compare
|
Thanks @ValuedMammal for the cool helper function and extra tests. I've fixed the doc issues @oleonardolima found. |
nymius
left a comment
There was a problem hiding this comment.
ACK 45f4e22
I've executed tests successfully.
Coverage output is confusing, but I think it's pointing to pre-existent uncovered code.
I summarized the context of these changes here, mainly as a helper for me, but hope it can help others too. Let me know if you spot any errors:
bdk_wallet 3.0.0 (Depends on bdk_chain 0.23)
├───Issue #6: Missing user facing events from wallet updates
| └──── PR #319
| ├─ PR #310
| | ├─ WalletEvents { ChainTipChanged, TxConfirmed, TxUnconfirmed, TxReplaced, TxDropped}
| | ├─ wallet_events ("wallet diff")
| | └─ Wallet::apply_update_events: get events after applying updates
| |
| ├─ PR #336 (release 2.2.0): block event variants
| | ├─ Wallet::apply_block_events
| | └─ Wallet::apply_block_connected_to_events
| |
| └─ ADR 3: Return user-facing events when applying updates after syncing
|
└───Issue #374: Missing user facing events from mempool changes
└─ PR #388
├─ Wallet::apply_unconfirmed_txs_events
├─ Wallet::apply_evicted_txs_events
└─ Wallet::events_helper
I agree with the conclusions derived from #319 (comment). I think it is a more elegant solution.
It's going to deprecate 0003_events.md and merit a new ADR amending it and referencing a canonical view ADR.
This is a high level summary of what I extracted from the ongoing discussions about next steps:
bdk_wallet x.x.x (Depends on bdk_chain 0.24)
├─── Remove WalletEvents { ... }
├─── Add TransactionEvents { ChainTipChanged, TxConfirmed, TxUnconfirmed, TxReplaced, TxDropped}
├─── Add fn diff(self: &CanonicalView, other: &CanonicalView) -> TransactionEvents
├─── Add fn canonical_view(self: &Wallet) -> CanonicalView to produce wallet snapshots and compute events from CanonicalView::diff
├─── Deprecate ADR 003?
├─── Add ADR 004? Referencing changes due to new CanonicalView. Maybe add a bdk_chain ADR too?
└─── Add notice for *_events method deprecation in bdk_wallet x+1.x.x?
|
ACK |
ValuedMammal
left a comment
There was a problem hiding this comment.
ACK 45f4e22; ran the tests locally
Description
fixes #374
Notes to the reviewers
To reduce duplicate code I also refactored all the _events functions to use new
wallet_txs_to_maphelper.Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features: