search

stellar / stellar-cli

CLI for Stellar developers
66 stars 58 forks source link

Consolidate HTTP client libraries to reduce complexity #1629

Open overcat opened 1 day ago

overcat commented 1 day ago

What problem does your feature solve?

Currently, the project is using multiple HTTP client libraries:

This diversity of HTTP clients may lead to several issues:

What would you like to see?

We should consider consolidating our HTTP client usage to a single library. This would simplify the codebase, reduce potential inconsistencies, and make maintenance easier.

What alternatives are there?

N/A

leighmcculloch commented 1 day ago

Where are the different libraries being used?

overcat commented 1 day ago

Where are the different libraries being used?

The following is the code in the project

ureq: https://github.com/stellar/stellar-cli/blob/c8c3871ef3a409328d9b0561f87d55c00bd41071/cmd/soroban-cli/src/commands/contract/init.rs#L22 hyper: https://github.com/stellar/stellar-cli/blob/c8c3871ef3a409328d9b0561f87d55c00bd41071/cmd/soroban-cli/src/commands/snapshot/create.rs#L413 reqwest (stellar-ledger): https://github.com/stellar/stellar-cli/blob/c8c3871ef3a409328d9b0561f87d55c00bd41071/cmd/crates/stellar-ledger/tests/utils/emulator_http_transport.rs#L7

leighmcculloch commented 1 day ago

Thanks.

hyper is also in use in the stellar/rs-stellar-rpc-client repo (the jsonrpsee http client lib uses hyper) which the cli imports.

reqwest also uses hyper.

leighmcculloch commented 1 day ago

If we were to make a change here, I'd suggest replacing ureq and reqwest with hyper.

overcat commented 1 day ago

If we were to make a change here, I'd suggest replacing ureq and reqwest with hyper.

I saw it written on https://github.com/hyperium/hyper:

If you are looking for a convenient HTTP client, then you may wish to consider reqwest.

If there are no special reasons, using reqwest seems like a good choice. Although it relies on hyper, we won't be using hyper directly in the code.

leighmcculloch commented 1 day ago

That sounds fine too. In which case we could update the ureq case and leave everything else.