fix: ensure we init rpc client with timeout (#2602)

* fix: ensure we init rpc client with timeout * fix: update prover sdk to compatible version * Update bin/sozo/src/commands/options/starknet.rs Co-authored-by: Ammar Arif <[email protected]> * chore: remove unused dep * fix: internalise request timeout and lower l2 inclusion timeout * fix: add comments and cleanup --------- Co-authored-by: Ammar Arif <[email protected]>
dojoengine · Nov 1, 2024 · 9f3f501 · 9f3f501
1 parent 05fb95b
commit 9f3f501
Show file tree

Hide file tree

Showing 21 changed files with 1,205 additions and 115 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -185,7 +185,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 }
 rpassword = "7.2.0"
 rstest = "0.18.2"
 rstest_reuse = "0.6.0"

diff --git a/bin/sozo/src/commands/execute.rs b/bin/sozo/src/commands/execute.rs
@@ -4,10 +4,9 @@ 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_ops::migration_ui::MigrationUi;
 use sozo_scarbext::WorkspaceExt;
 use sozo_walnut::WalnutDebugger;
 use starknet::core::types::{Call, Felt};
@@ -79,15 +78,17 @@ impl ExecuteArgs {
             self.starknet.url(profile_config.env.as_ref())?,
         );
 
-        config.tokio_handle().block_on(async {
-            let mut spinner = MigrationUi::new("").with_silent();
+        let txn_config: TxnConfig = self.transaction.into();
 
+        config.tokio_handle().block_on(async {
+            // We could save the world diff computation extracting the account directly from the
+            // options.
             let (world_diff, account, _) = utils::get_world_diff_and_account(
                 self.account,
                 self.starknet.clone(),
                 self.world,
                 &ws,
-                &mut spinner,
+                &mut None,
             )
             .await?;
 
@@ -100,8 +101,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,
@@ -121,7 +120,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?;
 

diff --git a/bin/sozo/src/commands/migrate.rs b/bin/sozo/src/commands/migrate.rs
@@ -50,15 +50,20 @@ impl MigrateArgs {
 
             let mut spinner = MigrationUi::new("Evaluating world diff...");
 
-            let mut txn_config: TxnConfig = self.transaction.into();
-            txn_config.wait = true;
-
-            let (world_diff, account, rpc_url) =
-                utils::get_world_diff_and_account(account, starknet, world, &ws, &mut spinner)
-                    .await?;
+            let (world_diff, account, rpc_url) = utils::get_world_diff_and_account(
+                account,
+                starknet,
+                world,
+                &ws,
+                &mut Some(&mut spinner),
+            )
+            .await?;
 
             let world_address = world_diff.world_info.address;
 
+            let mut txn_config: TxnConfig = self.transaction.into();
+            txn_config.wait = true;
+
             let migration = Migration::new(
                 world_diff,
                 WorldContract::new(world_address, &account),

diff --git a/bin/sozo/src/commands/options/starknet.rs b/bin/sozo/src/commands/options/starknet.rs
@@ -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;
@@ -18,6 +21,11 @@ pub struct StarknetOptions {
 }
 
 impl StarknetOptions {
+    /// The default request timeout in milliseconds. This is not the transaction inclusion timeout.
+    /// Refer to [`dojo_utils::tx::waiter::TransactionWaiter::DEFAULT_TIMEOUT`] for the transaction
+    /// inclusion timeout.
+    const DEFAULT_REQUEST_TIMEOUT: Duration = Duration::from_secs(30);
+
     /// Returns a [`JsonRpcClient`] and the rpc url.
     ///
     /// It would be convenient to have the rpc url retrievable from the Provider trait instead.
@@ -26,8 +34,12 @@ impl StarknetOptions {
         env_metadata: Option<&Environment>,
     ) -> 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()))
+
+        let client =
+            ClientBuilder::default().timeout(Self::DEFAULT_REQUEST_TIMEOUT).build().unwrap();
+
+        let transport = HttpTransport::new_with_client(url.clone(), client);
+        Ok((JsonRpcClient::new(transport), url.to_string()))
     }
 
     // We dont check the env var because that would be handled by `clap`.

diff --git a/bin/sozo/src/commands/options/transaction.rs b/bin/sozo/src/commands/options/transaction.rs
@@ -41,12 +41,6 @@ pub struct TransactionOptions {
     #[arg(help = "Display the link to debug the transaction with Walnut.")]
     #[arg(global = true)]
     pub walnut: bool,
-
-    #[arg(long)]
-    #[arg(help = "The timeout in milliseconds for the transaction wait.")]
-    #[arg(value_name = "TIMEOUT-MS")]
-    #[arg(global = true)]
-    pub timeout: Option<u64>,
 }
 
 impl TransactionOptions {
@@ -67,7 +61,6 @@ impl TransactionOptions {
                 max_fee_raw: self.max_fee_raw,
                 fee_estimate_multiplier: self.fee_estimate_multiplier,
                 walnut: self.walnut,
-                timeout_ms: self.timeout,
             }),
         }
     }
@@ -81,7 +74,6 @@ impl From<TransactionOptions> for TxnConfig {
             receipt: value.receipt,
             max_fee_raw: value.max_fee_raw,
             walnut: value.walnut,
-            timeout_ms: value.timeout,
         }
     }
 }
diff --git a/bin/sozo/src/utils.rs b/bin/sozo/src/utils.rs
@@ -145,7 +145,7 @@ pub async fn get_world_diff_and_account(
     starknet: StarknetOptions,
     world: WorldOptions,
     ws: &Workspace<'_>,
-    ui: &mut MigrationUi,
+    ui: &mut Option<&mut MigrationUi>,
 ) -> Result<(WorldDiff, SozoAccount<JsonRpcClient<HttpTransport>>, String)> {
     let profile_config = ws.load_profile_config()?;
     let env = profile_config.env.as_ref();
@@ -154,15 +154,19 @@ pub async fn get_world_diff_and_account(
         get_world_diff_and_provider(starknet.clone(), world, ws).await?;
 
     // Ensures we don't interfere with the spinner if a password must be prompted.
-    ui.stop();
+    if let Some(ui) = ui {
+        ui.stop();
+    }
 
     let account = {
         account
             .account(provider, world_diff.world_info.address, &starknet, env, &world_diff)
             .await?
     };
 
-    ui.restart("Verifying account...");
+    if let Some(ui) = ui {
+        ui.restart("Verifying account...");
+    }
 
     if !dojo_utils::is_deployed(account.address(), &account.provider()).await? {
         return Err(anyhow!("Account with address {:#x} doesn't exist.", account.address()));

diff --git a/crates/dojo/utils/src/tx/declarer.rs b/crates/dojo/utils/src/tx/declarer.rs
@@ -8,7 +8,6 @@
 
 use std::collections::HashMap;
 use std::sync::Arc;
-use std::time::Duration;
 
 use starknet::accounts::ConnectedAccount;
 use starknet::core::types::{
@@ -103,15 +102,8 @@ where
             "Declared class."
         );
 
-        // Since TxnConfig::wait doesn't work for now, we wait for the transaction manually.
         if txn_config.wait {
-            let receipt = if let Some(timeout_ms) = txn_config.timeout_ms {
-                TransactionWaiter::new(transaction_hash, &account.provider())
-                    .with_timeout(Duration::from_millis(timeout_ms))
-                    .await?
-            } else {
-                TransactionWaiter::new(transaction_hash, &account.provider()).await?
-            };
+            let receipt = TransactionWaiter::new(transaction_hash, &account.provider()).await?;
 
             if txn_config.receipt {
                 return Ok(TransactionResult::HashReceipt(transaction_hash, Box::new(receipt)));

diff --git a/crates/dojo/utils/src/tx/deployer.rs b/crates/dojo/utils/src/tx/deployer.rs
@@ -1,7 +1,5 @@
 //! The deployer is in charge of deploying contracts to starknet.
 
-use std::time::Duration;
-
 use starknet::accounts::ConnectedAccount;
 use starknet::core::types::{
     BlockId, BlockTag, Call, Felt, InvokeTransactionResult, StarknetError,
@@ -74,13 +72,8 @@ where
         );
 
         if self.txn_config.wait {
-            let receipt = if let Some(timeout_ms) = self.txn_config.timeout_ms {
-                TransactionWaiter::new(transaction_hash, &self.account.provider())
-                    .with_timeout(Duration::from_millis(timeout_ms))
-                    .await?
-            } else {
-                TransactionWaiter::new(transaction_hash, &self.account.provider()).await?
-            };
+            let receipt =
+                TransactionWaiter::new(transaction_hash, &self.account.provider()).await?;
 
             if self.txn_config.receipt {
                 return Ok(TransactionResult::HashReceipt(transaction_hash, Box::new(receipt)));

diff --git a/crates/dojo/utils/src/tx/invoker.rs b/crates/dojo/utils/src/tx/invoker.rs
@@ -1,7 +1,5 @@
 //! Invoker to invoke contracts.
 
-use std::time::Duration;
-
 use starknet::accounts::ConnectedAccount;
 use starknet::core::types::Call;
 use tracing::trace;
@@ -61,13 +59,8 @@ where
         trace!(transaction_hash = format!("{:#066x}", tx.transaction_hash), "Invoke contract.");
 
         if self.txn_config.wait {
-            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?
-            };
+            let receipt =
+                TransactionWaiter::new(tx.transaction_hash, &self.account.provider()).await?;
 
             if self.txn_config.receipt {
                 return Ok(TransactionResult::HashReceipt(tx.transaction_hash, Box::new(receipt)));
@@ -94,13 +87,8 @@ where
         );
 
         if self.txn_config.wait {
-            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?
-            };
+            let receipt =
+                TransactionWaiter::new(tx.transaction_hash, &self.account.provider()).await?;
 
             if self.txn_config.receipt {
                 return Ok(TransactionResult::HashReceipt(tx.transaction_hash, Box::new(receipt)));