Add IndexedChain trait and implement for Esplora & Electrum #490
Conversation
johncantrell97
force-pushed
the
master
branch
2 times, most recently
from
2d0d749 to
34ef964
Compare
src/blockchain/electrum.rs
Outdated
| @@ -48,6 +48,63 @@ pub struct ElectrumBlockchain { | |||
| stop_gap: usize, | |||
| } | |||
|
|
|||
| impl IndexedChain for ElectrumBlockchain { | |||
There was a problem hiding this comment.
Since each of these queries requires a different underlying index I think it makes sense to have a different trait for each index e.g. GetBlockHeader, GetPositionInBlock etc.
There was a problem hiding this comment.
Concept ack on breaking down the Blockchain into multiple traits. I think this will be eventually needed to make the lib more versatile.
Code changes looks good to me. If the difference between vanilla Blockchain and the other types are based on the capabilities (indexes) of different backends, then it might make sense to have such per capability traits as @LLFourn suggested.
But for now, I think its a good start.
Just one doc request.
src/blockchain/mod.rs
Outdated
| pub block_height: Option<u32>, | ||
| } | ||
|
|
||
| /// A trait Blockchains can implement if they support querying chain data |
There was a problem hiding this comment.
By "support querying chain data" do you mean availability of certain indexing in those blockchain? In that case it might make sense to elaborate the doc a little to explain what are those special requirements. Also how IndexedChain is different from regular backends with the Blockchain trait.
In general a little explainer doc for this new trait would be very nice.
johncantrell97
force-pushed
the
master
branch
2 times, most recently
from
66e9aa8 to
261e333
Compare
|
Now that #501 is in you'll need to rebase on |
notmandatory
modified the milestones:
Release 0.16.0 Feature Freeze,
Release 0.17.0 Feature Freeze
0cc4700 Fix typo in CHANGELOG and doc in wallet/mod.rs (Steve Myers) 660faab Fix typo in CHANGELOG (LLFourn) 45767fc Remove max_addresses sync param (LLFourn) fbb50ad apply doc suggestions from @notmandatory (Lloyd Fournier) 035307e [rpc] Filter out unrelated transactions (LLFourn) c0e75fc Split get_tx into its own trait (LLFourn) dcd90f8 Restore but depreciate new_offline (LLFourn) 410a513 Add SyncOptions as the second argument to Wallet::sync (LLFourn) 326bfe8 Remove Blockchain from wallet (LLFourn) Pull request description: While trying to finish bitcoindevkit#490 I thought that it'd be better to try the idea of getting rid of a lot of the async macros and just having tow different traits for sync and async stuff. While trying to do that I felt that this needed to be done first. The goal of this change is to decouple the wallet from the blockchain trait. A wallet is something that keeps track of UTXOs and transactions (and can sign things). The only reason it should need to talk to the blockchain is if doing a `sync`. So we remove all superfluous calls to the blockchain and precisely define the requirements for the blockchain to be used to sync with two new traits: `WalletSync` and `GetHeight`. 1. Stop requesting height when wallet is created 2. `new_offline` is just `new` now. 3. a `WalletSync + GetHeight` is now the first argument to `sync`. 4. `SyncOptions` replaces the existing options to `sync` and allows for less friction when making breaking changes in the fuutre (no more noop_progress!). 5. We `Box` the `Progress` now to avoid type parameters 6. broadcast has been removed from `Wallet`. You just use the blockchain directly to broadcast now. ### Notes to the reviewers This needs bitcoindevkit#502 before it can be merged but can reviewed now. Be on the look up for stale documentation from this change. Our doc build looks broken because of ureq or something. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: a8f3e21e45359be642b1f30d4ac1ba74785439e1b770dbeab0a5a4b8aab1eef4bb6b855aea4382e289257bc890fa713ca832a8b6c9655f7a59e96d412b4da3e6
|
Concept ACK - would love to see this functionality get merged. |
|
FWIW my plan is not to merge this PR or something similar but rather to reduce the surface area that the main BDK lib has with blockchain interactions. I have got feedback that people really want a fully featured rust interface to esplora's http api though. The question to me is do we really need traits in BDK that define this behaviour? It seems like a small thing to leave up to the application developer if they indeed want to have blockchain data polymorphism. |
- stop depending on bdk fork (it's not longer required, migrate code from PR bitcoindevkit/bdk#490 directly into this repository) - clean up code (some variable renames, remove unnecessary trait bounds) Note: esplora-related code was not migrated for now, can be added at any later point.
- stop depending on bdk fork (it's not longer required, migrate code from PR bitcoindevkit/bdk#490 directly into this repository) - clean up code (some variable renames, remove unnecessary trait bounds) - add a simple test that verifies that LightningWallet can be initialised Note: esplora-related code was not migrated for now, can be added at any later point.
johncantrell97
force-pushed
the
master
branch
from
872636d to
5c42102
Compare
Description
This PR introduces the trait IndexedChain that Blockchains can implement if they have access to chain data. There are lots of methods that could be added to this trait (the entire esplora/electrum apis) but I started with the data I needed to provide LDK with the chain data it needs to function.
I have implemented the trait for Esplora and Electrum blockchains.
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md