Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add wallet option to select which blockchain client to use - #37

Closed
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt
Closed

Add wallet option to select which blockchain client to use#37
notmandatory wants to merge 4 commits into
bitcoindevkit:masterfrom
notmandatory:blockchain_client_opt

Conversation

@notmandatory

Copy link
Copy Markdown
Member

Description

Added the --blockchain_client or -b wallet option so the user can explicitly select which blockchain client to use if multiple are available in the build. This will make adding new blockchain clients (such as #36) easier. Currently electrum is the default.
I also added a default esplora server url so the user doesn't need to specify one if selecting that client, which is how the electrum and compact_filters clients work.

The new wallet options looks like this (when all optional clients are enabled --features esplora,compact_filters):

bdk-cli-wallet 0.2.1-dev
Wallet mode
USAGE:
bdk-cli wallet [FLAGS] [OPTIONS] --descriptor <DESCRIPTOR> <SUBCOMMAND>
FLAGS:
-v, --verbose Adds verbosity, returns PSBT in JSON format alongside serialized
-h, --help Prints help information
-V, --version Prints version information
OPTIONS:
-n, --node <ADDRESS:PORT>...
Compact filters blockchain client peer full node IP address:port [default: 127.0.0.1:18444]
-b, --blockchain_client <BLOCKCHAIN_CLIENT>
Blockchain client protocol [default: electrum] [possible values: electrum, esplora, compact_filters]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
--conn_count <CONNECTIONS>
Compact filters blockchain client number of parallel node connections [default: 4]
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
--esplora_concurrency <ESPLORA_CONCURRENCY> Esplora blockchain client request concurrency [default: 4]
-e, --esplora <ESPLORA_URL>
Esplora blockchain client server url [default: https://blockstream.info/api/]
-p, --proxy <PROXY_ADDRS:PORT> Blockchain client SOCKS5 proxy
-r, --retries <PROXY_RETRIES> Blockchain client SOCKS5 proxy retries [default: 5]
-t, --timeout <PROXY_TIMEOUT> Electrum blockchain client SOCKS5 proxy timeout
-a, --proxy_auth <PROXY_USER:PASSWD> Blockchain client SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Electrum blockchain client server url [default: ssl://electrum.blockstream.info:60002]
-k, --skip_blocks <SKIP_BLOCKS>
Compact filters blockchain client skip initial blocks [default: 0]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

Notes to the reviewers

In the docs for the blockchain_client option it displays the default [electrum] even if that feature is not enabled and all possible blockchain clients are displayed even if only some are enabled. I couldn't find a nice way to fix this, but the cli will throw an error if a blockchain client is selected that wasn't configured as a feature in the build.

I also simplified the CHANGELOG to focus on what a user would see as a change while using the bin.

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
  • I've updated CHANGELOG.md

Bugfixes:

  • 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

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Thanks @notmandatory . This is a possible way to do it, but unfortunately, this also doesn't reduce our option arg namespace. So you will get the same conflicts with -b flag, as without it. Because electrum is on by default ( that turns on ProxyOpts too) even if we don't need it at all.

I think the blockchain selection flag is kinda redundant because we are selecting blockchain backend at the build time itself with --features flag.

I have tried around a few things and it seems to me that the easiest thing to do to solve all of our problems, is if we drop the default blockchain, and just specify a backend each time we build bdk-cli.

We can have the repl feature as default.

Without any blockchain feature bdk-cli will just do the repl things.

To have a full wallet we will specify a backend with --feature flag.

This will allow us to have the same arg option names for different blockchins. Because using feature guard removes the codes from the binary, so clap won't complain because for it those options don't exist.

And I think we can reasonably explain this in the usage docs too.

I have tried this manually, and it produces nice compact --help doc also only with the blockchain that is enabled. like this

--features rpc

OPTIONS:
-n, --rpc-node <ADDRESS:PORT>
Sets the full node address for rpc connection [default: 127.0.0.1:18443]
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-x, --skip-blocks <SKIP_BLOCKS> Optionally skip initial `skip_blocks` blocks
-A, --rpc-auth <USER:PASSWD>
Sets the rpc authentication username:password [default: admin:password]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main]

--features electrum

OPTIONS:
-c, --change_descriptor <CHANGE_DESCRIPTOR> Sets the descriptor to use for internal addresses
-d, --descriptor <DESCRIPTOR> Sets the descriptor to use for the external addresses
-p, --proxy <PROXY_ADDRS:PORT> Sets the SOCKS5 proxy for Blockchain backend
-r, --retries <PROXY_RETRIES> Sets the SOCKS5 proxy retries for the Electrum client [default: 5]
-t, --timeout <PROXY_TIMEOUT> Sets the SOCKS5 proxy timeout for the Electrum client
-a, --proxy-auth <PROXY_USER:PASSWD> Sets the SOCKS5 proxy credential
-s, --server <SERVER:PORT>
Sets the Electrum server to use [default: ssl://electrum.blockstream.info:60002]
-w, --wallet <WALLET_NAME> Selects the wallet to use [default: main] 

If this is something we wanna do, I can add it to my open PR. It's not a big change set.

@notmandatory

Copy link
Copy Markdown
MemberAuthor

@rajarshimaitra OK if this doesn't solve the conflicting params issue I'll close it and open a new one with your one blockchain client at a time solution. I'll add some docs and a compile error if users try building with two blockchain client features enabled. I'd rather keep this change as a separate PR to keep it simple to review.

@notmandatory
notmandatory deleted the blockchain_client_opt branch August 4, 2021 22:57
@rajarshimaitra

Copy link
Copy Markdown
Contributor

Yes that makes sense. Better to do it via separate PR.

Sign up for freeto 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

@notmandatory@rajarshimaitra