chore(gas_price_service): define port for GasPriceDatabase #2226

rymnc · 2024-09-20T07:14:28Z

Note

This is PR 2/n in cleaning up the gas price service. This PR is first of few that moves the algorithm_updater from the sub_services module of fuel-core to fuel-core-gas-price-service.

Please review #2224 before getting to this one ;)

chore(gas_price_service): define port for gas price db lookups

Linked Issues/PRs

Description

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?

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs

MitchTurner · 2024-09-20T12:59:49Z

I guess this is still in draft. So maybe shouldn't have improved, but since this is all being done incrementally... I'm good with what I'm seeing.

netrome

Looks good to me. Just two todos that are missing a reference to an issue.

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs

xgreenx · 2024-09-20T14:14:35Z

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

@@ -51,41 +34,6 @@ mod l2_source_tests;

 pub mod storage;

-impl<Storage> MetadataStorage for StructuredStorage<Storage>


The main idea of this code was that gas price crate defined default implementation of the MetadataStorage port. I don't see why we don't want to follow that idea=)

Having default implementation allows us test it within this crate, and inherit it later in the fuel-core, if we want to, of course=)

retained in 100b5b8

xgreenx · 2024-09-20T14:19:04Z

crates/fuel-core/src/service/adapters/gas_price_adapters.rs

+        HistoricalView::latest_height(self)
+    }
+
+    fn rollback_last_block(&self) -> StorageResult<()> {


I think we don't need to leak this kind of functionality to the service.

I think it would be better if FuelService were responsible for database integrity. It is okay if other databases are behind than on-chain database. It is the responsibility of the service to catch up(since only the service knows how to advance the state). But if the on-chain database is behind other databases, then FuelService should roll back them since only FuelService knows that the database supports the state rewind feature.

Hmmmm. Kinda confused. InitializationTask currently handles that and I thought we wanted to get rid of it and move that code into GasPriceService.

I think @xgreenx right that it's leaking something, but at the same time we have to leak the idea that they can be out of sync to include the syncing logic into GasPriceService.

i.e. I'm not sure how the FuelService could handle it without accessing the metadata and updater code.

I think @xgreenx here means that the FuelService should be aware of rollback strategy for each of the db's included within CombinedDatabase, which isn't that complex at the moment where we're leaking business logic, its just rolling back to the same height as the OnChainDb

addressed in 9575a21
seems cleaner than before :)

slightly unsure about where you'd like to place the rewind/rollback call ~ i've placed it before the init_sub_services call :)

crates/fuel-core/src/combined_database.rs

acerone85 · 2024-09-24T10:57:44Z

crates/services/gas_price_service/src/ports.rs

+    fn set_metadata(&mut self, metadata: &UpdaterMetadata) -> Result<()>;
+}
+
+pub trait GasPriceData: Send + Sync {


the name of the port is GasPriceData, but its only function returns a BlockHeight.

Would it be possible to have a comment that explains how the two are related?

addressed in d51d494

The gas_price_service crate doesn't know about the Database<GasPriceDatabase>=)

The comment and name of the trait should be more clear regarding what expectation to the type that implements it, and maybe some description why

clearer in 64818a6

crates/services/gas_price_service/src/fuel_gas_price_updater/tests.rs

acerone85 · 2024-09-24T11:21:09Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

    }

    Ok(())
 }
-
-fn revert_gas_price_db_to_height(


why is this fragment of code removed? Is it related to the temporary fix in combined_database.rs?

yes we don't need this code anymore since we delegate rolling back of db to FuelService

MitchTurner

Sometimes it's hard to know what I should give feedback on or just accept as a stopgap. With that in mind, I don't see any immediate need to hold this up.

MitchTurner · 2024-09-24T10:55:12Z

crates/fuel-core/src/combined_database.rs

+        S: ShutdownListener,
+    {
+        if let Some(on_chain_height) = self.on_chain().latest_height_from_metadata()? {
+            // todo(https://github.com/FuelLabs/fuel-core/issues/2239): This is a temporary fix


Yeah. We definitely want to throw an error if this fails.

MitchTurner · 2024-09-24T11:20:49Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

+    GasPriceStore: GasPriceData
+        + MetadataStorage
+        + KeyValueInspect<Column = GasPriceColumn>
+        + Modifiable,


We should be careful about these all being bound together. Obviously this is still in an intermediate state, so maybe goes without saying, but ultimately the different ports should be handled independently. If they are the same thing under the hood, that shouldn't be leaked to like this.

I also don't know the reason for making it generic in this PR; I think we can do it later when we switch from the V0 to the V1 algorithm.

xgreenx · 2024-09-24T11:27:23Z

crates/services/gas_price_service/src/ports.rs

@@ -12,3 +16,13 @@ pub trait L2Data: Send + Sync {
        height: &BlockHeight,
    ) -> StorageResult<Option<Block<Transaction>>>;
 }
+
+pub trait MetadataStorage: Send + Sync {


Why do we want to move this trait here? I thought that each service(v0 and v1) would have its own ports. It is the same for storage traits as well.

The metadata storage would remain the same since we discussed that we will keep it versioned right

It was up to you=D I was thinking that you will choose the approach with two different columns, since it is more flexible.

xgreenx · 2024-09-24T11:27:58Z

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs

@@ -36,25 +30,6 @@ fn database() -> StorageTransaction<InMemoryStorage<GasPriceColumn>> {
    InMemoryStorage::default().into_transaction()
 }

-#[tokio::test]
-async fn get_metadata__can_get_most_recent_version() {


Why do we want to remove this test?

functionally the same as set_metadata__can_set_metadata
there's already asserts for getting the metadata in that test.

No. They are testing different functions. It doesn't matter if they are redundant, the point isn't coverage. We should keep it.

addressed in 62343d7

xgreenx · 2024-09-24T11:31:00Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

+    GasPriceStore: GasPriceData
+        + MetadataStorage
+        + KeyValueInspect<Column = GasPriceColumn>
+        + Modifiable,


I also don't know the reason for making it generic in this PR; I think we can do it later when we switch from the V0 to the V1 algorithm.

xgreenx · 2024-09-24T11:31:29Z

crates/fuel-core/src/service/adapters/gas_price_adapters.rs

+    }
+}
+
+impl MetadataStorage for Database<GasPriceDatabase> {


Why do we want to duplicate the implementation here instead of deriving the blanket implementation from gas price service crate?

addressed in 417459d

rymnc · 2024-09-25T15:10:02Z

already merged into master

rymnc added the no changelog Skip the CI check of the changelog modification label Sep 20, 2024

rymnc self-assigned this Sep 20, 2024

rymnc changed the base branch from master to chore/gas-price-service-ports September 20, 2024 07:14

rymnc force-pushed the chore/gas-price-service-ports-p2 branch from 588fa87 to a0a97e8 Compare September 20, 2024 07:40

rymnc force-pushed the chore/gas-price-service-ports branch from 352cd0b to 100fee8 Compare September 20, 2024 08:03

rymnc force-pushed the chore/gas-price-service-ports-p2 branch 2 times, most recently from ed1ad85 to 97b1cf0 Compare September 20, 2024 08:06

rymnc force-pushed the chore/gas-price-service-ports branch from 100fee8 to d45a323 Compare September 20, 2024 08:10

rymnc force-pushed the chore/gas-price-service-ports-p2 branch from 97b1cf0 to 4096322 Compare September 20, 2024 10:14

Base automatically changed from chore/gas-price-service-ports to master September 20, 2024 11:17

chore(gas_price_service): define port for gas price db lookups

5702371

rymnc force-pushed the chore/gas-price-service-ports-p2 branch from 4096322 to 5702371 Compare September 20, 2024 11:29

rymnc commented Sep 20, 2024

View reviewed changes

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs Outdated Show resolved Hide resolved

rymnc mentioned this pull request Sep 20, 2024

chore(gas_price_service): remove dependency on Config from fuel-core #2230

Closed

9 tasks

fix: pass metadata by ref

ac699b9

MitchTurner previously approved these changes Sep 20, 2024

View reviewed changes

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs Outdated Show resolved Hide resolved

rymnc marked this pull request as ready for review September 20, 2024 13:16

rymnc requested review from xgreenx and Dentosal as code owners September 20, 2024 13:16

rymnc requested a review from a team September 20, 2024 13:16

netrome reviewed Sep 20, 2024

View reviewed changes

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs Outdated Show resolved Hide resolved

...ces/gas_price_service/src/fuel_gas_price_updater/fuel_core_storage_adapter/metadata_tests.rs Outdated Show resolved Hide resolved

xgreenx reviewed Sep 20, 2024

View reviewed changes

Merge branch 'master' into chore/gas-price-service-ports-p2

1c296b4

rymnc dismissed MitchTurner’s stale review via 2735d80 September 23, 2024 09:23

chore: use structured storage strat

100b5b8

rymnc force-pushed the chore/gas-price-service-ports-p2 branch from 2735d80 to 100b5b8 Compare September 23, 2024 09:30

chore: rollback centrally

9575a21

rymnc requested review from MitchTurner and xgreenx September 23, 2024 09:40

rymnc added 2 commits September 23, 2024 16:35

fix: dont sync if nothing in onchain db

e5f5d92

test: swallow error for soft rollback

b2aa337

rymnc commented Sep 23, 2024

View reviewed changes

crates/fuel-core/src/combined_database.rs Outdated Show resolved Hide resolved

fix: log warning if rollback failed

2953019

acerone85 reviewed Sep 24, 2024

View reviewed changes

crates/services/gas_price_service/src/fuel_gas_price_updater/tests.rs Show resolved Hide resolved

acerone85 reviewed Sep 24, 2024

View reviewed changes

MitchTurner previously approved these changes Sep 24, 2024

View reviewed changes

xgreenx reviewed Sep 24, 2024

View reviewed changes

fix: retain test

62343d7

rymnc dismissed MitchTurner’s stale review via 62343d7 September 24, 2024 12:58

rymnc added 4 commits September 24, 2024 18:30

chore: add comment

d51d494

fix: remove impl for Database

417459d

fix: docstring

2e6a9d3

fix: improved comment for GasPriceData

64818a6

rymnc requested review from xgreenx and MitchTurner September 25, 2024 04:17

Merge branch 'master' into chore/gas-price-service-ports-p2

986ef13

xgreenx approved these changes Sep 25, 2024

View reviewed changes

MitchTurner approved these changes Sep 25, 2024

View reviewed changes

Merge branch 'master' into chore/gas-price-service-ports-p2

b439c8d

rymnc closed this Sep 25, 2024

rymnc deleted the chore/gas-price-service-ports-p2 branch September 25, 2024 15:10

rymnc mentioned this pull request Sep 27, 2024

chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService #2256

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gas_price_service): define port for GasPriceDatabase #2226

chore(gas_price_service): define port for GasPriceDatabase #2226

rymnc commented Sep 20, 2024 •

edited

Loading

MitchTurner commented Sep 20, 2024

netrome left a comment

xgreenx Sep 20, 2024

xgreenx Sep 20, 2024

rymnc Sep 23, 2024

xgreenx Sep 20, 2024

MitchTurner Sep 23, 2024 •

edited

Loading

MitchTurner Sep 23, 2024

rymnc Sep 23, 2024

rymnc Sep 23, 2024

rymnc Sep 23, 2024

acerone85 Sep 24, 2024

rymnc Sep 24, 2024

xgreenx Sep 24, 2024

rymnc Sep 24, 2024

acerone85 Sep 24, 2024

rymnc Sep 24, 2024

MitchTurner left a comment

MitchTurner Sep 24, 2024

MitchTurner Sep 24, 2024

xgreenx Sep 24, 2024

xgreenx Sep 24, 2024

rymnc Sep 24, 2024

xgreenx Sep 24, 2024

xgreenx Sep 24, 2024

rymnc Sep 24, 2024

MitchTurner Sep 24, 2024 •

edited

Loading

rymnc Sep 24, 2024

rymnc Sep 24, 2024

xgreenx Sep 24, 2024

xgreenx Sep 24, 2024

rymnc Sep 24, 2024

rymnc commented Sep 25, 2024

		@@ -51,41 +34,6 @@ mod l2_source_tests;

		pub mod storage;

		impl<Storage> MetadataStorage for StructuredStorage<Storage>

chore(gas_price_service): define port for GasPriceDatabase #2226

chore(gas_price_service): define port for GasPriceDatabase #2226

Conversation

rymnc commented Sep 20, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

MitchTurner commented Sep 20, 2024

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

MitchTurner Sep 23, 2024 • edited Loading

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

MitchTurner 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

MitchTurner Sep 24, 2024 • edited Loading

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

rymnc commented Sep 25, 2024

rymnc commented Sep 20, 2024 •

edited

Loading

MitchTurner Sep 23, 2024 •

edited

Loading

MitchTurner Sep 24, 2024 •

edited

Loading