fix: sozo migrate hanging #2441
Conversation
WalkthroughOhayo, sensei! The changes introduce a timeout mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPCProvider
participant Timeout
Client->>RPCProvider: Request spec_version()
alt Successful Response
RPCProvider-->>Client: Return spec_version
else Timeout Occurs
Timeout-->>Client: Return error (timeout)
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
Early access features: enabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
bin/sozo/src/commands/migrate.rs (1)
159-159: Capitalize 'CORS' in the error messageFor clarity and correctness, 'CORS' should be capitalized in the error message.
Apply this diff to correct the message:
159- Err(anyhow!( "Unable to connect to RPC provider: timeout, might be cors issue")) 159+ Err(anyhow!( "Unable to connect to RPC provider: timeout, might be CORS issue"))
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- bin/sozo/src/commands/migrate.rs (3 hunks)
Additional comments not posted (1)
bin/sozo/src/commands/migrate.rs (1)
153-165: Ohayo, sensei! Implementing timeout for RPC provider connectionAdding a timeout to prevent indefinite blocking when connecting to the RPC provider enhances the robustness of the connection handling. Great job on implementing this improvement.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2441 +/- ##
=======================================
Coverage 68.21% 68.21%
=======================================
Files 365 365
Lines 48022 48026 +4
=======================================
+ Hits 32757 32760 +3
- Misses 15265 15266 +1 ☔ View full report in Codecov by Sentry. |
| @@ -147,7 +150,19 @@ pub async fn setup_env<'a>( | |||
| let provider = starknet.provider(env)?; | |||
| trace!(?provider, "Provider initialized."); | |||
|
|
|||
| let spec_version = provider.spec_version().await?; | |||
There was a problem hiding this comment.
What's the current timeout by default?
There was a problem hiding this comment.
hmmm would be better if we can introduce this timeout in the Provider implementation itself. so that it'd be applied to all request.
i assume the current approach is to just add the timeout on the very first provider request for a command.
There was a problem hiding this comment.
Agree, would be here @notV4l:
. Creating a client with the builder and timeout instead.
| @@ -147,7 +150,19 @@ pub async fn setup_env<'a>( | |||
| let provider = starknet.provider(env)?; | |||
| trace!(?provider, "Provider initialized."); | |||
|
|
|||
| let spec_version = provider.spec_version().await?; | |||
There was a problem hiding this comment.
hmmm would be better if we can introduce this timeout in the Provider implementation itself. so that it'd be applied to all request.
i assume the current approach is to just add the timeout on the very first provider request for a command.
| let get_spec_version = provider.spec_version(); | ||
| let timeout = time::sleep(Duration::from_secs(5)); | ||
| tokio::pin!(timeout); | ||
|
|
||
| let spec_version = tokio::select! { | ||
| _ = &mut timeout => { | ||
| Err(anyhow!( "Unable to connect to RPC provider: timeout, might be cors issue")) | ||
| }, | ||
| version = get_spec_version => { | ||
| Ok(version.unwrap()) | ||
| }, |
There was a problem hiding this comment.
can be simplified to
let timeout_duration = Duration::from_secs(5);
tokio::time::timeout(timeout_duration, provider.spec_version()).await?;There was a problem hiding this comment.
ha nice way less verbose!
Summary by CodeRabbit