Skip to content
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

Add cli esplora example #1040

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Conversation

evanlinjin
Copy link
Member

Description

This PR builds on top of #1034 and adds a cli-example for our esplora chain-src crate.

Notes to the reviewers

Don't merge this until #1034 is merged. The only relevant commit is 5ff0412.

Changelog notice

  • Add cli-example for esplora.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • 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

@evanlinjin evanlinjin self-assigned this Jul 20, 2023
@evanlinjin evanlinjin added this to the 1.0.0-alpha.2 milestone Jul 20, 2023
@evanlinjin evanlinjin force-pushed the esplora_example branch 2 times, most recently from 2bacc1f to f1b7da3 Compare July 28, 2023 06:10
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
@LLFourn
Copy link
Contributor

LLFourn commented Aug 3, 2023

Thanks. In general this example could do with a lot more comments explaining what's happening and why. With examples it's better to be on the side of too many comments rather than too little.

@notmandatory
Copy link
Member

notmandatory commented Aug 8, 2023

While testing I found a bug in esplora blocking_ext.rs and also exists in async_ext.rs. When syncing the stop_gap is set to usize::MAX and the update_tx_graph functions fail on the addition here:

                if last_index > last_active_index.map(|i| i + stop_gap as u32) {
                    break;
                }

Easy enough to fix:

                if last_index > last_active_index.map(|i| i.saturating_add(stop_gap as u32)) {
                    break;
                }

@LLFourn
Copy link
Contributor

LLFourn commented Aug 15, 2023

LGTM but i think we are waiting on #1065 before this one.

@danielabrozzoni
Copy link
Member

I just pushed:

While testing I found a bug in esplora blocking_ext.rs and also exists in async_ext.rs. When syncing the stop_gap is set to usize::MAX and the update_tx_graph functions fail on the addition here:

@notmandatory I don't understand how to reproduce this. Does it happen with every call to sync?

LLFourn added a commit to LLFourn/bdk that referenced this pull request Aug 25, 2023
LLFourn addressing his comments on bitcoindevkit#1040
LLFourn added a commit to evanlinjin/bdk that referenced this pull request Aug 25, 2023
LLFourn addressing his comments on bitcoindevkit#1040
@LLFourn
Copy link
Contributor

LLFourn commented Aug 25, 2023

I've made some requested changes. Rather than go back and forth too much I made a PR to show what I had in mind: evanlinjin#6

(note I accidentally push to this PR branch at some point. Ignore that. I've reset it back to how it was).

example-crates/example_esplora/src/main.rs Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
example-crates/example_esplora/src/main.rs Outdated Show resolved Hide resolved
danielabrozzoni pushed a commit to evanlinjin/bdk that referenced this pull request Aug 30, 2023
@danielabrozzoni
Copy link
Member

Changes from my last push (019fa4c):

@evanlinjin I'd say this is ready to merge. Can you review the commits pushed on top, squash them, and ACK? Then I'll ACK as well :)

danielabrozzoni pushed a commit to evanlinjin/bdk that referenced this pull request Aug 31, 2023
@danielabrozzoni
Copy link
Member

Rebased after #1048 in order to fix CI

Copy link
Member Author

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this forward @danielabrozzoni. Just a really small nit. Looks good otherwise.

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member Author

I also tested everything. Works fine.

evanlinjin and others added 2 commits August 31, 2023 13:07
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f41cc1c

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f41cc1c

I was also not able to reproduce the sync error I saw before.

@notmandatory
Copy link
Member

I'm dismissing @LLFourn blocking review so this can be merged since it looks like comments are all addressed.

@notmandatory notmandatory dismissed LLFourn’s stale review August 31, 2023 19:55

All comments addressed

@notmandatory notmandatory merged commit 8321aaa into bitcoindevkit:master Aug 31, 2023
@notmandatory
Copy link
Member

ACK f41cc1c

I was also not able to reproduce the sync error I saw before.

I found the sync error mentioned above, created tests to repro and added fix in #1110 .

danielabrozzoni added a commit that referenced this pull request Sep 12, 2023
2090021 refactor: rename methods in EsploraExt and EsploraExtAsync (Vladimir Fomene)

Pull request description:

  ### Description

  This PR fixes #1058. Built on top of #1040

  ### 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

ACKs for top commit:
  realeinherjar:
    ACK 2090021
  danielabrozzoni:
    ACK 2090021 - code looks good, `example_esplora` and `wallet_esplora_blocking` work just fine.

Tree-SHA512: 5b5285aaa67a0c4e8174e480cceec7d934ec04a74d81740e1c82f6b8673c3e3d9c676dc43257a170320089efe2d3cb0d33123b4a395fc3e7fec63f85bdf70c79
@notmandatory notmandatory mentioned this pull request Oct 11, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants