Skip to content

feat(chain,wallet)!: rm ConfirmationTime#1643

Merged
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
evanlinjin:fix/rm-confirmationtime
Oct 24, 2024
Merged

feat(chain,wallet)!: rm ConfirmationTime#1643
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
evanlinjin:fix/rm-confirmationtime

Conversation

@evanlinjin
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin commented Oct 8, 2024

Description

This PR removes ConfirmationTime, and favors ChainPosition<ConfirmationBlockTime> instead. The only difference between these two structures is that ChainPosition<ConfirmationBlockTime> contains an additional BlockHash. Additionally, ConfirmationTime was not used in many places. It was mainly for displaying information in bdk_wallet::Wallet.

We also impl serde::Deserialize and serde::Serialize for ChainPosition.

Notes to the reviewers

Changelog notice

  • Remove bdk_chain::ConfirmationTime. Use ChainPosition<ConfirmationBlockTime> in place.

Checklists

All Submissions:

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

We rm `ConfirmationTime` because it is essentially the same thing as
`ChainPosition<ConfirmationBlockTime>` without the block hash.

We also impl `serde::Deserialize` and `serde::Serialize` for
`ChainPosition`.
@evanlinjin evanlinjin marked this pull request as ready for review October 9, 2024 04:43
@evanlinjin evanlinjin self-assigned this Oct 10, 2024
@evanlinjin evanlinjin added api A breaking API change chore Non-coding related work labels Oct 10, 2024
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK a3d4eef

@LagginTimes
Copy link
Copy Markdown
Contributor

ACK a3d4eef

@notmandatory
Copy link
Copy Markdown
Member

@evanlinjin should we add this to the 1.0-beta milestone? It's a breaking change but looks straight forward and has two ACKs.

@evanlinjin
Copy link
Copy Markdown
Member Author

@notmandatory I agree. I think this makes sense to be part of 1.0-beta.

Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK a3d4eef

@ValuedMammal ValuedMammal merged commit 647d285 into bitcoindevkit:master Oct 24, 2024
notmandatory added a commit that referenced this pull request Nov 12, 2024
b0dc3dd feat(wallet)!: make `seen_at` mandatory for `Wallet::apply_update_at` (志宇)
00c568d revert(wallet)!: rm `Wallet::unbroadcast_transactions` (志宇)
200a16d fix(wallet)!: delete method `insert_tx` (valued mammal)
ab27884 test(wallet): improve usage of test utils (valued mammal)
9bdf4cb chore: fix imports (valued mammal)
28d8061 test(wallet): fix test descriptor getters (valued mammal)
3135e29 test(wallet): add helpers to `test_utils` (valued mammal)
823bb39 feat(wallet): add module `test_utils` (valued mammal)
297bd9a test(wallet): refactor helper `insert_anchor_from_conf` (valued mammal)

Pull request description:

  Follow up to #1643, refactor `insert_anchor_from_conf` to just insert an anchor of type `ConfirmationBlockTime`

  ### Notes to the reviewers

  The PR introduces a public `test_utils` module and "test-utils" cargo feature that exposes common helpers such as `get_funded_wallet`. Credit to #1492 for inspiring that idea. Usage of test utilities is enhanced overall, and tests are less dependent on problematic APIs that may be removed in the future.

  ### Changelog notice

  - `bdk_wallet`: Added "test-utils" feature flag that exposes common helpers for testing and development
  - Removed methods `Wallet::insert_tx`, `Wallet::insert_checkpoint`, `Wallet::unbroadcast_transactions`

  ### 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
  * [x] I've added docs for the new feature
  * [x] This pull request breaks the existing API

Top commit has no ACKs.

Tree-SHA512: 561757595c65b4531dbf8b81f44387af6ac60114ecca493693cd975188741b5ff7b75a0dcf1dafc9d5750566baad81c644e7463c3c412a8331ad73de29601016
@notmandatory notmandatory mentioned this pull request Dec 11, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change chore Non-coding related work

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants