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 remote rpc support #1785

Closed
wants to merge 5 commits into from
Closed

Conversation

eddiewang
Copy link

Never written rust before, so might need some polish here before it's merge ready.
Mainly quality of life improvements for users that aren't using a localhost Bitcoin node.

  • Adds rpc_user and rpc_pass arguments to the binary
  • Uses rpc_user / rpc_pass if arguments exist, otherwise defaults back to cookie mode
  • Ensures all wallet commands properly pass the /wallet/ even if the rpc_url is set

@ghost
Copy link

ghost commented Feb 17, 2023

Concept ACK

What happens if only username or password is provided and one argument is missing?

@eddiewang
Copy link
Author

Right now, the logic will default to cookie unless both arguments are provided.
My understanding of how .is_none() works is that if the value is set, even if it's to an empty string "", it should still return False.

If my assumption is correct, I think the desired state is to explicitly pass in both the user and pass in order to activate this mode, defaulting to the cookie approach otherwise.

Comment on lines +147 to +150
let auth = if !rpc_pass.is_none() && !rpc_user.is_none() {
let ruser = rpc_user.unwrap();
let rpass = rpc_pass.unwrap();
log::info!("Using RPC credentials from command line {ruser}");
Copy link

Choose a reason for hiding this comment

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

rpc_pass and rpc_user are Option<String>, which you can get at in a nicer way than checking ! is_none() then calling unwrap() (calling unwrap is generally (strongly) discouraged), by using if let and destructuring:

let number = Some(7);
let another = Some(12);
let missing: Option<i32> = None;

if let (Some(i), Some(j)) = (number, another) {
    println!("Matched {:?} and {:?}!", i, j);
}
// ^ Prints "Matched 7 and 12!"

if let (Some(i), Some(j)) = (number, missing) {
    println!("Matched {:?} and {:?}!", i, j);
} else {
    println!("Didn't match");
}
// ^ Prints "Didn't match"

so something like

Suggested change
let auth = if !rpc_pass.is_none() && !rpc_user.is_none() {
let ruser = rpc_user.unwrap();
let rpass = rpc_pass.unwrap();
log::info!("Using RPC credentials from command line {ruser}");
if let (Some(rpass), Some(ruser)) = (rpc_pass, rpc_user) {
log::info!("Using RPC credentials from command line {ruser}");

(Caveat I'm also a Rust noob so if the compiler complains, sorry!)

@AngusP
Copy link

AngusP commented Mar 3, 2023

Also note #1527 was trying to add this as well

@eddiewang eddiewang force-pushed the add-remote-rpc branch 2 times, most recently from 93cd6d8 to 45b3715 Compare March 6, 2023 19:34
@raphjaph
Copy link
Collaborator

raphjaph commented May 1, 2023

We added this check out PR #1527

@raphjaph raphjaph closed this May 1, 2023
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.

4 participants