Add Gas Price Updater Service #1938
Conversation
Co-authored-by: Hannes Karppila <2204863+Dentosal@users.noreply.github.com>
CHANGELOG.md
Outdated
| @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
| - [#1888](https://github.com/FuelLabs/fuel-core/pull/1888): Upgraded `fuel-vm` to `0.51.0`. See [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.51.0) for more information. | |||
|
|
|||
| ### Added | |||
| - [#1889](https://github.com/FuelLabs/fuel-core/pull/1889): Add new `FuelGasPriceProvider` that receives the gas price algorithm from a `GasPriceService` | |||
There was a problem hiding this comment.
We need to move this into unreleased section
| Self(Arc::new(RwLock::new(algo))) | ||
| } | ||
|
|
||
| pub async fn read(&self) -> RwLockReadGuard<A> { |
There was a problem hiding this comment.
It is a bad idea to return RwLockReadGuard from the service. This means anyone outside the service can lock it. Could you add two separate methods like execute and estimate?
There was a problem hiding this comment.
Yeah. You're right. I don't like it just for code cleanliness either.
To add an execute and estimate I'll have to constrain A somehow with a new trait.
The other approach would be just make A Clone, and clone it out of the lock.
There was a problem hiding this comment.
I think it is okay to add constrains for the algorithm=) Gas provider expects that algorithm will provide next gas price=D
| A: GasPriceEstimate + Send + Sync, | ||
| { | ||
| async fn worst_case_gas_price(&self, height: BlockHeight) -> u64 { | ||
| self.algorithm.read().await.estimate(height) |
There was a problem hiding this comment.
The estimate naming is too vague. It would be nice to include the idea that it is a prediction of the price change for some period.
There was a problem hiding this comment.
I changed it to match the provider: worst_case_gas_price.
| where | ||
| A: GasPriceAlgorithm + Send + Sync, | ||
| { | ||
| async fn execute_algorithm(&self, block_bytes: u64) -> u64 { |
There was a problem hiding this comment.
[nit] next_gas_price sounds more clear to me=)
| pub struct BlockFullness { | ||
| used: u64, | ||
| capacity: u64, | ||
| } | ||
|
|
||
| impl BlockFullness { | ||
| pub fn new(used: u64, capacity: u64) -> Self { | ||
| Self { used, capacity } | ||
| } | ||
|
|
||
| pub fn used(&self) -> u64 { | ||
| self.used | ||
| } | ||
|
|
||
| pub fn capacity(&self) -> u64 { | ||
| self.capacity | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems is not used anywhere
| /// The algorithm that can be used in the next block | ||
| next_block_algorithm: SharedGasPriceAlgo<A>, | ||
| /// The code that is run to update your specific algorithm | ||
| update_algorithm: U, |
There was a problem hiding this comment.
For me, it looks like the updater and algorithm should be one entity since only the algorithm knows how it works inside and what data it requires.
But we can leave it for now as it is and see how it will look like with a nonstatic algorithm.
There was a problem hiding this comment.
I'm leaning to this approach so that the consumer of the algo doesn't need to have all the generics for the updater to read the values.
Could also use dyn but that's gross :)
| let (watch_sender, watch_receiver) = tokio::sync::watch::channel(State::Started); | ||
| let watcher = StateWatcher::from(watch_receiver); |
There was a problem hiding this comment.
You can re-use helper function
| let gas_price = if let Some(inner) = gas_price { | ||
| inner | ||
| } else { | ||
| self.gas_price_provider.gas_price(max_block_bytes).await? |
There was a problem hiding this comment.
It is better to move all calculations here=) Could you move the duplicated code into a small function that we could reuse in both places, please?=)
Plus, maybe for dry_run, we could just use the previous gas price without.
crates/services/txpool/src/ports.rs
Outdated
| async fn gas_price( | ||
| &self, | ||
| block_bytes: u64, | ||
| ) -> fuel_core_types::services::txpool::Result<GasPrice>; |
There was a problem hiding this comment.
| ) -> fuel_core_types::services::txpool::Result<GasPrice>; | |
| ) -> TxPoolResult<GasPrice>; |
## Version v0.29.0 ### Added - [#1889](#1889): Add new `FuelGasPriceProvider` that receives the gas price algorithm from a `GasPriceService` ### Changed - [#1942](#1942): Sequential relayer's commits. - [#1952](#1952): Change tip sorting to ratio between tip and max gas sorting in txpool - [#1960](#1960): Update fuel-vm to v0.53.0. ### Fixed - [#1950](#1950): Fix cursor `BlockHeight` encoding in `SortedTXCursor` ## What's Changed * Fix code coverage compilation and tests by @Dentosal in #1943 * Weekly `cargo update` by @github-actions in #1949 * Fix cursor block height decoding in SortedTXCursor by @AurelienFT in #1950 * Sequential relayer's commits by @xgreenx in #1942 * Add Gas Price Updater Service by @MitchTurner in #1938 * Change tip sorting to ratio between tip and max gas sorting in txpool by @AurelienFT in #1952 * deps(client): update eventsource-client to fix CVE(s) by @Br1ght0ne in #1954 * Update fuel-vm to v0.53.0 by @Dentosal in #1960 ## New Contributors * @AurelienFT made their first contribution in #1950 **Full Changelog**: v0.28.0...v0.29.0
Closes: #1956
This is a subtask of #1624
In this PR we add the generic service that will post an algorithm for the providers to use. Not bothering with a real algorithm, that will be implemented later. For now, just show that the provider can get the value generated by the service.
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]