-
Notifications
You must be signed in to change notification settings - Fork 56
(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
base: master
Are you sure you want to change the base?
Conversation
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 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> { |
There was a problem hiding this comment.
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)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enforce consensus transaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: