Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TxBuilder APIs #611

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions bdk-ffi/src/bdk.udl
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ interface CreateTxError {
UnknownUtxo(string outpoint);
MissingNonWitnessUtxo(string outpoint);
MiniscriptPsbt(string error_message);
PushBytesError();
LockTimeConversionError();
};

[Error]
Expand Down Expand Up @@ -709,6 +711,12 @@ interface TxBuilder {

TxBuilder set_exact_sequence(u32 nsequence);

TxBuilder add_data(sequence<u8> data);

TxBuilder current_height(u32 height);

TxBuilder nlocktime(LockTime locktime);

[Throws=CreateTxError]
Psbt finish([ByRef] Wallet wallet);
};
Expand All @@ -718,6 +726,10 @@ interface BumpFeeTxBuilder {

BumpFeeTxBuilder set_exact_sequence(u32 nsequence);

BumpFeeTxBuilder current_height(u32 height);

BumpFeeTxBuilder nlocktime(LockTime locktime);

[Throws=CreateTxError]
Psbt finish([ByRef] Wallet wallet);
};
Expand Down
13 changes: 13 additions & 0 deletions bdk-ffi/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use bitcoin_ffi::OutPoint;

use bdk_core::bitcoin::script::PushBytesError;
use bdk_electrum::electrum_client::Error as BdkElectrumError;
use bdk_esplora::esplora_client::{Error as BdkEsploraError, Error};
use bdk_wallet::bitcoin::address::ParseError as BdkParseError;
Expand Down Expand Up @@ -195,6 +196,12 @@ pub enum CreateTxError {

#[error("miniscript psbt error: {error_message}")]
MiniscriptPsbt { error_message: String },

#[error("attempt to prepare too many bytes to be pushed into script")]
PushBytesError,

#[error("invalid lock time value")]
LockTimeConversionError,
}

#[derive(Debug, thiserror::Error)]
Expand Down Expand Up @@ -951,6 +958,12 @@ impl From<BdkCreateTxError> for CreateTxError {
}
}

impl From<PushBytesError> for CreateTxError {
fn from(_: PushBytesError) -> Self {
CreateTxError::PushBytesError
}
}

impl From<BdkCreateWithPersistError<chain::rusqlite::Error>> for CreateWithPersistError {
fn from(error: BdkCreateWithPersistError<chain::rusqlite::Error>) -> Self {
match error {
Expand Down
73 changes: 70 additions & 3 deletions bdk-ffi/src/tx_builder.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
use crate::bitcoin::Psbt;
use crate::error::CreateTxError;
use crate::types::ScriptAmount;
use crate::types::{LockTime, ScriptAmount};
use crate::wallet::Wallet;

use bdk_wallet::KeychainKind;
use bitcoin_ffi::{Amount, FeeRate, Script};

use bdk_wallet::bitcoin::absolute::LockTime as BdkLockTime;
use bdk_wallet::bitcoin::amount::Amount as BdkAmount;
use bdk_wallet::bitcoin::script::PushBytesBuf;
use bdk_wallet::bitcoin::Psbt as BdkPsbt;
use bdk_wallet::bitcoin::ScriptBuf as BdkScriptBuf;
use bdk_wallet::bitcoin::{OutPoint, Sequence, Txid};
use bdk_wallet::ChangeSpendPolicy;
use bdk_wallet::KeychainKind;

use std::collections::BTreeMap;
use std::collections::HashMap;
use std::collections::HashSet;
use std::convert::{TryFrom, TryInto};
use std::str::FromStr;
use std::sync::Arc;

Expand All @@ -33,6 +36,9 @@ pub struct TxBuilder {
pub(crate) drain_wallet: bool,
pub(crate) drain_to: Option<BdkScriptBuf>,
pub(crate) sequence: Option<u32>,
pub(crate) data: Vec<u8>,
pub(crate) current_height: Option<u32>,
pub(crate) locktime: Option<LockTime>,
}

impl TxBuilder {
Expand All @@ -51,6 +57,9 @@ impl TxBuilder {
drain_wallet: false,
drain_to: None,
sequence: None,
data: Vec::new(),
current_height: None,
locktime: None,
}
}

Expand Down Expand Up @@ -193,6 +202,27 @@ impl TxBuilder {
})
}

pub(crate) fn add_data(&self, data: Vec<u8>) -> Arc<Self> {
Arc::new(TxBuilder {
data,
..self.clone()
})
}

pub(crate) fn current_height(&self, height: u32) -> Arc<Self> {
Arc::new(TxBuilder {
current_height: Some(height),
..self.clone()
})
}

Comment on lines +216 to +222
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewing commit by commit, this looks good, I wrote a test for it and it passed ✅

Copy link
Member Author

Choose a reason for hiding this comment

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

You sir are building good tests. Do you want to maybe add them as a commit to this PR? I'd merge that for sure.

pub(crate) fn nlocktime(&self, locktime: LockTime) -> Arc<Self> {
Arc::new(TxBuilder {
locktime: Some(locktime),
..self.clone()
})
}

Comment on lines +223 to +229
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks great, wrote a test and passed ✅

pub(crate) fn finish(&self, wallet: &Arc<Wallet>) -> Result<Arc<Psbt>, CreateTxError> {
// TODO: I had to change the wallet here to be mutable. Why is that now required with the 1.0 API?
let mut wallet = wallet.get_wallet();
Expand Down Expand Up @@ -237,6 +267,18 @@ impl TxBuilder {
if let Some(sequence) = self.sequence {
tx_builder.set_exact_sequence(Sequence(sequence));
}
if !&self.data.is_empty() {
let push_bytes = PushBytesBuf::try_from(self.data.clone())?;
tx_builder.add_data(&push_bytes);
}
Comment on lines +288 to +291
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm reviewing these commit by commit as recommended, and for this add_data I was wondering if we needed to manually validate the size of the data here based on Bitcoin's standard transaction relay policy. I'm wondering this because that thought popped into my head so I started writing some tests around it like:

  • test_add_data
  • test_add_data_within_limit
  • test_add_data_exceeding_limit
  • test_add_empty_data

and I couldn't get fn test_add_data_exceeding_limit test to work without adding something like

            if self.data.len() > 80 {
                return Err(CreateTxError::PushBytesError);
            }

in the finish method.

Thoughts? I could totally be missing something though.

Also let me know if adding the specific implementation of my tests is helpful too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I don't think add_data should land for 1.0.0. PushBytesBuf doesn't have a clear API and is_standard_op_return isn't even a method yet in rust-bitcoin (I am patching the method right now). Ideally everything about OP_RETURN is left to the Rust-side and FFI has no logic for handling this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally everything about OP_RETURN is left to the Rust-side and FFI has no logic for handling this

Def agree with this 100%. I just happened to be running into an issue with it when I was coming up with test to test what happened when I gave it data exceeding that limit. But maybe I wrote my test incorrectly, will double back and look at it again

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no check on the Rust side and currently it will allow for invalid OP_RETURN data

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking back at my test the other thing that played into the way I wrote it was how I approached non-standard transactions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no check on the Rust side and currently it will allow for invalid OP_RETURN data

Thanks for this! I didn't see it before I wrote:

Looking back at my test the other thing that played into the way I wrote it was how I approached non-standard transactions.

I'm actually really interested in where we land on this conversation even if for the time being overall we end up at this place you mentioned rob:

I don't think add_data should land for 1.0.0

Copy link
Member Author

Choose a reason for hiding this comment

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

I think enforcing standardness will come with bitcoindevkit/bdk#1726. I agree we should not have logic handling standardness here, and leave it to bdk_wallet (or rust-bitcoin when your patch is merged @rob, however that shakes out).

This was already a (somewhat) faulty API in the 0.32.0 release, and IMO people using it will just have to know how it works if they want to build relayable transactions (just like it was in bdk_wallet <= beta.5). The fix in beta 6 or the final 1.0 release will enforce standardness.

if let Some(height) = self.current_height {
tx_builder.current_height(height);
}
// let bdk_locktime = locktime.try_into().map_err(CreateTxError::LockTimeConversionError)?;
if let Some(locktime) = &self.locktime {
let bdk_locktime: BdkLockTime = locktime.try_into()?;
tx_builder.nlocktime(bdk_locktime);
}

let psbt = tx_builder.finish().map_err(CreateTxError::from)?;

Expand All @@ -249,14 +291,18 @@ pub(crate) struct BumpFeeTxBuilder {
pub(crate) txid: String,
pub(crate) fee_rate: Arc<FeeRate>,
pub(crate) sequence: Option<u32>,
pub(crate) current_height: Option<u32>,
pub(crate) locktime: Option<LockTime>,
}

impl BumpFeeTxBuilder {
pub(crate) fn new(txid: String, fee_rate: Arc<FeeRate>) -> Self {
Self {
BumpFeeTxBuilder {
txid,
fee_rate,
sequence: None,
current_height: None,
locktime: None,
}
}

Expand All @@ -267,6 +313,20 @@ impl BumpFeeTxBuilder {
})
}

pub(crate) fn current_height(&self, height: u32) -> Arc<Self> {
Arc::new(BumpFeeTxBuilder {
current_height: Some(height),
..self.clone()
})
}

pub(crate) fn nlocktime(&self, locktime: LockTime) -> Arc<Self> {
Arc::new(BumpFeeTxBuilder {
locktime: Some(locktime),
..self.clone()
})
}

pub(crate) fn finish(&self, wallet: &Arc<Wallet>) -> Result<Arc<Psbt>, CreateTxError> {
let txid = Txid::from_str(self.txid.as_str()).map_err(|_| CreateTxError::UnknownUtxo {
outpoint: self.txid.clone(),
Expand All @@ -277,6 +337,13 @@ impl BumpFeeTxBuilder {
if let Some(sequence) = self.sequence {
tx_builder.set_exact_sequence(Sequence(sequence));
}
if let Some(height) = self.current_height {
tx_builder.current_height(height);
}
if let Some(locktime) = &self.locktime {
let bdk_locktime: BdkLockTime = locktime.try_into()?;
tx_builder.nlocktime(bdk_locktime);
}

let psbt: BdkPsbt = tx_builder.finish()?;

Expand Down
16 changes: 15 additions & 1 deletion bdk-ffi/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::bitcoin::{Address, Transaction, TxOut};
use crate::error::RequestBuilderError;
use crate::error::{CreateTxError, RequestBuilderError};

use bitcoin_ffi::Amount;
use bitcoin_ffi::OutPoint;
Expand Down Expand Up @@ -29,6 +29,7 @@ use bdk_wallet::LocalOutput as BdkLocalOutput;
use bdk_wallet::Update as BdkUpdate;

use std::collections::HashMap;
use std::convert::TryFrom;
use std::sync::{Arc, Mutex};

#[derive(Debug)]
Expand Down Expand Up @@ -405,6 +406,19 @@ impl From<BdkLockTime> for LockTime {
}
}

impl TryFrom<&LockTime> for BdkLockTime {
type Error = CreateTxError;

fn try_from(value: &LockTime) -> Result<Self, CreateTxError> {
match value {
LockTime::Blocks { height } => BdkLockTime::from_height(*height)
.map_err(|_| CreateTxError::LockTimeConversionError),
LockTime::Seconds { consensus_time } => BdkLockTime::from_time(*consensus_time)
.map_err(|_| CreateTxError::LockTimeConversionError),
}
}
}

#[derive(Debug, Clone)]
pub enum Satisfaction {
Partial {
Expand Down
Loading