feat: apply_unconfirmed_txs on wallet #704
Conversation
reez
changed the title
bdk-ffi/src/bdk.udl
Outdated
| dictionary UnconfirmedTx { | ||
| Transaction tx; | ||
| u64 last_seen; | ||
| }; |
There was a problem hiding this comment.
Applying our idea of using macros when possible on changes and new things, this block here can be replaced with
typedef dictionary UnconfirmedTx;and the Rust code can simply add
#[derive(uniffi::Record)]
pub struct UnconfirmedTx {
pub tx: Arc<Transaction>,
pub last_seen: u64,
}To leverage the procedural macros.
There was a problem hiding this comment.
yes totally! my first macro here, updated now.
| @@ -592,3 +592,7 @@ impl From<BdkTx> for Tx { | |||
| } | |||
| } | |||
| } | |||
| pub struct UnconfirmedTx { | |||
There was a problem hiding this comment.
If we go with the proc macros proposed above, a little bit of API docs to describe this type might be nice.
There was a problem hiding this comment.
Created a UnconfirmedTx type, similar to how we needed to create a SentAndReceivedValues type instead of returning a tuple. Totally open to naming it something different, but just needed a type instead of a tuple.
Since it's not a 1-for-1 of a type I didn't add docs (but would have liked to! just wasn't sure if/how to do it best since its not 1-for-1), what are you thinking for a little bit of API docs? Here's the method docs https://docs.rs/bdk_wallet/latest/bdk_wallet/struct.Wallet.html#method.apply_unconfirmed_txs let me know what you think and I'll def add!
There was a problem hiding this comment.
Maybe something like this?
/// This type replaces the Rust tuple `(tx, last_seen)` used in the Wallet::apply_unconfirmed_txs` method,
/// where `last_seen` is the timestamp of when the transaction `tx` was last seen in the mempool.
bdk-ffi/src/wallet.rs
Outdated
| self.get_wallet().apply_unconfirmed_txs( | ||
| unconfirmed_txs | ||
| .into_iter() | ||
| .map(|utx| (Arc::new((&*utx.tx).into()), utx.last_seen)), |
There was a problem hiding this comment.
I find this line hard to parse, but I understand it's required. You need to dereference the transaction in the Arc before taking a reference to it. I think in this case using
.map(|utx| (Arc::new(utx.tx.as_ref().into()), utx.last_seen)),would work as well and might be easier to read? Just a nit, pick whatever line you like best.
There was a problem hiding this comment.
Thank you sir, much much easier to read, so thanks for calling this out. Updated
reez
force-pushed
the
apply
branch
2 times, most recently
from
f98e3f4 to
14a3c53
Compare
|
We can merge this one next as soon as the tests pass, and I'll rebase my 705 and we can merge that after. |
cool catch you in a few hours when tests pass 😆 |
Description
Add apply_unconfirmed_txs to Wallet, resolving #636 potentially.
Notes to the reviewers
Created a
UnconfirmedTxtype, similar to how we needed to create aSentAndReceivedValuestype instead of returning a tuple. Totally open to naming it something different, but just needed a type instead of a tuple.Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: