-
Notifications
You must be signed in to change notification settings - Fork 239
Support devnet in --network
flag
#3786
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
base: master
Are you sure you want to change the base?
Conversation
--network
flag
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.
Once 3603-3-devnet-interactions-docs
is merged, let's mention ability to use --network devnet
in the docs 👍 .
The `--network` flag supports the following networks: | ||
|
||
- **mainnet** - Connects to Starknet mainnet using a free RPC provider | ||
- **sepolia** - Connects to Starknet Sepolia testnet using a free RPC provider | ||
- **devnet** - Attempts to auto-detect running starknet-devnet instances | ||
|
||
When using **mainnet** or **sepolia**, `sncast` will randomly select one of the free RPC providers. | ||
When using free providers you may experience rate limits and other unexpected behavior. | ||
|
||
For **devnet**, `sncast` will try to detect running `starknet-devnet` instance and connect to it, but it may fail. If detection fails, then use the `--url` flag instead. |
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.
Once 3603-3-devnet-interactions-docs is merged, let's mention ability to use --network devnet in the docs 👍 .
Since we have interaction-docs, after adding info about --network devnet
there. Should I remove this changes in overview
? I am aware that it is not the best place and we may want to preserve only short messages 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.
It's fine, leave it there too.
|
||
fn detect_devnet_from_processes() -> Result<String, String> { | ||
match find_devnet_process_info() { | ||
Ok(info) => Ok(format!("http://{}:{}", info.host, info.port)), |
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.
You could probably do this
Ok(info) => Ok(format!("http://{}:{}", info.host, info.port)), | |
Ok(DevnetProcessInfo { host, port }) => Ok(format!("http://{}:{}", host, port)), |
|
||
if host.is_none() | ||
&& let Ok(host_env) = std::env::var("HOST") | ||
&& !host_env.is_empty() |
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.
We validate port but take host directly, to be consistent we should probably either validate both or none.
What was the motivation for only validating one of them?
if port.is_none() { | ||
port = std::env::var("PORT") | ||
.ok() | ||
.and_then(|port_env| parse_valid_port(&port_env)); |
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.
This way we silently ignore user settings if they are invalid. If we aren't doing any nice error handling for invalid values I'd rather we panic than silently ignore them.
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.
This goes for all places we do so, not only here
Command::new("curl") | ||
.args(["-s", "-f", "--max-time", "1", &url]) | ||
.output() | ||
.map(|output| output.status.success()) | ||
.unwrap_or(false) |
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.
Rather than rely on curl being installed, couldn't we use some library for HTTP requests?
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.
We already have a dependency on reqwest
let value_str = after_pattern | ||
.split_whitespace() | ||
.next() | ||
.unwrap_or("") |
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.
If the iterator returns nothing (we unwrap with default), doesn't that mean we won't find anything anyway?
Also a nit, but probably unwrap_or_default
could do here.
where | ||
T: OutputLink + Clone, | ||
{ | ||
if (!config.show_explorer_links || rpc.is_localhost(&config.url)) |
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.
Why do we no longer check for localhost?
|
||
- **mainnet** - Connects to Starknet mainnet using a free RPC provider | ||
- **sepolia** - Connects to Starknet Sepolia testnet using a free RPC provider | ||
- **devnet** - Attempts to auto-detect running starknet-devnet instances |
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.
We fail on multiple
- **devnet** - Attempts to auto-detect running starknet-devnet instances | |
- **devnet** - Attempts to auto-detect running starknet-devnet instance |
- **devnet** - Attempts to auto-detect running starknet-devnet instances | ||
|
||
When using **mainnet** or **sepolia**, `sncast` will randomly select one of the free RPC providers. | ||
When using free providers you may experience rate limits and other unexpected behavior. |
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.
This could be >
note
When using **mainnet** or **sepolia**, `sncast` will randomly select one of the free RPC providers. | ||
When using free providers you may experience rate limits and other unexpected behavior. | ||
|
||
For **devnet**, `sncast` will try to detect running `starknet-devnet` instance and connect to it, but it may fail. If detection fails, then use the `--url` flag instead. |
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.
For **devnet**, `sncast` will try to detect running `starknet-devnet` instance and connect to it, but it may fail. If detection fails, then use the `--url` flag instead. | |
For **devnet**, `sncast` will try to detect running `starknet-devnet` instance and connect to it, but detection is not guaranteed. If `sncast` is unable to detect your devnet instance, provide the `--url` directly. |
} | ||
|
||
#[cfg(test)] | ||
mod tests { |
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.
Didn't review the tests yet
The `--network` flag supports the following networks: | ||
|
||
- **mainnet** - Connects to Starknet mainnet using a free RPC provider | ||
- **sepolia** - Connects to Starknet Sepolia testnet using a free RPC provider | ||
- **devnet** - Attempts to auto-detect running starknet-devnet instances | ||
|
||
When using **mainnet** or **sepolia**, `sncast` will randomly select one of the free RPC providers. | ||
When using free providers you may experience rate limits and other unexpected behavior. | ||
|
||
For **devnet**, `sncast` will try to detect running `starknet-devnet` instance and connect to it, but it may fail. If detection fails, then use the `--url` flag instead. |
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's fine, leave it there too.
if let Some(pos) = cmdline.find(flag) { | ||
let after_pattern = &cmdline[pos + flag.len()..]; | ||
let value_str = after_pattern | ||
.split_whitespace() | ||
.next() | ||
.unwrap_or("") | ||
.trim_start_matches('=') | ||
.trim_start_matches(':'); | ||
|
||
if !value_str.is_empty() { | ||
return Some(value_str.to_string()); | ||
} | ||
} | ||
None | ||
} |
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.
Wouldn't regex be better here?
[] => Err(DevnetDetectionError::NoInstance), | ||
[single] => Ok(single.clone()), | ||
_ => Err(DevnetDetectionError::MultipleInstances), |
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.
Ok
at the top:
[] => Err(DevnetDetectionError::NoInstance), | |
[single] => Ok(single.clone()), | |
_ => Err(DevnetDetectionError::MultipleInstances), | |
[single] => Ok(single.clone()), | |
[] => Err(DevnetDetectionError::NoInstance), | |
_ => Err(DevnetDetectionError::MultipleInstances), |
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.
We can make a separate module devnet
, place this file and the one containing DevnetProvider
.
fn extract_docker_mapping(cmdline: &str) -> Option<(String, u16)> { | ||
let port_flags = ["-p", "--publish"]; | ||
|
||
for flag in &port_flags { | ||
if let Some(port_mapping) = extract_string_from_flag(cmdline, flag) { | ||
let parts: Vec<&str> = port_mapping.split(':').collect(); | ||
if parts.len() == 3 | ||
&& let Ok(host_port) = parts[1].parse::<u16>() | ||
{ | ||
return Some((parts[0].to_string(), host_port)); | ||
} else if parts.len() == 2 | ||
&& let Ok(host_port) = parts[0].parse::<u16>() | ||
{ | ||
return Some((DEFAULT_DEVNET_HOST.to_string(), host_port)); | ||
} | ||
} | ||
} | ||
|
||
None | ||
} |
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.
Same remark about regex usage
Closes #3764
Introduced changes
devnet
detection for--network
flagChecklist
CHANGELOG.md