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 1 commit
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
15 changes: 8 additions & 7 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 @@ impl CallArgs {
};

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?;

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 std::str::FromStr;
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 @@ impl ExecuteArgs {
self.starknet.url(profile_config.env.as_ref())?,
);

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

config.tokio_handle().block_on(async {
let (world_diff, account, _) = utils::get_world_diff_and_account(
self.account,
self.starknet.clone(),
self.starknet,
self.world,
txn_config,
&ws,
)
.await?;
Expand All @@ -96,8 +99,6 @@ impl ExecuteArgs {
}
.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 @@ impl ExecuteArgs {
selector: snutils::get_selector_from_name(&self.entrypoint)?,
};

let invoker = Invoker::new(&account, tx_config);
let invoker = Invoker::new(&account, txn_config);
// TODO: add walnut back, perhaps at the invoker level.
let tx_result = invoker.invoke(call).await?;

Expand Down
3 changes: 2 additions & 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 @@ -2,6 +2,7 @@ use anyhow::Result;
use clap::Args;
use colored::*;
use dojo_types::naming;
use dojo_utils::TxnConfig;
use dojo_world::diff::{ResourceDiff, WorldDiff, WorldStatus};
use dojo_world::ResourceType;
use scarb::core::Config;
Expand Down Expand Up @@ -37,7 +38,7 @@ impl InspectArgs {

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?;

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 @@ impl MigrateArgs {
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?;

let world_address = world_diff.world_info.address;

Expand Down Expand Up @@ -100,7 +101,7 @@ pub struct Banner {

/// 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)?;

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 @@ impl StarknetOptions {
pub fn provider(
&self,
env_metadata: Option<&Environment>,
request_timeout_ms: Option<u64>,
) -> 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, "Creating JsonRpcClient with given RPC URL and timeout.");
glihm marked this conversation as resolved.
Show resolved Hide resolved

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()
};
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()))
kariy marked this conversation as resolved.
Show resolved Hide resolved
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 std::str::FromStr;
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 fn is_address(tag_or_address: &str) -> bool {
pub async fn get_world_diff_and_provider(
starknet: StarknetOptions,
world: WorldOptions,
provider_request_timeout_ms: Option<u64>,
ws: &Workspace<'_>,
) -> Result<(WorldDiff, JsonRpcClient<HttpTransport>, String)> {
let world_local = ws.load_world_local()?;
Expand All @@ -111,7 +113,7 @@ pub async fn get_world_diff_and_provider(

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)?;
trace!(?provider, "Provider initialized.");

let spec_version = provider.spec_version().await?;
Expand Down Expand Up @@ -143,13 +145,14 @@ pub async fn get_world_diff_and_account(
account: AccountOptions,
starknet: StarknetOptions,
world: WorldOptions,
txn_config: TxnConfig,
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?;
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
Loading