Return wallet events when applying updates and blocks (3.0 milestone) #319
Conversation
There was a problem hiding this comment.
Hey @notmandatory thanks for your work on this! I left some comments related to code quality. As always, nits are non-blocking.
| .collect::<BTreeMap<Txid, (Arc<Transaction>, ChainPosition<ConfirmationBlockTime>)>>(); | ||
|
|
||
| // apply update | ||
| self.apply_update(update)?; |
There was a problem hiding this comment.
Any reason not to apply the update and use the resulting changeset to produce a summary of what changed in the Wallet?
There was a problem hiding this comment.
The resulting Wallet::ChangeSet only contains the new blocks, tx, and anchors found after applying the sync update but don't show how this new data changes the canonical status of existing tx.
There was a problem hiding this comment.
I added this as an "Option 4" to the ADR 0003.
| if chain_tip1 != chain_tip2 { | ||
| events.push(WalletEvent::ChainTipChanged { | ||
| old_tip: chain_tip1, | ||
| new_tip: chain_tip2, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think it would be helpful to know which blocks were connected and/or disconnected.
There was a problem hiding this comment.
Do you mean a list of connected/disconnected block ids? I didn't add any other info here because @tnull didn't need it for LDK's use case. Do you have some use or user in mind for this extra info?
There was a problem hiding this comment.
Yeah I mean the block IDs. It's just a more complete way of describing how the chain tip changed.
Pull Request Test Coverage Report for Build 18893642494Details
💛 - Coveralls |
notmandatory
force-pushed
the
feat/changeset_events_300
branch
2 times, most recently
from
21eaa1f to
488b373
Compare
|
Addressed initial comments from @ValuedMammal and rebased on Next steps:
|
notmandatory
force-pushed
the
feat/changeset_events_300
branch
from
488b373 to
2891853
Compare
|
I will cherry-pick and incorporate commits from #336 for |
notmandatory
force-pushed
the
feat/changeset_events_300
branch
from
d09c5e2 to
8d580ed
Compare
# Conflicts: # src/wallet/event.rs
WalletEvent is a enum of user facing events that are generated when a sync update is applied to a wallet using the Wallet::apply_update_events function.
per suggestions from ValuedMammal: 1. re-export WalletEvent type 2. add comments to wallet_events function 3. rename ambiguous variable names in wallet_events from cp to pos 4. remove signing from wallet_event tests 5. change wallet_events function assert_eq to debug_asset_eq 6. update ADR 0003 decision outcome and add option 4 re: creating events only from Update
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.
Co-authored-by: Steve Myers <[email protected]>
Also did minor cleanup of apply_update_events tests.
notmandatory
force-pushed
the
feat/changeset_events_300
branch
from
8d580ed to
6c5c755
Compare
|
Finished cherry-picking from #336 and rebased on |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #319 +/- ##
==========================================
+ Coverage 84.81% 85.18% +0.37%
==========================================
Files 23 24 +1
Lines 8145 8418 +273
==========================================
+ Hits 6908 7171 +263
- Misses 1237 1247 +10
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:
|
notmandatory
changed the title
4 participants
Description
Cherry-pick of commits from #310 and #336.
DRAFT and may modify based on feed back from users with 2.2.
Notes to the reviewers
Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: