Skip to content

(draft) feat: add from_string to Transaction #804

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reez
Copy link
Collaborator

@reez reez commented Jul 7, 2025

TODO: move away from UDL in PR once other PR's merged (had to use UDL until then)

Description

relates to 803 issue

Dont mind the UDL stuff, will rebase on recent changes that move that error out of UDL.

Notes to the reviewers

Added a From trait for automatic error conversion to make things cleaner, consistent with what we usually try to do anyway.

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
Copy link
Member

I'm not against this per se and will certainly not oppose merging, but notice that this is not part of the Rust API (you cannot create a transaction using something like Transaction::from_str. The Rust API requires bytes, and the hex to byte transformation is done elsewhere.

In general I think if we add these sorts of utility APIs, we should have a way to mark them as our own (and not coming from our Rust dependencies). They should also get special attention in tests because of that fact (since they're not tested on the dependencies' side).

@reez
Copy link
Collaborator Author

reez commented Jul 7, 2025

I'm not against this per se and will certainly not oppose merging, but notice that this is not part of the Rust API (you cannot create a transaction using something like Transaction::from_str. The Rust API requires bytes, and the hex to byte transformation is done elsewhere.

In general I think if we add these sorts of utility APIs, we should have a way to mark them as our own (and not coming from our Rust dependencies). They should also get special attention in tests because of that fact (since they're not tested on the dependencies' side).

Discussion definitely here but I'm interested in touching on this at Bindings call tomorrow too, I just took a quick scan of how many ffi convenience methods that dont mirror the Rust API we have in bdk-ffi (just so I could contextualize for myself)

@@ -333,6 +334,13 @@ impl Transaction {
Ok(Transaction(tx))
}

/// Creates a new `Transaction` instance from a hexadecimal string representation.
#[uniffi::constructor]
pub fn from_string(tx_hex: String) -> Result<Self, TransactionError> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

naming convention>?

/// Creates a new `Transaction` instance from a hexadecimal string representation.
#[uniffi::constructor]
pub fn from_string(tx_hex: String) -> Result<Self, TransactionError> {
let tx_bytes = Vec::from_hex(&tx_hex)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

enforce consensus transaction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants