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/new client with headers #13

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented Sep 27, 2024

What

Adds the ability to pass in headers to the HTTP client that is created in this client.

Companion cli PR: stellar/stellar-cli#1618

Why

Closes stellar/stellar-cli#1583

In order to allow for passing in headers to rpc providers, we need to build the HTTP client used in this client to include those headers.

Known limitations

N/A

@@ -698,11 +686,48 @@ impl Client {
Ok(client)
}

/// Create a new client with additional headers
/// # Errors
pub fn new_with_headers(base_url: &str, additional_headers: HeaderMap) -> Result<Self, Error> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Slack, we talked about creating an init fn that was more future-proof than this, that allowed for passing in a function that could customize the client as it was being created. The issues I'm running into with that are:

  1. The http_client field on this rpc Client is private and so we cannot get or set it from outside of this file. This is a pretty straightforward fix in that we could make that field public, or create a setter method. But do we want to expose the http_client outside of the rpc-client?
  2. The headers field on the HttpClient (created by jsonrpsee crate) is also private, so we wouldn't be able to just update the headers on the http_client, but instead need to rebuild a new http_client.

If we did create an init fn that took a fn as an arg, it would look something like the following. We'd also need to add http_client_setter:

    pub fn set_http_client(&mut self, http_client: HttpClient) {
        self.http_client = Arc::new(http_client);
    }

    pub fn init<F>(base_url: &str, f: F) -> Result<Self, Error>
    where
        F: FnOnce(Self) -> Self,
    {
        let client = Self::new(base_url)?;
        Ok(f(client))
    }

And then the cli code would end up looking something like this...

 pub fn new(network: Network) -> Result<Self, Error> {

       // get the additional headers from the network config

        let customize =  |mut client: rpc::Client| -> rpc::Client {
            let base_url = client.base_url();
            let mut headers = rpc::Client::default_http_headers().clone();

            for (key, value) in additional_headers {
                headers.insert(key.unwrap(), value);
            }

            let http_client = HttpClientBuilder::default()
                    .set_headers(headers)
                    .build(&base_url).unwrap();
            client.set_http_client(http_client);

            client
       };
       
        let client = rpc::Client::init(&network.rpc_url, customize)?;
      
       Ok(Self { client })
}

I kind of feel like this solution would bit more complicated than what is in the PR already, while still only allowing us to customize the HTTP client. I'm also not sure we want to expose the HTTP client outside of the rpc-client. But I'm curious to hear what others think!

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.

CLI should support rpc providers which require an API key
1 participant