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

Jump to bottom

fix: time difference calculations for new_tps #2655

Merged
merged 1 commit into from
Nov 7, 2024
Merged

fix: time difference calculations for new_tps #2655

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions crates/torii/core/src/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::collections::HashMap;
use std::mem;
use std::str::FromStr;
use std::time::{SystemTime, UNIX_EPOCH};

use anyhow::{Context, Result};
use dojo_types::schema::{Struct, Ty};
Expand Down Expand Up @@ -333,8 +334,10 @@
let new_tps = if new_timestamp - cursor_timestamp != 0 {
num_transactions / (new_timestamp - cursor_timestamp)
} else {
let now = Instant::now();
num_transactions / (now.elapsed().as_secs() - cursor_timestamp)
let current_time =
SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs();

num_transactions / (current_time - cursor_timestamp)

Check warning on line 340 in crates/torii/core/src/executor.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/executor.rs#L337-L340 

Added lines #L337 - L340 were not covered by tests
Comment on lines +337 to +340
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo sensei! The time calculation needs safer handling.

While the change addresses the overflow panic, there are several concerns with the current implementation:

  1. Using unwrap() on duration_since could panic if the system clock is set before UNIX_EPOCH
  2. The time difference could still be zero if system time equals cursor_timestamp
  3. The calculation might be incorrect if system time is less than cursor_timestamp

Here's a safer implementation:

-                            let current_time =
-                                SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs();
-
-                            num_transactions / (current_time - cursor_timestamp)
+                            let current_time = SystemTime::now()
+                                .duration_since(UNIX_EPOCH)
+                                .unwrap_or_default()
+                                .as_secs();
+                            
+                            if current_time <= cursor_timestamp {
+                                // Fallback to 1 second if timestamps are invalid
+                                num_transactions
+                            } else {
+                                num_transactions / (current_time - cursor_timestamp)
+                            }

This implementation:

  1. Uses unwrap_or_default() to handle duration_since errors
  2. Handles the case where system time ≤ cursor_timestamp
  3. Provides a reasonable fallback for TPS calculation
📝 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 current_time =
SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs();
num_transactions / (current_time - cursor_timestamp)
let current_time = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap_or_default()
.as_secs();
if current_time <= cursor_timestamp {
// Fallback to 1 second if timestamps are invalid
num_transactions
} else {
num_transactions / (current_time - cursor_timestamp)
}

};

cursor.last_pending_block_contract_tx =
Expand Down
Loading