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.

Sign up for GitHub

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

Jump to bottom

Unify changeset APIs between all structures #1022

Closed
Tracked by #66
, #70
...
LLFourn opened this issue Jun 28, 2023 · 2 comments
Closed
Tracked by #66
, #70
...

Unify changeset APIs between all structures #1022

LLFourn opened this issue Jun 28, 2023 · 2 comments
Assignees
@danielabrozzoni
Milestone
1.0.0-alpha.2

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Jun 28, 2023 •
edited
Loading

  • Each structure has a different name for its changeset. For example TxGraph it's called Additions while IndexedTxGraph is called IndexedAdditions. Rather they should have the same name and users should just use different module prefixes e.g. tx_graph::Additions or indexed_tx_graph::Additions.
  • Since these are monotone they don't need a "preview" methods. You can always just apply the update right away. I'd also include LocalChain in this policy despite it not being monotone (yet).
@LLFourn LLFourn added this to the 1.0.0-alpha.1 milestone Jun 28, 2023
@notmandatory notmandatory mentioned this issue Jul 3, 2023
Closed
16 tasks
@notmandatory notmandatory moved this to Todo in BDK Jul 26, 2023
@danielabrozzoni danielabrozzoni self-assigned this Jul 31, 2023
@danielabrozzoni danielabrozzoni mentioned this issue Aug 1, 2023
Merged
5 tasks
@nondiremanuel nondiremanuel mentioned this issue Aug 1, 2023
Closed
19 tasks
@notmandatory notmandatory moved this from Todo to In Progress in BDK Aug 7, 2023
@evanlinjin
Copy link
Member

evanlinjin commented Aug 7, 2023 •
edited
Loading

Since we are already on this, can I request for some more renamings?

  • CanonicalTx::observed_as should be renamed to CanonicalTx::chain_position.
  • CanonicalTx::node should be renamed to CanonicalTx::tx_node.

I think this will provide more clarity.

Thanks @danielabrozzoni

danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 7, 2023
@danielabrozzoni
Unify changeset APIs between all structures
de71800 
This commit renames:
indexed_tx_graph::IndexedAdditions -> indexed_tx_graph::ChangeSet
tx_graph::Additions -> tx_graph::ChangeSet
keychain::DerivationAdditions -> keychain::ChangeSet
CanonicalTx::node -> CanonicalTx::tx_node,
CanonicalTx::observed_as -> CanonicalTx::chain_position

This commit also changes the visibility of the determine_changeset
methods

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 7, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
b70a8f4 
This commit renames:

indexed_tx_graph::IndexedAdditions -> indexed_tx_graph::ChangeSet
tx_graph::Additions -> tx_graph::ChangeSet
keychain::DerivationAdditions -> keychain::ChangeSet
CanonicalTx::node -> CanonicalTx::tx_node,
CanonicalTx::observed_as -> CanonicalTx::chain_position

This commit also changes the visibility of the determine_changeset
methods to be pub(crate)

Solves bitcoindevkit#1022
@danielabrozzoni danielabrozzoni mentioned this issue Aug 7, 2023
Merged
3 tasks
@danielabrozzoni danielabrozzoni moved this from In Progress to Needs Review in BDK Aug 7, 2023
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 7, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
bad30dc 
This commit renames:

indexed_tx_graph::IndexedAdditions -> indexed_tx_graph::ChangeSet
tx_graph::Additions -> tx_graph::ChangeSet
keychain::DerivationAdditions -> keychain::ChangeSet
CanonicalTx::node -> CanonicalTx::tx_node,
CanonicalTx::observed_as -> CanonicalTx::chain_position

This commit also changes the visibility of TxGraph::determine_changeset
to be pub(crate)

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 7, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
de25407 
This commit renames:

- indexed_tx_graph::IndexedAdditions -> indexed_tx_graph::ChangeSet
- tx_graph::Additions -> tx_graph::ChangeSet
- keychain::DerivationAdditions -> keychain::ChangeSet
- CanonicalTx::node -> CanonicalTx::tx_node
- CanonicalTx::observed_as -> CanonicalTx::chain_position

This commit also changes the visibility of TxGraph::determine_changeset
to be pub(crate)

This commit removes:
- `TxGraph::insert_txout_preview`
- `TxGraph::insert_tx_preview`
- `insert_anchor_preview`
- `insert_seen_at_preview`

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 16, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
10ed46f 
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)

This commit removes:
- `TxGraph::insert_txout_preview`
- `TxGraph::insert_tx_preview`
- `insert_anchor_preview`
- `insert_seen_at_preview`

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 16, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
b9296e1 
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)

This commit removes:
- `TxGraph::insert_txout_preview`
- `TxGraph::insert_tx_preview`
- `insert_anchor_preview`
- `insert_seen_at_preview`

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 16, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
dc55016 
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)

This commit removes:
- `TxGraph::insert_txout_preview`
- `TxGraph::insert_tx_preview`
- `insert_anchor_preview`
- `insert_seen_at_preview`

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 16, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
c7fff96 
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)

This commit removes:
- `TxGraph::insert_txout_preview`
- `TxGraph::insert_tx_preview`
- `insert_anchor_preview`
- `insert_seen_at_preview`

Solves bitcoindevkit#1022
danielabrozzoni added a commit to danielabrozzoni/bdk that referenced this issue Aug 18, 2023
@danielabrozzoni
refactor!(chain): Unify ChangeSet nomenclature
ea50b6a 
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
evanlinjin added a commit that referenced this issue Aug 18, 2023
@evanlinjin
Merge #1065: refactor!(chain): Unify ChangeSet nomenclature
d6bcd9b 
62f2531 ci: Update tokio version (Daniela Brozzoni)
80f5ecf feat(chain): Add `initial_changeset` to graphs (Daniela Brozzoni)
ea50b6a refactor!(chain): Unify ChangeSet nomenclature (Daniela Brozzoni)

Pull request description:

  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:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  evanlinjin:
    ACK 62f2531

Tree-SHA512: 401bdf49c5f2082971cfcd54725ce98abd08263de7571205a324a2adbb5359be4f48454def415bedadde0a9c81db437b91c13cd6e892ba54b215059f75006dd5
@danielabrozzoni
Copy link
Member

#1065 fixed this

@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Aug 18, 2023
LagginTimes pushed a commit to LagginTimes/bdk that referenced this issue Aug 24, 2023
@danielabrozzoni @LagginTimes
refactor!(chain): Unify ChangeSet nomenclature
6e6f889 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danielabrozzoni danielabrozzoni

Labels
None yet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha.2
Development

No branches or pull requests
4 participants
@LLFourn @notmandatory @evanlinjin @danielabrozzoni