fix(gas_price_service_v1): block committer api format #2506

rymnc · 2024-12-17T11:59:00Z

Note

This PR is wip, we need to decide if we should sunset bundleId's or not

Linked Issues/PRs

Description

the block committer deviated from the spec on the api format, this PR updates the code to match the implementation.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

…itter-api

rymnc · 2024-12-17T23:28:52Z

crates/services/gas_price_service/src/common/fuel_core_storage_adapter.rs

+    fn get_recorded_height(
+        &self,
+        block_height: &BlockHeight,
+    ) -> GasPriceResult<Option<BlockHeight>> {
        let bundle_id = self


perhaps this should be

let recorded_height = self

crates/services/gas_price_service/src/common/fuel_core_storage_adapter.rs

rymnc · 2024-12-17T23:30:31Z

crates/services/gas_price_service/src/common/fuel_core_storage_adapter/storage.rs

-type BundleId = u32;
-
-impl Mappable for BundleIdTable {
+impl Mappable for RecordedHeights {


perhaps a comment here about using a fixed key vs different keys to store the last recorded block height?

I think I prefer this since we get to explicitly call for the height specified in the metadata. In the case that the metadata is behind the DB height, we can still get the right height.

…_adapter.rs Co-authored-by: Aaryamann Challani <[email protected]>

rymnc · 2024-12-17T23:30:46Z

crates/services/gas_price_service/src/lib.rs

@@ -2,6 +2,7 @@
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]
+extern crate core;


do we need this?

crates/services/gas_price_service/src/v1/da_source_service/service.rs

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

rymnc · 2024-12-17T23:37:01Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

+    #![allow(non_snake_case)]
+
+    use super::*;
+    use crate::v1::da_source_service::block_committer_costs::fake_server::FakeServer;


think this import can be simplified

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

…vice.rs Co-authored-by: Aaryamann Challani <[email protected]>

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

netrome

Some nits and minor questions, otherwise lgtm

netrome · 2024-12-18T09:00:29Z

crates/services/gas_price_service/src/v1/da_source_service.rs

@@ -9,6 +9,7 @@ pub mod service;
 #[derive(Debug, Default, Clone, Eq, Hash, PartialEq)]
 pub struct DaBlockCosts {
    pub bundle_id: u32,
+    // TODO: Should this be a range?


Sounds like it should

netrome · 2024-12-18T09:03:54Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

-        number: u32,
-    ) -> DaBlockCostsResult<Option<RawDaBlockCosts>> {
+    async fn get_latest_costs(&self) -> DaBlockCostsResult<Option<RawDaBlockCosts>> {
+        // Latest: http://localhost:8080/v1/costs?variant=latest&limit=5


Suggested change

// Latest: http://localhost:8080/v1/costs?variant=latest&limit=5

I actually like these. Shows what we're trying to reproduce.

netrome · 2024-12-18T09:04:00Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

+        &self,
+        l2_block_number: u32,
+    ) -> DaBlockCostsResult<Vec<RawDaBlockCosts>> {
+        // Specific: http://localhost:8080/v1/costs?variant=specific&value=19098935&limit=5


Suggested change

// Specific: http://localhost:8080/v1/costs?variant=specific&value=19098935&limit=5

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

netrome · 2024-12-18T09:11:00Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

+                    let variant = values.next().unwrap();
+                    tracing::info!("Variant: {:?}", variant);
+                    match variant {
+                        // Latest: http://localhost:8080/v1/costs?variant=latest&limit=5


Suggested change

// Latest: http://localhost:8080/v1/costs?variant=latest&limit=5

netrome · 2024-12-18T09:11:08Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

+                                most_recent.into_iter().collect();
+                            serde_json::to_string(&response).unwrap().into()
+                        }
+                        // Specific: http://localhost:8080/v1/costs?variant=specific&value=19098935&limit=5


Suggested change

// Specific: http://localhost:8080/v1/costs?variant=specific&value=19098935&limit=5

crates/services/gas_price_service/src/common/fuel_core_storage_adapter/storage.rs

rafal-ch · 2024-12-18T12:39:08Z

crates/services/gas_price_service/src/common/fuel_core_storage_adapter.rs

-        self.storage_as_mut::<BundleIdTable>()
-            .insert(block_height, &bundle_id)
+        self.storage_as_mut::<RecordedHeights>()
+            .insert(block_height, &recorded_height)


It seems like we only insert to the DB. Should we consider pruning old data at some point?

We only have one key now, so it's very small.

crates/services/gas_price_service/src/common/fuel_core_storage_adapter/storage.rs

rafal-ch · 2024-12-18T12:51:59Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

+            bundle_id: value.bundle_id,
+            l2_blocks: (value.start_height..=value.end_height).collect(),
+            bundle_size_bytes: value.size_bytes,
+            blob_cost_wei: value.cost_wei,


Suggested change

bundle_id: value.bundle_id,

l2_blocks: (value.start_height..=value.end_height).collect(),

bundle_size_bytes: value.size_bytes,

blob_cost_wei: value.cost_wei,

Self::from(&value)

rafal-ch · 2024-12-18T12:52:32Z

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs

        Ok(())
    }
 }

+impl From<RawDaBlockCosts> for DaBlockCosts {


Nit: I'd suggest moving the two From<... implementations close to each other.

fix(gas_price_service_v1): block committer api format

ae2b760

rymnc force-pushed the fix/block-committer-api branch from c7e96da to ae2b760 Compare December 17, 2024 11:59

rymnc added the no changelog Skip the CI check of the changelog modification label Dec 17, 2024

rymnc self-assigned this Dec 17, 2024

MitchTurner added 6 commits December 17, 2024 11:05

Merge branch 'chore/add-tests-for-v1-gas-service' into fix/block-comm…

f01a94c

…itter-api

Move to latest recorded block height concept instead of bundle id

1ed6d98

Update the mock server, move to use in UTs, write some UTs

5175631

Fix spelling

a701116

Fix integ test

13880ed

Remove tracing subscriber

05574d5

rymnc commented Dec 17, 2024

View reviewed changes

crates/services/gas_price_service/src/common/fuel_core_storage_adapter.rs Outdated Show resolved Hide resolved

Lint toml file

519533d

rymnc commented Dec 17, 2024

View reviewed changes

Update crates/services/gas_price_service/src/common/fuel_core_storage…

6956ca3

…_adapter.rs Co-authored-by: Aaryamann Challani <[email protected]>

rymnc commented Dec 17, 2024

View reviewed changes

MitchTurner and others added 7 commits December 17, 2024 16:41

Update crates/services/gas_price_service/src/v1/da_source_service/blo…

a9e5067

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

Update crates/services/gas_price_service/src/v1/da_source_service/blo…

a19b0b7

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

Update crates/services/gas_price_service/src/v1/da_source_service/ser…

295e5e9

…vice.rs Co-authored-by: Aaryamann Challani <[email protected]>

Update crates/services/gas_price_service/src/v1/da_source_service/blo…

2c4cb2c

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

Update crates/services/gas_price_service/src/v1/da_source_service/blo…

5744f3d

…ck_committer_costs.rs Co-authored-by: Aaryamann Challani <[email protected]>

Appease Clippy-sama

24637d7

Fix import signature

c215b04

MitchTurner marked this pull request as ready for review December 18, 2024 00:01

MitchTurner requested review from xgreenx and Dentosal as code owners December 18, 2024 00:01

lint toml

7a8e1b4

netrome approved these changes Dec 18, 2024

View reviewed changes

rafal-ch reviewed Dec 18, 2024

View reviewed changes

Change latest recorded block to only have one key, add some logs

4655de6

MitchTurner added 4 commits December 18, 2024 11:09

Add more logs

eef1e9a

Remove test that has different behavior now

eb29a3a

Use range for v1 algorithm commit blocks

fc107cc

Nitpick From impls and add comment

38eebde

MitchTurner merged commit 108aa0d into chore/add-tests-for-v1-gas-service Dec 18, 2024
9 checks passed

MitchTurner deleted the fix/block-committer-api branch December 18, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gas_price_service_v1): block committer api format #2506

fix(gas_price_service_v1): block committer api format #2506

rymnc commented Dec 17, 2024

rymnc Dec 17, 2024

rymnc Dec 17, 2024

MitchTurner Dec 17, 2024

rymnc Dec 17, 2024

MitchTurner Dec 17, 2024

rymnc Dec 17, 2024

MitchTurner Dec 18, 2024

netrome left a comment

netrome Dec 18, 2024

netrome Dec 18, 2024

MitchTurner Dec 18, 2024

netrome Dec 18, 2024

netrome Dec 18, 2024

netrome Dec 18, 2024

rafal-ch Dec 18, 2024

MitchTurner Dec 18, 2024

rafal-ch Dec 18, 2024

rafal-ch Dec 18, 2024

fix(gas_price_service_v1): block committer api format #2506

fix(gas_price_service_v1): block committer api format #2506

Conversation

rymnc commented Dec 17, 2024

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment