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.

Sign up for GitHub

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

Jump to bottom

fix: sozo migrate hanging #2441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: sozo migrate hanging #2441

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion bin/sozo/src/commands/migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::time::Duration;

use anyhow::{anyhow, Context, Result};
use clap::{Args, Subcommand};
use dojo_utils::TxnConfig;
Expand All @@ -12,6 +14,7 @@
use starknet::core::utils::parse_cairo_short_string;
use starknet::providers::jsonrpc::HttpTransport;
use starknet::providers::{JsonRpcClient, Provider, ProviderError};
use tokio::time;
use tracing::trace;

use super::options::account::{AccountOptions, SozoAccount};
Expand Down Expand Up @@ -147,7 +150,19 @@
let provider = starknet.provider(env)?;
trace!(?provider, "Provider initialized.");

let spec_version = provider.spec_version().await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the current timeout by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

forever

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, would be here @notV4l:

dojo/bin/sozo/src/commands/options/starknet.rs

Line 27 in 21ec45e

Ok(JsonRpcClient::new(HttpTransport::new(url)))
. Creating a client with the builder and timeout instead.

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())
},
glihm marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +153 to +163
Copy link
Member

Choose a reason for hiding this comment

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

can be simplified to

let timeout_duration = Duration::from_secs(5);
tokio::time::timeout(timeout_duration, provider.spec_version()).await?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ha nice way less verbose!

}?;

Check warning on line 164 in bin/sozo/src/commands/migrate.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/migrate.rs#L164 

Added line #L164 was not covered by tests

trace!(spec_version);

if !is_compatible_version(&spec_version, RPC_SPEC_VERSION)? {
Expand Down
Loading