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

opt-refactor(torii-core): engine start health_check_provider #2729

Merged
merged 4 commits into from
Nov 29, 2024
+29 −2
Merged

opt-refactor(torii-core): engine start health_check_provider #2729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f4c228
[ISSUES#]2721 health_check_provider.
847850277 Nov 28, 2024
d8a489d
[ISSUES#]2721 health_check_provider.
847850277 Nov 28, 2024
14e9f34
[ISSUES#]2721 health_check_provider.
847850277 Nov 29, 2024
1c06800
fix: change info to trace and refacto structured logging
glihm Nov 29, 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
6 changes: 6 additions & 0 deletions crates/torii/core/src/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
};
use crate::sql::{Cursors, Sql};
use crate::types::{Contract, ContractType};
use crate::utils::health_check_provider;

type EventProcessorMap<P> = HashMap<Felt, Vec<Box<dyn EventProcessor<P>>>>;

Expand Down Expand Up @@ -241,6 +242,11 @@
}

pub async fn start(&mut self) -> Result<()> {
if let Err(e) = health_check_provider(self.provider.clone()).await {
error!(target: LOG_TARGET,"Provider health check failed during engine start");
return Err(e);
}

Check warning on line 249 in crates/torii/core/src/engine.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/engine.rs#L245-L249 

Added lines #L245 - L249 were not covered by tests
let mut backoff_delay = Duration::from_secs(1);
let max_backoff_delay = Duration::from_secs(60);

Expand Down
25 changes: 23 additions & 2 deletions crates/torii/core/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
use chrono::{DateTime, Utc};
use futures_util::TryStreamExt;
use ipfs_api_backend_hyper::{IpfsApi, IpfsClient, TryFromUri};
use starknet::core::types::{BlockId, BlockTag};
use starknet::providers::Provider;
use tokio_util::bytes::Bytes;
use tracing::info;
use tracing::{info, trace};

use crate::constants::{
IPFS_CLIENT_MAX_RETRY, IPFS_CLIENT_PASSWORD, IPFS_CLIENT_URL, IPFS_CLIENT_USERNAME,
Expand All @@ -20,7 +22,6 @@
pub fn utc_dt_string_from_timestamp(timestamp: u64) -> String {
must_utc_datetime_from_timestamp(timestamp).to_rfc3339()
}

pub async fn fetch_content_from_ipfs(cid: &str, mut retries: u8) -> Result<Bytes> {
let client = IpfsClient::from_str(IPFS_CLIENT_URL)?
.with_credentials(IPFS_CLIENT_USERNAME, IPFS_CLIENT_PASSWORD);
Expand All @@ -46,6 +47,26 @@
IPFS_CLIENT_MAX_RETRY, cid
)))
}

pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>(
provider: P,
) -> Result<(), anyhow::Error> {
Comment on lines +51 to +53
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 for provider health check.

Ohayo sensei! The provider health check should include a timeout mechanism to prevent hanging in case of network issues.

 pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>(
     provider: P,
 ) -> Result<(), anyhow::Error> {
+    use tokio::time::timeout;
+    use std::time::Duration;
+
+    const HEALTH_CHECK_TIMEOUT_SECS: u64 = 5;
+
+    timeout(
+        Duration::from_secs(HEALTH_CHECK_TIMEOUT_SECS),
+        provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest))
+    ).await.map_err(|_| anyhow::anyhow!("Health check timed out"))??;

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

match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await {
Ok(block) => {
trace!(

Check warning on line 56 in crates/torii/core/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/utils.rs#L51-L56 

Added lines #L51 - L56 were not covered by tests
latest_block = ?block,
"Provider health check."

Check warning on line 58 in crates/torii/core/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/utils.rs#L58 

Added line #L58 was not covered by tests
);
Ok(())

Check warning on line 60 in crates/torii/core/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/utils.rs#L60 

Added line #L60 was not covered by tests
}
Err(_) => {
let error_info =
format!("Unhealthy provider {:?}, please check your configuration.", provider);
Err(anyhow::anyhow!(error_info))

Check warning on line 65 in crates/torii/core/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/utils.rs#L63-L65 

Added lines #L63 - L65 were not covered by tests
}
Comment on lines +62 to +66
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling specificity.

The error handling discards the original error information which could be valuable for debugging. Consider including the original error in the error chain.

-        Err(_) => {
+        Err(e) => {
             let error_info =
-                format!("Unhealthy provider {:?}, please check your configuration.", provider);
-            Err(anyhow::anyhow!(error_info))
+                format!("Unhealthy provider, please check your configuration: {}", e);
+            Err(anyhow::anyhow!(error_info).context(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
Err(_) => {
let error_info =
format!("Unhealthy provider {:?}, please check your configuration.", provider);
Err(anyhow::anyhow!(error_info))
}
Err(e) => {
let error_info =
format!("Unhealthy provider, please check your configuration: {}", e);
Err(anyhow::anyhow!(error_info).context(e))
}

}
}

Check warning on line 68 in crates/torii/core/src/utils.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/utils.rs#L68 

Added line #L68 was not covered by tests
Comment on lines +51 to +68
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Ohayo sensei! Based on the investigation, I can now provide the final response:

Consider implementing custom Debug for JsonRpcProvider to avoid exposing RPC URL

The JsonRpcProvider struct contains a sensitive rpc_url field that could contain API keys or tokens if they are part of the URL. Since the struct automatically derives Debug (as evidenced by its usage in the health check), this URL would be exposed in logs. Consider implementing a custom Debug that masks the URL or only shows the host.

Example locations that need attention:

  • crates/saya/provider/src/rpc/mod.rs: Implement custom Debug for JsonRpcProvider
  • crates/torii/core/src/utils.rs: Update health check to use a safe display method
🔗 Analysis chain

Review provider Debug implementation usage.

The current implementation uses Debug formatting for the provider, which might expose sensitive information (like API keys) in logs. Consider implementing a custom display format for the provider that only shows safe information.

Let's check if the Provider trait or its implementations contain sensitive information:

Consider adding a safe_display() method to the Provider trait for logging purposes.

The previous ast-grep pattern might not have worked due to syntax. Let's try a different approach to gather information about Provider implementations and potential sensitive data in the codebase.

We need to check the Provider trait definition and its JsonRpcProvider implementation to see if it contains any sensitive information that might be exposed through Debug formatting.

We need to check the actual structure of JsonRpcProvider to see what fields might be exposed through Debug formatting.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Provider implementations that might contain sensitive data
ast-grep --pattern 'impl Provider for $type {
  $$$
}'

Length of output: 56

Script:

#!/bin/bash
# Let's search for Provider trait definition and its implementations
rg -A 10 "trait Provider" 

# Also search for any struct that might be implementing Provider
rg "impl.*Provider for"

# Let's also check if there are any URL or key related fields in the codebase
rg -i "api.*key|url|endpoint|secret"

Length of output: 75522

Script:

#!/bin/bash
# Let's check the Provider trait definition and its implementations more specifically
rg -A 5 "trait Provider" crates/saya/provider/src/provider.rs

# Let's also check if there are any sensitive fields in the Provider implementations
rg -B 2 -A 5 "impl.*Provider" crates/saya/provider/src/rpc/mod.rs

Length of output: 797

Script:

#!/bin/bash
# Let's check the JsonRpcProvider struct definition to see what fields it contains
rg -A 10 "struct JsonRpcProvider" crates/saya/provider/src/rpc/mod.rs

# Also check if there are any Debug derives on the Provider implementations
rg "#\[derive.*Debug.*\].*struct" crates/saya/provider/src/rpc/mod.rs

Length of output: 494


// tests
#[cfg(test)]
mod tests {
Expand Down
Loading