refactor(async): refactoring async client and minor documentation improvements #93

oleonardolima · 2024-08-21T22:29:43Z

Description

It builds on top of #95. Applies minor improvements on some docstrings, and Cargo.toml standard.

The main change is adding the common HTTP methods for the AsyncClient, it removes duplicated code from each Esplora API request, and follows the approach done for BlockingClient.

It makes it easier to extract these methods into an AsyncEsploraClient trait (to be done in another PR, initially done here 9cbc387), which the user can implement with any HTTP client of its choice. Also, makes it simpler to rebase and update the AsyncAnonymizedClient from #67.

Notes to the reviewers

It has some commits from #95, as it builds on top of it and should be merged afterward. Please let me know what you think about the proposed changes and approach.

Changelog notice

Applies minor improvements on documentation.
Add common get_response and post_request methods to AsyncClient, previously duplicated through the esplora API calls. It follows the approach done for BlockinClient.

Checklists

All Submissions:

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

coveralls · 2024-08-22T14:35:18Z

Pull Request Test Coverage Report for Build 10530778457

Details

104 of 123 (84.55%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+1.3%) to 88.792%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/async.rs	102	121	84.3%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	1	97.08%

Totals
Change from base Build 10514244019:	1.3%
Covered Lines:	1014
Relevant Lines:	1142

💛 - Coveralls

ValuedMammal

Light review ACK

src/async.rs

ValuedMammal · 2024-08-28T19:56:22Z

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

oleonardolima · 2024-08-30T12:56:03Z

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think

I think it could be helpful to get exposed in async/reqwest too, I'll check it.

ValuedMammal · 2024-08-30T13:49:26Z

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think

I think it could be helpful to get exposed in async/reqwest too, I'll check it.

Ah ok so maybe not necessary since you can already get a handle to the reqwest::Client?

oleonardolima · 2024-09-01T21:30:11Z

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think
I think it could be helpful to get exposed in async/reqwest too, I'll check it.

Ah ok so maybe not necessary since you can already get a handle to the reqwest::Client?

Yes, that's what I thought.

ValuedMammal

ACK 9cb662a

Overall this looks great.

In 9cb662a

refactor(async)!: add common GET and POST methods

Maybe I missed it but I didn't see how this change is API breaking in any way?

oleonardolima · 2024-09-03T14:51:42Z

ACK 9cb662a

Overall this looks great.

In 9cb662a

refactor(async)!: add common GET and POST methods

Maybe I missed it but I didn't see how this change is API breaking in any way?

Sweet! I updated the commit message, and add a new one applying the same style on other methods.

1a4d5cf chore(rust+clippy): bump `edition` to 2021, and add `.clippy.toml` (Leonardo Lima) b7636e8 fix(fmt): apply suggested fixes from `rustfmt` (Leonardo Lima) 3f2ca2f refactor(ci)!: add new `fmt` and `clippy` jobs (Leonardo Lima) 9f888c1 chore(deps): bump `actions/checkout` from v3 to v4 (Leonardo Lima) Pull request description:  ### Description This PR does some improvements on CI, these are some changes that I ended up doing on other refactoring and feature PRs (making them too convoluted), but had a specific CI scope so I'm moving them to a specific PR. This PR does: - bump the `actions/checkout@v3` to `actions/checkout@v4`. - adds two new jobs for `fmt` and `clippy` (clippy has been moved to a specific job). - fix the newly found `fmt` problems. - bump the rust edition to `2021`. - adds `.clippy.toml` file with `msrv=1.63.0`.  ### Notes to the reviewers I hope this PR reduces the scope convolution from the other ones #67 #93, and makes the review easier.  ### Changelog notice - Bump the `actions/checkout@v3` to `actions/checkout@v4`. - Adds two new jobs for `fmt` and `clippy` (clippy has been moved to a specific job). - Multiple fixes for the newly found `fmt` problems. - Bump the rust edition to `2021`. - Adds `.clippy.toml` file with `msrv=1.63.0`.   ### 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: ValuedMammal: ACK 1a4d5cf notmandatory: ACK 1a4d5cf Tree-SHA512: e693baeea112dffa12ccc576271f38f3188dc24669a70af7196e33e5eea08c5d82940792330682b8a4b1ec48ef98e1cbaa2f713736f393555744fdf44d79a26a

oleonardolima · 2024-09-04T13:24:24Z

I rebased this one on top of master, as #95 got merged. It should be ready to go now.

oleonardolima · 2024-09-04T14:16:14Z

fyi: I'm attempting to fix the code coverage CI step issue on #96.

ValuedMammal

ACK a660346

I made comments only if you plan to retouch

src/async.rs

- apply some standard on `Cargo.toml` deps. - minor docstring improvements, and fix missing docstrings.

- remove duplicated HTTP client code for handling GET and POST requests. - adds a few new methods to `AsyncClient` implementation, the new methods are responsible to handle common HTTP requests and parsing. It was previously duplicated throughout the Esplora API implementation, but now it follows the same approach already implemented for blocking client (`BlockingClient`).

evanlinjin · 2024-09-12T05:37:17Z

@oleonardolima since it seems we are planning to go down the #97 route, is this PR still relevant?

oleonardolima · 2024-09-12T13:50:36Z

@oleonardolima since it seems we are planning to go down the #97 route, is this PR still relevant?

I guess this one can still land if we'd like to cut a release with these improvements, and mainly with #98. Then, cut a new release specifically with the refactor of #97. 🤔

ValuedMammal

ACK 1103936

notmandatory

ACK 1103936

Overall this is a great cleanup and should be much easier to maintain.

@e1a0a0ea

b96270e feat(async,blocking)!: implement retryable calls (valued mammal) Pull request description: Based on #93 the PR implements retryable calls for request failure due to too many requests (429) or service unavailable (503). Inspired by #71 h/t @e1a0a0ea ### Notes I've added the field `max_retries` to the `Builder`. `max_retries` is also added to each of `AsyncClient`, `BlockingClient`. I added the dependency `async-std` in order to have async `sleep`. Instead of implementing a trait on the `Request` type as in #71, the approach is to add a method on the client that sends a get request to a url and returns the response after allowing for retries. I tested on the bdk `wallet_esplora_*` example crates against https://blockstream.info/testnet/api and it seemed to resolve the 429 issue. ACKs for top commit: oleonardolima: ACK b96270e notmandatory: tACK b96270e Tree-SHA512: 78124106959ba9a5cce58e343bbf30c29b4bc7e1ac434ba71eeb2e774d73ea003d0520139781062947ed27563748925c24998b2a5be450bc511bb2c7090a6682

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from bf3f412 to c13af74 Compare August 22, 2024 14:32

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch 3 times, most recently from 431c72c to 9cbc387 Compare August 22, 2024 21:17

oleonardolima mentioned this pull request Aug 23, 2024

chore: upgrade and bump some CI dependencies #95

Merged

3 tasks

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from 9cbc387 to ac64e04 Compare August 23, 2024 18:45

oleonardolima requested review from notmandatory and ValuedMammal August 26, 2024 20:23

oleonardolima self-assigned this Aug 26, 2024

oleonardolima marked this pull request as ready for review August 26, 2024 20:23

ValuedMammal reviewed Aug 28, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from ac64e04 to 9cb662a Compare September 1, 2024 21:32

oleonardolima requested a review from ValuedMammal September 1, 2024 21:33

ValuedMammal approved these changes Sep 2, 2024

View reviewed changes

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from 9cb662a to a61d978 Compare September 3, 2024 14:44

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from a61d978 to a660346 Compare September 4, 2024 13:23

oleonardolima changed the title ~~refactor(async)!: refactoring async client and minor documentation improvements~~ refactor(async): refactoring async client and minor documentation improvements Sep 4, 2024

ValuedMammal mentioned this pull request Sep 7, 2024

Implement retryable calls #98

Merged

ValuedMammal approved these changes Sep 7, 2024

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

src/async.rs Outdated Show resolved Hide resolved

oleonardolima added 2 commits September 9, 2024 14:56

chore(docs): minor improvements on docstrings

31dfa4b

- apply some standard on `Cargo.toml` deps. - minor docstring improvements, and fix missing docstrings.

oleonardolima added 2 commits September 9, 2024 14:56

chore(style): update new methods style

442789c

chore(style+docs): style and docs retouch

1103936

oleonardolima force-pushed the refactor/improve-docs-and-async-client-methods branch from a660346 to 1103936 Compare September 9, 2024 18:02

ValuedMammal approved these changes Sep 20, 2024

View reviewed changes

notmandatory approved these changes Sep 25, 2024

View reviewed changes

notmandatory added the documentation Improvements or additions to documentation label Sep 25, 2024

notmandatory merged commit ce6a635 into bitcoindevkit:master Sep 25, 2024
25 checks passed

oleonardolima deleted the refactor/improve-docs-and-async-client-methods branch September 25, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(async): refactoring async client and minor documentation improvements #93

refactor(async): refactoring async client and minor documentation improvements #93

oleonardolima commented Aug 21, 2024 •

edited

Loading

coveralls commented Aug 22, 2024 •

edited

Loading

ValuedMammal left a comment

ValuedMammal commented Aug 28, 2024

oleonardolima commented Aug 30, 2024

ValuedMammal commented Aug 30, 2024

oleonardolima commented Sep 1, 2024

ValuedMammal left a comment

oleonardolima commented Sep 3, 2024

oleonardolima commented Sep 4, 2024

oleonardolima commented Sep 4, 2024

ValuedMammal left a comment

evanlinjin commented Sep 12, 2024

oleonardolima commented Sep 12, 2024

ValuedMammal left a comment

notmandatory left a comment •

edited

Loading

refactor(async): refactoring async client and minor documentation improvements #93

refactor(async): refactoring async client and minor documentation improvements #93

Conversation

oleonardolima commented Aug 21, 2024 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

coveralls commented Aug 22, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10530778457

Details

💛 - Coveralls

ValuedMammal left a comment

Choose a reason for hiding this comment

ValuedMammal commented Aug 28, 2024

oleonardolima commented Aug 30, 2024

ValuedMammal commented Aug 30, 2024

oleonardolima commented Sep 1, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

oleonardolima commented Sep 3, 2024

oleonardolima commented Sep 4, 2024

oleonardolima commented Sep 4, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

evanlinjin commented Sep 12, 2024

oleonardolima commented Sep 12, 2024

ValuedMammal left a comment

Choose a reason for hiding this comment

notmandatory left a comment • edited Loading

Choose a reason for hiding this comment

oleonardolima commented Aug 21, 2024 •

edited

Loading

coveralls commented Aug 22, 2024 •

edited

Loading

notmandatory left a comment •

edited

Loading