feat: Unified configurations #385
Open
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
snowmead
force-pushed
the
feat/unified-configurations
branch
from
b0257e4 to
c57da17
Compare
ffarall
reviewed
Mar 1, 2025
Comment on lines
+1
to
+5
| // TODO CONSTANTS | ||
| pub const MAX_PENDING_EVENTS: usize = 2000; | ||
| pub const MAX_TASKS_SPAWNED_PER_QUEUE: usize = 2000; | ||
|
|
||
| // TODO CONSTANTS |
There was a problem hiding this comment.
Thank you for spotting, marked them as constants to remove to make them cli parameters
| // Send extrinsic to increase capacity | ||
| match self.send_extrinsic(call, Default::default()).await { | ||
| match self | ||
| .send_extrinsic(call, &SendExtrinsicOptions::new(extrinsic_retry_timeout)) |
There was a problem hiding this comment.
This doesn't seem to be useful at all if we're not calling watch_transaction (which we shouldn't)
| } | ||
|
|
||
| impl SendExtrinsicOptions { | ||
| pub fn new() -> Self { | ||
| Self::default() | ||
| pub fn new(timeout: Duration) -> Self { |
There was a problem hiding this comment.
Why is this a "required" parameter now?
Comment on lines
28
to
29
| chrono = { workspace = true } | ||
|
|
||
| # Substrate |
Comment on lines
+259
to
+295
| if let Some(c) = msp_delete_file { | ||
| storage_hub_builder.with_msp_delete_file_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = msp_charge_fees { | ||
| storage_hub_builder.with_msp_charge_fees_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = msp_move_bucket { | ||
| storage_hub_builder.with_msp_move_bucket_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = bsp_upload_file { | ||
| storage_hub_builder.with_bsp_upload_file_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = bsp_move_bucket { | ||
| storage_hub_builder.with_bsp_move_bucket_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = bsp_charge_fees { | ||
| storage_hub_builder.with_bsp_charge_fees_config(c.clone()); | ||
| } | ||
|
|
||
| if let Some(c) = bsp_submit_proof { | ||
| storage_hub_builder.with_bsp_submit_proof_config(c.clone()); | ||
| } | ||
|
|
||
| // Setup specific configuration for the MSP node. | ||
| if *provider_type == ProviderType::Msp { | ||
| storage_hub_builder | ||
| .with_notify_period(*msp_charging_period) | ||
| .with_indexer_db_pool(maybe_db_pool); | ||
| } | ||
|
|
||
| if let Some(c) = blockchain_service { | ||
| storage_hub_builder.with_blockchain_service_config(c.clone()); |
There was a problem hiding this comment.
I think this would look better if all these with_... methods accepted an Option<>, so that way we can call them regardless of whether the options was passed or not. It would just look cleaner.
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.
Extracted all constants that should be exposed as cli parameters. This mostly were constants used by BSP and MSP tasks. They are now in the form of task specific configuration structs which are optionally specified by the user, otherwise they use their reasonable defaults we had already set to those previous constants.
The blockchain service now has its own configuration struct which only has at the moment the
extrinsic_retry_timeout.Another initiative outside of the scope of this change is to separate the
StorageHubHandlerinto specific actor handlers which would allow us to better handle required and non-required parameters based on the actor (provider_type) chosen by the user - #386. As of right now this change applies the same pattern as we do for the rest of the existing builder parameters, where everything is optional.I suggest we separate the builder even further into a actor specific builder structs to only expose
with_*methods that apply to those actors specifically. This would be to prevent an MSP actor adding BSP related configurations.There are 2 new configuration TOML examples in the root of the project -
msp_config.tomlandbsp_config.tomlwhich demonstrate the structure and all possible fields you can specify as input.Task specific cli parameters are grouped.