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 all 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 @@ -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 }
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
15 changes: 7 additions & 8 deletions bin/sozo/src/commands/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@
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};
Expand Down Expand Up @@ -79,15 +78,17 @@
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();

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L81 was not covered by tests

config.tokio_handle().block_on(async {

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L83 was not covered by tests
// 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,

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L91 was not covered by tests
)
.await?;

Expand All @@ -100,8 +101,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 @@ -121,7 +120,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 123 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

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

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

Expand Down
17 changes: 11 additions & 6 deletions bin/sozo/src/commands/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,20 @@

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

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/migrate.rs#L53-L60

Added lines #L53 - L60 were not covered by tests

let world_address = world_diff.world_info.address;

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

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/migrate.rs#L64-L65

Added lines #L64 - L65 were not covered by tests

let migration = Migration::new(
world_diff,
WorldContract::new(world_address, &account),
Expand Down
16 changes: 14 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,11 @@
}

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.
Expand All @@ -26,8 +34,12 @@
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()))

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/options/starknet.rs#L38-L42

Added lines #L38 - L42 were not covered by tests
}

// 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: 7 additions & 3 deletions bin/sozo/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
starknet: StarknetOptions,
world: WorldOptions,
ws: &Workspace<'_>,
ui: &mut MigrationUi,
ui: &mut Option<&mut MigrationUi>,

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
) -> Result<(WorldDiff, SozoAccount<JsonRpcClient<HttpTransport>>, String)> {
let profile_config = ws.load_profile_config()?;
let env = profile_config.env.as_ref();
Expand All @@ -154,15 +154,19 @@
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();
}

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L157-L159

Added lines #L157 - L159 were not covered by tests

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...");
}

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/utils.rs#L167-L169

Added lines #L167 - L169 were not covered by tests

if !dojo_utils::is_deployed(account.address(), &account.provider()).await? {
return Err(anyhow!("Account with address {:#x} doesn't exist.", account.address()));
Expand Down
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 @@
"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?;

Check warning on line 106 in crates/dojo/utils/src/tx/declarer.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/utils/src/tx/declarer.rs#L106

Added line #L106 was not covered by tests

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 @@
);

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

Check warning on line 76 in crates/dojo/utils/src/tx/deployer.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/utils/src/tx/deployer.rs#L75-L76

Added lines #L75 - L76 were not covered by tests

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

Check warning on line 63 in crates/dojo/utils/src/tx/invoker.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/utils/src/tx/invoker.rs#L62-L63

Added lines #L62 - L63 were not covered by tests

if self.txn_config.receipt {
return Ok(TransactionResult::HashReceipt(tx.transaction_hash, Box::new(receipt)));
Expand All @@ -94,13 +87,8 @@
);

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

Check warning on line 91 in crates/dojo/utils/src/tx/invoker.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/utils/src/tx/invoker.rs#L90-L91

Added lines #L90 - L91 were not covered by tests
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
Loading
Loading