Remove option on shared algo, give default algo, reorder service startup #2113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xgreenx
reviewed
Aug 20, 2024
| @@ -332,11 +332,10 @@ pub fn init_sub_services( | |||
| #[allow(unused_mut)] | |||
| // `FuelService` starts and shutdowns all sub-services in the `services` order | |||
| let mut services: SubServices = vec
There was a problem hiding this comment.
We can move graph_ql to the off-chian worker and add it there.
…ore-ready # Conflicts: # CHANGELOG.md # bin/fuel-core/chainspec/local-testnet/state_transition_bytecode.wasm
xgreenx
approved these changes
Aug 21, 2024
rymnc
approved these changes
Aug 21, 2024
netrome
approved these changes
Aug 21, 2024
There was a problem hiding this comment.
Looks mostly good to me.
However, I would prefer to avoid returning arbitrary default values in favor of using stronger typing. I.e. if a value can be unset we should return an Option as per the previous behavior.
I understand that we implicitly guarantee by the changed startup order of the sub services that the default values will never be exposed to a caller, this means that the default logic introduced in this PR is dead code - and it would be nice if we could get rid of it.
Since that may be a very complex change, and perhaps not worth prioritizing right now I am alright with leaving things as-is for now. But if anyone thinks it might be feasible to look into this, it would be nice to create a follow-up ticket for it.
| @@ -79,7 +79,7 @@ where | |||
| } | |||
|
|
|||
| #[derive(Debug, Default)] | |||
| pub struct SharedGasPriceAlgo<A>(Arc<RwLock<Option<A>>>); | |||
| pub struct SharedGasPriceAlgo<A>(Arc<RwLock<A>>); | |||
There was a problem hiding this comment.
Nit (unrelated to this PR): I found it a bit confusing to read this struct, since the declaration has nothing to do with the fact that the content is a gas price algorithm. Since the type is very generic, I'd find it easier to read if it was just named Shared.
Merged
xgreenx
added a commit
that referenced
this pull request
Aug 28, 2024
## Version v0.35.0 ### Added - [2122](#2122): Changed the relayer URI address to be a vector and use a quorum provider. The `relayer` argument now supports multiple URLs to fetch information from different sources. - [2119](#2119): GraphQL query fields for retrieving information about upgrades. ### Changed - [2113](#2113): Modify the way the gas price service and shared algo is initialized to have some default value based on best guess instead of `None`, and initialize service before graphql. - [2112](#2112): Alter the way the sealed blocks are fetched with a given height. - [2120](#2120): Added `submitAndAwaitStatus` subscription endpoint which returns the `SubmittedStatus` after the transaction is submitted as well as the `TransactionStatus` subscription. - [2115](#2115): Add test for `SignMode` `is_available` method. - [2124](#2124): Generalize the way p2p req/res protocol handles requests. #### Breaking - [2040](#2040): Added full `no_std` support state transition related crates. The crates now require the "alloc" feature to be enabled. Following crates are affected: - `fuel-core-types` - `fuel-core-storage` - `fuel-core-executor` - [2116](#2116): Replace `H160` in config and cli options of relayer by `Bytes20` of `fuel-types` ### Fixed - [2134](#2134): Perform RecoveryID normalization for AWS KMS -generated signatures. ## What's Changed * Change TODO clippy to wait for false positive correction by @AurelienFT in #2110 * Remove option on shared algo, give default algo, reorder service startup by @MitchTurner in #2113 * chore(sealed_blocks): optimize fetching of sealed block header at a given height by @rymnc in #2112 * chore(p2p_service): remove unnecessary cast to usize by @rymnc in #2123 * Added a benchmark for the predicate with ed19 verification by @xgreenx in #2127 * Add test SignMode is_available by @AurelienFT in #2115 * feat: Add graphql query fields for retreiving information about upgrades by @netrome in #2119 * Small code optimizations by @MoneyBund in #2035 * Change relayer URI address to be vector and use quorum provider by @AurelienFT in #2122 * chore(p2p_service): clean up processing of p2p req/res protocol by @rymnc in #2124 * feat: add `submitAndAwaitStatus` subscription endpoint by @maschad in #2120 * Replace H160 in config and cli options of relayer by Bytes20 by @AurelienFT in #2116 * Secp256k1 RecoveryID correction for KMS-genrated signatures by @Dentosal in #2134 * Added `no_std` support state transition related crates by @xgreenx in #2040 ## New Contributors * @MoneyBund made their first contribution in #2035 * @maschad made their first contribution in #2120 **Full Changelog**: v0.34.0...v0.35.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Linked Issues/PRs
Closes: #2107
Description
This code will help ensure that some gas price algorithm is provided to the consumers. We do this by removing the
Optionand always initializing with our best value. We also prioritize the initialization of the gas price service.Checklist
Before requesting review
After merging, notify other teams