Add calculate_fee and calculate_fee_rate on wallet #437
Conversation
reez
force-pushed
the
calculate-fee
branch
3 times, most recently
from
8d01bd4 to
c47aed1
Compare
There was a problem hiding this comment.
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.
reez
changed the title
reez
force-pushed
the
calculate-fee
branch
4 times, most recently
from
b6d2270 to
2459eb4
Compare
|
Updates:
This is ready for review again, but any suggestions welcome. |
bdk-android/lib/src/androidTest/kotlin/org/bitcoindevkit/LiveWalletTest.kt
Outdated
Show resolved
Hide resolved
bdk-ffi/src/wallet.rs
Outdated
|
|
||
| pub fn calculate_fee(&self, tx: &Transaction) -> Result<u64, CalculateFeeError> { | ||
| self.get_wallet() | ||
| .calculate_fee(&tx.clone().into()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() }
}There was a problem hiding this comment.
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_receivedcalculate_feecalculate_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-jvm/lib/src/test/kotlin/org/bitcoindevkit/LiveWalletTest.kt
Outdated
Show resolved
Hide resolved
| MissingTxOut(sequence<OutPoint> out_points); | ||
| NegativeFee(i64 fee); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
calculate-fee
branch
5 times, most recently
from
a796d75 to
e1c3ded
Compare
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:
|
|
Rebased |
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
BdkCalculateFeeErrorbe placed somewhere different in the UDL, etc).Currently I haveBdkCalculateFeeErrorwith 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
CalculateFeeErrorwith associated values and expose them in the UDL as well.Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: