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.

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
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
25 changes: 12 additions & 13 deletions Cargo.lock

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
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
18 changes: 7 additions & 11 deletions bin/sozo/src/commands/execute.rs
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,14 +78,12 @@ 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.world,
&ws,
)
.await?;
let (world_diff, account, _) =
utils::get_world_diff_and_account(self.account, self.starknet, self.world, &ws)
.await?;

let contract_address = match &descriptor {
ContractDescriptor::Address(address) => Some(*address),
Expand All @@ -96,8 +94,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 +113,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
6 changes: 3 additions & 3 deletions bin/sozo/src/commands/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ impl MigrateArgs {
let mut spinner =
MigrationUi::Spinner(Spinner::new(frames, "Evaluating world diff...", None));

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).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),
Expand Down
15 changes: 13 additions & 2 deletions bin/sozo/src/commands/options/starknet.rs
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 @@ -18,6 +21,8 @@ pub struct StarknetOptions {
}

impl StarknetOptions {
const DEFAULT_REQUEST_TIMEOUT_MS: u64 = 20_000;

/// Returns a [`JsonRpcClient`] and the rpc url.
///
/// It would be convenient to have the rpc url retrievable from the Provider trait instead.
Expand All @@ -26,8 +31,14 @@ 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(Duration::from_millis(Self::DEFAULT_REQUEST_TIMEOUT_MS))
.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`.
Expand Down
8 changes: 0 additions & 8 deletions bin/sozo/src/commands/options/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
}),
}
}
Expand All @@ -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,
}
}
}
10 changes: 1 addition & 9 deletions crates/dojo/utils/src/tx/declarer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

use std::collections::HashMap;
use std::sync::Arc;
use std::time::Duration;

use starknet::accounts::ConnectedAccount;
use starknet::core::types::{
Expand Down Expand Up @@ -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)));
Expand Down
11 changes: 2 additions & 9 deletions crates/dojo/utils/src/tx/deployer.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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)));
Expand Down
20 changes: 4 additions & 16 deletions crates/dojo/utils/src/tx/invoker.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//! Invoker to invoke contracts.

use std::time::Duration;

use starknet::accounts::ConnectedAccount;
use starknet::core::types::Call;
use tracing::trace;
Expand Down Expand Up @@ -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)));
Expand All @@ -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?;
Comment on lines +90 to +91
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 extracting common transaction waiting logic, sensei!

The transaction waiting logic is duplicated between invoke() and multicall(). Consider extracting this into a private helper method to improve maintainability.

impl<A> Invoker<A>
where
    A: ConnectedAccount + Send + Sync,
{
+    async fn wait_for_transaction(
+        &self,
+        tx_hash: primitive_types::H256,
+    ) -> Result<TransactionResult, TransactionError<A::SignError>> {
+        let receipt = TransactionWaiter::new(tx_hash, &self.account.provider()).await?;
+        
+        if self.txn_config.receipt {
+            return Ok(TransactionResult::HashReceipt(tx_hash, Box::new(receipt)));
+        }
+        
+        Ok(TransactionResult::Hash(tx_hash))
+    }

Committable suggestion skipped: line range outside the PR's diff.


if self.txn_config.receipt {
return Ok(TransactionResult::HashReceipt(tx.transaction_hash, Box::new(receipt)));
Expand Down
8 changes: 4 additions & 4 deletions crates/dojo/utils/src/tx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ pub struct TxnConfig {
/// The multiplier for how much the actual transaction max fee should be relative to the
/// estimated fee. If `None` is provided, the multiplier is set to `1.1`.
pub fee_estimate_multiplier: Option<f64>,
/// Whether to wait for the transaction to be accepted or reverted on L2.
pub wait: bool,
/// Whether to display the transaction receipt.
pub receipt: bool,
/// The maximum fee to pay for the transaction.
pub max_fee_raw: Option<Felt>,
/// Whether to use the `walnut` fee estimation strategy.
pub walnut: bool,
pub timeout_ms: Option<u64>,
}

#[derive(Debug, Copy, Clone)]
Expand All @@ -40,11 +43,8 @@ pub enum TxnAction {
wait: bool,
receipt: bool,
max_fee_raw: Option<Felt>,
/// The multiplier for how much the actual transaction max fee should be relative to the
/// estimated fee. If `None` is provided, the multiplier is set to `1.1`.
fee_estimate_multiplier: Option<f64>,
walnut: bool,
timeout_ms: Option<u64>,
},
Estimate,
Simulate,
Expand Down
3 changes: 2 additions & 1 deletion crates/dojo/utils/src/tx/waiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ impl<'a, P> TransactionWaiter<'a, P>
where
P: Provider + Send,
{
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(300);
/// The default timeout for a transaction to be accepted or reverted on L2.
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(30);
/// Interval for use with 3rd party provider without burning the API rate limit.
const DEFAULT_INTERVAL: Duration = Duration::from_millis(2500);

Expand Down
2 changes: 1 addition & 1 deletion crates/saya/core/Cargo.toml
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