rename scan to scan_with_keychain #1117

448-OG · 2023-09-11T11:00:54Z

The purpose of this PR is to make changes regarding issue #1112

Notes to the reviewers

This is a draft pull request

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing (I run the tests using cargo test --all-features
I'm linking the issue being fixed by this PR

crates/electrum/src/electrum_ext.rs

danielabrozzoni

Left a few comments. Tests are failing, check the CI logs :)

crates/electrum/src/electrum_ext.rs

crates/esplora/src/blocking_ext.rs

crates/electrum/src/electrum_ext.rs

448-OG · 2023-09-14T12:27:18Z

I have made some changes, a review is most welcome.

danielabrozzoni · 2023-09-14T13:01:57Z

You have a few conflicts, rebase on master to fix them

448-OG · 2023-09-15T07:41:15Z

You have a few conflicts, rebase on master to fix them

Sorry about that. I have fixed the conflicts

…trumExt` no longer takes `txids` and `outpoints` as arguments Fix: `example_electrum` and `wallet_electrum` in `example-crates` no longer take values for these arguments that implement the `scan` method of trait `ElectrumExt`

…ethod `scan` to `scan_with_keychain` BREAKING CHANGE: In `electrum` crate rename the `ElectrumExt` trait method `scan_without_keychain` to `sync` Fix: Modify the `example_electrum` and `wallet_electrum` examples to use new methods from `ElectrumExt` trait

Chore: Implement recommendations from cargo clippy

…ethod `scan` to `scan_with_keychain` BREAKING CHANGE: In `electrum` crate rename the `ElectrumExt` trait method `scan_without_keychain` to `sync` Fix: Modify the `example_electrum` and `wallet_electrum` examples to use new methods from `ElectrumExt` trait This commit has been rebased to use commits at hash `c20a4da9fc902a062eb217b39e37a32d9e64a148` The requested updates have been applied using this commits and previous commits are mostly outdated

vladimirfomene · 2023-09-15T10:29:48Z

crates/bdk/src/wallet/coin_selection.rs

@@ -836,7 +836,7 @@ mod test {
        let drain_script = ScriptBuf::default();
        let target_amount = 250_000 + FEE_AMOUNT;

-        let result = LargestFirstCoinSelection::default()
+        let result = LargestFirstCoinSelection


I think these changes on coin selection slipped in because of the rebase. You have to remove them. There is more other files. Just cross check

These changes were as a result of running clippy. If you run clippy on master you will get recommendations to remove them. Since in rust tuple struct LargestFirstCoinSelection you dont need to call ::default() to initialize it

Generally it's better to handle fixing clippy changes in a separate commit or PR to make the review easier. Also a nit on commit messages, instead of BREAKING CHANGE: I'd use the notation refactor!(<module name>): .... See: https://www.conventionalcommits.org/en/v1.0.0/

And as @LLFourn mentioned we may end up incorporating this rename into another PR but if you have time it wouldn't hurt to fix this one up in case we can use it.

vladimirfomene · 2023-09-15T10:31:04Z

crates/electrum/src/electrum_ext.rs

-        txids: impl IntoIterator<Item = Txid>,
-        outpoints: impl IntoIterator<Item = OutPoint>,
+        _txids: impl IntoIterator<Item = Txid>,
+        _outpoints: impl IntoIterator<Item = OutPoint>,


why don't we completely remove _txids and _outpoints? I don't see their utility

They are used in bdk/example-crates/example_electrum/src/main.rs line 249, do I remove the code in examples too to reflect this?

txids and outpoints is meant to return you any relevant transactions to these items. They can't just be _ignored. A more significant refactoring is needed so that this method does not call scan_with_keychain but implements its own scanning logic.

crates/electrum/src/electrum_ext.rs

vladimirfomene · 2023-09-15T11:04:04Z

crates/electrum/src/electrum_ext.rs

        &self,
        prev_tip: Option<CheckPoint>,
        keychain_spks: BTreeMap<K, impl IntoIterator<Item = (u32, ScriptBuf)>>,
-        txids: impl IntoIterator<Item = Txid>,
-        outpoints: impl IntoIterator<Item = OutPoint>,


Now that we are removing txids and outpoints it doesn't make sense to be calling populate_with_txids and populate_with_outpoints here. Should separate methods be created to get these updates? @LLFourn @evanlinjin

Yes the separate method is scan. scan is still meant to be doing this.

448-OG · 2023-09-15T13:57:22Z

I will fix doc issues ASAP

danielabrozzoni

Make sure that all your commits are clean, and that don't introduce changes that you later undo in newer commits. For example, 2fa5fc6 introduces a couple of unwanted "<<<<<<< HEAD" that you later remove in 2a9a2e0 - instead, squash the two commits together.

Regarding commit messages:

We use conventional commits (https://www.conventionalcommits.org/en/v1.0.0/), which use the ! in the commit title to signal breaking changes (https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with--to-draw-attention-to-breaking-change). Also, we don't put "In ... crate", we simply put "name of the crate:" at the start of the title. As an example, 7e91849's title should go from:

BREAKING CHANGE: In electrum crate the method scan of trait `Elec…
…trumExt` no longer takes `txids` and `outpoints` as arguments

to:

refactor(electrum)!: `ElectrumExt::scan` no longer takes `txids`, `outpoints`

Notice that the part before : is in lowercase characters, and the part after : is capitalized.
9655e31's now looks like this:

Chore: Run cargo fmt

instead, it should be:

chore: Run cargo clippy

Since you're running clippy and not fmt :) Also, if the commit title is explanatory enough, you don't have to write a commit message.

If you've never rewritten commit messages, this can be of help: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

crates/electrum/src/electrum_ext.rs

…scan_with_keychain` This solves the issue where CI tests for docs.rs error on rustdoc warning `--cfg docsrs -Dwarnings`

448-OG · 2023-09-15T14:09:31Z

Make sure that all your commits are clean, and that don't introduce changes that you later undo in newer commits. For example, 2fa5fc6 introduces a couple of unwanted "<<<<<<< HEAD" that you later remove in 2a9a2e0 - instead, squash the two commits together.

Regarding commit messages:
* We use conventional commits (https://www.conventionalcommits.org/en/v1.0.0/), which use the `!` in the commit title to signal breaking changes (https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with--to-draw-attention-to-breaking-change). Also, we don't put "In ... crate", we simply put "_name of the crate_:" at the start of the title. As an example, [7e91849](https://github.com/bitcoindevkit/bdk/commit/7e91849f7c1896d2bdc4affc329eb50b3411828c)'s title should go from:
BREAKING CHANGE: In electrum crate the method scan of trait `Elec…
…trumExt` no longer takes `txids` and `outpoints` as arguments
to:
refactor(electrum)!: `ElectrumExt::scan` no longer takes `txids`, `outpoints`
Notice that the part before : is in lowercase characters, and the part after : is capitalized. 9655e31's now looks like this:
Chore: Run cargo fmt
instead, it should be:
chore: Run cargo clippy
Since you're running clippy and not fmt :) Also, if the commit title is explanatory enough, you don't have to write a commit message.

If you've never rewritten commit messages, this can be of help: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Thanks. I will look into commit messages

…at ` in the

danielabrozzoni · 2023-10-04T15:19:00Z

Hey @448-OG, are you still working on this? Do you need help with anything?

448-OG · 2023-10-05T09:36:44Z

Hey @448-OG, are you still working on this? Do you need help with anything?

Hey @danielabrozzoni I had finished doing changes, there were some comments @vladimirfomene made that I commented on. Unless there are new changes requested, my PR is ready for review

nondiremanuel · 2023-11-07T15:18:08Z

After today's discussion in the Lib Team Call, a check on whether all the comments were addressed is needed on this PR. Also it will probably need to be rebased after the ci fix of #1182

LLFourn · 2023-11-10T02:22:16Z

My opinion is that we probably want to close this PR in favor of one that solves all the problems in #1195 at once.

notmandatory · 2024-01-16T15:59:46Z

@448-OG thanks for your work on this but the issue got fixed with #1235.

448-OG · 2024-02-27T08:40:43Z

Thanks for the opportunity and reviews I got, I learnt quite a few things about the library.

448-OG commented Sep 11, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

448-OG commented Sep 11, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

danielabrozzoni requested changes Sep 12, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

LLFourn reviewed Sep 13, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

LLFourn reviewed Sep 13, 2023

View reviewed changes

crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved

448-OG commented Sep 14, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

448-OG added 4 commits September 15, 2023 12:29

Chore: Run cargo fmt

9655e31

Chore: Implement recommendations from cargo clippy

vladimirfomene suggested changes Sep 15, 2023

View reviewed changes

danielabrozzoni reviewed Sep 15, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

Fix: Replace previous docs ElectrumExt::scan with `ElectrumClient::…

25fb58a

…scan_with_keychain` This solves the issue where CI tests for docs.rs error on rustdoc warning `--cfg docsrs -Dwarnings`

docs(electrum): Dropped usage of NOTE and replaced it with `Note th…

95d0826

…at ` in the

448-OG marked this pull request as ready for review September 18, 2023 08:27

nondiremanuel mentioned this pull request Sep 23, 2023

W39 Lib Team Call bitcoindevkit/.github#78

Closed

40 tasks

nondiremanuel added this to the 1.0.0-alpha.3 milestone Sep 26, 2023

nondiremanuel mentioned this pull request Oct 6, 2023

W41 Lib Team Call bitcoindevkit/.github#80

Closed

39 tasks

notmandatory assigned 448-OG Oct 13, 2023

nondiremanuel mentioned this pull request Oct 22, 2023

W43 Lib Team Call bitcoindevkit/.github#83

Closed

33 tasks

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

nondiremanuel modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 20, 2023

nondiremanuel mentioned this pull request Dec 2, 2023

W49 Lib Team Call bitcoindevkit/.github#87

Closed

55 tasks

nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024

notmandatory added the api A breaking API change label Jan 16, 2024

notmandatory closed this Jan 16, 2024

notmandatory removed this from the 1.0.0 milestone Jan 16, 2024

448-OG deleted the sync_with_keychain branch February 27, 2024 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename scan to scan_with_keychain #1117

rename scan to scan_with_keychain #1117

448-OG commented Sep 11, 2023 •

edited

Loading

danielabrozzoni left a comment

448-OG commented Sep 14, 2023

danielabrozzoni commented Sep 14, 2023

448-OG commented Sep 15, 2023

vladimirfomene Sep 15, 2023

448-OG Sep 15, 2023

notmandatory Nov 13, 2023

vladimirfomene Sep 15, 2023

448-OG Sep 15, 2023

LLFourn Oct 6, 2023

vladimirfomene Sep 15, 2023

LLFourn Oct 6, 2023

448-OG commented Sep 15, 2023

danielabrozzoni left a comment

448-OG commented Sep 15, 2023

danielabrozzoni commented Oct 4, 2023

448-OG commented Oct 5, 2023

nondiremanuel commented Nov 7, 2023

LLFourn commented Nov 10, 2023

notmandatory commented Jan 16, 2024 •

edited

Loading

448-OG commented Feb 27, 2024

rename scan to scan_with_keychain #1117

rename scan to scan_with_keychain #1117

Conversation

448-OG commented Sep 11, 2023 • edited Loading

Notes to the reviewers

Checklists

All Submissions:

Bugfixes:

danielabrozzoni left a comment

Choose a reason for hiding this comment

448-OG commented Sep 14, 2023

danielabrozzoni commented Sep 14, 2023

448-OG commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

448-OG commented Sep 15, 2023

danielabrozzoni left a comment

Choose a reason for hiding this comment

448-OG commented Sep 15, 2023

danielabrozzoni commented Oct 4, 2023

448-OG commented Oct 5, 2023

nondiremanuel commented Nov 7, 2023

LLFourn commented Nov 10, 2023

notmandatory commented Jan 16, 2024 • edited Loading

448-OG commented Feb 27, 2024

448-OG commented Sep 11, 2023 •

edited

Loading

notmandatory commented Jan 16, 2024 •

edited

Loading