Skip to content
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

[suggestion] Separate "user" config from "resolved" config #3500

Closed
0x009922 opened this issue May 21, 2023 · 3 comments
Closed

[suggestion] Separate "user" config from "resolved" config #3500

0x009922 opened this issue May 21, 2023 · 3 comments
Labels
config-changes Changes in configuration and start up of the Iroha Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@0x009922
Copy link
Contributor

0x009922 commented May 21, 2023

Feature request

It is already done with Configuration (i.e. resolved) and ConfigurationProxy (i.e. user) structs. The problem is that they are coupled to each other too tight, and there is not much semantic separation. Meanwhile, Iroha does have a semantic of parsing "raw" configuration and validating it.

Resolved config should have different fields. For example, user config might have public_key and private_key fields, while the resolved configuration will only have a single key_pair. It might be worth to define two separate structures (user and resolved configs) if there is a large difference between fields.

I propose to change naming as well, to reflect the purpose clearer: UserConfig instead of ConfigurationProxy, and ResolvedConfig instead of Configuration. Or something like that.

Additionally, it will be helpful to print the resolved configuration (with sensitive data being sanitised) to DEBUG/TRACE level.

Motivation

The transformation from "user" to "resolved" is the great place for validation, deprecation warnings, default fallbacks, and any other custom logic.

@0x009922 0x009922 added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST config-changes Changes in configuration and start up of the Iroha labels May 21, 2023
@0x009922 0x009922 changed the title [suggestion] [suggestion] Separate "user" config from "resolved" config May 21, 2023
@mversic
Copy link
Contributor

mversic commented May 25, 2023

In code we need only two structs: Configuration and ConfigurationBuilder that produces previous. IMO there should be no such concept as UserConfig. Any conversion from a serialization format (that is provided by the user) should be done via a custom deserialization code. Additionally, I also favor having getters and making configuration fields private.

Also I would consider making the Configuration immutable. If you want to make changes to a Configuration instance degrade it into a ConfigurationBuilder and change that one

@appetrosyan
Copy link
Contributor

appetrosyan commented Jun 4, 2023

Also I would consider making the Configuration immutable. If you want to make changes to a Configuration instance degrade it into a ConfigurationBuilder and change that one

There's a better way to do it. The config itself is meaningless. What it configures is the thing for which modification makes sense. When I was dealing with #1486 and related issues, what I found was that Configuration instances should not exist for very long. Instead there should be channels that allow you to e.g. adjust the log level. In #1638 we're dealing with the one valid use case for storing the configuration after init, but it doesn't really add much unless it actually inspects the actual variables which control the behaviour.

Perhaps instead of a struct-centric approach there is a trait-based one. So each configuration parameter is its own struct. And each user of said struct has to implement methods for interpreting that information.

In the terminology of @0x009922 the user config is an ephemeral immutable struct that gets constructed once for each source and consumed, while the resolved config is the value of each variable that we have for each object.

I'll probably need to open a PR with a PoC to make it more clear.

@0x009922
Copy link
Contributor Author

Closed by #4239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-changes Changes in configuration and start up of the Iroha Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

No branches or pull requests

3 participants