Skip to content

Commit

Permalink
Zombienet Improve DevX with new methods (#261)
Browse files Browse the repository at this point in the history
Revamped version of #252, didn't overwrite that PR in case you guys want
to compare both solutions.

Fixes #251 


TL;DR -> here is the suggestion:
```rust
let network_config = NetworkConfigBuilder::with_nodes_and_chain(
    vec!["alice".to_string(), "bob".to_string()],
    "rococo-local",
)
.with_collators_and_parachain_id(vec!["collator1".to_string(), "collator2".to_string()], 100)
.build()
.unwrap();
```

I found an even better way than creating a wrapper. Here is the summary:

- I want to protect the safety measures provided by the original crate
as we discussed.
- At the same time, I don't want to opt-out of the high-degree of
configurability that `configuration` crate offers. Even though the aim
is to grant better DevX to the community, it should still preserve the
configuration options.
- “advanced users can still use the `configuration` crate if they wanted
to” is not a good argument imo. Here is the reason:
- although there are many common settings amongst the projects in the
ecosystem, probably most of the projects only tinkers with a specific
setting w.r.t their needs, and this specific setting is most likely
changing across projects. So, if we do not expose the tinkering options
to people with this wrapper approach, most of the projects won’t use
this wrapper. Then what is the point?
- The aim should be providing the default options with a better DevX,
whilst still providing a way to configure niche details somehow within
the same API.
- As first trial, I completed simple to use wrapper builders with the
default settings. However, to expose the niche configurations, I had to
copy-paste nearly every other function/method in the `configuration`
crate.
- And also, in order to comply with the `configuration` crate’s type
state pattern, I had to export nearly all the states from
`configuration` crate for the builder types in the `lib.rs`. The whole
thing was quickly becoming ugly.
- The difference between my wrapper and the `configuration` crate was,
basically the extra methods that granted better DevX (for initializing
the structs with default settings).
- So I thought, instead of creating a new wrapper with tremendous amount
of duplications, I can simply put these new methods into the
`configuration` crate itself!

Notes:

- there are 2 new methods: (edited after last commit)
    - `with_nodes_and_chain`
    - `with_collators_and_parachain_id`
- I also expanded the tests for these new methods.
- I want to signal to the developers that these new methods are easier
and faster to use compared to their equivalents, since they are
utilizing the default settings, and should be much less intimidating to
newcomers
- So, I tried to name the methods accordingly, but they turned out to be
a bit long. Don't know whether we can do better, I'm open to all
suggestions.

Hope it makes sense!
  • Loading branch information
ozgunozerk authored Oct 3, 2024
1 parent bd51865 commit 3460ec1
Show file tree
Hide file tree
Showing 2 changed files with 191 additions and 2 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ members = [
"crates/provider",
#"crates/test-runner",
"crates/prom-metrics-parser",
"crates/file-server"
"crates/file-server",
]

[workspace.package]
Expand Down
191 changes: 190 additions & 1 deletion crates/configuration/src/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use crate::{
parachain::{self, ParachainConfig, ParachainConfigBuilder},
relaychain::{self, RelaychainConfig, RelaychainConfigBuilder},
shared::{
helpers::merge_errors_vecs,
errors::{ConfigError, ValidationError},
helpers::{merge_errors, merge_errors_vecs},
macros::states,
node::NodeConfig,
types::{Arg, AssetLocation, Chain, Command, Image, ValidationContext},
Expand Down Expand Up @@ -324,6 +325,32 @@ impl NetworkConfigBuilder<Initial> {
Self::default()
}

/// uses the default options for both the relay chain and the nodes
/// the only required fields are the name of the nodes,
/// and the name of the relay chain ("rococo-local", "polkadot", etc.)
pub fn with_chain_and_nodes(
relay_name: &str,
node_names: Vec<String>,
) -> NetworkConfigBuilder<WithRelaychain> {
let network_config = NetworkConfigBuilder::new().with_relaychain(|relaychain| {
let mut relaychain_with_node = relaychain
.with_chain(relay_name)
.with_node(|node| node.with_name(node_names.first().unwrap_or(&"".to_string())));

for node_name in node_names.iter().skip(1) {
relaychain_with_node = relaychain_with_node
.with_node(|node_builder| node_builder.with_name(node_name));
}
relaychain_with_node
});

Self::transition(
network_config.config,
network_config.validation_context,
network_config.errors,
)
}

/// Set the relay chain using a nested [`RelaychainConfigBuilder`].
pub fn with_relaychain(
self,
Expand Down Expand Up @@ -399,6 +426,43 @@ impl NetworkConfigBuilder<WithRelaychain> {
}
}

/// uses default settings for setting for:
/// - the parachain,
/// - the global settings
/// - the hrmp channels
///
/// the only required parameters are the names of the collators as a vector,
/// and the id of the parachain
pub fn with_parachain_id_and_collators(self, id: u32, collator_names: Vec<String>) -> Self {
if collator_names.is_empty() {
return Self::transition(
self.config,
self.validation_context,
merge_errors(
self.errors,
ConfigError::Parachain(id, ValidationError::CantBeEmpty().into()).into(),
),
);
}

self.with_parachain(|parachain| {
let mut parachain_config = parachain.with_id(id).with_collator(|collator| {
collator
.with_name(collator_names.first().unwrap_or(&"".to_string()))
.validator(true)
});

for collator_name in collator_names.iter().skip(1) {
parachain_config = parachain_config
.with_collator(|collator| collator.with_name(collator_name).validator(true));
}
parachain_config
})

// TODO: if need to set global settings and hrmp channels
// we can also do in here
}

/// Add an HRMP channel using a nested [`HrmpChannelConfigBuilder`].
pub fn with_hrmp_channel(
self,
Expand Down Expand Up @@ -1437,4 +1501,129 @@ mod tests {
});
});
}

#[test]
fn with_chain_and_nodes_works() {
let network_config = NetworkConfigBuilder::with_chain_and_nodes(
"rococo-local",
vec!["alice".to_string(), "bob".to_string()],
)
.build()
.unwrap();

// relaychain
assert_eq!(network_config.relaychain().chain().as_str(), "rococo-local");
assert_eq!(network_config.relaychain().nodes().len(), 2);
let mut node_names = network_config.relaychain().nodes().into_iter();
let node1 = node_names.next().unwrap().name();
assert_eq!(node1, "alice");
let node2 = node_names.next().unwrap().name();
assert_eq!(node2, "bob");

// parachains
assert_eq!(network_config.parachains().len(), 0);
}

#[test]
fn with_chain_and_nodes_should_fail_with_empty_relay_name() {
let errors = NetworkConfigBuilder::with_chain_and_nodes("", vec!["alice".to_string()])
.build()
.unwrap_err();

assert_eq!(
errors.first().unwrap().to_string(),
"relaychain.chain: can't be empty"
);
}

#[test]
fn with_chain_and_nodes_should_fail_with_empty_node_list() {
let errors = NetworkConfigBuilder::with_chain_and_nodes("rococo-local", vec![])
.build()
.unwrap_err();

assert_eq!(
errors.first().unwrap().to_string(),
"relaychain.nodes[''].name: can't be empty"
);
}

#[test]
fn with_chain_and_nodes_should_fail_with_empty_node_name() {
let errors = NetworkConfigBuilder::with_chain_and_nodes(
"rococo-local",
vec!["alice".to_string(), "".to_string()],
)
.build()
.unwrap_err();

assert_eq!(
errors.first().unwrap().to_string(),
"relaychain.nodes[''].name: can't be empty"
);
}

#[test]
fn with_parachain_id_and_collators_works() {
let network_config = NetworkConfigBuilder::with_chain_and_nodes(
"rococo-local",
vec!["alice".to_string(), "bob".to_string()],
)
.with_parachain_id_and_collators(
100,
vec!["collator1".to_string(), "collator2".to_string()],
)
.build()
.unwrap();

// relaychain
assert_eq!(network_config.relaychain().chain().as_str(), "rococo-local");
assert_eq!(network_config.relaychain().nodes().len(), 2);
let mut node_names = network_config.relaychain().nodes().into_iter();
let node1 = node_names.next().unwrap().name();
assert_eq!(node1, "alice");
let node2 = node_names.next().unwrap().name();
assert_eq!(node2, "bob");

// parachains
assert_eq!(network_config.parachains().len(), 1);
let &parachain1 = network_config.parachains().first().unwrap();
assert_eq!(parachain1.id(), 100);
assert_eq!(parachain1.collators().len(), 2);
let mut collator_names = parachain1.collators().into_iter();
let collator1 = collator_names.next().unwrap().name();
assert_eq!(collator1, "collator1");
let collator2 = collator_names.next().unwrap().name();
assert_eq!(collator2, "collator2");

assert_eq!(parachain1.initial_balance(), 2_000_000_000_000);
}

#[test]
fn with_parachain_id_and_collators_should_fail_with_empty_collator_list() {
let errors =
NetworkConfigBuilder::with_chain_and_nodes("polkadot", vec!["alice".to_string()])
.with_parachain_id_and_collators(1, vec![])
.build()
.unwrap_err();

assert_eq!(
errors.first().unwrap().to_string(),
"parachain[1].can't be empty"
);
}

#[test]
fn with_parachain_id_and_collators_should_fail_with_empty_collator_name() {
let errors =
NetworkConfigBuilder::with_chain_and_nodes("polkadot", vec!["alice".to_string()])
.with_parachain_id_and_collators(1, vec!["collator1".to_string(), "".to_string()])
.build()
.unwrap_err();

assert_eq!(
errors.first().unwrap().to_string(),
"parachain[1].collators[''].name: can't be empty"
);
}
}

0 comments on commit 3460ec1

Please sign in to comment.