-
Notifications
You must be signed in to change notification settings - Fork 354
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
feat: add blockchain traits module with esplora_async impl #1139
Conversation
4ce9011
to
8856611
Compare
Concept NACK.
|
/// Defines the options when syncing spks with an Electrum or Esplora blockchain client. | ||
#[derive(Debug)] | ||
pub struct SpkSyncMode { | ||
/// Sync all spks the wallet has ever derived | ||
pub all_spks: bool, | ||
/// Sync only derived spks that have not been used, only applies if `all_spks` is false | ||
pub unused_spks: bool, | ||
/// Sync wallet utxos | ||
pub utxos: bool, | ||
/// Sync unconfirmed transactions | ||
pub unconfirmed_tx: bool, | ||
} | ||
|
||
impl Default for SpkSyncMode { | ||
fn default() -> Self { | ||
Self { | ||
all_spks: false, | ||
unused_spks: true, | ||
utxos: true, | ||
unconfirmed_tx: true, | ||
} | ||
} | ||
} |
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.
From our discussion in FFI bindings meeting: can this be an enum
?
I want to @LLFourn's points that re-introducing separate async/blocking traits might make it hard to switch between syncing methods during runtime. If these are introduced, I'd prefer if we could make sure all relevant traits are implemented for all syncing methods (i.e., also have async bitcoind/electrum implementations, for instance). |
Is the purpose of introducing these traits to make it esier to implement FFI bindings? |
c6573e8
to
78c2e2a
Compare
The initial reason I started looking into this, as @evanlinjin mentioned above, is so the mobile bindings and Rust wallet users don't need to rewrite the same sync logic themselves. Instead I thought I could just throw the the scan/sync logic from the
Based on below I can see even if I could do what I'm trying to do without a Mutex it isn't the right approach.
Yes this is a problem and I agree we don't want to get back into all the MSRV and feature problems.
Long term I'd like to see users using CBF too, but most users today need esplora/electrum so I want to make their lives as easy as possible.
I'm going to close this PR, but I like your suggestion for how to add some sort of helper traits in the |
Awesome. I'd say start with something like this: #1153 (comment) which simplifies it down to three lines. Shouldn't need any traits either. That approach is better than what I was hinting at above. |
Description
WIP to make a simpler API for users to do common sync operations.
Notes to the reviewers
Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: