Add calculate_fee and calculate_fee_rate on wallet #437

reez · 2023-12-17T01:33:28Z

Description

~~Add calculate_fee on wallet.~~
Add calculate_fee and calculate_fee_rate on wallet.

Continuing to implement functions/types associated with PR on bdk "Remove TransactionDetails from Wallet API".

Notes to the reviewers

Open to hearing where to best place the new code in each file too (should BdkCalculateFeeError be placed somewhere different in the UDL, etc).

Currently I have BdkCalculateFeeError with associated values, but I am ignoring associated values in the UDL, and will address that in a follow up PR with all of the other changes we need to possibly do to Error types and associating data with them.
I have CalculateFeeError with associated values and expose them in the UDL as well.

Changelog notice

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

thunderbiscuit

Nice little addition! Only small issue I think is that the name is reversed; you're exposing a new error type called BdkCalculateFeeError, but we usually expose the a type with the same name as in Rust and internally transform the Rust type into a BdkSomethingSomething type. Just a switcharoo of those will make this PR ready to go.

bdk-ffi/src/bdk.udl

bdk-ffi/src/error.rs

reez · 2023-12-19T17:30:01Z

Updates: 

did switcharoo of CalculateFeeError and BdkCalculateFeeError per Thunder suggestion 
exposed CalculateFeeError with associated values in the UDL per uniffi 
added calculate_fee_rate
added a simplified FeeRate constructor and removed the from_sat_per_vb constructor 
updated tests across all 4 platforms

 This is ready for review again, but any suggestions welcome.

thunderbiscuit

Awesome work! I have a few comments but overall this is shaping up.

bdk-android/lib/src/androidTest/kotlin/org/bitcoindevkit/LiveWalletTest.kt

bdk-ffi/src/types.rs

thunderbiscuit · 2023-12-21T19:41:04Z

bdk-ffi/src/wallet.rs

+
+    pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> {
+        self.get_wallet()
+            .calculate_fee(&tx.clone().into())


The clone is required here because we've only implemented the From trait on the owned Transaction type and you're passing in a reference. But if you define it for the referenced version (should be the exact same code really) then you won't need the clones here on both the new methods.

Mmmm after playing with it, I realize it actually requires the clone anyway haha. But I do think it's maybe more elegant and leaves the implementation out of the calculate_fee method.

impl From<&Transaction> for BdkTransaction { fn from(tx: &Transaction) -> Self { tx.inner.clone() } }

Sounds like a plan to me. I added your suggested code snippet:

impl From<&Transaction> for BdkTransaction { fn from(tx: &Transaction) -> Self { tx.inner.clone() } }

... then updated 4 functions that had a line related to transaction that could have .clone() removed.

wallet.rs

sent_and_received

calculate_fee

calculate_fee_rate

esplora.rs

broadcast

... there was still 1 transaction related .clone() and it was wallet.rs transactions so I added the reference version (we already have the owned version):

impl From<&BdkTransaction> for Transaction { fn from(tx: &BdkTransaction) -> Self { Transaction { inner: tx.clone() } } }

... and then updated the transactions function code to remove .clone().

I tested bdk-ffi bdk-swift bdk-jvm locally with these changes and tests still passed.

Hopefully that all makes sense to have updated and added along with your suggestion!

bdk-ffi/src/wallet.rs

bdk-jvm/lib/src/test/kotlin/org/bitcoindevkit/LiveWalletTest.kt

bdk-python/tests/test_live_wallet.py

thunderbiscuit · 2023-12-21T20:15:14Z

bdk-ffi/src/bdk.udl

+  MissingTxOut(sequence<OutPoint> out_points);
+  NegativeFee(i64 fee);


This is the first time we use this pattern of defining errors as interfaces in the UDL. Can you confirm that the errors print out what we expect them to? Just making sure it all works well.

I just added two tests in the error.rs file to address this. Let me know if:

those two tests make sense to you

that is the best place to test these two errors

... or any other thoughts you had on it too. Thanks, great suggestion!

reez · 2023-12-28T19:42:58Z

Awesome work! I have a few comments but overall this is shaping up.

Great suggestions/thoughts/catches. Appreciated!

I updated all of the small items (and just marked the conversation resolved since they had been updated and were now “Outdated”) hopefully that’s cool, but I left the 2 items that I updated code for but you might still want to converse about too: 

trait on Transaction. I added comments to this as well as updated code, but just leaving it open for any other feedback or thoughts you might have
error testing. I added comments and code/tests for this but just leaving it open too for any other feedback or thoughts.

reez · 2024-01-08T18:47:47Z

Rebased

thunderbiscuit

ACK 3789c1d. Thanks!

reez changed the title ~~feat: add calculate_fee on wallet~~ Add calculate_fee on wallet Dec 17, 2023

reez force-pushed the calculate-fee branch 3 times, most recently from 8d01bd4 to c47aed1 Compare December 17, 2023 01:43

reez marked this pull request as ready for review December 18, 2023 16:40

reez requested a review from thunderbiscuit December 18, 2023 16:44

thunderbiscuit requested changes Dec 18, 2023

View reviewed changes

reez force-pushed the calculate-fee branch from c47aed1 to 4546aaf Compare December 18, 2023 21:29

reez commented Dec 18, 2023

View reviewed changes

bdk-ffi/src/bdk.udl Show resolved Hide resolved

reez commented Dec 18, 2023

View reviewed changes

bdk-ffi/src/error.rs Show resolved Hide resolved

reez commented Dec 18, 2023

View reviewed changes

bdk-ffi/src/error.rs Show resolved Hide resolved

reez force-pushed the calculate-fee branch from 4546aaf to a6b8ecd Compare December 18, 2023 22:04

reez changed the title ~~Add calculate_fee on wallet~~ Add calculate_fee and calculate_fee_rate on wallet Dec 18, 2023

reez force-pushed the calculate-fee branch 4 times, most recently from b6d2270 to 2459eb4 Compare December 19, 2023 17:02

reez force-pushed the calculate-fee branch from 2459eb4 to cd2bac4 Compare December 19, 2023 21:36

thunderbiscuit requested changes Dec 21, 2023

View reviewed changes

reez force-pushed the calculate-fee branch 5 times, most recently from a796d75 to e1c3ded Compare December 27, 2023 22:16

reez mentioned this pull request Dec 28, 2023

Use new FeeRate type in TxBuilder #438

Closed

reez force-pushed the calculate-fee branch from e1c3ded to 9e169ea Compare December 28, 2023 19:34

feat: add calculate_fee and calculate_fee_rate on wallet

3789c1d

reez force-pushed the calculate-fee branch from 9e169ea to 3789c1d Compare January 8, 2024 18:19

thunderbiscuit approved these changes Jan 10, 2024

View reviewed changes

thunderbiscuit merged commit 3789c1d into bitcoindevkit:master Jan 10, 2024
28 checks passed

reez deleted the calculate-fee branch January 10, 2024 20:35

reez mentioned this pull request Jan 19, 2024

feat: use FeeRate type in TxBuilder #444

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add calculate_fee and calculate_fee_rate on wallet #437

Add calculate_fee and calculate_fee_rate on wallet #437

reez commented Dec 17, 2023 •

edited

thunderbiscuit left a comment

reez commented Dec 19, 2023

thunderbiscuit left a comment

thunderbiscuit Dec 21, 2023

thunderbiscuit Dec 21, 2023

reez Dec 28, 2023

thunderbiscuit Dec 21, 2023

reez Dec 28, 2023

reez commented Dec 28, 2023

reez commented Jan 8, 2024

thunderbiscuit left a comment

		MissingTxOut(sequence<OutPoint> out_points);
		NegativeFee(i64 fee);

Add calculate_fee and calculate_fee_rate on wallet #437

Add calculate_fee and calculate_fee_rate on wallet #437

Conversation

reez commented Dec 17, 2023 • edited

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

thunderbiscuit left a comment

Choose a reason for hiding this comment

reez commented Dec 19, 2023

thunderbiscuit left a comment

Choose a reason for hiding this comment

thunderbiscuit Dec 21, 2023

Choose a reason for hiding this comment

thunderbiscuit Dec 21, 2023

Choose a reason for hiding this comment

reez Dec 28, 2023

Choose a reason for hiding this comment

thunderbiscuit Dec 21, 2023

Choose a reason for hiding this comment

reez Dec 28, 2023

Choose a reason for hiding this comment

reez commented Dec 28, 2023

reez commented Jan 8, 2024

thunderbiscuit left a comment

Choose a reason for hiding this comment

reez commented Dec 17, 2023 •

edited