fix(wallet)!: Improve test utilities #1658
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ValuedMammal
commented
Oct 24, 2024
Contributor
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
Concept ACK
Although I didn't get it yet why the change, mentioned in the comment below.
ValuedMammal
force-pushed
the
test/wallet-insert-anchor
branch
from
d33116f to
771d7c1
Compare
ValuedMammal
changed the title
insert_anchor_from_conf
ValuedMammal
force-pushed
the
test/wallet-insert-anchor
branch
2 times, most recently
from
95c14a2 to
869d7e6
Compare
ValuedMammal
force-pushed
the
test/wallet-insert-anchor
branch
from
869d7e6 to
f5eeafd
Compare
Member
|
Is this an API break change or something that should go in the 1.0-beta milestone? |
The common test utils are moved to a public `test_utils` module behind a new test-utils feature flag
- `get_funded_wallet` requires two descriptors - `get_funded_wallet_single` returns a single-descriptor wallet
ValuedMammal
force-pushed
the
test/wallet-insert-anchor
branch
from
f5eeafd to
1d61fde
Compare
Collaborator
Author
|
I cherry picked some of the changes from #1644. The main difference here is removing |
Inserting unconfirmed txs can be done using the existing method `apply_unconfirmed_txs`. Also removed `insert_checkpoint`, as the API is unclear regarding where in the local chain the given block should connect. Analogs for these methods are found in `test_utils` module and are mainly used to facilitate testing.
This is no longer relevant as we direct callers to only insert tx into the wallet after a successful broadcast.
Not including a `seen_at` alongside the update means the unconfirmed txs of the update will not be considered to be part of the canonical history. Therefore, transactions created with this wallet may replace the update's unconfirmed txs (which is unintuitive behavior). Also updated the docs.
ValuedMammal
force-pushed
the
test/wallet-insert-anchor
branch
from
1d61fde to
b0dc3dd
Compare
ValuedMammal
changed the title
Member
|
This also fixes #1622 by removing functions that were only needed for testing. |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #1643, refactor
insert_anchor_from_confto just insert an anchor of typeConfirmationBlockTimeNotes to the reviewers
The PR introduces a public
test_utilsmodule and "test-utils" cargo feature that exposes common helpers such asget_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 developmentWallet::insert_tx,Wallet::insert_checkpoint,Wallet::unbroadcast_transactionsChecklists
All Submissions:
cargo fmtandcargo clippybefore committing