forked from bitcoindevkit/bdk
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Matthias/sqlx #11
Open
matthiasdebernardini
wants to merge
29
commits into
evanlinjin:matthias/sqlx
Choose a base branch
from
matthiasdebernardini:matthias/sqlx
base: matthias/sqlx
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Matthias/sqlx #11
matthiasdebernardini
wants to merge
29
commits into
evanlinjin:matthias/sqlx
from
matthiasdebernardini:matthias/sqlx
Conversation
This file contains 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
matthiasdebernardini
commented
Aug 16, 2024
The bitcoin and rand dependencies are required to build examples independently and not from the top level bdk workspace.
…sync 7b5657e docs(electrum): enhance API docs for fetch_prev_txouts argument (thunderbiscuit) Pull request description: This PR also serves as an issue; it can't really be merged as is. I just didn't want to open an issue and just ask for better docs and instead decided to open a PR with some unfinished new API docs. I am working on a page for the Book of BDK that focuses on bdk_wallet + bdk_electrum syncing. A few things have been unclear to me, and I think slight additions to the API docs would fix that for others. ~~1. I was wondering what exactly this `confirmation_time` field on the `bdk_chain::ConfirmationTimeHeightAnchor` type represents. After digging into it (at least for the electrum client), it represents the timestamp specified by the block header where the tx was confirmed. _Question: I'd like to confirm that this always the case, e.g. that this is the timestamp used in cases where your client is an Esplora or bitcoind RPC node?_ If so, my addition to the sentence will make sense and is ready to go.~~ Edit: this is no longer a type after the rebase. 2. I think it'd be great to add context to the `fetch_prev_txouts` argument on the full_scan and sync methods on the `BdkElectrumClient`. It says that this is necessary for "fee calculation". What does that mean? I assume it means "for calculating the fee rate on the given transactions"? (let me know if that's wrong). Even so, I'm left wondering what happens if I don't fetch them. Will my fee calculations be just plain wrong? Or will they be unavailable? A bit more context for the caller of the method would be great here. If someone knows the definite answer to this let me know and feel free to propose a doc line and I'll amend the commit! ### Checklists * [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 ACKs for top commit: evanlinjin: re-ACK 7b5657e Tree-SHA512: f0ac9ae4a116c9a3f5006d6a41f626ac36c3f8495204a9eaa06d2c8003cabe0005be33fcc810028d314c505c3385a5facd2bedb3b2218ddf272b0fa2220abd39
… electrum client f965f95 feat: enable selecting use-rustls-ring feature on electrum client (thunderbiscuit) Pull request description: This PR is a companion to bitcoindevkit/rust-electrum-client#135. It enables choosing the `ring` dependency on rustls instead of the new default (as of 0.23.0) `aws-lc-rs`. The AWS dependency breaks the Android and Swift builds. I wrote a more detailed explanation on [bitcoindevkit#135](bitcoindevkit/rust-electrum-client#135). ### Notes to the reviewers Do not merge before: - [x] [bitcoindevkit#135](bitcoindevkit/rust-electrum-client#135) is merged - [x] A new version of rust-electrum-client is released (will be 0.21.0) - [x] The dependency points to the new version of the client rather than my fork of it. ### Changelog notice ```md Added - bdk_electrum now enables choosing either the `use-rustls` or `use-rustls-ring` feature ``` ### 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 ACKs for top commit: LLFourn: ACK f965f95 notmandatory: utACK f965f95 oleonardolima: ACK f965f95 Tree-SHA512: c82afa82ef8603bc8e6d024ee5030fa1ec6ab71fbce090182ce3a297ce1a788c1db48f593f05331b1de1931a731a5fc03f804cb1b17f8c7832286fda6c09aa4b
that allows creating a wallet with a single descriptor.
when selecting a wallet row from sqlite. This is consistent with the structure of the wallet `ChangeSet` where the fields `descriptor`, `change_descriptor`, and `network` are all optional.
to just `descriptor` that takes a `KeychainKind` and optional `D: IntoWalletDescriptor` representing the expected value of the descriptor in the changeset. Add method `LoadParams::extract_keys` that will use any private keys in the provided descriptors to add a signer to the wallet.
…or wallets 13e7008 doc(wallet): clarify docs for `Wallet::load` (valued mammal) 3951110 fix(wallet)!: Change method `LoadParams::descriptors` (valued mammal) b802714 example(wallet): simplify miniscript compiler example (valued mammal) 2ca8b6f test(wallet): Add tests for single descriptor wallet (valued mammal) 31f1c2d fix(wallet): Change FromSql type to `Option<_>` (valued mammal) 75155b7 feat(wallet): Add method `Wallet::create_single` (valued mammal) Pull request description: The change descriptor is made optional, making this an effective reversion of bitcoindevkit#1390 and enables creating wallets with a single descriptor. fixes bitcoindevkit#1511 ### Notes to the reviewers PR 1390 also removed an error case [`ChangePolicyDescriptor`](https://github.com/bitcoindevkit/bdk/blob/8eef350bd08057acc39b6fc50b1217db5e29b968/crates/wallet/src/wallet/mod.rs#L1529-L1533) and this can possibly be added back. In the case the wallet only has a single descriptor we allow any utxos to fund a tx that aren't specifically marked unspendable regardless of the change spend policy. ### Changelog notice * Added method `Wallet::create_single` that expects a single `D: IntoWalletDescriptor` as input and enables building a wallet with no internal keychain. ### 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 #### New Features: * [x] This pull request breaks the existing API * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: evanlinjin: ACK 13e7008 Tree-SHA512: 3e6fe5d9165d62332ac1863ec769c4bc5c7cd3c7f3fbdb8f505906bcdc681fa73b3fef2571adb0e52e9a23d4257f66a6145838b90ec68596b5f4c64054a047fa
Some users would like to use esplora updates with custom `ChainOracle`s (not `LocalChain`). We introduce "low-level" extension methods that populate an update `TxGraph` with associated data. Additionally, much of the logic has been made more efficient. We make use of the `/tx/:txid` endpoint (`get_tx_info` method) to do a single call to get both the tx and tx_status. If the tx already exists, we only fetch the tx_status. The logic for fetching data based on outpoints has been reworked to be more efficient and it now supports parallelism Additionally, the logic for fetching data based on outpoints has been reworked to be more efficient and it now supports parallelism. Documentation has also been reworked. Note that this is NOT a breaking change because the API of `full_scan` and `sync` are not changed.
Prevouts are needed to calculate fees for transactions. They are introduced as floating txouts in the update `TxGraph`. A helper method `insert_prevouts` is added to insert the floating txouts using the `Vin`s returned from Esplora. Also replaced `anchor_from_status` with `insert_anchor_from_status` as we always insert the anchor into the update `TxGraph` after getting it. Also removed `bitcoin` dependency as `bdk_chain` already depends on `bitcoin` (and it's re-exported).
Change `FullScanRequest` and `SyncRequest` take in a `chain_tip` as an option. In turn, `FullScanResult` and `SyncResult` are also changed to return the update `chain_tip` as an option. This allows the caller to opt-out of getting a `LocalChain` update. Rework `FullScanRequest` and `SyncRequest` to have better ergonomics when inspecting the progress of items of a sync request. Richer progress data is provided to the inspect closure. Introduce `FullScanRequestBuilder` and `SyncRequestBuilder`. Separating out request-construction and request-consumption in different structs simplifies the API and method names. Simplify `EsploraExt` and `EsploraAsyncExt` back to having two methods (`full_scan` and `sync`). The caller can still opt out of fetching a `LocalChain` update with the new `FullScanRequest` and `SyncRequest`.
and use consistent generic parameter names across `SyncRequest` and `SyncRequestBuilder`.
…dates with `FullScanRequest`/`SyncRequest` structures 6d77e2e refactor(chain)!: Rename `spks_with_labels` to `spks_with_indices` (志宇) 584b10a docs(esplora): README example, uncomment async import (志宇) 3eb5dd1 fix(chain): correct `Iterator::size_hint` impl (志宇) 96023c0 docs(chain): improve `SyncRequestBuilder::spks_with_labels` docs (志宇) 0234f70 docs(esplora): Fix typo (志宇) 38f86fe fix: no premature collect (志宇) 44e2a79 feat!: rework `FullScanRequest` and `SyncRequest` (志宇) 16c1c2c docs(esplora): Simplify crate docs (志宇) c93e6fd feat(esplora): always fetch prevouts (志宇) cad3533 feat(esplora): make ext traits more flexible (志宇) Pull request description: Closes bitcoindevkit#1528 ### Description Some users use `bdk_esplora` to update `bdk_chain` structures *without* a `LocalChain`. ~~This PR introduces "low-level" methods to `EsploraExt` and `EsploraAsyncExt` which populates a `TxGraph` update with associated data.~~ We change `FullScanRequest`/`SyncRequest` to take in the `chain_tip` parameter as an option. Spk-based chain sources (`bdk_electrum` and `bdk_esplora`) will not fetch a chain-update if `chain_tip` is `None`, allowing callers to opt-out of receiving updates for `LocalChain`. We change `FullScanRequest`/`SyncRequest` to have better ergonomics when inspecting the progress of syncing (refer to bitcoindevkit#1528). We change `FullScanRequest`/`SyncRequest` to be constructed with a builder pattern. This is a better API since we separate request-construction and request-consumption. Additionally, much of the `bdk_esplora` logic has been made more efficient (less calls to Esplora) by utilizing the `/tx/:txid` endpoint (`get_tx_info` method). This method returns the tx and tx_status in one call. The logic for fetching updates for outpoints has been reworked to support parallelism. Documentation has also been updated. ### Notes to reviewers This PR has evolved somewhat. Initially, we were adding more methods on `EsploraExt`/`EsploraAsyncExt` to make syncing/scanning more modular. However, it turns out we can just make the `chain_tip` parameter of the request structures optional. Since we are changing the request structures, we might as well go further and improve the ergonomics of the whole structures (refer to bitcoindevkit#1528). This is where we are at with this PR. Unfortunately, the changes are now breaking. I think this is an okay tradeoff for the API improvements (before we get to stable). ### Changelog notice * Change request structures in `bdk_chain::spk_client` to be constructed via a builder pattern, make providing a `chain_tip` optional, and have better ergonomics for inspecting progress while syncing. * Change `bdk_esplora` to be more efficient by reducing the number of calls via the `/tx/:txid` endpoint. The logic for fetching outpoint updates has been reworked to support parallelism. * Change `bdk_esplora` to always add prev-txouts to the `TxGraph` update. ### 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 #### New Features: * [ ] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: ValuedMammal: ACK 6d77e2e notmandatory: ACK 6d77e2e Tree-SHA512: 806cb159a8801f4e33846d18e6053b65d105e452b0f3f9d639b0c3f2e48fb665e632898bf42977901526834587223b0d7ec7ba1f73bb796b5fd8fe91e6f287f7
The network and genesis_hash methods on the LoadParams struct have been renamed to check_network and check_genesis_hash to better reflect their use.
…ds that validate rather than set 2391b76 refactor(wallet)!: rename LoadParams methods (thunderbiscuit) Pull request description: This PR is a follow up to the dev call discussion where we decided it was better to PR these changes separate from bitcoindevkit#1533. This is a breaking change, but I think it's worth it because those will potentially cause runtime errors for users that expect one thing to happen and realize it's something else. Left out of this PR but as surfaced in the call probably worth discussing is whether these methods make sense at all or whether they should be removed entirely. What does it mean to return an error when someone loads a wallet from persistence for which the genesis hash doesn't match the one persisted? Maybe worth a new issue; this PR simply attempts to name them correctly. ### Description See [Q1 in comment here](bitcoindevkit#1533 (comment)) for more context into the initial question. Two of the methods on the builder that loads a wallet were checkers/validators rather than setters: - `network()` - `genesis_hash()` This is confusing for users because when loading a wallet from persistence those things are persisted and cannot be changed, and yet seemingly the option to do that is there with those methods (so now you're re-thinking what you think you know about the changeset and persistence). Moreover, the fields on the [`LoadParams` type](https://docs.rs/bdk_wallet/1.0.0-beta.1/src/bdk_wallet/wallet/params.rs.html#116-124) are correctly named `check_network` and `check_genesis_hash`. This PR simply brings those two methods in line with what they actually do and set on the struct they modify. This modification was not done on the `descriptors()` method, because that one is both a validator and a setter if the descriptors passed contain private keys. Note that I chose the names `validate_network` and `validate_genesis_hash` but I'm not married to them. If others prefer `check_network` and `check_genesis_hash`, I'm happy to fix them that way instead! ### Changelog notice ```md Breaking: - The `LoadParams` type used in the wallet load builder renamed its `network()` and `genesis_hash` methods to `check_network()` and `check_genesis_hash`. [bitcoindevkit#1537] [bitcoindevkit#1537]: bitcoindevkit#1537 ``` ### 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 #### New Features: * [ ] I've added tests for the new feature * [ ] I've added docs for the new feature #### Bugfixes: * [x] This pull request breaks the existing API * [ ] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR ACKs for top commit: notmandatory: ACK 2391b76 Tree-SHA512: 6852ad165bab230a003b92ae0408176055f8c9101a0936d2d5a41c2a3577e258b045a7f4b796d9bab29ed261caf80b738c4338d4ff9322fbddc8c273ab0ff914
…ndencies 3675a9e ci: add job to build example-crates independently (Steve Myers) 1adf63c fix(example_cli): add bitcoin and rand dependencies (Steve Myers) Pull request description: ### Description Fix building `example_cli` by adding the bitcoin and rand dependencies so it, and the crates that depend on it, can be built independently and not only from the workspace. ### Notes to the reviewers I also added the build-examples CI job to verify we can build examples individually. ### Changelog notice None. ### 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 #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR ACKs for top commit: ValuedMammal: ACK 3675a9e storopoli: ACK 3675a9e Tree-SHA512: c88fdf6cde72959c88c9f4563834824c573afedb1e5136b0f902d919b42b0de18424fb0d05f275c63a0c05da8062dc53ad5825bdc8dc4b12441890e1b799378b
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.