Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 3 commits
1c4af14
c6e5ba2
b2e1722
79e12b3
cd598db
fc56739
c10f903
45b5504
946bb62
12cc842
4e2804d
18c2b60
d59d982
d67a607
12275bc
e8f32ea
59a96f2
1534839
d90bc5d
4fb8679
0ce2ed8
88cd056
781901f
ddd05cf
baaa7ba
8808af6
b44da09
5febc73
ec4b7d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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-rsenv
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 withenv
in some way, and it turned out it worked, then I'd change it to: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 instellar 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 👌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 handlingSTELLAR_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 handleSTELLAR_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.