Zombienet Improve DevX with new methods

[ozgunozerk]

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:

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!

[jmg-duarte]

I'm sorry but I fail to see how this is any better than the previous ideas. It seems like a shortcut, not an improvement. Furthermore, it seems to me that you're still misusing/misunderstanding the typestate pattern.

It's pretty clear to me that you see the value in restricting the APIs the user has access and when, but the way you're going about it shows you're missing a bigger and much more import part of the puzzle.

The whole point of typestates is to restrict the possible state and transition space to one where you can't go into invalid/inconsistent states. However, you're doing precisely that by allowing a transition to WithRelaychain where the relay chain has an empty name, effectively made worse by the fact that you're effectively lying to the user by saying you were able to transition to WithRelaychain when that is not true!!

I get that you don't want to abandon the typestate pattern and taking it out is too much work now (even though I believe it would be beneficial for both the library authors and the users), but this is not a solution to the DX issue. It does not solve the fact that you're lying about the true current state which then leads to providing defaults that you can't trust.

In #251 I provided suggestions (and backing to them) that would mitigate this issues, it seems to me that they were not taken seriously, which frustrates me as a user of the library and as just someone that spent time trying to provide constructive feedback. If this iterating process is not meant to include/consider outsider (users) feedback, then either move it to a private channel or just note it as such.

[ozgunozerk]

@jmg-duarte all your comments have been packed with information and I appreciate them greatly! I read all of them really carefully.

However I assume your contributions here are 100% voluntary just as mines. Nobody here is obliged to obey or working under anyone. The condescending tone in your comment is uncalled for and does not resonate with the open-source culture. I'm spending time and effort on this voluntarily because I simply want to improve the ecosystem.

Having said that, I really appreciate the content and information you provide, so let me address your concerns one by one:

The whole point of typestates is to restrict the possible state and transition space to one where you can't go into invalid/inconsistent states. However, you're doing precisely that by allowing a transition to WithRelaychain where the relay chain has an empty name, effectively made worse by the fact that you're effectively lying to the user by saying you were able to transition to WithRelaychain when that is not true!!

You say it like this is something I'm introducing with my code, but the very same issues also exist for the current code: https://github.com/paritytech/zombienet-sdk/blob/main/crates/configuration/src/network.rs#L328
where with_relaychain function calls RelaychainConfigBuilder's build() method, and if it fails to build a relaychain, it still transitions into WithRelaychain state (but it propagates the error to end).

So, yes, at first, I thought new_with_defaults() should return a Result, and return early if the name for relaychain is incorrect/empty, so that we prevent transitioning into that state (same goes for node_names). I should have left that as a TODO, or mentioned that in the PR body, my bad on that. Then, I discovered how this library handles error. Because this very error you mention is also present for the current code from what I understood. We can for sure improve it. My main motivation for submitting this code was to comply with the existing strategies of this library as much as I can, without introducing a breaking change, since I'm not writing a new crate, but new methods.

I also would prefer to not transition into that state and return a Result instead for example (or any other approach that will prevent transitioning into the next state (although is it propagating the error). So if @pepoviola also agrees, I can gladly introduce this breaking change and make both my method and the current method in the library safer in this regard.

In #251 I provided suggestions (and backing to them) that would mitigate this issues, it seems to me that they were not taken seriously, which frustrates me as a user of the library and as just someone that spent time trying to provide constructive feedback. If this iterating process is not meant to include/consider outsider (users) feedback, then either move it to a private channel or just note it as such.

If you mean the comments:

• Zombienet Simplified DevX #252 (review)
• Zombienet Simplified DevX #252 (comment)
• Zombienet Simplified DevX #252 (comment)

I definitely did take them seriously. That's the reason I didn't submit this PR just out of nowhere but provided the whole context of what I did so far. I honestly think this approach will be better than creating an additional wrapper which might confuse the current users of the library and the newcomers.

More context on your comments:

While the typestate pattern makes the builder safe by construction, I believe it also makes the DX worse for both developers and users and as such it should be kept only for very critical parts of the system.

• That comment made me realize this whole thing can be done in a much simpler way, and thanks for that! Your comment is the reason that I approached the problem from a different perspective and come up with this PR, instead of creating a new library with type-state-pattern (which would be my initial attempt btw).

#252 (comment)

• I know about sealed traits and I'm really eager to utilize them. But I'm trying to come up with the best/simplest solution for the ecosystem (as we all do), hence didn't use it. Because, I'm not using the type-state-pattern in the first place, I'm just adding new methods that are easy to use and will shorten the configuration process.

#252 (comment)

• This was the comment that appreciated the most! Providing a better API without intermediate .unwrap() or .expect() calls is just amazing. Thanks a lot again!

It saddened me that you felt that I'm not taking your comments seriously.

Coming back to the main topic:

I'm sorry but I fail to see how this is any better than the previous ideas. It seems like a shortcut, not an improvement.

The current way:

    let mut network = NetworkConfigBuilder::new()
        .with_relaychain(|r| {
            r.with_chain("rococo-local")
                .with_default_command("polkadot")
                .with_node(|node| node.with_name("alice"))
                .with_node(|node| node.with_name("bob"))
        })
        .with_parachain(|p| {
            p.with_id(100)
                .cumulus_based(true)
                //.with_registration_strategy(RegistrationStrategy::UsingExtrinsic)
                .with_collator(|n| n.with_name("collator").with_command("polkadot-parachain"))
        })
        .build()
        .unwrap()
        .spawn_native()
        .await?;

The way I'm proposing now:

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();

Let's say that this solution obeys the safety of type-state-pattern (we can easily make it so), then I think what I'm proposing is a much better way than other solutions. The reasons are:

• there won't be "yet another crate" that people have to get accustomed to
• there won't be "but what's happening behind the curtains" mystery compared to another crate that would use this library under the hood
• there won't be many duplications between another potential crate and this one (which might breed more confusion)
• people can still have access to all the configuration they want to, in the same way that they are used to do them

It seems like a shortcut

• Exactly. The other ideas would have been shortcuts as well. The main idea was to shorten to code by utilizing default settings and provide a better API for the users of this library. If we can do that by adding new methods to the existing library, why should we create another crate just for this?

The main goal for this initiative was to make zombienet-sdk easier to use and make it less intimidating for the newcomers, and hopefully increase adoption.

Our first suggestion was to create another library that wraps zombienet-sdk, then @pepoviola also agreed with the vision and we decided to not create another library, but contribute directly to zombienet-sdk. Then, instead of creating wrapper inside zombienet-sdk, I found a way to achieve the same ease of use with just adding those 2 methods, which sounds like the best possible outcome to me.

My aim was to shorten the current code I gave above, and I think this solution achieves it.

If I somehow misinterpreted any of your comments or intentions, feel free to correct me. But I assure you I'm delighted by your exquisite comments!

[jmg-duarte]

Morning @ozgunozerk! Let me start by apologizing for my tone, it was not my intention to come out as condescending. My contributions are indeed voluntary and I am very aware that no one is obliged to pick them up, however, I was frustrated as a consequence of not feeling that my suggestions were taken.

Having read your comment I see where the mismatch happened.

Let me try to explain myself — I'm not interested in beef, I want to collaborate and to make this library better since if it's an headache, it affects my work too.

• About the sealing, I don't care about that for now, it's more of a constraint for library safety that is mostly useful to truly ensure the user doesn't fuck up, in general, the user would need to go out of their way to add extra states, etc.
• About the wrapper, I'm also not interested in that, I even suggested that one of the improvements right now would be to re-export all zombienet-* crates in zombienet-sdk to reduce adding all crates one by one!

The main goal for this initiative was to make zombienet-sdk easier to use and make it less intimidating for the newcomers, and hopefully increase adoption.

I see now that I didn't understand your original goal when you spoke about improving DX, we ended up talking a bit past each other, I also misjudged your role in the project, my bad!

My main issue with the current API happens at the core level, which ends up undermining your current efforts. As the API currently works, I think your suggestions will help people getting up and running faster, however, once they need to change something they'll still run into issues due to the current API limitations.

I don't think our suggestions are incompatible, we're just aiming for different levels!

IMO there is core work that needs to be done to improve the whole API, a breaking change will be in order to achieve it — normally I wouldn't suggest it so lightly, however, the crate explicitly states it is a WIP and 0.*.*, meaning breaking changes may happen and users are supposed to cope with them — it's a balancing effort after all, if you want better libraries you need to be open to and willing to change.

[ozgunozerk]

Morning @jmg-duarte,

IMO there is core work that needs to be done to improve the whole API, a breaking change will be in order to achieve it — normally I wouldn't suggest it so lightly, however, the crate explicitly states it is a WIP and 0.., meaning breaking changes may happen and users are supposed to cope with them — it's a balancing effort after all, if you want better libraries you need to be open to and willing to change.

I see, and it makes perfect sense to me. As you said, I guess we are aiming for different levels at this point. At OpenZeppelin, I volunteered to simplify this API and achieve this in the fastest and most efficient way possible since I'm also busy with other stuff as well.

What I'm proposing here is solving our problem, and I believe it will be helpful for the other projects and hopefully for the newcomers as well by making the whole code less intimidating.

Maybe some time in the future, if I have more free time, I'd gladly take a whack on revizing the core API, and of course keeping your suggestions in mind. Already learned a new neat trick from you (simplifying the error handling via traits) 💯

Cheers!

[pepoviola]

Hi @jmg-duarte / @ozgunozerk, thanks both for your feedback and contributions 🙌 . I think we have two different scopes of improvements to tack. One is simplify the api for newcomers in order to make more simple and less confusing how to just spawn a simple network as exploration. In this direction, I think adding some helpers methods to shortcut the builder could help but we need to ensure not allow to construct invalid configurations.
The other scope is at core level, and is more relative to dx and how we can improve the sdk and make it more ergonomic and ensure a smooth usage to create complex scenarios. In this direction, I'm open to breaking changes, and I also think we need those.
Out of curiosity, @ozgunozerk / @jmg-duarte what are your main pain-points related to the usage of the sdk (beside exports in sdk). For example, I think the clousures are nice but very restrictives and make the usage of the sdk less flexible.

Again, thanks both for your feedback and contributions!
Thx!

[ozgunozerk]

For example, I think the clousures are nice but very restrictives and make the usage of the sdk less flexible.

Yes, one of the pain points for me when writing the wrapper SDK (which I abandoned) was the closures. I think it makes the code unnecessarily complex, hard to work with, and also not an eye-candy to look at it. I'd prefer having objects as parameters. For example, with_relaychain could take a Relaychain object instead of a closure that would build the Relaychain internally.

This would also help to ensure type safety across states as @jmg-duarte mentioned. For example, if we ensure the Relaychain::build() method will produce a Result (it already works like that IIRC), then we could pass the successful Relaychain object to the with_relaychain() function, and there won't be any need to handle the error related to Relaychain::build() inside with_relaychain(). Thus, we can ensure with_relaychain will always proceed to the next state without propagating the error to the last Network::build() call.

The same approach can be followed for nearly everything across this crate. I can tackle this change in another PR if you guys like the idea. Won't be a huge task and won't consume too much of my time, so I'm definitely willing to do this.

[ozgunozerk]

Aside from closures, I think @jmg-duarte mentioned some really nice ideas on how to improve the core library, I agree with most if not all on what he suggested. I can't foresee how much time that it would take to implement those.

So with basically 3 things, I think this library would be much better:

1. better helper methods with defaults that I'm proposing with this PR
2. replacing closures with objects (I can tackle that on another PR)
3. Jose Duarte's suggestions (the biggest item)

[pepoviola]

Suggested change

	errors.push(ConfigError::Relaychain(anyhow!("relaychain name can't be empty")).into());
	errors.push(ConfigError::Relaychain("relaychain name can't be empty").into());

why wrapped in anyhow if we already have an error variant.

[ozgunozerk]

The ConfigError::Relaychain variant expects an anyhow::Error, but the suggestion passes a &str. Casting into() was unfortunately not sufficient to make the compiler happy.

anyhow!("relaychain name can't be empty") wraps the &str into an Error as required by the enum.

[ozgunozerk]

If you want, I can replace anyhow::Error with String in the Error enum in my next PR that will address closures and objects. Makes sense to merge both of the refactorings into a single PR imo.

[ozgunozerk]

Thanks for the suggestions! Accepted all of them.

I guess everything has resolved aside from the error handling @pepoviola. We can proceed with wrapping them into anyhow! macro, or we can modify the Error enum so that it won't require anyhow::Error.

If there is a better way and if I'm missing something, please let me know!

[pepoviola]

• About the wrapper, I'm also not interested in that, I even suggested that one of the improvements right now would be to re-export all zombienet-* crates in zombienet-sdk to reduce adding all crates one by one!

Hi @jmg-duarte, I will add a pr for tackle this suggestion. And also I think we can revamp some of the core api to make it more easy to use.

Thanks for your feedback!

[pepoviola]

If first fails means that we already have errors, so will be fail the whole config.

[pepoviola]

Hi @ozgunozerk, sorry about the delay. I checked the current validation and we are ensuring that the chain is not empty (and not contains white spaces) here. I think we can also ensure that the node name is not empty as part of the existing api so we can remove the is_empty calls and just add those helpers methods.
wdyt?

[ozgunozerk]

@pepoviola I also removed the checks for empty lists (node and collator name lists).

There were no helper method for them, but then I realized we don't need these helper methods for lists.

If the list is empty, the code provides an empty string as the default option (to evade returning Result in the early stages before build()).
So, even in the case of list is empty, we will get an empty name error, which still makes sense and won't confuse the user imo.

Hence, adding new code is not necessary, all tests are still passing. Should be good to go!