-
Notifications
You must be signed in to change notification settings - Fork 66
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
Feat/add rpc header arg #1618
base: main
Are you sure you want to change the base?
Feat/add rpc header arg #1618
Conversation
41ca940
to
81ce815
Compare
eda3a8a
to
b2e1722
Compare
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.
Couple thoughts. I realise this PR is incomplete and doesn't yet use the header. 😅
/// Optional RPC provider api key headers | ||
#[arg( | ||
long = "rpc-header", | ||
requires = "rpc_url", | ||
env = "STELLAR_RPC_HEADER", | ||
help_heading = HEADING_RPC, | ||
)] | ||
pub rpc_header: Option<String>, |
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.
I think this should probably be a multi-value key, therefore be a Vec<String>
, but as far as I know the clap-rs env
feature doesn't work well with it. We'd have to try it out.
Given all use cases we have only need a single header at this time, then as it is looks good, but if you wanted to do a mini timeboxed exploration on whether Vec<String>
is supported with env
in some way, and it turned out it worked, then I'd change it to:
/// Optional RPC provider api key headers | |
#[arg( | |
long = "rpc-header", | |
requires = "rpc_url", | |
env = "STELLAR_RPC_HEADER", | |
help_heading = HEADING_RPC, | |
)] | |
pub rpc_header: Option<String>, | |
/// Optional RPC provider api key headers | |
#[arg( | |
long = "rpc-header", | |
num_args(0..), | |
requires = "rpc_url", | |
env = "STELLAR_RPC_HEADERS", | |
help_heading = HEADING_RPC, | |
)] | |
pub rpc_header: Vec<String>, |
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.
I like the idea of making this a vec of strings - it seems like there's a possibility that users may want to pass multiple headers to their rpc requests.
I think that we are doing something similar to this with the --with-example
flag in stellar contract init
, that allows for passing multiple values like this: stellar contract init --with-example alloc --with-example auth
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.
Just re-reading your comment, and I missed the env
part the first time. I'll timebox looking into this today 👌
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
env = "STELLAR_RPC_HEADER", | ||
help_heading = HEADING_RPC, | ||
)] | ||
pub rpc_header: Option<String>, |
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.
It might be better to define it as Vec<(String, String)>
. This would help allow users to more clearly pass in multiple headers, and it would also be simpler when handling STELLAR_RPC_HEADER
env.
The code may be helpful: https://github.com/lightsail-network/stellar-cli/blob/5d93640d2d2f629c91cabd0340ec7f59bac6ebd9/cmd/soroban-cli/src/config/network.rs#L122-L152
(This is the code I wrote a few days ago, and at that time I didn't realize there was already an existing PR here.)
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 @overcat! I agree that rpc_header
should be a vec, and I like that you made it a vec of tuples instead! 👌 That definitely makes it easier to handle STELLAR_RPC_HEADER
. 🎉
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.
Does that work with env vars though? I don't think the clap env feature knows what to do with multiple values inside the env. Or can you share an example of what hte cli and env UX becomes with this?
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.
Hi @leighmcculloch, I think it is supported, please check the screenshot.
Because we used value_delimiter = '\n'
, it will need to be line-separated, but you can use a different delimiter based on your needs.
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.
Adding to this, I've put screenshots in the pr description showing that multiple headers work for adding a network and using the configured rpc provider.
help_heading = HEADING_RPC, | ||
num_args = 1, | ||
action = clap::ArgAction::Append, | ||
value_delimiter = ',', |
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.
For more general purposes, we may need to consider whether we should use ,
as a separator, because , is likely to appear in the value. For example, my User-Agent contains it: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36
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.
I like the idea of using new line as you did in the other example, given that new lines are the natural delimiter of headers. New lines may create issues in some environments for env vars, but also, we expect only a single header to be used so I'm not sure it's worth dwelling on that.
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 call, I can push up a fix for that today.
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.
Couple requests inline.
let header_components = header.split(':').collect::<Vec<&str>>(); | ||
if header_components.len() != 2 { | ||
return Err(Error::InvalidHeader(format!( | ||
"Missing a header name and/or value: {header}" | ||
))); | ||
} | ||
|
||
let key = header_components[0].trim().to_string(); | ||
let value = header_components[1].trim().to_string(); |
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.
I think technically :
can appear in the header value, so splitting and exploding that into a Vec on every :
is probably not a resilient, as splitting on the first :
and otherwise keeping the rest of the header intact.
It should be a small change to something like:
let header_components = header.split(':').collect::<Vec<&str>>(); | |
if header_components.len() != 2 { | |
return Err(Error::InvalidHeader(format!( | |
"Missing a header name and/or value: {header}" | |
))); | |
} | |
let key = header_components[0].trim().to_string(); | |
let value = header_components[1].trim().to_string(); | |
let header_components = header.split(':'); | |
let Some(key) = header_components.next() else { | |
return Err(Error::InvalidHeader(format!( | |
"Missing a header name and/or value: {header}" | |
))); | |
}; | |
let value = header_components.remaining(); |
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.
That makes sense to me - the one piece that I'm noticing is that remainder
is only available with the unstable nightly build at the moment. Though, I think we can get a similar effect by just using next
again.
https://doc.rust-lang.org/std/str/struct.Split.html#impl-Split%3C'a,+P%3E
cmd/soroban-cli/src/rpc_client.rs
Outdated
#[derive(Debug)] | ||
pub struct RpcClient { | ||
client: rpc::Client, | ||
} | ||
|
||
impl RpcClient { | ||
pub fn new(network: &Network) -> Result<Self, Error> { | ||
let mut additional_headers = HeaderMap::new(); | ||
for header in &network.rpc_headers { | ||
let header_name = HeaderName::from_bytes(header.0.as_bytes())?; | ||
let header_value = HeaderValue::from_str(&header.1)?; | ||
|
||
additional_headers.insert(header_name, header_value); | ||
} | ||
|
||
let client = rpc::Client::new_with_headers(&network.rpc_url, additional_headers)?; | ||
Ok(Self { client }) | ||
} | ||
} | ||
|
||
// implementing Deref in order to delegate all method calls to `rpc::Client` | ||
impl Deref for RpcClient { | ||
type Target = rpc::Client; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.client | ||
} | ||
} | ||
|
||
// implementing DerefMut for mutable access | ||
impl DerefMut for RpcClient { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
&mut self.client | ||
} | ||
} |
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.
The use of Deref to sub one type another one is misusing the Deref trait a bit. It is a pattern used at times, but also considered an antipattern if the benefits don't truly outweigh the cons. There's some conversation about it here: https://rust-unofficial.github.io/patterns/anti_patterns/deref.html
I don't think we need to use this pattern here. Can I suggest we instead do something a little more boring, and replace this:
- let client = Client::new(&network.rpc_url)?;
+ let client = RpcClient::new(network)?;
with:
- let client = Client::new(&network.rpc_url)?;
+ let client = network.rpc_client()?;
where rpc_client
is a function on Network
that returns an rpc::Client
and constructs it from the necessary parameters? That approach wouldn't require any new types, or magic derefing of types.
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 to know that using Deref like this is an antipattern. Thanks for the resource!
And I like this refactor - definitely simpler. I'll get a change pushed up shortly.
647efa2
to
b13f732
Compare
What
Adds a
--rpc-headers
argument to allow users to pass in an auth header, so that RPC providers that require API keys can be used with the CLI.Companion rpc-client PR: stellar/rs-stellar-rpc-client#13
Here are some screenshots showing that this change works for multiple headers, with both adding a new network, and using a configured rpc provider:
using multiple
—rpc-headers
args for adding a networkusing env vars for multiple headers for adding a network
using multiple —rpc-headers args for using the rpc provider
using the env var for multiple headers for using the rpc provider
Why
This allows users to use one of the rpc providers that require an API passed in via a header.
Closes #1583
Known limitations
This change requires this change from the rpc-client: stellar/rs-stellar-rpc-client#13