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

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
cbc6dee
chore(gas_price_service_v1): strictly ensure last_recorded_height is …
rymnc Dec 30, 2024
dba77da
fix
rymnc Dec 30, 2024
3b60422
chore: add test for gas price service
rymnc Dec 30, 2024
b151207
fix: remove unnecessary storage set, add test for uninit task instead
rymnc Dec 31, 2024
0a86e08
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
rymnc Jan 7, 2025
62d81ed
chore: force a recorded height to be passed into da source service up…
rymnc Jan 7, 2025
73cf35b
fix: recorded height test in da source service
rymnc Jan 8, 2025
4ca7a94
fix: idiomatic set
rymnc Jan 8, 2025
159eb84
fix: remove unnecessary test now that it is enforced
rymnc Jan 8, 2025
b11e03f
fix: integ test
rymnc Jan 8, 2025
8a544b9
fix: remove dbg log
rymnc Jan 8, 2025
53a96cf
fix: disambiguate the l2 block range start and recorded height start
rymnc Jan 8, 2025
64385ad
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
rymnc Jan 9, 2025
dce9367
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
rymnc Jan 9, 2025
687a23b
chore: remove unused trait bound
rymnc Jan 10, 2025
2df50bf
fix: unused first_run
rymnc Jan 10, 2025
2fe4efe
fix: comments
rymnc Jan 10, 2025
f9bfc8a
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
rymnc Jan 10, 2025
8a0764e
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
MitchTurner Jan 14, 2025
efc3676
Fix bad merge
MitchTurner Jan 14, 2025
b12c9e9
Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…
MitchTurner Jan 14, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl<Storage> GasPriceServiceAtomicStorage for Storage
where
Storage: 'static,
Storage: GetMetadataStorage + GetLatestRecordedHeight,
Storage: KeyValueInspect<Column = GasPriceColumn> + Modifiable + Send + Sync,
Storage: KeyValueInspect<Column = GasPriceColumn> + Modifiable + Send + Sync + Clone,
{
type Transaction<'a> = StorageTransaction<&'a mut Storage> where Self: 'a;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ use std::ops::Deref;

#[async_trait::async_trait]
pub trait BlockCommitterApi: Send + Sync {
/// Used on first run to get the latest costs and seqno
async fn get_latest_costs(&self) -> DaBlockCostsResult<Option<RawDaBlockCosts>>;
/// Used to get the costs for a specific seqno
async fn get_costs_by_l2_block_number(
&self,
Expand Down Expand Up @@ -103,7 +101,10 @@ where
.get_costs_by_l2_block_number(*next_height.deref())
.await?
}
None => self.client.get_latest_costs().await?.into_iter().collect(),
None => {
// prevent calling the latest endpoint
return Err(anyhow!("last_recorded_height is None"));
}
};

tracing::info!("raw_da_block_costs: {:?}", raw_da_block_costs);
Expand Down Expand Up @@ -158,23 +159,6 @@ impl BlockCommitterApi for BlockCommitterHttpApi {
Ok(vec![])
}
}

async fn get_latest_costs(&self) -> DaBlockCostsResult<Option<RawDaBlockCosts>> {
// Latest: http://localhost:8080/v1/costs?variant=latest&limit=5
if let Some(url) = &self.url {
tracing::info!("getting latest costs");
let formatted_url = format!("{url}/v1/costs?variant=latest&limit=1");
tracing::info!("Formatted URL: {:?}", formatted_url);
let response = self.client.get(formatted_url).send().await?;
tracing::info!("response: {:?}", response);
let raw_da_block_costs = response.json::<Vec<RawDaBlockCosts>>().await?;
tracing::info!("Parsed: {:?}", raw_da_block_costs);
// only take the first element, since we are only looking for the most recent
Ok(raw_da_block_costs.first().cloned())
} else {
Ok(None)
}
}
}

#[cfg(test)]
Expand Down Expand Up @@ -290,56 +274,6 @@ mod test_block_committer_http_api {
// then
assert_eq!(actual, expected);
}

#[test]
fn get_latest_costs__when_url_is_none__then_returns_none() {
let rt = tokio::runtime::Runtime::new().unwrap();

// given
let block_committer = BlockCommitterHttpApi::new(None);

// when
let actual =
rt.block_on(async { block_committer.get_latest_costs().await.unwrap() });

// then
assert_eq!(actual, None);
}

#[test]
fn get_latest_costs__when_url_is_some__then_returns_expected_costs() {
let rt = tokio::runtime::Runtime::new().unwrap();
let mut mock = FakeServer::new();
let url = mock.url();

// given
let block_committer = BlockCommitterHttpApi::new(Some(url));
let not_expected = RawDaBlockCosts {
id: 1,
start_height: 1,
end_height: 10,
da_block_height: 1u64.into(),
cost: 1,
size: 1,
};
mock.add_response(not_expected);
let expected = RawDaBlockCosts {
id: 2,
start_height: 11,
end_height: 20,
da_block_height: 2u64.into(),
cost: 2,
size: 2,
};
mock.add_response(expected.clone());

// when
let actual =
rt.block_on(async { block_committer.get_latest_costs().await.unwrap() });

// then
assert_eq!(actual, Some(expected));
}
}
#[cfg(any(test, feature = "test-helpers"))]
pub mod fake_server {
Expand Down Expand Up @@ -471,9 +405,6 @@ mod tests {

#[async_trait::async_trait]
impl BlockCommitterApi for MockBlockCommitterApi {
async fn get_latest_costs(&self) -> DaBlockCostsResult<Option<RawDaBlockCosts>> {
Ok(self.value.clone())
}
async fn get_costs_by_l2_block_number(
&self,
l2_block_number: u32,
Expand Down Expand Up @@ -504,19 +435,17 @@ mod tests {
}

#[tokio::test]
async fn request_da_block_cost__when_last_value_is_none__then_get_latest_costs_is_called(
) {
async fn request_da_block_cost__when_last_value_is_none__then_error_is_thrown() {
// given
let da_block_costs = test_da_block_costs();
let expected = vec![(&da_block_costs).into()];
let mock_api = MockBlockCommitterApi::new(Some(da_block_costs));
let mut block_committer = BlockCommitterDaBlockCosts::new(mock_api, None);

// when
let actual = block_committer.request_da_block_costs().await.unwrap();
let actual = block_committer.request_da_block_costs().await;

// then
assert_eq!(actual, expected);
assert!(actual.is_err());
}

#[tokio::test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ where
/// This function polls the source according to a polling interval
/// described by the DaBlockCostsService
async fn run(&mut self, state_watcher: &mut StateWatcher) -> TaskNextAction {
tracing::debug!("111111111111111111111111111111111");
tokio::select! {
biased;
_ = state_watcher.while_started() => {
Expand Down
1 change: 1 addition & 0 deletions crates/services/gas_price_service/src/v1/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ mod tests {
},
},
ports::{
GetLatestRecordedHeight,
GetMetadataStorage,
SetMetadataStorage,
},
Expand Down
48 changes: 48 additions & 0 deletions crates/services/gas_price_service/src/v1/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -793,3 +793,51 @@ async fn uninitialized_task__init__if_metadata_behind_l2_height_then_sync() {

assert_eq!(on_chain_height, algo_updater_height);
}

#[tokio::test]
async fn uninitialized_task__init__sets_block_height_for_da_source_before_starting() {
// given
let metadata_height = 100;
let l2_height = 200;
let config = zero_threshold_arbitrary_config();

let metadata = V1Metadata {
new_scaled_exec_price: 100,
l2_block_height: metadata_height,
new_scaled_da_gas_price: 0,
gas_price_factor: NonZeroU64::new(100).unwrap(),
total_da_rewards_excess: 0,
latest_known_total_da_cost_excess: 0,
last_profit: 0,
second_to_last_profit: 0,
latest_da_cost_per_byte: 0,
unrecorded_block_bytes: 0,
};
let gas_price_db = gas_price_database_with_metadata(&metadata);
let mut onchain_db = FakeOnChainDb::new(l2_height);
for height in 1..=l2_height {
let block = arb_block();
onchain_db.blocks.insert(BlockHeight::from(height), block);
}
let da_source = FakeDABlockCost::never_returns();
let latest_received_height_arc = da_source.latest_received_height.clone();

let service = UninitializedTask::new(
config,
Some(metadata_height.into()),
0.into(),
FakeSettings::default(),
empty_block_stream(),
gas_price_db,
da_source,
onchain_db.clone(),
)
.unwrap();

// when
let _ = service.init(&StateWatcher::started()).await.unwrap();

// then
let latest_received_height = (*latest_received_height_arc.lock().unwrap()).unwrap();
assert_eq!(latest_received_height, l2_height.into());
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,13 @@ where
if let Some(last_recorded_height) = self.gas_price_db.get_recorded_height()? {
self.da_source.set_last_value(last_recorded_height).await?;
tracing::info!("Set last recorded height to {}", last_recorded_height);
} else {
tracing::info!("No recorded height found");
self.da_source
.set_last_value(latest_block_height.into())
.await?;
}

let poll_duration = self
.config
.da_poll_interval
Expand Down
8 changes: 4 additions & 4 deletions tests/tests/gas_price.rs
Original file line number Diff line number Diff line change
Expand Up @@ -612,9 +612,9 @@ fn produce_block__l1_committed_block_affects_gas_price() {
let mut mock = FakeServer::new();
let url = mock.url();
let costs = RawDaBlockCosts {
id: 1,
start_height: 1,
end_height: 1,
id: 2,
start_height: 2,
end_height: 2,
da_block_height: DaBlockHeight(100),
cost: 100,
size: 100,
Expand All @@ -634,7 +634,7 @@ fn produce_block__l1_committed_block_affects_gas_price() {
]);

// when
let new_gas_price = rt
let new_gas_price: u64 = rt
.block_on(async {
let driver = FuelCoreDriver::spawn_with_directory(temp_dir, &args)
.await
Expand Down
Loading