Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!(chain): Unify ChangeSet nomenclature #1065

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

danielabrozzoni
Copy link
Member

@danielabrozzoni danielabrozzoni commented Aug 7, 2023

Solves #1022

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

@danielabrozzoni danielabrozzoni added this to the 1.0.0-alpha.2 milestone Aug 7, 2023
@danielabrozzoni danielabrozzoni self-assigned this Aug 7, 2023
@danielabrozzoni danielabrozzoni force-pushed the issue/1022 branch 2 times, most recently from bad30dc to de25407 Compare August 7, 2023 16:06
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member

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

@LLFourn
Copy link
Contributor

LLFourn commented Aug 8, 2023

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).

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
crates/chain/src/keychain.rs Outdated Show resolved Hide resolved
@LLFourn LLFourn mentioned this pull request Aug 15, 2023
4 tasks
@danielabrozzoni danielabrozzoni force-pushed the issue/1022 branch 4 times, most recently from dc55016 to c7fff96 Compare August 16, 2023 15:05
@danielabrozzoni
Copy link
Member Author

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
Copy link
Member Author

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 danielabrozzoni force-pushed the issue/1022 branch 2 times, most recently from e8df529 to 4ac9c03 Compare August 16, 2023 15:54
@danielabrozzoni
Copy link
Member Author

danielabrozzoni commented Aug 16, 2023

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

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. ACK 4ac9c03

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
@notmandatory
Copy link
Member

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

@LLFourn
Copy link
Contributor

LLFourn commented Aug 18, 2023

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
Copy link
Member Author

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

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 62f2531

@evanlinjin evanlinjin merged commit d6bcd9b into bitcoindevkit:master Aug 18, 2023
@notmandatory notmandatory mentioned this pull request Oct 11, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants