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

fix: ensure we init rpc client with timeout #2602

Merged
merged 7 commits into from
Nov 1, 2024
Merged

fix: ensure we init rpc client with timeout #2602

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5e7038a
fix: ensure we init rpc client with timeout
glihm Oct 31, 2024
9d28be8
fix: update prover sdk to compatible version
glihm Oct 31, 2024
ab9be68
Update bin/sozo/src/commands/options/starknet.rs
glihm Oct 31, 2024
b10fd79
chore: remove unused dep
glihm Nov 1, 2024
4faa650
fix: internalise request timeout and lower l2 inclusion timeout
glihm Nov 1, 2024
84251fe
fix: add comments and cleanup
glihm Nov 1, 2024
5d96621
merge main
glihm Nov 1, 2024
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
26 changes: 13 additions & 13 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ pretty_assertions = "1.2.1"
rand = "0.8.5"
rayon = "1.8.0"
regex = "1.10.3"
reqwest = { version = "0.12.7", features = [ "blocking", "json", "rustls-tls" ], default-features = false }
reqwest = { version = "0.11.27", features = [ "blocking", "json", "rustls-tls" ], default-features = false }
kariy marked this conversation as resolved.
Show resolved Hide resolved
rpassword = "7.2.0"
rstest = "0.18.2"
rstest_reuse = "0.6.0"
Expand Down
1 change: 1 addition & 0 deletions bin/sozo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ dojo-world.workspace = true
hex = "0.4.3"
hex-literal = "0.4.1"
itertools.workspace = true
jsonrpsee = { version = "0.16.2", default-features = false }
kariy marked this conversation as resolved.
Show resolved Hide resolved
katana-rpc-api.workspace = true
notify = "6.0.1"
num-bigint = "0.4.3"
Expand Down
9 changes: 7 additions & 2 deletions bin/sozo/src/commands/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,13 @@
};

config.tokio_handle().block_on(async {
let (world_diff, provider, _) =
utils::get_world_diff_and_provider(self.starknet.clone(), self.world, &ws).await?;
let (world_diff, provider, _) = utils::get_world_diff_and_provider(
self.starknet.clone(),
self.world,
Default::default(),
&ws,
)
.await?;

Check warning on line 76 in bin/sozo/src/commands/call.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/call.rs#L70-L76 

Added lines #L70 - L76 were not covered by tests

let calldata = if let Some(cd) = self.calldata {
calldata_decoder::decode_calldata(&cd)?
Expand Down
11 changes: 6 additions & 5 deletions bin/sozo/src/commands/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
use anyhow::{anyhow, Result};
use clap::Args;
use dojo_types::naming;
use dojo_utils::Invoker;
use dojo_utils::{Invoker, TxnConfig};
use dojo_world::contracts::naming::ensure_namespace;
use scarb::core::Config;
use sozo_scarbext::WorkspaceExt;
Expand Down Expand Up @@ -78,11 +78,14 @@
self.starknet.url(profile_config.env.as_ref())?,
);

let txn_config: TxnConfig = self.transaction.into();

Check warning on line 82 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L81-L82 

Added lines #L81 - L82 were not covered by tests
config.tokio_handle().block_on(async {
let (world_diff, account, _) = utils::get_world_diff_and_account(
self.account,
self.starknet.clone(),
self.starknet,

Check warning on line 86 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L86 

Added line #L86 was not covered by tests
self.world,
txn_config,

Check warning on line 88 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L88 

Added line #L88 was not covered by tests
&ws,
)
.await?;
Expand All @@ -96,8 +99,6 @@
}
.ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))?;

let tx_config = self.transaction.into();

trace!(
contract=?descriptor,
entrypoint=self.entrypoint,
Expand All @@ -117,7 +118,7 @@
selector: snutils::get_selector_from_name(&self.entrypoint)?,
};

let invoker = Invoker::new(&account, tx_config);
let invoker = Invoker::new(&account, txn_config);

Check warning on line 121 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L121 

Added line #L121 was not covered by tests
// TODO: add walnut back, perhaps at the invoker level.
let tx_result = invoker.invoke(call).await?;

Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/inspect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

config.tokio_handle().block_on(async {
let (world_diff, _, _) =
utils::get_world_diff_and_provider(starknet.clone(), world, &ws).await?;
utils::get_world_diff_and_provider(starknet.clone(), world, None, &ws).await?;

Check warning on line 40 in bin/sozo/src/commands/inspect.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/inspect.rs#L40 

Added line #L40 was not covered by tests

if let Some(resource) = resource {
inspect_resource(&resource, &world_diff);
Expand Down
5 changes: 3 additions & 2 deletions bin/sozo/src/commands/migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
txn_config.wait = true;

let (world_diff, account, rpc_url) =
utils::get_world_diff_and_account(account, starknet, world, &ws).await?;
utils::get_world_diff_and_account(account, starknet, world, txn_config, &ws)
.await?;

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/migrate.rs#L61-L62 

Added lines #L61 - L62 were not covered by tests

let world_address = world_diff.world_info.address;

Expand Down Expand Up @@ -100,7 +101,7 @@

/// Prints the migration banner.
async fn print_banner(ws: &Workspace<'_>, starknet: &StarknetOptions) -> Result<()> {
let (provider, rpc_url) = starknet.provider(None)?;
let (provider, rpc_url) = starknet.provider(None, None)?;

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L104 was not covered by tests

let chain_id = provider.chain_id().await?;
let chain_id = parse_cairo_short_string(&chain_id)
Expand Down
19 changes: 17 additions & 2 deletions bin/sozo/src/commands/options/starknet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use std::time::Duration;

use anyhow::Result;
use clap::Args;
use dojo_utils::env::STARKNET_RPC_URL_ENV_VAR;
use dojo_world::config::Environment;
use reqwest::ClientBuilder;
use starknet::providers::jsonrpc::HttpTransport;
use starknet::providers::JsonRpcClient;
use tracing::trace;
Expand All @@ -24,10 +27,22 @@
pub fn provider(
&self,
env_metadata: Option<&Environment>,
request_timeout_ms: Option<u64>,

Check warning on line 30 in bin/sozo/src/commands/options/starknet.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L30 

Added line #L30 was not covered by tests
) -> Result<(JsonRpcClient<HttpTransport>, String)> {
let url = self.url(env_metadata)?;
trace!(?url, "Creating JsonRpcClient with given RPC URL.");
Ok((JsonRpcClient::new(HttpTransport::new(url.clone())), url.to_string()))
trace!(?url, timeout = ?request_timeout_ms, "Creating JsonRpcClient.");

Check warning on line 33 in bin/sozo/src/commands/options/starknet.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L33 

Added line #L33 was not covered by tests

let client = if let Some(request_timeout_ms) = request_timeout_ms {
ClientBuilder::default()
.timeout(Duration::from_millis(request_timeout_ms))
.build()
.unwrap()

Check warning on line 39 in bin/sozo/src/commands/options/starknet.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L35-L39 

Added lines #L35 - L39 were not covered by tests
} else {
ClientBuilder::default().build().unwrap()

Check warning on line 41 in bin/sozo/src/commands/options/starknet.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L41 

Added line #L41 was not covered by tests
};
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding error handling for client builder.

The current implementation uses unwrap() which could panic if the client builder fails. Consider proper error handling.

     let client = if let Some(request_timeout_ms) = request_timeout_ms {
         ClientBuilder::default()
             .timeout(Duration::from_millis(request_timeout_ms))
             .build()
-            .unwrap()
+            .map_err(|e| anyhow::anyhow!("Failed to build HTTP client: {}", e))?
     } else {
         ClientBuilder::default().build()
-            .unwrap()
+            .map_err(|e| anyhow::anyhow!("Failed to build default HTTP client: {}", e))?
     };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let client = if let Some(request_timeout_ms) = request_timeout_ms {
ClientBuilder::default()
.timeout(Duration::from_millis(request_timeout_ms))
.build()
.unwrap()
} else {
ClientBuilder::default().build().unwrap()
};
let client = if let Some(request_timeout_ms) = request_timeout_ms {
ClientBuilder::default()
.timeout(Duration::from_millis(request_timeout_ms))
.build()
.map_err(|e| anyhow::anyhow!("Failed to build HTTP client: {}", e))?
} else {
ClientBuilder::default().build()
.map_err(|e| anyhow::anyhow!("Failed to build default HTTP client: {}", e))?
};


let transport = HttpTransport::new_with_client(url.clone(), client);
Ok((JsonRpcClient::new(transport), url.to_string()))

Check warning on line 45 in bin/sozo/src/commands/options/starknet.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L44-L45 

Added lines #L44 - L45 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need to expose the timeout config to user tho. perhaps having a sensible default value should suffice ?

the same goes for the timeout on the TransactionWaiter

dojo/crates/dojo/utils/src/tx/invoker.rs

Lines 64 to 69 in 861d0b6

let receipt = if let Some(timeout_ms) = self.txn_config.timeout_ms {
TransactionWaiter::new(tx.transaction_hash, &self.account.provider())
.with_timeout(Duration::from_millis(timeout_ms))
.await?
} else {
TransactionWaiter::new(tx.transaction_hash, &self.account.provider()).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.

I'm just wondering at which poing a declaration can last based on the network + size of a class. This is the main reason why I exposed it.

But I do agree, if we can find something that could be handled without actually exposing it, it's definitely better I agree.
Retry once with bigger timeout and fail afterward perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm good point.

I think if we do retry, we'd just be sending the same request twice. In the case of class declaration, the timeout will be caused by the server processing the request too slow. And because the timeout is happening on the client, the server would still be processing the request, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm good point.

I think if we do retry, we'd just be sending the same request twice. In the case of class declaration, the timeout will be caused by the server processing the request too slow. And because the timeout is happening on the client, the server would still be processing the request, no?

Yep, that's a good point. Which may lead to undesired effect.
So I guess it's preferable to have a longer timeout, but may be shortened manually then?

Copy link
Member

Choose a reason for hiding this comment

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

not exactly sure... a longer timeout (something like 20s) is probably a good number. if a request takes more than 20s, and if its not due to super slow machine, then there's probably something wrong in how katana process the request.

katana also times out at 20s.

dojo/crates/katana/node/src/lib.rs

Lines 308 to 311 in 712422f

let middleware = tower::ServiceBuilder::new()
.option_layer(cors)
.layer(ProxyGetRequestLayer::new("/", "health")?)
.timeout(Duration::from_secs(20));

Copy link
Member

Choose a reason for hiding this comment

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

i should benchmark the addDeclareTransaction endpoint

}

// We dont check the env var because that would be handled by `clap`.
Expand Down
7 changes: 5 additions & 2 deletions bin/sozo/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use anyhow::{anyhow, Context, Result};
use camino::Utf8PathBuf;
use colored::*;
use dojo_utils::TxnConfig;
use dojo_world::config::ProfileConfig;
use dojo_world::diff::WorldDiff;
use dojo_world::local::WorldLocal;
Expand Down Expand Up @@ -102,6 +103,7 @@
pub async fn get_world_diff_and_provider(
starknet: StarknetOptions,
world: WorldOptions,
provider_request_timeout_ms: Option<u64>,

Check warning on line 106 in bin/sozo/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L106 

Added line #L106 was not covered by tests
ws: &Workspace<'_>,
) -> Result<(WorldDiff, JsonRpcClient<HttpTransport>, String)> {
let world_local = ws.load_world_local()?;
Expand All @@ -111,7 +113,7 @@

let world_address = get_world_address(&profile_config, &world, &world_local)?;

let (provider, rpc_url) = starknet.provider(env)?;
let (provider, rpc_url) = starknet.provider(env, provider_request_timeout_ms)?;

Check warning on line 116 in bin/sozo/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L116 

Added line #L116 was not covered by tests
trace!(?provider, "Provider initialized.");

let spec_version = provider.spec_version().await?;
Expand Down Expand Up @@ -143,13 +145,14 @@
account: AccountOptions,
starknet: StarknetOptions,
world: WorldOptions,
txn_config: TxnConfig,

Check warning on line 148 in bin/sozo/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L148 

Added line #L148 was not covered by tests
ws: &Workspace<'_>,
) -> Result<(WorldDiff, SozoAccount<JsonRpcClient<HttpTransport>>, String)> {
let profile_config = ws.load_profile_config()?;
let env = profile_config.env.as_ref();

let (world_diff, provider, rpc_url) =
get_world_diff_and_provider(starknet.clone(), world, ws).await?;
get_world_diff_and_provider(starknet.clone(), world, txn_config.timeout_ms, ws).await?;

Check warning on line 155 in bin/sozo/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L155 

Added line #L155 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding timeout validation and documentation, sensei!

While the transaction configuration integration looks good, consider these improvements:

  1. Add documentation for the new txn_config parameter
  2. Consider validating the timeout value to ensure it's within reasonable bounds

Apply this diff to enhance the documentation:

 /// Sets up the world diff from the environment and returns associated starknet account.
 ///
 /// Returns the world address, the world diff, the account and the rpc url.
 /// This would be convenient to have the rpc url retrievable from the [`Provider`] trait.
+///
+/// # Arguments
+///
+/// * `account` - Account options for configuration
+/// * `starknet` - Starknet-specific options
+/// * `world` - World configuration options
+/// * `txn_config` - Transaction configuration including timeout settings
+/// * `ws` - Workspace reference

Consider adding timeout validation:

const MAX_TIMEOUT_MS: u64 = 60_000; // 60 seconds

if let Some(timeout) = txn_config.timeout_ms {
    if timeout > MAX_TIMEOUT_MS {
        return Err(anyhow!("Timeout value {} ms exceeds maximum allowed {} ms", timeout, MAX_TIMEOUT_MS));
    }
}


let account = {
account
Expand Down
2 changes: 1 addition & 1 deletion crates/saya/core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ celestia-types = "0.5.0"
cairo-proof-parser = { git = "https://github.com/cartridge-gg/cairo-proof-parser.git", rev = "f175d58" }
cairo1-playground = { git = "https://github.com/chudkowsky/cairo1-playground.git", rev = "3fda965" }
herodotus_sharp_playground = { git = "https://github.com/chudkowsky/herodotus_sharp_playground.git", rev = "db64bfd" }
prover-sdk = { git = "https://github.com/cartridge-gg/http-prover", rev = "24256d5" }
prover-sdk = { git = "https://github.com/cartridge-gg/http-prover", rev = "f239ade" }
reqwest.workspace = true
serde-felt = { git = "https://github.com/cartridge-gg/cairo-proof-parser.git", rev = "f175d58" }
tempdir = "0.3.7"
Loading