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

[cli] Add client faucet command to get gas #15940

Merged
merged 3 commits into from
Jan 27, 2024

Conversation

stefan-mysten
Copy link
Contributor

@stefan-mysten stefan-mysten commented Jan 26, 2024

Description

Added a new command sui client faucet to make it easier to get gas from devnet, testnet, or local network. If a different network than the usual ones (fullnode.network.sui.io) is used, it will complain and ask to pass the url to its faucet.

There are two options for the command:
--url, which is the URL to the gas faucet server
--address, which can be either an address or the alias name corresponding to an address in the wallet.

Usage

sui client faucet => will use the active network and active address
sui client faucet --address alias_name => will use the address corresponding to this alias, and the active network
sui client faucet --url => will use the active address and the faucet server at that endpoint
sui client faucet --address alias_name --url https://myfaucet.mysui.io/v5/gas => will use that address corresponding to the alias, and the faucet server at that endpoint.

Output

It will only output that the request was successful and the task id that it got from the faucet server. The user has to manually check for the gas coin if it was sent to the address or not (due to rate-limits, an automated approach fails easily).

Test Plan

image

If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Added a new command sui client faucet to make it easier to get gas from devnet, testnet, or a local network. If a different network than the usual ones (fullnode.network.sui.io) is used, it will complain and ask to pass the URL to the faucet server.

There are two options for the command:
--url, which is the URL to the gas faucet server
--address, which can be either an address or the alias name corresponding to an address in the wallet.

Usage

sui client faucet => will use the active network and active address
sui client faucet --address alias_name => will use the address corresponding to this alias, and the active network
sui client faucet --url => will use the active address and the faucet server at that endpoint
sui client faucet --address alias_name --url https://myfaucet.mysui.io/v5/gas => will use that address corresponding to the alias, and the faucet server at that endpoint.

Output

It will only output that the request was successful and instruct the user to wait up to 60 seconds and then manually check (sui client gas) for the gas coin if it was sent to the address or not.

@stefan-mysten stefan-mysten requested review from a team January 26, 2024 03:03
Copy link

vercel bot commented Jan 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2024 3:16am
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2024 3:16am
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2024 3:16am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2024 3:16am
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2024 3:16am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 27, 2024 3:16am

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks @stefan-mysten! This is timely, because @giac-mysten was just lamenting how he couldn't easily request gas from the CLI!

Comment on lines +1314 to +1315
SUI_DEVNET_URL => "https://faucet.devnet.sui.io/v1/gas",
SUI_TESTNET_URL => "https://faucet.testnet.sui.io/v1/gas",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is right, but just to confirm -- is this the version that uses batching? (the new form of the API?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I know, yes.

crates/sui/src/client_commands.rs Outdated Show resolved Hide resolved
}
};
request_tokens_from_faucet(address, url).await?;
SuiClientCommandResult::NoOutput
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is useful to replace some other command results as well (perhaps as a follow-up)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the plan, but I did not want to include non related changes here.

Comment on lines 2237 to 2238
println!("Requested gas from faucet: {url}");
println!(
Copy link
Contributor

Choose a reason for hiding this comment

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

What's our current stance on println! vs tracing vs other output methods for the CLI? Do we have a standard now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking on how to do this. What we wanted to do is to use eprintln for stderr stuff, and the rest should be mostly tabulated output. What I wanted to have as output for this is the new coin object id, but that's difficult because of the rate limiter on the faucet server (in my tests, I run the request and then the next request to check for task status returned a 429, so not easy). An alternative is to call the gas command before the faucet, and then in a loop wait until we have 1 extra coin, and then do a diff to get the id. It's hacky, but this solves the rate limiter issue.

Thoughts?

Comment on lines 2254 to 2255
println!("Faucet task id: {task_id}");
println!("The coin should be available in the next couple of minutes. Run sui client gas to find your coins.");
Copy link
Contributor

Choose a reason for hiding this comment

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

It is usually quicker than a couple of minutes, so is it worth waiting for it here? (Perhaps guarded by a flag/option?)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is something that we can follow up with as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with waiting is that often due to the rate limiter, we cannot make a request to the faucet server for the status as it will return 429. The hacky way is to call gas command, and then in a loop call the gas command and get the coins until we have coins.len() + 1.
Maybe a --wait-for-coin-id flag can be used to indicate that the user wants to wait to get the coin id?

The main issue is that I could have my set active network to X, and then issue faucet --url Y, so when calling gas command, it will use the X network instead of the Y one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to do something and realized that if a custom RPC's faucet endpoint does not follow the usual faucet.network.address it's hard to "deconstruct" the RPC endpoint to get the gas objects, so it is not so straightforward. For now, it will just tell the user to check for gas. If anyone has better ideas, I am all ears :D

resp.status()
);
if resp.status() == 429 {
bail!("Faucet received too many requests from this ip address. Please try again after 60 minutes.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bail!("Faucet received too many requests from this ip address. Please try again after 60 minutes.");
bail!("Faucet received too many requests from this IP address. Please try again after 60 minutes.");

let faucet_resp: FaucetResponse = resp.json().await?;

let task_id = if let Some(err) = faucet_resp.error {
bail!("Faucet request was unsuccessful. Error is {err:?}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bail!("Faucet request was unsuccessful. Error is {err:?}")
bail!("Faucet request was unsuccessful: {err}")

Don't use debug output for errors -- most of the time the impl Display for the error type displays a human-readable message, and we should use that. In the cases it doesn't, we should fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad. Thanks for spotting this!

} else {
faucet_resp.task
};
println!("Faucet task id: {task_id}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
println!("Faucet task id: {task_id}");
println!("Faucet task ID: {task_id}");

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.

None yet

2 participants