Return wallet events when applying updates (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
from
826bb68 to
21eaa1f
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.
notmandatory
force-pushed
the
feat/changeset_events_300
branch
from
21eaa1f to
488b373
Compare
|
Addressed initial comments from @ValuedMammal and rebased on Next steps:
|
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
notmandatory
force-pushed
the
feat/changeset_events_300
branch
from
488b373 to
2891853
Compare
|
I will cherry-pick and incorporate commits from #336 for |
3 participants
Description
Cherry-pick of commits from #310.
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: