-
Notifications
You must be signed in to change notification settings - Fork 94
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
use rsplitn for rpc info extraction in case passwords have @ #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just needs a rustfmt fix and then we can land it.
src/cli.rs
Outdated
@@ -45,11 +45,12 @@ pub(crate) fn parse_startup_args() -> Result<LdkUserInfo, ()> { | |||
return Err(()); | |||
} | |||
let bitcoind_rpc_info = env::args().skip(1).next().unwrap(); | |||
let bitcoind_rpc_info_parts: Vec<&str> = bitcoind_rpc_info.split("@").collect(); | |||
let mut bitcoind_rpc_info_parts: Vec<&str> = bitcoind_rpc_info.rsplitn(2,"@").collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and on the line below, rustfmt
demands a space after the ,
in the parameter list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know about this tool yet, thanks, should be fixed in the latest commit
src/cli.rs
Outdated
if bitcoind_rpc_info_parts.len() != 2 { | ||
println!("ERROR: bad bitcoind RPC URL provided"); | ||
return Err(()); | ||
} | ||
bitcoind_rpc_info_parts.swap(0,1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe more readable to just change the two lines that access bitcoind_rpc_info_parts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good suggestion, I tried this originally but wasn't sure if the reverse index access would be confusing, sounds like this is the better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you squash the commits down into one commit to keep the git history clean?
remove swap, fix rustfmt issue remove return
should be squashed now. |
@
being a character in password, the current behaviour will split n times depending on how many of the @ special character appears and then will fail on URL since there are not two vectors.