Add TxBuilder APIs #611

thunderbiscuit · 2024-10-22T16:57:25Z

This PR brings in some of the missing APIs on the TxBuilder outlined in #597.

Changelog notice

Added:
  - TxBuilder::add_data [#611]
  - TxBuilder::current_height [#611]
  - TxBuilder::nlocktime [#611]
  - TxBuilder::allow_dust [#611]
  - TxBuilder::allow_dust [#611]

[#611]: /~https://github.com/bitcoindevkit/bdk-ffi/pull/611

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

reez

Watching Steve's tabconf talk last week he used this functionality and I actually jotted down a note about it wanting to try it out in bdk-ffi/iOS

ACK e06f2a9

Side note: the two checkboxes I've added tests for the new feature + I've added docs for the new feature are checked, are you adding a test as a follow up and/or adding docs as a follow up to whatever PR we do adding docstrings, or were those just checked accidentally?

rustaceanrob · 2024-11-15T16:59:11Z

With the discussion on bitcoindevkit/bdk#1676 I think we should add a set_version or similar while we are at it. Transaction version one is a fingerprint, and users might be clever enough to use transaction version three

thunderbiscuit · 2024-11-22T15:57:36Z

This is finally ready for review. I recommend reviewing 1 commit at a time, as they are individual independent pieces. There are 3 methods left on the TxBuilder identified in #597, but all 3 require a bit more work/thought behind them, and would not be creating breaking changes when we add them.

I think merging the 5 methods proposed here makes sense for the 1.0 release.

reez · 2024-11-22T17:21:23Z

bdk-ffi/src/tx_builder.rs

+        if !&self.data.is_empty() {
+            let push_bytes = PushBytesBuf::try_from(self.data.clone())?;
+            tx_builder.add_data(&push_bytes);
+        }


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.

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

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

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

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

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

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.

reez · 2024-11-22T17:28:08Z

bdk-ffi/src/tx_builder.rs

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


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

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.

reez · 2024-11-22T17:38:27Z

bdk-ffi/src/tx_builder.rs

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


looks great, wrote a test and passed ✅

reez · 2024-11-22T17:38:53Z

bdk-ffi/src/tx_builder.rs

+    pub(crate) fn allow_dust(&self, allow_dust: bool) -> Arc<Self> {
+        Arc::new(TxBuilder {
+            allow_dust,
+            ..self.clone()
+        })
+    }
+


looks great as well, wrote a test and passed ✅

reez · 2024-11-22T17:50:46Z

bdk-ffi/src/tx_builder.rs

+    pub(crate) fn version(&self, version: i32) -> Arc<Self> {
+        Arc::new(TxBuilder {
+            version: Some(version),
+            ..self.clone()
+        })
+    }
+


Was able to write two tests (one for version 1 and one for version 2) and both passed, looks good!

reez

did a review, left comments for each function I tested. the only one I have outstanding questions on is add_data, so just requesting changes atm but I could be wrong on my side

good job with these tho!

reez · 2024-11-25T15:37:22Z

ACK d0bec77

Wouldn't mind seeing docstrings added for these, but not a blocker-

thunderbiscuit · 2024-11-25T16:04:19Z

@reez docstrings are absolutely required at this point thanks for reminding me and feel free put your foot down a little harder when I forget and use the full might of the full stop character just kidding that would hurt my feelings.

But jokes aside I do expect PRs going forward to have docstrings so please hold me to it. We've gotten out of the habit but it's not an excuse.

reez · 2024-11-25T16:15:20Z

I've added docs for the new feature

Haha, no problem, sounds good, my plan is to just look for this checkbox to be ✅ on PR's with new methods from now on

reez approved these changes Oct 30, 2024

View reviewed changes

thunderbiscuit force-pushed the feature/add-txbuilder-apis branch from 0ac8dce to 337d676 Compare November 15, 2024 16:36

thunderbiscuit force-pushed the feature/add-txbuilder-apis branch 2 times, most recently from e9714e6 to 1d8b95d Compare November 21, 2024 18:25

thunderbiscuit added 3 commits November 21, 2024 14:29

feat: expose TxBuilder::add_data method

c6cbfb9

feat: expose TxBuilder::current_height

cfc0308

feat: expose TxBuilder::nlocktime

99adbf7

thunderbiscuit force-pushed the feature/add-txbuilder-apis branch from 0741588 to 99adbf7 Compare November 21, 2024 19:29

thunderbiscuit added 2 commits November 22, 2024 10:26

feat: expose TxBuilder::allow_dust

465dbeb

feat: expose TxBuilder::version

d0bec77

thunderbiscuit self-assigned this Nov 22, 2024

thunderbiscuit added this to the 1.0.0-beta milestone Nov 22, 2024

reez reviewed Nov 22, 2024

View reviewed changes

reez requested changes Nov 22, 2024

View reviewed changes

notmandatory mentioned this pull request Nov 23, 2024

fix(tx_builder)!: make TxBuilder Send safe, remove Clone trait bitcoindevkit/bdk#1737

Merged

5 tasks

reez approved these changes Nov 25, 2024

View reviewed changes

thunderbiscuit added 2 commits November 25, 2024 15:19

docs: add docstrings for TxBuilder

ee80d5d

docs: add docstrings for BumpFeeTxBuilder

4f702eb

thunderbiscuit merged commit 4f702eb into bitcoindevkit:master Nov 26, 2024
27 checks passed

thunderbiscuit deleted the feature/add-txbuilder-apis branch November 26, 2024 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TxBuilder APIs #611

Add TxBuilder APIs #611

thunderbiscuit commented Oct 22, 2024 •

edited

Loading

reez left a comment

rustaceanrob commented Nov 15, 2024

thunderbiscuit commented Nov 22, 2024

reez Nov 22, 2024

rustaceanrob Nov 22, 2024

reez Nov 22, 2024

rustaceanrob Nov 22, 2024

reez Nov 22, 2024

reez Nov 22, 2024

thunderbiscuit Nov 25, 2024

reez Nov 22, 2024

thunderbiscuit Nov 25, 2024

reez Nov 22, 2024

reez Nov 22, 2024

reez Nov 22, 2024

reez left a comment

reez commented Nov 25, 2024

thunderbiscuit commented Nov 25, 2024

reez commented Nov 25, 2024

Add TxBuilder APIs #611

Add TxBuilder APIs #611

Conversation

thunderbiscuit commented Oct 22, 2024 • edited Loading

Changelog notice

Checklists

All Submissions:

New Features:

reez left a comment

Choose a reason for hiding this comment

rustaceanrob commented Nov 15, 2024

thunderbiscuit commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reez left a comment

Choose a reason for hiding this comment

reez commented Nov 25, 2024

thunderbiscuit commented Nov 25, 2024

reez commented Nov 25, 2024

thunderbiscuit commented Oct 22, 2024 •

edited

Loading