refactor!(chain): Unify ChangeSet nomenclature by danielabrozzoni · Pull Request #1065 · bitcoindevkit/bdk

danielabrozzoni · 2023-08-07T15:50:26Z

Changelog notice

Rename indexed_tx_graph::IndexedAdditions to indexed_tx_graph::ChangeSet
Rename indexed_tx_graph::IndexedAdditions::graph_additions to indexed_tx_graph::ChangeSet::graph
Rename indexed_tx_graph::IndexedAdditions::index_additions to indexed_tx_graph::ChangeSet::indexer
Rename tx_graph::Additions to tx_graph::ChangeSet
Rename keychain::DerivationAdditions to keychain::ChangeSet
Rename CanonicalTx::node to CanonicalTx::tx_node
Rename CanonicalTx::observed_as to CanonicalTx::chain_position
Rename LocalChangeSet to WalletChangeSet
Rename LocalChangeSet::chain_changeset to WalletChangeSet::chain
Rename LocalChangeSet::indexed_additions to WalletChangeSet::indexed_tx_graph
Rename LocalUpdate to WalletUpdate
Make TxGraph::determine_changeset pub(crate)
Add TxGraph::initial_changeset
Add IndexedTxGraph::initial_changeset
Remove TxGraph::insert_txout_preview
Remove TxGraph::insert_tx_preview
Remove insert_anchor_preview
Remove insert_seen_at_preview

Notes to reviewers

I'm not sure whether we want to keep TxGraph::determine_changeset public or not

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

notmandatory

LGTM! much easier to read using the same nomenclature across the code base. I added a couple comments/questions. I think this should go in before #1048 and then I'll rebase that PR on these changes.

crates/chain/src/tx_graph.rs

crates/chain/tests/test_tx_graph.rs

evanlinjin · 2023-08-08T05:16:29Z

LGTM too. I can ACK and merge after wording changes as mentioned by @notmandatory.

LLFourn · 2023-08-08T09:20:49Z

Haven't reviewed but got a note here: #1060 (comment) that TxGraph doesn't have an "initial_changeset" method to dump the full state as a changeset. It should in order to be consistent. Same with IndexedTxGraph. Can we also make sure all changeset types are must_use (except for the wallet one).

LLFourn

Approved as it is but I've made a few extra renaming suggestions and I do think now is the time to create initial_changeset methods for the tx graphs. The tx graph one could probably be implemented something like:

fn initial_changeset(&self) -> ChangeSet {
    Self::default().apply_update(self.clone())
}

but for IndexedTxGraph you might have to add a initial_changeset method to the Indexer trait to get it done properly.

crates/chain/src/indexed_tx_graph.rs

crates/chain/src/keychain.rs

danielabrozzoni · 2023-08-16T15:06:58Z

I rebased and fixed the review comments. I'm now working on adding initial_changeset for the tx_graph and the indexed_tx_graph, but since this PR is holding up #1040, feel free to merge before I'm done and I'll open a new PR

danielabrozzoni · 2023-08-16T15:13:13Z

Also, every ChangeSet is #[must_use] apart from the local_chain one, but since it's just a type alias, when I tried to add it the compiler warned me that it would have no effect:

   Compiling bdk_chain v0.5.0 (/home/daniela/Developer/bitcoindevkit/bdk/crates/chain)
warning: `#[must_use]` has no effect when applied to a type alias
  --> crates/chain/src/local_chain.rs:14:1
   |
14 | #[must_use]
   | ^^^^^^^^^^^
   |
   = note: `#[warn(unused_attributes)]` on by default

danielabrozzoni · 2023-08-16T15:59:16Z

Ok, this took less than what I expected :) I just pushed 4ac9c03, adding initial_changeset to tx_graph and indexed_tx_graph.

notmandatory

Looks good. ACK 4ac9c03

vladimirfomene

Review ACK 4ac9c03. Feel free to push the TxGraph refactoring to another PR.

crates/chain/src/tx_graph.rs

crates/chain/src/indexed_tx_graph.rs

crates/chain/src/tx_graph.rs

evanlinjin

ACK 4ac9c03

Looks good. My comments are regarding documentation, variable naming and minute implementation details (things that are not mission-critical and can be addressed later).

crates/chain/src/indexed_tx_graph.rs

crates/chain/src/tx_graph.rs

notmandatory · 2023-08-18T04:03:24Z

@danielabrozzoni looks like some small but useful suggestions so I'll wait until you've had a chance to address them before merging.

LLFourn · 2023-08-18T09:00:10Z

ACK 4ac9c03

This commit renames: - indexed_tx_graph::IndexedAdditions -> indexed_tx_graph::ChangeSet - indexed_tx_graph::IndexedAdditions::graph_additions -> indexed_tx_graph::ChangeSet::graph - indexed_tx_graph::IndexedAdditions::index_additions -> indexed_tx_graph::ChangeSet::indexer - tx_graph::Additions -> tx_graph::ChangeSet - keychain::DerivationAdditions -> keychain::ChangeSet - CanonicalTx::node -> CanonicalTx::tx_node - CanonicalTx::observed_as -> CanonicalTx::chain_position - LocalChangeSet -> WalletChangeSet - LocalChangeSet::chain_changeset -> WalletChangeSet::chain - LocalChangeSet::indexed_additions -> WalletChangeSet::indexed_tx_graph - LocalUpdate -> WalletUpdate This commit also changes the visibility of TxGraph::determine_changeset to be pub(crate), and the method accepts a TxGraph instead of &TxGraph This commit removes: - `TxGraph::insert_txout_preview` - `TxGraph::insert_tx_preview` - `insert_anchor_preview` - `insert_seen_at_preview` Solves bitcoindevkit#1022

Add `initial_changeset` to TxGraph and IndexedTxGraph

danielabrozzoni · 2023-08-18T10:17:10Z

Thanks for the reviews! Updated, and pushed a commit on top to fix the CI

evanlinjin

ACK 62f2531

danielabrozzoni added this to the 1.0.0-alpha.2 milestone Aug 7, 2023

danielabrozzoni self-assigned this Aug 7, 2023

danielabrozzoni force-pushed the issue/1022 branch 2 times, most recently from bad30dc to de25407 Compare August 7, 2023 16:06

notmandatory reviewed Aug 7, 2023

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

nondiremanuel mentioned this pull request Aug 8, 2023

W33 Lib Team Call bitcoindevkit/.github#72

Closed

26 tasks

LLFourn mentioned this pull request Aug 9, 2023

Further enhancements to Update and ChangeSet structures #1061

Closed

LLFourn approved these changes Aug 9, 2023

View reviewed changes

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/keychain.rs Outdated Show resolved Hide resolved

LLFourn mentioned this pull request Aug 15, 2023

Add cli esplora example #1040

Merged

4 tasks

danielabrozzoni force-pushed the issue/1022 branch 4 times, most recently from dc55016 to c7fff96 Compare August 16, 2023 15:05

danielabrozzoni force-pushed the issue/1022 branch 2 times, most recently from e8df529 to 4ac9c03 Compare August 16, 2023 15:54

notmandatory approved these changes Aug 17, 2023

View reviewed changes

vladimirfomene reviewed Aug 17, 2023

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

evanlinjin approved these changes Aug 18, 2023

View reviewed changes

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/indexed_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

danielabrozzoni added 2 commits August 18, 2023 11:51

feat(chain): Add initial_changeset to graphs

80f5ecf

Add `initial_changeset` to TxGraph and IndexedTxGraph

danielabrozzoni force-pushed the issue/1022 branch from 4ac9c03 to 80f5ecf Compare August 18, 2023 09:52

ci: Update tokio version

62f2531

danielabrozzoni mentioned this pull request Aug 18, 2023

Use apply_update instead of determine_changeset + apply_changeset around the code #1081

Closed

evanlinjin approved these changes Aug 18, 2023

View reviewed changes

evanlinjin merged commit d6bcd9b into bitcoindevkit:master Aug 18, 2023

danielabrozzoni mentioned this pull request Aug 18, 2023

Unify changeset APIs between all structures #1022

Closed

nondiremanuel mentioned this pull request Aug 22, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

notmandatory mentioned this pull request Oct 11, 2023

Release 1.0.0-alpha.2 #1168

Closed

12 tasks

Conversation

danielabrozzoni commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog notice

Notes to reviewers

Checklists

All Submissions:

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Aug 8, 2023

Uh oh!

LLFourn commented Aug 8, 2023

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni commented Aug 16, 2023

Uh oh!

danielabrozzoni commented Aug 16, 2023

Uh oh!

danielabrozzoni commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirfomene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

notmandatory commented Aug 18, 2023

Uh oh!

LLFourn commented Aug 18, 2023

Uh oh!

danielabrozzoni commented Aug 18, 2023

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danielabrozzoni commented Aug 7, 2023 •

edited

Loading

danielabrozzoni commented Aug 16, 2023 •

edited

Loading