Remove TransactionDetails from Wallet API #1048

notmandatory · 2023-07-27T00:53:34Z

Description

Removed TransactionDetails and changed Wallet::get_tx to return a CanonicalTx, and TxBuilder::finish to return only a PartiallySignedTransaction. This should fix #922 and fix #1015.

I also added Wallet functions to get a Transaction send and receive amounts, fee, and FeeRate.

see: #922 (comment)

Notes to the reviewers

Alot of wallet tests had to change since TxBuilder::finish only returns a PSBT now.

I added a new CalculateFeeError which follows changes coming in #1028.

Changelog notice

Added

Wallet::sent_and_received function
Wallet::calculate_fee and Wallet::calculate_fee_rate functions
Wallet::error::CalculateFeeError
Wallet::insert_txout function to allow inserting foreign TxOuts

BREAKING CHANGES:

Removed

TransactionDetails struct

Changed

Wallet::get_tx now returns CanonicalTx instead of TransactionDetails
TxBuilder::finish now returns only a PartiallySignedTransaction

Checklists

All Submissions:

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

New Features:

I've added tests for the new feature
I've added docs for the new feature

LLFourn · 2023-07-27T02:06:50Z

I think the plan is to deprecate TransactionDetails altogether. What about just doing #1015 or is the plan to change this test to test the things in #1015 when they're added?

EDIT: this comment was made before seeing the discussion in #1015 -- I've commented over there.

crates/bdk/src/wallet/mod.rs

danielabrozzoni

I just have a nit

ACK 40d5af3

crates/bdk/src/wallet/mod.rs

notmandatory · 2023-08-01T06:27:06Z

@LLFourn continuing #1015 (comment) here.

Are you ok with the name TotalSentReceived for the struct and total_sent_received for the function? I just don't think and in the names is as useful as total. But if you really hate it I can live with SentAndReceived/sent_and_received.

I'm now calling the TxGraph::calculate_fee function but added a friendlier Result return type and new CalculateFeeError. If you like this approach should I move it down to TxGraph?

crates/bdk/src/wallet/mod.rs

LLFourn

Concept ACK. See minor comments.

crates/chain/src/spk_txout_index.rs

crates/bdk/tests/wallet.rs

crates/bdk/src/wallet/mod.rs

LLFourn

ACK 1992c62

left some nits

crates/bdk/src/wallet/mod.rs

Added - Wallet::sent_and_received function - Wallet::calculate_fee and Wallet::calculate_fee_rate functions - Wallet::error::CalculateFeeError BREAKING CHANGES: Removed - TransactionDetails struct Changed - Wallet::get_tx now returns CanonicalTx instead of TransactionDetails - TxBuilder::finish now returns only a PartiallySignedTransaction

…alculateFeeError> added - tx_graph::CalculateFeeError enum BREAKING CHANGES: changed - TxGraph::calculate_fee function to return Result<u64,CalculateFeeError> instead of Option<i64>

…fee functions added - Wallet::insert_txout function to allow inserting foreign TxOuts - test to verify error when trying to calculate fee with missing foreign utxo - test to calculate fee with inserted foreign utxo updated - docs for Wallet::calculate_fee, Wallet::calculate_fee_rate, and TxGraph::calculate_fee with note about missing foreign utxos

notmandatory · 2023-08-30T18:21:46Z

I've rebased, fixed docs and fixed a new MSRV issue. Just need @evanlinjin to ACK and then I'll merge, which will also fix master branch CI.

…utxo

…f psbt.fee_amount() - removed test_calculate_fee_with_inserted_foreign_utxo() since it now duplicates test in test_add_foreign_utxo()

evanlinjin

ACK 5fb5061

notmandatory added tests module-wallet labels Jul 27, 2023

notmandatory self-assigned this Jul 27, 2023

notmandatory mentioned this pull request Jul 27, 2023

Wallet::sent_and_received doesn't exist #1015

Closed

notmandatory force-pushed the wallet_txdetails branch from c26c9c6 to 51fe7f9 Compare July 27, 2023 01:09

notmandatory changed the title ~~test(wallet): add wallet test test_get_funded_wallet_tx_details~~ test(wallet): add test to verify wallet::get_tx TransactionDetails amounts Jul 27, 2023

notmandatory marked this pull request as draft July 27, 2023 14:06

notmandatory changed the title ~~test(wallet): add test to verify wallet::get_tx TransactionDetails amounts~~ Remove TransactionDetails from Wallet API, add Wallet::sent_and_received function Jul 27, 2023

notmandatory changed the title ~~Remove TransactionDetails from Wallet API, add Wallet::sent_and_received function~~ Remove TransactionDetails from Wallet API Jul 27, 2023

notmandatory force-pushed the wallet_txdetails branch from 51fe7f9 to d147f66 Compare July 28, 2023 03:47

notmandatory commented Jul 28, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory force-pushed the wallet_txdetails branch 5 times, most recently from ebb9f82 to 40d5af3 Compare July 28, 2023 04:25

notmandatory marked this pull request as ready for review July 28, 2023 04:29

nondiremanuel mentioned this pull request Jul 29, 2023

W31 Lib Team Call bitcoindevkit/.github#70

Closed

19 tasks

danielabrozzoni approved these changes Jul 31, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory force-pushed the wallet_txdetails branch 7 times, most recently from 16452c5 to 1667496 Compare August 1, 2023 06:14

LLFourn reviewed Aug 1, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

LLFourn reviewed Aug 1, 2023

View reviewed changes

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

notmandatory force-pushed the wallet_txdetails branch 2 times, most recently from bf88b7e to 1992c62 Compare August 18, 2023 23:33

LLFourn reviewed Aug 21, 2023

View reviewed changes

crates/bdk/tests/wallet.rs Outdated Show resolved Hide resolved

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved

LLFourn approved these changes Aug 21, 2023

View reviewed changes

LLFourn mentioned this pull request Aug 21, 2023

Implement an easy way to get missing txouts from data source crates for fee calculation #1083

Open

nondiremanuel mentioned this pull request Aug 22, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

notmandatory force-pushed the wallet_txdetails branch 4 times, most recently from 346521c to f1d868d Compare August 24, 2023 15:30

evanlinjin reviewed Aug 29, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

notmandatory force-pushed the wallet_txdetails branch from f1d868d to b3cbfb9 Compare August 30, 2023 16:52

notmandatory added 3 commits August 30, 2023 11:55

feat(tx_graph)!: change TxGraph::calculate_fee to return Result<u64,C…

d443fe7

…alculateFeeError> added - tx_graph::CalculateFeeError enum BREAKING CHANGES: changed - TxGraph::calculate_fee function to return Result<u64,CalculateFeeError> instead of Option<i64>

notmandatory force-pushed the wallet_txdetails branch from b3cbfb9 to 2f9318d Compare August 30, 2023 17:13

notmandatory added 3 commits August 30, 2023 13:31

docs(wallet): update docs for calculate_fee/fee_rate and add_foreign_…

465d53c

…utxo

test(wallet): add check_fee!(wallet,psbt) macro and use it in place o…

dd5b8d7

…f psbt.fee_amount() - removed test_calculate_fee_with_inserted_foreign_utxo() since it now duplicates test in test_add_foreign_utxo()

ci: fix msrv dependency versions for rustls

5fb5061

notmandatory force-pushed the wallet_txdetails branch from 2f9318d to 5fb5061 Compare August 30, 2023 18:31

evanlinjin approved these changes Aug 31, 2023

View reviewed changes

evanlinjin merged commit 93e8eaf into bitcoindevkit:master Aug 31, 2023

This was referenced Aug 31, 2023

Add cli esplora example #1040

Merged

ci: pin rustls 0.21.7 #1105

Closed

nondiremanuel mentioned this pull request Sep 10, 2023

W37 Lib Team Call bitcoindevkit/.github#76

Closed

41 tasks

reez mentioned this pull request Nov 29, 2023

Add sent_and_received method on Wallet bitcoindevkit/bdk-ffi#428

Merged

8 tasks

reez mentioned this pull request Dec 18, 2023

Add calculate_fee and calculate_fee_rate on wallet bitcoindevkit/bdk-ffi#437

Merged

8 tasks

This was referenced Dec 9, 2024

Change coin_selection and DescriptorExt::dust_value to use Amount type #1763

Merged

refactor(wallet): cleanup and remove unused code in create_tx #1768

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TransactionDetails from Wallet API #1048

Remove TransactionDetails from Wallet API #1048

notmandatory commented Jul 27, 2023 •

edited

Loading

LLFourn commented Jul 27, 2023 •

edited

Loading

danielabrozzoni left a comment

notmandatory commented Aug 1, 2023 •

edited

Loading

LLFourn left a comment

LLFourn left a comment

notmandatory commented Aug 30, 2023 •

edited

Loading

evanlinjin left a comment

Remove TransactionDetails from Wallet API #1048

Remove TransactionDetails from Wallet API #1048

Conversation

notmandatory commented Jul 27, 2023 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

LLFourn commented Jul 27, 2023 • edited Loading

danielabrozzoni left a comment

Choose a reason for hiding this comment

notmandatory commented Aug 1, 2023 • edited Loading

LLFourn left a comment

Choose a reason for hiding this comment

LLFourn left a comment

Choose a reason for hiding this comment

notmandatory commented Aug 30, 2023 • edited Loading

evanlinjin left a comment

Choose a reason for hiding this comment

notmandatory commented Jul 27, 2023 •

edited

Loading

LLFourn commented Jul 27, 2023 •

edited

Loading

notmandatory commented Aug 1, 2023 •

edited

Loading

notmandatory commented Aug 30, 2023 •

edited

Loading