Change methods of IndexedTxGraph/TxGraph/Wallet that insert txs to be more generic#1586
Merged
notmandatory merged 3 commits intobitcoindevkit:masterfrom Sep 9, 2024
Merged
Conversation
evanlinjin
force-pushed
the
tx_graph_generic_tx
branch
2 times, most recently
from
b178493 to
74b14a7
Compare
Instead of having `Transaction` as input, we have a generic parameter where the bound is `Into<Arc<Transaction>>` for the following methods: * `IndexedTxGraph::insert_tx` * `IndexedTxGraph::batch_insert_unconfirmed` * `TxGraph::batch_insert_unconfirmed`
Instead of having `Transaction` as input, have `T: Into<Arc<Transaction>>`.
…ransaction` * `Wallet::apply_unconfirmed_txs` * `IndexedTxGraph::batch_insert_relevant` * `IndexedTxGraph::batch_insert_relevant_unconfirmed`
evanlinjin
force-pushed
the
tx_graph_generic_tx
branch
from
74b14a7 to
87e6121
Compare
evanlinjin
changed the title
IndexedTxGraph and TxGraph methods that insert txs to be more genericIndexedTxGraph/TxGraph/Wallet that insert txs to be more generic
Collaborator
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
utACK 87e6121
Looks good! I do agree that it definitely helps being able to reuse the Arc pointer. It looks fine to me, but I wonder what others think about the breaking changes.
notmandatory
approved these changes
Sep 9, 2024
Member
notmandatory
left a comment
notmandatory
left a comment
There was a problem hiding this comment.
ACK 87e6121
This minor API change also works better with the UniFFI language bindings where all return types are wrapped in an Arc.
This was referenced Sep 13, 2024
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We want to reuse the
Arcpointer whenever possible. However, some methods onTxGraphandIndexedTxGraphthat insert transactions take in&TransactionorTransaction(thus forcing us to create a newArc<Transaction>internally by cloning, even if the input tx is under aArc).This PR changes these methods to take in a generic parameter,
T: Into<Arc<Transaction>>, allowing us to reuse theArcpointer whenever possible.Notes to the reviewers
Methods that previously took in
Transactioncan be changed to take inT: Into<Arc<Transaction>>and be non-breaking (sinceArc<T>implsFrom<T>automatically). These changes are contained in the first two commits.Methods that previously took in
&Transactionwill break. However, I think these api changes are small and the improvements are substantial enough to be worth it. These changes are contained in the last commit.Changelog notice
IndexedTxGraphmethodsinsert_tx,batch_insert_relevant,batch_insert_relevant_unconfirmed,batch_insert_unconfirmedto take inT: Into<Arc<Transaction>>instead ofTransactionor&Transaction.TxGraphmethodbatch_insert_unconfirmedto take inT: Into<Arc<Transaction>>instead ofTransaction.Walletmethodsinsert_tx,apply_unconfirmed_txsto take inT: Into<Arc<Transaction>>instead ofTransactionor&Transaction.Checklists
All Submissions:
cargo fmtandcargo clippybefore committing