[refactor] #2118: WIP: `Default` config #2583

ilchu · 2022-08-02T15:36:33Z

Signed-off-by: Ilia Churin [email protected]

Description of the Change

A new trait Combine in iroha_config_base::proxy and its corresponding error
Refactor iroha_config_base::derive crate into modules
Proxy derive macro
Deeper refactor of Configurable trait to make it more readable
Make Proxy completely conform to Configurable and implement all its methods
Placeholder trait DocsDefault to make kagami work for the time being

All of these changes won't probably go into a final real PR (quite likely nothing in its current form besides the crate refactor), so instead this draft may serve as a ground for comments that can become guidelines for my future work. Moreover, the code doesn't even compile at the time of writing due to the unfinished point 5 from the list above.

Issue

Was part of work towards #2118, but in a discussion with @appetrosyan it was decided to make it an epic and split it into more smaller ones to make the progress more trackable.

The epic for tracking: #2585.

Benefits

Ideally, the end result should not only allow flexible loading of configuration, but also give informative messages/suggestions in case of failure. E.g. if no keys were provided, suggest a command to generate them (or have a flag for generation on the fly). Moreover, it wouldn't hurt if at start Iroha logs explicitly, which parts of configs are loaded as default values due to them not being provided.

Possible Drawbacks

Current design of Configurable trait suffers from interface multiplicity, that is, it serves two functions at once which should eventually be decoupled: things pertaining to configuration loading and initialization, and all the things related to querying it or modifying parameters at runtime.

Alternate Designs

Not necessarily alternate designs, but a few things mentioned in previous discussion:

To save space, some of the values in config proxies could be stored as ConstStr.
Typechecking in procedural macros. This is more of a general issue relating to all our macro work. Instead of trying to naively check some AST node ident against a string, it would make sense to have a small function that's part of the macro expansion doing this job.
Despite all of the fields in ConfigProxy being wrapped in Option to signify the possibility of them being uninitialized, there are some fields which have to be initialized eventually, and those that don't and can instead rely on the default value. This distinction could be marked by having a separate struct for the non-defaultable option fields.

Signed-off-by: Ilia Churin <[email protected]>

ilchu · 2022-08-02T15:43:59Z

config/base/src/lib.rs

+        /// from the length of the array in
+        /// [`self.sumeragi.trusted_peers.peers`], most likely due to two
+        /// (or more) peers having the same public key.
+        fn from_path<P: AsRef<Path> + Debug + Clone>(path: P) -> eyre::Result<Self, eyre::Error> {


Don't know how robust it is to try to make a default from_path method for all proxy structs, but there definitely is some space for improvement in existing code, as we have 3 or 4 implementations of almost the same function scattered as impl Configuration here and there.

Should be a separate trait. I'd say LoadFromDisk and make it a little richer.

ilchu · 2022-08-02T15:46:20Z

tools/kagami/src/main.rs

@@ -209,7 +210,7 @@ mod docs {

    use super::*;

-    impl<E: Debug, C: Configurable<Error = E> + Send + Sync + Default> PrintDocs for C {}
+    impl<E: Debug, C: Configurable<Error = E> + Send + Sync + DocsDefault> PrintDocs for C {}


This seemed to work as a stopgap when we take away the #[serde(default)] on config structs, but we need something more sophisticated here. That would be done at a later stage.

Create a ticket, reference in // TODO.

ilchu · 2022-08-02T15:50:17Z

config/base/src/lib.rs

+                .wrap_err(format!("Failed to deserialize json {:?} from reader", path))
+        }
+
+        // fn finalize(&mut self) -> Result<()> {


Left commented code here as it marks an important point: even with the public/private keys being most important non-defaultable config parameters, they probably don't need to be specified in all the overlapping structures. I.e. it could always be enough to specify it either in torii or iroha config only, and then propagate it into the empty one. Something evolved from this func could be a mechanism to help that.

Good point. I'd add this to the front of the PR and to the description of the ticket that fixes it.

Also consider making it a doc-test.

Maybe we should consider making keys it's own thing and then pass independently to parts of system that reqiures them?
Like we have Secrets and ConfigMaps in k8s.

ilchu · 2022-08-02T15:55:14Z

config/base/derive/src/config.rs

+            } else if is_option {
+                quote! {
+                    #l_value = Some(serde_json::from_value(var.into())
+                        .map_err(|e| iroha_config_base::derive::Error::field_error(stringify!(#ident), e))?)
+                }
+            }


This code path should get run when deriving Configurable for our Proxy structs.

config/base/derive/src/proxy.rs

ilchu · 2022-08-02T16:02:24Z

config/base/derive/src/config.rs

+                quote! {
+                    // <#inner>::load_environment()?;
+                    #l_value.unwrap().load_environment()?;
+                    // if let Ok(config) = <#inner>::load_environment() {
+                    //     #l_value = Some(config)
+                    // };
+                }


An unfinished mess here as the whole Configurable trait is not implemented yet for proxy and I wasn't able to test this piece. What gets done to load the inner configuration of fields wrapped inside Option still needs to be thought through.

appetrosyan · 2022-08-04T09:32:04Z

cli/src/lib.rs

@@ -8,7 +8,8 @@ use std::{panic, path::PathBuf, sync::Arc};

 use color_eyre::eyre::{eyre, Result, WrapErr};
 use iroha_actor::{broker::*, prelude::*};
-use iroha_config::iroha::Configuration;
+use iroha_config::base::proxy::DocsDefault;
+use iroha_config::iroha::{Configuration, ConfigurationProxy};


Suggested change

use iroha_config::iroha::{Configuration, ConfigurationProxy};

use iroha_config::iroha::{Configuration, ConfigurableProxy};

appetrosyan · 2022-08-04T09:34:28Z

tools/kagami/src/main.rs

@@ -209,7 +210,7 @@ mod docs {

    use super::*;

-    impl<E: Debug, C: Configurable<Error = E> + Send + Sync + Default> PrintDocs for C {}
+    impl<E: Debug, C: Configurable<Error = E> + Send + Sync + DocsDefault> PrintDocs for C {}


Create a ticket, reference in // TODO.

appetrosyan · 2022-08-04T09:36:19Z

config/src/sumeragi.rs

@@ -53,7 +54,7 @@ view! {
    }
 }

-impl Default for Configuration {
+impl DocsDefault for Configuration {


Not a huge fan of DocsDefault for a trait name. Maybe ReferenceDefault? ExampleDefault?

Also, the point is that we shouldn't allow people anything resembling a Default trait. The point is to prevent this kind of usage, which we can achieve by

enforcing that the default is either implemented only for the proxy structure, and no concrete Configuration impl Default which we can check.

By encoding the default values in place where they're used. So there isn't a general Default trait, just an example struct instance.

Maybe something like trait Example with method fn example() -> Self?

appetrosyan · 2022-08-04T09:43:31Z

config/base/src/lib.rs

+    use super::*;
+
+    /// Pseudo-default trait only used for doc generation
+    pub trait DocsDefault {


Which is very much the exact same thing as Default, can be used in the same way, and people will probably abuse in the same way.

We need to get rid not only of core::Default but also of any trait that can be used like it. Adding a DocsDefault trait in the exact same spot doesn't solve anything.

config/base/src/lib.rs

appetrosyan · 2022-08-04T09:46:50Z

config/base/src/lib.rs

+        /// from the length of the array in
+        /// [`self.sumeragi.trusted_peers.peers`], most likely due to two
+        /// (or more) peers having the same public key.
+        fn from_path<P: AsRef<Path> + Debug + Clone>(path: P) -> eyre::Result<Self, eyre::Error> {


Should be a separate trait. I'd say LoadFromDisk and make it a little richer.

appetrosyan · 2022-08-04T09:47:41Z

config/base/src/lib.rs

+                .wrap_err(format!("Failed to deserialize json {:?} from reader", path))
+        }
+
+        // fn finalize(&mut self) -> Result<()> {


Good point. I'd add this to the front of the PR and to the description of the ticket that fixes it.

Also consider making it a doc-test.

appetrosyan · 2022-08-04T09:48:52Z

config/base/src/lib.rs

+                .wrap_err(format!("Failed to deserialize json {:?} from reader", path))
+        }
+
+        // fn finalize(&mut self) -> Result<()> {


Suggested change

// fn finalize(&mut self) -> Result<()> {

// fn finish(&mut self) -> Result<()> {

"-ize" in names can confuse users. Rust uses the "-ize" in all of its docs. Our compny policy is to use "-ise". To avoid confusion, use a word that has neither.

config/base/src/lib.rs

appetrosyan · 2022-08-04T09:51:05Z

config/src/client.rs

@@ -137,6 +137,7 @@ impl Configuration {
        AccountId::from_str("alice@wonderland").expect("Account ID not valid")
    }

+    // TODO: Delete this?


Just remember to do it.

cli/src/samples.rs

config/base/src/lib.rs

Erigara · 2022-08-04T11:34:07Z

config/base/src/lib.rs

+                .wrap_err(format!("Failed to deserialize json {:?} from reader", path))
+        }
+
+        // fn finalize(&mut self) -> Result<()> {


Maybe we should consider making keys it's own thing and then pass independently to parts of system that reqiures them?
Like we have Secrets and ConfigMaps in k8s.

Erigara · 2022-08-04T11:38:54Z

config/base/src/lib.rs

@@ -13,6 +15,7 @@ pub mod derive {
    /// View contains a subset of the fields that the type has.
    ///
    /// Works only with structs.
+    // TODO: alter as won't be true after yeeting [`Default`]


I belive that we already can drop Default requirement and remove part of macro that produce Default for View.
Since we have only one way conversation Config -> View.

Erigara · 2022-08-04T11:46:26Z

config/src/sumeragi.rs

@@ -53,7 +54,7 @@ view! {
    }
 }

-impl Default for Configuration {
+impl DocsDefault for Configuration {


Maybe something like trait Example with method fn example() -> Self?

config/base/derive/src/proxy.rs

Erigara · 2022-08-04T13:04:04Z

config/base/derive/src/config.rs

+                quote! {
+                    // <#inner>::load_environment()?;
+                    #l_value.unwrap().load_environment()?;
+                    // if let Ok(config) = <#inner>::load_environment() {


#l_value = <#inner>::load_enviroment().ok()

appetrosyan · 2022-08-12T08:52:55Z

I suggest closing this PR in favour of the real PRs.

[hyperledger-iroha#2118]: WIP: Default config

45f3643

Signed-off-by: Ilia Churin <[email protected]>

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Aug 2, 2022

ilchu changed the title ~~[#2118]: WIP: Default config~~ [refactor] #2118: WIP: Default config Aug 2, 2022

ilchu commented Aug 2, 2022

View reviewed changes

config/base/derive/src/proxy.rs Show resolved Hide resolved

ilchu commented Aug 2, 2022

View reviewed changes

ilchu mentioned this pull request Aug 3, 2022

Tracking Issue for Configuration Overhaul #2585

Closed

appetrosyan reviewed Aug 4, 2022

View reviewed changes

Erigara reviewed Aug 4, 2022

View reviewed changes

appetrosyan closed this Aug 12, 2022

ilchu mentioned this pull request Aug 16, 2022

Finalize Config if needed #2627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] #2118: WIP: `Default` config #2583

[refactor] #2118: WIP: `Default` config #2583

ilchu commented Aug 2, 2022 •

edited

Loading

ilchu Aug 2, 2022

appetrosyan Aug 4, 2022

ilchu Aug 2, 2022

appetrosyan Aug 4, 2022

ilchu Aug 2, 2022

appetrosyan Aug 4, 2022

Erigara Aug 4, 2022

ilchu Aug 2, 2022

ilchu Aug 2, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

Erigara Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

appetrosyan Aug 4, 2022

Erigara Aug 4, 2022

Erigara Aug 4, 2022

Erigara Aug 4, 2022

Erigara Aug 4, 2022

appetrosyan commented Aug 12, 2022

	use iroha_config::iroha::{Configuration, ConfigurationProxy};
	use iroha_config::iroha::{Configuration, ConfigurableProxy};

	// fn finalize(&mut self) -> Result<()> {
	// fn finish(&mut self) -> Result<()> {

[refactor] #2118: WIP: Default config #2583

[refactor] #2118: WIP: Default config #2583

Conversation

ilchu commented Aug 2, 2022 • edited Loading

Description of the Change

Issue

Benefits

Possible Drawbacks

Alternate Designs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appetrosyan commented Aug 12, 2022

[refactor] #2118: WIP: `Default` config #2583

[refactor] #2118: WIP: `Default` config #2583

ilchu commented Aug 2, 2022 •

edited

Loading