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

Merged
merged 7 commits into from
Oct 14, 2024
Merged

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

@elizabethengelman elizabethengelman force-pushed the feat/new-client-with-headers branch from 0fd159b to a4b0a92 Compare September 27, 2024 16:32
@elizabethengelman elizabethengelman marked this pull request as ready for review September 30, 2024 14:58
@elizabethengelman elizabethengelman force-pushed the feat/new-client-with-headers branch from c9c5a95 to a4b0a92 Compare October 3, 2024 15:21
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.

One ask, otherwise I think this is good as is.

src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@elizabethengelman
Copy link
Contributor Author

elizabethengelman commented Oct 7, 2024

@leighmcculloch

It seems that the main branch of this repo is incompatible with main on the CLI because of the xdr update.

Should we wait until protocol 22 to merge in the accompanying CLI PR? Or, does it make sense to have this PR target v21.4.0 and release 21.4.1 here in the rpc client instead so we can get this API key support out there sooner?

@leighmcculloch
Copy link
Member

@janewang What's the urgency on API support?

If we can delay on API support, I'd rather keep backports at this point to patch releases fixing bugs, rather than new features, just to reduce the divergence in code that we have to manage. But if API key support is urgent then lets target v21 here, and we can backport the CLI change.

@janewang
Copy link
Collaborator

janewang commented Oct 9, 2024

@leighmcculloch The API support change can wait for v22. It's asked by the community but I think it could wait for a few weeks

@elizabethengelman
Copy link
Contributor Author

@leighmcculloch @janewang, sounds good to delay support for this until v22. I'll make sure to keep this branch up-to-date with main in the meantime.

@elizabethengelman
Copy link
Contributor Author

Actually, I suppose that means we can merge this in now - does that sound right @leighmcculloch ?

src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Willem Wyndham <[email protected]>
@leighmcculloch leighmcculloch merged commit 7554d4c into main Oct 14, 2024
6 checks passed
@leighmcculloch leighmcculloch deleted the feat/new-client-with-headers branch October 14, 2024 02:39
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
6 participants