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

Feat/add rpc header arg #1618

Merged
merged 43 commits into from
Oct 14, 2024
Merged

Feat/add rpc header arg #1618

merged 43 commits into from
Oct 14, 2024

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented Sep 24, 2024

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 network

Screenshot 2024-09-30 at 10 08 47 AM

using env vars for multiple headers for adding a network

Screenshot 2024-09-30 at 10 09 53 AM

using multiple —rpc-headers args for using the rpc provider

Screenshot 2024-09-30 at 10 14 52 AM

using the env var for multiple headers for using the rpc provider

Screenshot 2024-09-30 at 10 18 04 AM

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

Copy link
Member

@leighmcculloch leighmcculloch left a 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. 😅

cmd/soroban-cli/src/config/network.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/config/network.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/config/network.rs Outdated Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Couple requests inline.

cmd/soroban-cli/src/config/network.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/rpc_client.rs Outdated Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I think this is good 👍🏻. Thanks for navigating both the rpc and cli changes. I've merged the rpc change, and bumped the ref in this pr to match it so this should be good to merge.

@elizabethengelman a couple minor asks for a follow up:

1️⃣ The error messages when passing invalid values of keys / values to the header could do a better job of helping someone know what to correct.

For example, it's easy to naively think you can just plop in an api key:

$ stellar network add lfg --rpc-header myapikeyyay
error: invalid value 'myapikeyyay' for '--rpc-header <RPC_HEADERS>': invalid header: Missing header value: myapikeyyay

For more information, try '--help'.

I understand what the error above needs of me, but I think devs without context won't.

I think we should tell them that the header should be in the format key: value.

2️⃣ White space could be ignored.

For example, it's common for folks to state headers with whitespace around the ::

$ stellar network add lfg \
    --rpc-url http://example.org \
    --network-passphrase "Example Network" \
    --rpc-header 'Authorization: myapikeyyay'

Which renders later with the whitespace retained:

❯ stellar network ls -l
Local "/Users/lfg/.soroban/network/lfg.toml"
Name: lfg
Network {
    rpc_url: "http://example.org",
    rpc_headers: [
        (
            "Authorization",
            " myapikeyyay",
        ),
    ],
    network_passphrase: "Example Network",
}

@@ -44,6 +53,17 @@ pub struct Args {
help_heading = HEADING_RPC,
)]
pub rpc_url: Option<String>,
/// RPC Header(s) to include in requests to the RPC provider
#[arg(
long = "rpc-header",
Copy link
Member

@leighmcculloch leighmcculloch Oct 14, 2024

Choose a reason for hiding this comment

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

@elizabethengelman while trying out the pr locally I made this tweak, I renamed the option --rpc-header (no plural) so that at the command line it's more obvious that using the option is intended to be a single arg per use, so it looks like this:

stellar ... --rpc-header A:B --rpc-header C:D

So that folks don't think they can do:

stellar ... --rpc-headers A:B C:D

It looked like you had --rpc-header in the help docs, so I think that was what you were intending to do in any case. Lmk if I misunderstood or if this doesn't fit with the overall plan here.

Comment on lines +142 to +145
let (key, value) = header_components
.map(str::trim)
.next_tuple()
.ok_or_else(|| Error::InvalidHeader)?;
Copy link
Member

Choose a reason for hiding this comment

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

I pushed a change to trim values, and to tweak the error message. The error generated by this line looks like this now:

$ stellar network add lfg \
    --rpc-url http://example.org \
    --network-passphrase "Example Network" \
    --rpc-header 'Authorization'
error: invalid value 'Authorization' for '--rpc-header <RPC_HEADERS>': invalid HTTP header: must be in the form 'key:value'

I removed passing in the string into the error, because the clap error handler will print out the value they passed in at the start of the error message anyway.

@leighmcculloch leighmcculloch enabled auto-merge (squash) October 14, 2024 03:42
@leighmcculloch leighmcculloch merged commit 9cc8fcd into main Oct 14, 2024
23 checks passed
@leighmcculloch leighmcculloch deleted the feat/add-rpc-header-arg branch October 14, 2024 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CLI should support rpc providers which require an API key
3 participants