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 electrum support #19

Merged
merged 1 commit into from
Mar 19, 2024
Merged

add electrum support #19

merged 1 commit into from
Mar 19, 2024

Conversation

nicbus
Copy link
Contributor

@nicbus nicbus commented Mar 11, 2024

This PR adds support for Electrum as indexer.

bp-wallet has been modified so:

  • a new electrum feature is available, adding required dependencies
  • a new electrum indexer has been added
  • a new any indexer and error have been added

bp-util has been modified so:

  • a new --electrum <URL> option can be specified
  • only one of --electrum and --esplora can be specified
  • the short -e version for esplora has been dropped
  • if an electrum URL is specified, electrum is used as indexer
  • if an esplora URL or none is specified, esplora is used as indexer

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 268 lines in your changes are missing coverage. Please review.

Project coverage is 0.0%. Comparing base (8552e16) to head (35249ab).

Files Patch % Lines
src/indexers/electrum.rs 0.0% 224 Missing ⚠️
src/indexers/any.rs 0.0% 41 Missing ⚠️
src/indexers/esplora.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master    #19    +/-   ##
======================================
  Coverage     0.0%   0.0%            
======================================
  Files           9     11     +2     
  Lines         932   1198   +266     
======================================
- Misses        932   1198   +266     
Flag Coverage Δ
rust 0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Mar 17, 2024

Great work. However, I dislike the idea of adding rust-bitcoin to the dependency list so much that I even did a refactoring of the electrum client to use BP libs instead of it: https://github.com/BP-WG/bp-electrum-client

rust-bitcoin takes so long to compile and breaks so much with each release... I know it's behind the feature gate, but in reality, we use --all-features in tests etc, so it will blow up the CI times at least.

Can you please update the dependency to use refactored electrum-client? It's API have not changed and will should work once you get rid of all (now unneeded) conversions to rust-bitcoin structures.

@nicbus nicbus marked this pull request as draft March 18, 2024 16:29
@nicbus
Copy link
Contributor Author

nicbus commented Mar 18, 2024

Can you please update the dependency to use refactored electrum-client? It's API have not changed and will should work once you get rid of all (now unneeded) conversions to rust-bitcoin structures.

Refactored to use bp-electrum-client in place of electrum-client and dropping rust-bitcoin (and hex).

I set the PR as draft as IMO it makes sense to wait for a release of bp-electrum-client and the merge of BP-WG/bp-core#82 before proceeding with this one.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

LGTM, will do a more thorough review later. Since you confirm that the refactored electrum client works and does everything it is expected I will release it now

@dr-orlovsky
Copy link
Member

bp-electrum-client v0.11.0-beta.5 released

@nicbus
Copy link
Contributor Author

nicbus commented Mar 19, 2024

updated to use the released bp-electrum

@nicbus nicbus marked this pull request as ready for review March 19, 2024 15:54
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 35249ab

@dr-orlovsky dr-orlovsky merged commit 690d8f6 into BP-WG:master Mar 19, 2024
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants