feat: gas price oracle v2 #2766
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant enhancements to the Katana project by adding a new optional field Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
crates/katana/core/src/backend/gas_oracle.rs (3)
191-191: Adjust log level for error reporting in the gas oracleOhayo sensei! Currently, errors in the gas oracle are logged at the
infolevel. To ensure that these errors receive proper attention during debugging and monitoring, consider changing the log level toerrororwarn.Apply this diff to update the log level:
- info!(%e, "Error running the gas oracle"); + warn!(%e, "Error running the gas oracle");
281-281: Fix typo in test function nameOhayo sensei! There's a small typo in the test function name
test_bufffer_multiple_samples_average. Correcting it will improve code readability and maintain consistency.Apply this diff to fix the typo:
-fn test_bufffer_multiple_samples_average() { +fn test_buffer_multiple_samples_average() {
340-340: Avoidsleepin asynchronous tests to improve test performanceOhayo sensei! Using
tokio::time::sleepin tests can slow down the test suite. Consider mocking the time-dependent components or usingtokio::time::advancein combination withtokio::testattributes to simulate the passage of time without actual delays.crates/katana/node/src/config/mod.rs (1)
53-53: Validate thel1_provider_urlto ensure correctnessOhayo sensei! When accepting user input for
l1_provider_url, consider adding validation to ensure the URL is correctly formatted and reachable. This will help prevent runtime errors caused by invalid URLs.crates/katana/cli/src/args.rs (1)
74-75: Ensure help message forl1_provider_urldisplays correctlyOhayo sensei! The help message for
l1_provider_urlis split across multiple lines, which might affect how it appears in the CLI. To ensure it displays properly, consider combining it into a single line or using raw strings.Apply this diff to adjust the help message:
#[arg(long = "l1.provider", value_name = "URL", alias = "l1-provider")] - #[arg(help = "The Ethereum RPC provider to sample the gas prices from to enable the gas \ - price oracle.")] + #[arg(help = "The Ethereum RPC provider to sample the gas prices from to enable the gas price oracle.")] pub l1_provider_url: Option<Url>,crates/katana/node/src/lib.rs (2)
145-150: Ohayo sensei! Consider adding error handling for the worker initialization.The gas oracle worker initialization should handle potential errors that might occur during startup.
Consider wrapping the worker initialization in a Result:
if let Some(ref url) = self.config.l1_provider_url { - self.backend.gas_oracle.run_worker(self.task_manager.task_spawner()); + if let Err(e) = self.backend.gas_oracle.run_worker(self.task_manager.task_spawner()) { + tracing::error!(%url, error = %e, "Failed to start Gas Price Oracle"); + return Err(anyhow::anyhow!("Failed to start Gas Price Oracle: {}", e)); + } info!(%url, "Gas Price Oracle started."); };
208-213: Document the gas price configuration priority logic.The code implements a priority-based configuration for gas prices, but this logic isn't immediately obvious.
Add a comment explaining the priority order:
+ // Gas price configuration priority: + // 1. Fixed prices from dev config if provided + // 2. Sampled prices from L1 provider URL if configured + // 3. Default fixed prices as fallback let gas_oracle = if let Some(fixed_prices) = &config.dev.fixed_gas_prices {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/katana/cli/src/args.rs(4 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/node/Cargo.toml(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)crates/katana/node/src/lib.rs(2 hunks)
🔇 Additional comments (2)
crates/katana/core/src/backend/gas_oracle.rs (1)
131-138: 🛠️ Refactor suggestion
Handle potential empty fee history to prevent runtime errors
Ohayo sensei! In the update_gas_price function, accessing the last elements of fee_history.base_fee_per_gas and fee_history.base_fee_per_blob_gas without checking if they're empty could cause runtime errors. Consider adding checks or default values to handle empty fee histories gracefully.
Apply this diff to add error handling:
let latest_gas_price = fee_history.base_fee_per_gas.last().context("Getting eth gas price")?;
+ if fee_history.base_fee_per_gas.is_empty() {
+ // Handle empty fee history case
+ return Err(anyhow::anyhow!("Fee history for base gas is empty"));
+ }
gas_price_buffer.add_sample(*latest_gas_price);
let blob_fee_history = fee_history.base_fee_per_blob_gas;
let avg_blob_base_fee = blob_fee_history.iter().last().context("Getting blob gas price")?;
+ if blob_fee_history.is_empty() {
+ // Handle empty blob fee history case
+ return Err(anyhow::anyhow!("Fee history for blob gas is empty"));
+ }
data_gas_price_buffer.add_sample(*avg_blob_base_fee);Likely invalid or redundant comment.
crates/katana/node/Cargo.toml (1)
30-30: Verify consistency of the url crate across the workspace
Ohayo sensei! Adding the url crate as a dependency is great. Please ensure that the version of url specified is consistent across all crates in the workspace to avoid potential version conflicts.
Run the following script to check for multiple versions of the url crate:
| .try_lock() | ||
| .map(|prices| prices.data_gas_prices.clone()) | ||
| .unwrap_or_else(GasPrices::default), |
There was a problem hiding this comment.
Ensure consistent data gas price retrieval by replacing try_lock() with lock()
Ohayo sensei! Similar to the gas prices, in the current_data_gas_prices method, using try_lock() can lead to returning default data gas prices if the mutex is locked elsewhere. To maintain consistency, consider using lock() instead.
Apply this diff:
- .try_lock()
+ .lock()📝 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.
| .try_lock() | |
| .map(|prices| prices.data_gas_prices.clone()) | |
| .unwrap_or_else(GasPrices::default), | |
| .lock() | |
| .map(|prices| prices.data_gas_prices.clone()) | |
| .unwrap_or_else(GasPrices::default), |
| .try_lock() | ||
| .map(|prices| prices.gas_prices.clone()) | ||
| .unwrap_or_else(GasPrices::default), |
There was a problem hiding this comment.
Ensure accurate gas price retrieval by using lock() instead of try_lock()
Ohayo sensei! In the current_gas_prices method, using try_lock() may return default gas prices if the mutex is already locked, potentially leading to inaccurate gas price data. Consider using lock() to wait for the mutex and ensure consistent gas price retrieval.
Apply this diff to change try_lock() to lock():
- .try_lock()
+ .lock()📝 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.
| .try_lock() | |
| .map(|prices| prices.gas_prices.clone()) | |
| .unwrap_or_else(GasPrices::default), | |
| .lock() | |
| .map(|prices| prices.gas_prices.clone()) | |
| .unwrap_or_else(GasPrices::default), |
| // Use fixed gas prices if provided in the configuration | ||
| L1GasOracle::fixed(fixed_prices.gas_price.clone(), fixed_prices.data_gas_price.clone()) | ||
| } | ||
| // TODO: for now we just use the default gas prices, but this should be a proper oracle in the | ||
| // future that can perform actual sampling. | ||
| else { | ||
| } else if let Some(url) = &config.l1_provider_url { | ||
| // Default to a sampled gas oracle using the given provider | ||
| L1GasOracle::sampled(url.clone()) | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Add URL validation before creating the sampled oracle.
The URL should be validated before being used to create the sampled oracle to prevent runtime issues.
Consider adding validation:
} else if let Some(url) = &config.l1_provider_url {
+ // Validate the URL by attempting to create a client
+ if let Err(e) = reqwest::Client::new().get(url.as_str()).build() {
+ return Err(anyhow::anyhow!("Invalid L1 provider URL: {}", e));
+ }
// Default to a sampled gas oracle using the given provider
L1GasOracle::sampled(url.clone())Committable suggestion skipped: line range outside the PR's diff.
augustin-v
force-pushed
the
feat/2558-katana-gas-oracle-v2
branch
from
309e1cd to
7490c57
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
crates/katana/core/src/backend/gas_oracle.rs (1)
15-17: Consider making constants configurableThe hardcoded values for buffer size, interval, and gwei might need to be configurable for different environments or testing scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
Cargo.toml(1 hunks)crates/katana/cli/src/args.rs(4 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/node/Cargo.toml(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)crates/katana/node/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/katana/node/src/config/mod.rs
- crates/katana/node/Cargo.toml
- crates/katana/cli/src/args.rs
🔇 Additional comments (3)
crates/katana/node/src/lib.rs (2)
145-150: LGTM! Gas oracle worker initialization is well-structured.
The implementation properly checks for the URL presence and initializes the worker task with appropriate logging.
208-214: Consider adding error handling for invalid URLs
Ohayo sensei! While the gas oracle construction logic is well-structured, it might benefit from URL validation before creating the sampled oracle.
crates/katana/core/src/backend/gas_oracle.rs (1)
69-71: Fix potential race conditions in price retrieval
Ohayo sensei! Using try_lock() in both current_gas_prices and current_data_gas_prices methods could lead to inconsistent price retrieval.
Also applies to: 81-83
Cargo.toml
Outdated
| @@ -265,7 +265,7 @@ slot = { git = "https://github.com/cartridge-gg/slot", rev = "1298a30" } | |||
| alloy-contract = { version = "0.3", default-features = false } | |||
| alloy-json-rpc = { version = "0.3", default-features = false } | |||
| alloy-network = { version = "0.3", default-features = false } | |||
| alloy-provider = { version = "0.3", default-features = false, features = [ "reqwest" ] } | |||
| alloy-provider = "0.3" | |||
There was a problem hiding this comment.
💡 Codebase verification
Explicit reqwest feature required in katana/core
Ohayo sensei! I found that katana/core explicitly requires the reqwest feature for alloy-provider. The simplified dependency in the workspace should be updated to match this requirement:
alloy-provider = { version = "0.3", features = ["reqwest"] }crates/katana/core/Cargo.toml: Usesalloy-providerwith explicitreqwestfeature
🔗 Analysis chain
Verify alloy-provider default features
Ohayo sensei! The simplified dependency declaration might miss required features. Please verify that the default features of alloy-provider include the necessary reqwest feature for HTTP functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if reqwest feature is included in default features
cargo metadata --format-version=1 | jq -r '.packages[] | select(.name == "alloy-provider") | .features.default'
Length of output: 209
Script:
#!/bin/bash
# Let's try to find the dependency information directly from Cargo.toml files
rg -A 5 '^\[.*dependencies.*\]' Cargo.toml
# Also search for any alloy-provider usage patterns
rg "alloy-provider.*features?\s*="
Length of output: 545
| #[tokio::test] | ||
| async fn test_gas_oracle() { | ||
| let url = Url::parse("https://eth.merkle.io/").expect("Invalid URL"); | ||
| let oracle = L1GasOracle::sampled(url.clone()); | ||
|
|
||
| let shared_prices = match &oracle { | ||
| L1GasOracle::Sampled(sampled) => sampled.prices.clone(), | ||
| _ => panic!("Expected sampled oracle"), | ||
| }; | ||
|
|
||
| let mut worker = GasOracleWorker::new(shared_prices.clone(), url); | ||
|
|
||
| for i in 0..3 { | ||
| let initial_gas_prices = oracle.current_gas_prices(); | ||
|
|
||
| // Verify initial state for first iteration | ||
| if i == 0 { | ||
| assert_eq!( | ||
| initial_gas_prices, | ||
| GasPrices { eth: 0, strk: 0 }, | ||
| "First iteration should start with zero prices" | ||
| ); | ||
| } | ||
|
|
||
| worker.update_once().await.expect("Failed to update prices"); | ||
|
|
||
| let updated_gas_prices = oracle.current_gas_prices(); | ||
| let updated_data_gas_prices = oracle.current_data_gas_prices(); | ||
|
|
||
| // Verify gas prices | ||
| assert!(updated_gas_prices.eth > 0, "ETH gas price should be non-zero"); | ||
|
|
||
| assert!(updated_data_gas_prices.eth > 0, "ETH data gas price should be non-zero"); | ||
|
|
||
| // For iterations after the first, verify that prices have been updated | ||
| if i > 0 { | ||
| // Give some flexibility for price changes | ||
| if initial_gas_prices.eth != 0 { | ||
| assert!( | ||
| initial_gas_prices.eth != updated_gas_prices.eth | ||
| || initial_gas_prices.strk != updated_gas_prices.strk, | ||
| "Prices should potentially change between updates" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // ETH current avg blocktime is ~12 secs so we need a delay to wait for block creation | ||
| tokio::time::sleep(std::time::Duration::from_secs(9)).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and coverage
The test has several potential issues:
- It uses a real URL which could make tests unstable
- The 9-second sleep might not be enough in all cases
- Missing test cases for error scenarios
Consider:
- Using a mock provider for testing
- Adding test cases for network errors and invalid responses
- Testing the backoff mechanism
| pub async fn run(&mut self) -> anyhow::Result<()> { | ||
| let provider = ProviderBuilder::new().on_http(self.l1_provider_url.clone()); | ||
| // every 60 seconds, Starknet samples the base price of gas and data gas on L1 | ||
| let mut interval = tokio::time::interval(INTERVAL); | ||
| interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); | ||
|
|
||
| loop { | ||
| // Wait for the interval to tick | ||
| interval.tick().await; | ||
|
|
||
| { | ||
| // Attempt to get the gas price from L1 | ||
| let last_block_number = provider.get_block_number().await?; | ||
|
|
||
| let fee_history = provider | ||
| .get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[]) | ||
| .await?; | ||
|
|
||
| let mut prices = self.prices.lock(); | ||
|
|
||
| if let Err(e) = update_gas_price( | ||
| &mut prices, | ||
| &mut self.gas_price_buffer, | ||
| &mut self.data_gas_price_buffer, | ||
| fee_history, | ||
| ) { | ||
| info!(%e, "Error running the gas oracle"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error recovery mechanism to the worker loop
The worker loop could benefit from a more robust error recovery mechanism. Currently, it logs errors but continues without any backoff strategy.
Consider implementing an exponential backoff:
pub async fn run(&mut self) -> anyhow::Result<()> {
let provider = ProviderBuilder::new().on_http(self.l1_provider_url.clone());
let mut interval = tokio::time::interval(INTERVAL);
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
+ let mut consecutive_errors = 0;
loop {
interval.tick().await;
{
let result = async {
let last_block_number = provider.get_block_number().await?;
let fee_history = provider
.get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[])
.await?;
let mut prices = self.prices.lock();
update_gas_price(
&mut prices,
&mut self.gas_price_buffer,
&mut self.data_gas_price_buffer,
fee_history,
)
}.await;
match result {
Ok(_) => {
consecutive_errors = 0;
}
Err(e) => {
info!(%e, "Error running the gas oracle");
consecutive_errors += 1;
if consecutive_errors > 0 {
tokio::time::sleep(Duration::from_secs(2u64.pow(consecutive_errors.min(6)))).await;
}
}
}
}
}
}📝 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.
| pub async fn run(&mut self) -> anyhow::Result<()> { | |
| let provider = ProviderBuilder::new().on_http(self.l1_provider_url.clone()); | |
| // every 60 seconds, Starknet samples the base price of gas and data gas on L1 | |
| let mut interval = tokio::time::interval(INTERVAL); | |
| interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); | |
| loop { | |
| // Wait for the interval to tick | |
| interval.tick().await; | |
| { | |
| // Attempt to get the gas price from L1 | |
| let last_block_number = provider.get_block_number().await?; | |
| let fee_history = provider | |
| .get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[]) | |
| .await?; | |
| let mut prices = self.prices.lock(); | |
| if let Err(e) = update_gas_price( | |
| &mut prices, | |
| &mut self.gas_price_buffer, | |
| &mut self.data_gas_price_buffer, | |
| fee_history, | |
| ) { | |
| info!(%e, "Error running the gas oracle"); | |
| } | |
| } | |
| } | |
| } | |
| pub async fn run(&mut self) -> anyhow::Result<()> { | |
| let provider = ProviderBuilder::new().on_http(self.l1_provider_url.clone()); | |
| let mut interval = tokio::time::interval(INTERVAL); | |
| interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); | |
| let mut consecutive_errors = 0; | |
| loop { | |
| interval.tick().await; | |
| { | |
| let result = async { | |
| let last_block_number = provider.get_block_number().await?; | |
| let fee_history = provider | |
| .get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[]) | |
| .await?; | |
| let mut prices = self.prices.lock(); | |
| update_gas_price( | |
| &mut prices, | |
| &mut self.gas_price_buffer, | |
| &mut self.data_gas_price_buffer, | |
| fee_history, | |
| ) | |
| }.await; | |
| match result { | |
| Ok(_) => { | |
| consecutive_errors = 0; | |
| } | |
| Err(e) => { | |
| info!(%e, "Error running the gas oracle"); | |
| consecutive_errors += 1; | |
| if consecutive_errors > 0 { | |
| tokio::time::sleep(Duration::from_secs(2u64.pow(consecutive_errors.min(6)))).await; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
awesome work @augustin-v!!
everything looks good, just one comment and once resolved im happy to merge this 🙂.
there are few optimizations that i think can be done eg allowing the interval and sample size to be configurable, but that can be done in future PR and for now this is already pretty good.
| match self { | ||
| L1GasOracle::Fixed(fixed) => fixed.current_data_gas_prices(), | ||
| L1GasOracle::Sampled(sampled) => sampled | ||
| .prices |
There was a problem hiding this comment.
Use .lock() instead of .try_lock(). We definitely don't want to fallback to GasPrices::default as this would return 0 gas prices. It's fine to block on the mutex here.
There was a problem hiding this comment.
Thank you for the review! Just pushed it using .lock()
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/katana/core/src/backend/gas_oracle.rs (1)
21-21: Remove outdated TODO commentOhayo sensei! The TODO comment is now outdated as the implementation properly samples L1 gas and data gas prices. The comment should be removed to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/core/src/backend/gas_oracle.rs(1 hunks)
🔇 Additional comments (2)
crates/katana/core/src/backend/gas_oracle.rs (2)
285-334: 🛠️ Refactor suggestion
Improve test reliability and coverage
Ohayo sensei! The current test implementation has several issues:
- Uses a real URL which makes tests unstable
- Fixed sleep duration might not be sufficient
- Missing error scenario tests
Consider:
- Using a mock HTTP server (e.g.,
wiremock) - Adding test cases for network errors
- Testing the backoff mechanism
- Adding integration tests with configurable timeouts
Apply this diff to improve the test:
+use wiremock::{Mock, MockServer, ResponseTemplate};
+use serde_json::json;
#[tokio::test]
async fn test_gas_oracle() {
- let url = Url::parse("https://eth.merkle.io/").expect("Invalid URL");
+ let mock_server = MockServer::start().await;
+ let url = Url::parse(&mock_server.uri()).expect("Invalid URL");
+
+ // Mock successful response
+ Mock::given(wiremock::matchers::method("POST"))
+ .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+ "result": {
+ "baseFeePerGas": ["0x1234"],
+ "baseFeePerBlobGas": ["0x5678"]
+ }
+ })))
+ .mount(&mock_server)
+ .await;
let oracle = L1GasOracle::sampled(url.clone());
// ... rest of the test ...
- tokio::time::sleep(std::time::Duration::from_secs(9)).await;
+
+ // Test error scenario
+ Mock::given(wiremock::matchers::method("POST"))
+ .respond_with(ResponseTemplate::new(500))
+ .mount(&mock_server)
+ .await;
+
+ assert!(worker.update_once().await.is_err());
}Likely invalid or redundant comment.
157-187: 🛠️ Refactor suggestion
Improve error handling in the worker loop
Ohayo sensei! The worker loop needs a more robust error handling mechanism:
- Add exponential backoff for consecutive errors
- Consider circuit breaker pattern for persistent failures
- Add metrics/monitoring for error tracking
Apply this diff to add error recovery:
pub async fn run(&mut self) -> anyhow::Result<()> {
let provider = ProviderBuilder::new().on_http(self.l1_provider_url.clone());
let mut interval = tokio::time::interval(INTERVAL);
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);
+ let mut consecutive_errors = 0;
+ let mut last_success = std::time::Instant::now();
loop {
interval.tick().await;
{
- let last_block_number = provider.get_block_number().await?;
- let fee_history = provider
- .get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[])
- .await?;
- let mut prices = self.prices.lock();
- if let Err(e) = update_gas_price(
- &mut prices,
- &mut self.gas_price_buffer,
- &mut self.data_gas_price_buffer,
- fee_history,
- ) {
- info!(%e, "Error running the gas oracle");
+ match async {
+ let last_block_number = provider.get_block_number().await?;
+ let fee_history = provider
+ .get_fee_history(1, BlockNumberOrTag::Number(last_block_number), &[])
+ .await?;
+ let mut prices = self.prices.lock();
+ update_gas_price(
+ &mut prices,
+ &mut self.gas_price_buffer,
+ &mut self.data_gas_price_buffer,
+ fee_history,
+ )
+ }.await {
+ Ok(_) => {
+ consecutive_errors = 0;
+ last_success = std::time::Instant::now();
+ }
+ Err(e) => {
+ consecutive_errors += 1;
+ let backoff = std::time::Duration::from_secs(2u64.pow(consecutive_errors.min(6)));
+ info!(%e, ?backoff, consecutive_errors, "Error running the gas oracle");
+
+ // If no successful updates for 5 minutes, return error
+ if last_success.elapsed() > std::time::Duration::from_secs(300) {
+ return Err(anyhow::anyhow!("Gas oracle failed to update for 5 minutes"));
+ }
+
+ tokio::time::sleep(backoff).await;
+ }
}
}
}
}Likely invalid or redundant comment.
| fn average(&self) -> u128 { | ||
| if self.buffer.is_empty() { | ||
| return 0; | ||
| } | ||
| let sum: u128 = self.buffer.iter().sum(); | ||
| sum / self.buffer.len() as u128 | ||
| } |
There was a problem hiding this comment.
Add overflow protection in average calculation
Ohayo sensei! The average calculation could overflow for large gas prices. Consider using checked arithmetic operations.
Apply this diff to prevent overflow:
fn average(&self) -> u128 {
if self.buffer.is_empty() {
return 0;
}
- let sum: u128 = self.buffer.iter().sum();
- sum / self.buffer.len() as u128
+ // Calculate running average to prevent overflow
+ let mut avg = 0u128;
+ for &price in &self.buffer {
+ avg = avg.checked_add(price / self.buffer.len() as u128)
+ .unwrap_or_else(|| {
+ info!("Gas price average calculation overflow, returning max value");
+ u128::MAX
+ });
+ }
+ avg
}📝 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.
| fn average(&self) -> u128 { | |
| if self.buffer.is_empty() { | |
| return 0; | |
| } | |
| let sum: u128 = self.buffer.iter().sum(); | |
| sum / self.buffer.len() as u128 | |
| } | |
| fn average(&self) -> u128 { | |
| if self.buffer.is_empty() { | |
| return 0; | |
| } | |
| // Calculate running average to prevent overflow | |
| let mut avg = 0u128; | |
| for &price in &self.buffer { | |
| avg = avg.checked_add(price / self.buffer.len() as u128) | |
| .unwrap_or_else(|| { | |
| info!("Gas price average calculation overflow, returning max value"); | |
| u128::MAX | |
| }); | |
| } | |
| avg | |
| } |
|
nvm i was wrong |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2766 +/- ##
==========================================
+ Coverage 55.51% 55.58% +0.07%
==========================================
Files 434 434
Lines 54295 54536 +241
==========================================
+ Hits 30140 30312 +172
- Misses 24155 24224 +69 ☔ View full report in Codecov by Sentry. |
Description
Created a new branch due to merging issues in my initial PR(!2652) for the gas price oracle.
Fixed the update gas price function and used blocking
Mutexfor the gas oracle.Right now there are 2 issues:
possibly coming from the
core/service/block_producer.rsdo_minefunction.Related issue
Fixes #2558
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
alloy-providerin the project configuration.alloy-providerto support TLS connections via Rustls.