-
Notifications
You must be signed in to change notification settings - Fork 49
Decentralization of configuration parameters phase 1 #2702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Decentralization of configuration parameters phase 1 #2702
Conversation
Test Results 4 files ± 0 168 suites +4 23m 26s ⏱️ -34s Results for commit 9ccbbe3. ± Comparison against base commit 098106d. This pull request removes 1 and adds 27 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
43f79ef
to
b890de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
3682084
to
910faf9
Compare
0f0d971
to
d4a42de
Compare
683fd96
to
73af399
Compare
e944a36
to
4b89712
Compare
…rait to retrieve epoch setting
…th available signe entity type and config
…MithrilNetworkConfiguration, adapt inform_epoch_settings signature
…f Aggregator client for the preloader checker
6c81af6
to
1a65b8c
Compare
1a65b8c
to
6488a66
Compare
…ork configuration
|
||
/// No signer registration round opened yet | ||
#[error("a signer registration round is not opened yet, please try again later")] | ||
RegistrationRoundNotYetOpened(#[source] StdError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this variant is used in this context. You can probably remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add this at the beginning of the lib.rs
to enforce correct documentation of public items in the crate:
#![warn(missing_docs)]
[package] | ||
name = "mithril-protocol-config" | ||
version = "0.1.0" | ||
description = "Configuraton parameters of the mithril network" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = "Configuraton parameters of the mithril network" | |
description = "Configuraton parameters for Mithril networks" |
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "signal"] } | ||
|
||
[dev-dependencies] | ||
#criterion = { version = "0.7.0", features = ["html_reports", "async_tokio"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed:
#criterion = { version = "0.7.0", features = ["html_reports", "async_tokio"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could let the aggregator_client
module outside of the http_client
as it is going to be replaced with the internal shared implementation. Like that you would have a single file module right away.
|
||
use crate::model::MithrilNetworkConfiguration; | ||
|
||
/// Trait to provide the current Mithril network configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Trait to provide the current Mithril network configuration. | |
/// A provider for the Mithril network configuration of the current epoch. |
@@ -0,0 +1,11 @@ | |||
/// HTTP request timeout duration in milliseconds | |||
const HTTP_REQUEST_TIMEOUT_DURATION: u64 = 30000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be moved in the aggregator_client
module.
async fn get_time_point(&self) -> Result<TimePoint, Error> { | ||
let time_point = self.ticker_service.get_current_time_point().await?; | ||
|
||
Ok(time_point) | ||
} | ||
|
||
pub async fn change_allowed_discriminants( | ||
&self, | ||
discriminants: &BTreeSet<SignedEntityTypeDiscriminants>, | ||
) { | ||
let mut available_signed_entity_types = self.available_signed_entity_types.write().await; | ||
*available_signed_entity_types = discriminants.clone(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these functions? If not, maybe you can get rid of RwLock
which are only interesting here to provide interior mutability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes its used in state_machine_tester.rs
by function aggregator_allow_signed_entities
, function called in test_create_cardano_transaction_single_signature
(in mithril-signer) to setup specific signed entity type discriminant (we add the same mecanism with Signer epoch setting before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_time_point
was used to have a Epoch align with ticker service and with other epochs used in the test test_create_cardano_transaction_single_signature
, but since I switch back to epoch from epoch_settings (here) It can be replace with a Epoch::dummy()
instead (since its not read for now)
let time_point = self.get_time_point().await?; | ||
|
||
let available_signed_entity_types = self.available_signed_entity_types.read().await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let time_point = self.get_time_point().await?; | |
let available_signed_entity_types = self.available_signed_entity_types.read().await; | |
let time_point = self.get_time_point().await?; | |
let available_signed_entity_types = self.available_signed_entity_types.read().await; |
pub signed_entity_types_config: | ||
HashMap<SignedEntityTypeDiscriminants, SignedEntityTypeConfiguration>, | ||
|
||
ticker_service: Arc<MithrilTickerService>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably use an Epoch
here instead of a ticker service.
#[async_trait] | ||
pub trait MithrilNetworkConfigurationProvider: Sync + Send { | ||
/// Get the Mithril network configuration for the current epoch. | ||
async fn get(&self) -> StdResult<MithrilNetworkConfiguration>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is more explicit:
async fn get(&self) -> StdResult<MithrilNetworkConfiguration>; | |
async fn get_network_configuration(&self) -> StdResult<MithrilNetworkConfiguration>; |
|
||
/// Protocol cryptographic parameters | ||
#[derive(Clone, Debug, Serialize, Deserialize)] | ||
#[derive(Clone, Debug, Serialize, Deserialize, Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to provide a default implementation for the protocol parameters:
#[derive(Clone, Debug, Serialize, Deserialize, Default)] | |
#[derive(Clone, Debug, Serialize, Deserialize)] |
/// This trait is mainly intended for mocking. | ||
#[async_trait] | ||
pub trait Runner: Send + Sync { | ||
///Fetch the configuration parameters of a Mithril network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///Fetch the configuration parameters of a Mithril network | |
/// Fetch the configuration parameters of the Mithril network |
async fn inform_epoch_settings(&self, epoch_settings: SignerEpochSettings) -> StdResult<()>; | ||
async fn inform_epoch_settings( | ||
&self, | ||
signer_epoch: Epoch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more the epoch retrieved from the aggregator than from the signer:
signer_epoch: Epoch, | |
aggregator_epoch: Epoch, |
Or better:
signer_epoch: Epoch, | |
aggregator_signer_registration_epoch: Epoch, |
let network_configuration_service = Arc::new(FakeMithrilNetworkConfigurationProvider::new( | ||
Default::default(), | ||
Default::default(), | ||
Default::default(), | ||
ticker_service.clone(), | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instantiation of the Fake provider is hard to read with all these Default::default()
, can you try to make it more readable?
let epoch_settings = SignerEpochSettings { | ||
epoch: Epoch(1), | ||
..SignerEpochSettings::dummy() | ||
// Signers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Signers |
} else if let Some(epoch_settings) = self | ||
} else if let Some(signer_registrations) = self | ||
.runner | ||
.get_epoch_settings() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function could be renamed for more readability:
.get_epoch_settings() | |
.get_signer_registrations_from_aggregator() |
.get_mithril_network_configuration() | ||
.await | ||
.map_err(|e| RuntimeError::KeepState { | ||
message: "could not retrieve mithril network configuration".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message: "could not retrieve mithril network configuration".to_string(), | |
message: "could not retrieve Mithril network configuration".to_string(), |
|
||
// Epoch settings | ||
let epoch_settings = SignerEpochSettings { | ||
//MithrilNetworkConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//MithrilNetworkConfiguration | |
// MithrilNetworkConfiguration |
let mut signed_entity_types_config = HashMap::new(); | ||
signed_entity_types_config.insert( | ||
SignedEntityTypeDiscriminants::CardanoTransactions, | ||
SignedEntityTypeConfiguration::CardanoTransactions( | ||
CardanoTransactionsSigningConfig::dummy(), | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also avoid creating a mutable map by using HashMap::from
.
|
||
#[tokio::test] | ||
async fn retrieve_epoch_settings() { | ||
//TODO split in two tests, to test only signers from retrieve_epoch_settings and CardanoTransactionsSigningConfig in a mithril_network_configuration_provider test ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this TODO?
|
||
#[tokio::test] | ||
async fn retrieve_aggregator_features() { | ||
//TODO aggregator features are now retrieved with mithril_network_configuration_provider, adapt or remove test and implementation ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove this TODO?
reqwest = { workspace = true, features = [ | ||
"default", | ||
"stream", | ||
"gzip", | ||
"zstd", | ||
"deflate", | ||
"brotli" | ||
] } | ||
semver = { workspace = true } | ||
serde = { workspace = true } | ||
serde_json = { workspace = true } | ||
slog = { workspace = true, features = [ | ||
"max_level_trace", | ||
"release_max_level_debug", | ||
] } | ||
thiserror = { workspace = true } | ||
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "signal"] } | ||
|
||
[dev-dependencies] | ||
#criterion = { version = "0.7.0", features = ["html_reports", "async_tokio"] } | ||
http = "1.3.1" | ||
httpmock = "0.7.0" | ||
mockall = { workspace = true } | ||
slog-async = { workspace = true } | ||
slog-term = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In library crates if a feature is not strictly needed by the business code, we should not enable them, as rust feature unification will enforce them on child crates.
Especially features on reqwest
, slog
, and tokio
as they may involve building additional dependencies that childs crates want to control, or even worse the **max_level**
features of slog
as they will remove the ability to configure them on child crates.
reqwest = { workspace = true, features = [ | |
"default", | |
"stream", | |
"gzip", | |
"zstd", | |
"deflate", | |
"brotli" | |
] } | |
semver = { workspace = true } | |
serde = { workspace = true } | |
serde_json = { workspace = true } | |
slog = { workspace = true, features = [ | |
"max_level_trace", | |
"release_max_level_debug", | |
] } | |
thiserror = { workspace = true } | |
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "signal"] } | |
[dev-dependencies] | |
#criterion = { version = "0.7.0", features = ["html_reports", "async_tokio"] } | |
http = "1.3.1" | |
httpmock = "0.7.0" | |
mockall = { workspace = true } | |
slog-async = { workspace = true } | |
slog-term = { workspace = true } | |
reqwest = { workspace = true } | |
semver = { workspace = true } | |
serde = { workspace = true } | |
serde_json = { workspace = true } | |
slog = { workspace = true } | |
thiserror = { workspace = true } | |
tokio = { workspace = true } | |
[dev-dependencies] | |
http = "1.3.1" | |
httpmock = "0.7.0" | |
mockall = { workspace = true } | |
slog-async = { workspace = true } | |
slog-term = { workspace = true } | |
tokio = { workspace = true, features = ["macros"] } |
@@ -0,0 +1,11 @@ | |||
/// HTTP request timeout duration in milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing missing_docs
clippy warn and a lib documentation.
Note
This warning addition will results in additional clippy warnings that must be solved if some public symbols or modules are undocumented.
/// HTTP request timeout duration in milliseconds | |
#![warn(missing_docs)] | |
//! This crate provides mechanisms to read and check the configuration parameters of a Mithril network. | |
/// HTTP request timeout duration in milliseconds |
use anyhow::anyhow; | ||
use std::{collections::HashMap, sync::Arc, time::Duration}; | ||
|
||
use crate::{ | ||
HTTP_REQUEST_TIMEOUT_DURATION, | ||
http_client::aggregator_client::{AggregatorClient, AggregatorHTTPClient}, | ||
interface::MithrilNetworkConfigurationProvider, | ||
model::{MithrilNetworkConfiguration, SignedEntityTypeConfiguration}, | ||
}; | ||
use async_trait::async_trait; | ||
use mithril_common::api_version::APIVersionProvider; | ||
use mithril_common::{StdResult, entities::SignedEntityTypeDiscriminants}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to enforce the following import order ?
use std and third parties
[blank line]
use mithril libraries
[blank line]
use crate
Note
You may split std and third parties imports with a blank line as well if you want (with std first).
Important
This comment apply to the whole crate
use anyhow::anyhow; | |
use std::{collections::HashMap, sync::Arc, time::Duration}; | |
use crate::{ | |
HTTP_REQUEST_TIMEOUT_DURATION, | |
http_client::aggregator_client::{AggregatorClient, AggregatorHTTPClient}, | |
interface::MithrilNetworkConfigurationProvider, | |
model::{MithrilNetworkConfiguration, SignedEntityTypeConfiguration}, | |
}; | |
use async_trait::async_trait; | |
use mithril_common::api_version::APIVersionProvider; | |
use mithril_common::{StdResult, entities::SignedEntityTypeDiscriminants}; | |
use anyhow::anyhow; | |
use async_trait::async_trait; | |
use std::{collections::HashMap, sync::Arc, time::Duration}; | |
use mithril_common::api_version::APIVersionProvider; | |
use mithril_common::{StdResult, entities::SignedEntityTypeDiscriminants}; | |
use crate::{ | |
HTTP_REQUEST_TIMEOUT_DURATION, | |
http_client::aggregator_client::{AggregatorClient, AggregatorHTTPClient}, | |
interface::MithrilNetworkConfigurationProvider, | |
model::{MithrilNetworkConfiguration, SignedEntityTypeConfiguration}, | |
}; |
edition = { workspace = true } | ||
homepage = { workspace = true } | ||
license = { workspace = true } | ||
repository = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other internal crates add a include
property, but since there's no .gitignore
it may not be needed here.
repository = { workspace = true } | |
repository = { workspace = true } | |
include = ["**/*.rs", "Cargo.toml", "README.md", ".gitignore"] |
|
||
/// Protocol cryptographic parameters | ||
#[derive(Clone, Debug, Serialize, Deserialize)] | ||
#[derive(Clone, Debug, Serialize, Deserialize, Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Default
impl should not be added lightly, and should not be added for test purpose (use Dummy
or fake_data
instead)
#[derive(Clone, Debug, Serialize, Deserialize, Default)] | |
#[derive(Clone, Debug, Serialize, Deserialize)] |
let metrics_service = Arc::new(MetricsService::new(self.root_logger())?); | ||
let preloader_activation = | ||
CardanoTransactionsPreloaderActivationSigner::new(aggregator_client.clone()); | ||
let network_configuration_service: Arc<dyn MithrilNetworkConfigurationProvider> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the explicit Arc<dyn MithrilNetworkConfigurationProvider>
type necessary ?
async fn transition_from_unregistered_to_one_of_registered_states( | ||
&self, | ||
epoch_settings: SignerEpochSettings, | ||
signer_epoch: Epoch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it came from the epoch settings it's not the signer epoch, it's the aggregator epoch.
signer_epoch: Epoch, | |
aggregator_epoch: Epoch, |
This have some implications that we should discuss, by the way it was already the case before but we simply named it epoch
so the we missed that point.
@@ -1,20 +1,17 @@ | |||
use anyhow::Error; | |||
use chrono::Local; | |||
use mithril_protocol_config::model::MithrilNetworkConfiguration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this import below the mithril import imports ?
use anyhow::anyhow; | ||
use chrono::DateTime; | ||
use mockall::predicate; | ||
use mithril_protocol_config::model::MithrilNetworkConfiguration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already imported in the parent context :).
use mithril_protocol_config::model::MithrilNetworkConfiguration; |
&mut self, | ||
epoch_settings: SignerEpochSettings, | ||
allowed_discriminants: BTreeSet<SignedEntityTypeDiscriminants>, | ||
signer_epoch: Epoch, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aggregator or signer epoch ? See comment in state_machine.rs
Content
Introduce a new crate called
mithril-protocol-config
to retrieve information about the Mithril Network (like epoch settings and aggregator features).This crate contains a new trait,
MithrilNetworkConfigurationProvider
, and a first implementation calling Aggregator REST API. (in the future, for the purpose of decentralization, a new implementation will be created reading the Cardanao chain).Crate
mithril-signer
use this new trait instead of using its internalAggregatorClient
.Pre-submit checklist
Issue(s)
Relates to #YYY