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

Drop --rpc-url and --network-passphrase. #1616

Closed
wants to merge 5 commits into from
Closed

Conversation

fnando
Copy link
Member

@fnando fnando commented Sep 18, 2024

What

Close #1610

Why

To simplify our codebase and improve the CLI usability by having only one way of setting the network.

Known limitations

N/A

@fnando fnando force-pushed the network-alias-everywhere branch 10 times, most recently from e6cc8d7 to ace5632 Compare September 19, 2024 00:37
@fnando
Copy link
Member Author

fnando commented Sep 19, 2024

@leighmcculloch only soroban e2e tests are failing. I believe this requires changing https://github.com/stellar/system-test, right? What's the process? First I update stellar/system-test then merge this, or is it the other way around?

@leighmcculloch
Copy link
Member

@leighmcculloch only soroban e2e tests are failing. I believe this requires changing stellar/system-test, right? What's the process? First I update stellar/system-test then merge this, or is it the other way around?

We don't have a process. We haven't broken it much. 🫢

Comment on lines -119 to -120
* `--rpc-url <RPC_URL>` — RPC server endpoint
* `--network-passphrase <NETWORK_PASSPHRASE>` — Network passphrase to sign the transaction sent to the rpc server
Copy link
Member

Choose a reason for hiding this comment

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

Something I'm wondering about is folks might be using these in scripts, or with env vars, so that they can interact with specific RPC in a way that doesn't rely on stateful changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair point. I wonder if it's worth all the complexity on our codebase due to how command flattening happens with clap though. 🤔

Any suggestions? Should we abandon this change?

Copy link
Contributor

@Ifropc Ifropc Sep 25, 2024

Choose a reason for hiding this comment

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

I think we should be able to do the same (interact with a specific RPC) with combination of existing commands. So we won't break the flow, but it would simplify things for (most) people. The only case it's more complex if people specifically want to override RPC for one command:
Now

stellar <command> --rpc-url <url>

Suggested

stellar <command> --rpc-url $(stellar network add --from mainnet --rpc-url <url> --name `uuidgen`)

or

#!/usr/bin/env bash
export STELLAR_NETWORK=$(stellar network add --from mainnet --rpc-url <url> --name `uuidgen`)
stellar <command_1>
stellar <command_2>

^ this looks way more complicated than 1st option obviously, but how often do people need to override rpc url? Getting rid of rpc-url and passphrase also allows us to make --network optional where it needs to be way more easily -> offers better UX (right now sometimes we need to pass --network even when it's not required for command that's happening offline. For example, generating keys)

Suggested method also requires stellar network add to

  • print an alias of a new network to stdout
  • support new --from flag (that copies network from existing)

@fnando fnando marked this pull request as ready for review September 21, 2024 02:14
Copy link
Contributor

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

Not related to the PR, but should we add
network info <name> command to list network passhprase and RPC? Currently, the flow is to do rm mainnet && add mainnet --rpc-url <my-url> --passphrase and it'd be easier to see the passphrase of mainnet in the CLI

cmd/soroban-cli/src/config/network.rs Show resolved Hide resolved
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.

Use alias for network
3 participants