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.

Sign up for GitHub

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

Jump to bottom

add validate domain option to electrum #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add validate domain option to electrum #155

wants to merge 1 commit into from

Conversation

dspicher
Copy link
Contributor

@dspicher dspicher commented Jun 9, 2023 •
edited
Loading

Description

Added the validate_domain option to Electrum arguments.

Since #151 seems dormant, I took another stab at this.

Fixes #134.

Changelog notice

Added the validate_domain option to Electrum arguments.

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

@dspicher dspicher changed the title Added validate_domain Electrum option Draft: add validate domain option to electrum Jun 9, 2023
@dspicher dspicher force-pushed the validate-domain branch 2 times, most recently from 985ff9f to c6c44e1 Compare June 9, 2023 08:47
@dspicher dspicher changed the title Draft: add validate domain option to electrum add validate domain option to electrum Jun 9, 2023
@notmandatory
Copy link
Member

Take a look at the utils.rs file and on line 403 you need to use this new param you're adding.

notmandatory
notmandatory reviewed Jun 16, 2023
View reviewed changes
src/commands.rs
@@ -320,6 +320,10 @@ pub struct ElectrumOpts {
default_value = "10"
)]
pub stop_gap: usize,

/// Enable domain validation when connecting to Electrum servers.
#[clap(name = "VALIDATE_DOMAIN", long = "validate_domain")]
Copy link
Member

Choose a reason for hiding this comment

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

In the utils.rs the default for this value is true, as you have it here the user will be required to set the value to true or false. It's better if you set the clap default_value to true then the user only needs to do something if they want it to be false.

Copy link
Contributor Author

@dspicher dspicher Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done. Also added a v short option.

@dspicher
Copy link
Contributor Author

Take a look at the utils.rs file and on line 403 you need to use this new param you're adding.

Ah yeah, makes sense. Fixed.

@dspicher dspicher force-pushed the validate-domain branch 5 times, most recently from 719c23b to 976373d Compare June 16, 2023 14:44
notmandatory
notmandatory previously approved these changes Jun 27, 2023
View reviewed changes
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 976373d

@notmandatory notmandatory self-requested a review June 27, 2023 18:46
@notmandatory notmandatory dismissed their stale review June 27, 2023 18:47

found problem after approving

notmandatory
notmandatory requested changes Jun 27, 2023
View reviewed changes
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.

I tested this and realized the approach won't work since clap treats bool as a "flag" with no way to set to false if the default is true. See comments for how to fix.

src/commands.rs Outdated
@@ -320,6 +320,14 @@ pub struct ElectrumOpts {
default_value = "10"
)]
pub stop_gap: usize,

/// Enable domain validation when connecting to Electrum servers.
Copy link
Member

Choose a reason for hiding this comment

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

Need to make it an Option and set the default in the code, see below.

Suggested change
/// Enable domain validation when connecting to Electrum servers.
/// Enable domain validation when connecting to Electrum servers [default: true]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/commands.rs Outdated
#[clap(
name = "VALIDATE_DOMAIN",
long = "validate_domain",
default_value = "true"
Copy link
Member

Choose a reason for hiding this comment

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

Remove default.

Suggested change
default_value = "true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/commands.rs Outdated
long = "validate_domain",
default_value = "true"
)]
pub validate_domain: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Change type to Option:

Suggested change
pub validate_domain: bool,
pub validate_domain: Option<bool>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/utils.rs Outdated
@@ -400,7 +400,7 @@ pub(crate) fn new_blockchain(
retry: wallet_opts.proxy_opts.retries,
timeout: wallet_opts.electrum_opts.timeout,
stop_gap: wallet_opts.electrum_opts.stop_gap,
validate_domain: true,
validate_domain: wallet_opts.electrum_opts.validate_domain,
Copy link
Member

Choose a reason for hiding this comment

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

Default to true here:

Suggested change
validate_domain: wallet_opts.electrum_opts.validate_domain,
validate_domain: wallet_opts.electrum_opts.validate_domain.unwrap_or(true),

You'll also need to fix the corresponding tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory requested changes

Assignees

@dspicher dspicher

Labels
None yet
Projects
BDK-CLI Roadmap
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validate_domain Electrum option
2 participants
@dspicher @notmandatory