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 From trait for &BitcoinTxIn #25

Merged

Conversation

thunderbiscuit
Copy link
Member

This trait is required for our use of the TxIn type.

I also:

  • Made a small clean up commit to use what I think is easier to read variable names for the From trait
  • Made the return type of the From trait explicit instead of using Self (my understanding is that it's more idiomatic that way).

@reez
Copy link
Collaborator

reez commented Oct 20, 2024

  • Made the return type of the From trait explicit instead of using Self (my understanding is that it's more idiomatic that way).

Good to know that's the more idiomatic way, I'll have to keep that in mind-

Copy link
Collaborator

@reez reez left a comment

Choose a reason for hiding this comment

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

ACK 165f910

(My assumption is the Failing Kotlin CI test will be resolved after #26 gets in)

@thunderbiscuit thunderbiscuit merged commit 4cd8e64 into bitcoindevkit:master Oct 20, 2024
7 checks passed
@thunderbiscuit thunderbiscuit deleted the feature/txin-addons branch October 20, 2024 23:32
@rustaceanrob
Copy link
Contributor

rustaceanrob commented Oct 21, 2024

For future reference, I am not sure I would go about implementing this From conversion. It abstracts away a script_buf.clone() that ideally we should have to call explicitly higher up the stack IMO. If I recall correctly from bdk-ffi, this is used when calling iter on vector of TxIn. Instead, I think you would want to call into_iter on a cloned Vec<TxIn>, which takes ownership of each of the TxIn. So the cost of cloning is more visible higher up the stack

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

Successfully merging this pull request may close these issues.

3 participants