refactor(node): add builder for Node

[dan-da]

Accomplishes three related goals:

1. remove unwraps() that were in Node::new()
2. enables/requires that a Node be instantiated infallibly
3. improves ergonomics for instantiating a Node

node.rs:

• add BlockBodyError::ParseError variant
• derive Builder for Node
• refactor Node::new() into:
  • NodeBuilder::genesis_code() and
  • NodeBuilder::build()

bench.rs, tests/network.rs, kindelia/src/main.rs:

• use NodeBuilder::build() instead of Node::new()

[dan-da]

hmm, CI is getting clippy errors that I don't get locally. Maybe it is using the new rust toolchain released a day or two ago?

error: casting to the same type is unnecessary (`usize` -> `usize`)
Error:    --> kindelia_core/src/bits.rs:130:18
    |
130 |   while bits.get(*index as usize)? {
    |                  ^^^^^^^^^^^^^^^ help: try: `*index`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast

There are a bunch of these type of errors unrelated to this PR, so I think they should be fixed in a separate PR.

[racs4]

I think this is a nice refactoring, but I'm also scared by the size of "boilerplate" code (I don't know if that would be the most correct term) that was needed to create this builder.

I think it definitely seems simpler to build a node using this method, even more so when default values are expected, but all the "boilerplate" (?) needed to create this builder is overcomplicating the node construction code. Of course, the code was not the simplest before, but now, in addition to understanding what is happening in the construction of the node, it would also be necessary to understand the macros in the properties and the new functions created for the builder to be possible (although it is all very well documented, I know some people who would criticize for the "issues" I just described).

For me this PR would be approved, but this time I think I need a third opinion to evaluate these points.

[dan-da]

Yeah, I don't like adding any unnecessary code either.

In this case, I figure that a builder is warranted.

First, I always consider there to be some code-smell if a Thing::new() returns a Result. I find it much cleaner if Thing::new() is infallible, or indeed ::new() does not need to exist at all. So I didn't want to add a Result to Node::new(), and yet it was doing fallible things that had to go somewhere. So those fallible things either had remain in new(), move to caller(s), or go into a builder of some type.

Second, any time there is a struct with a lot of properties, including some that are optional/defaultable, a builder can be considered. It can make usage much more ergonomic for the caller. Given that Node is a huge struct and is central to everything, I think it is worth making usage as ergonomic and idiomatic as possible.

An indicator of the code-smell in this area is the #[allow(clippy::too_many_arguments)] annotation for Node::new(), which is no longer needed with the builder.

Now, in the past, I've made my own builders, so this is the first time I've worked with derive_builder. I found it quite nice except for the limitation that the builder cannot have any properties not in the target struct, so I opened an issue about that. If they were to add the requested feature, I could make the builder API cleaner for callers.

Of course, I could also do it right now by manually implementing a builder, and perhaps that is the best thing here. That would remove all the Builder annotations from Node and might result in less total compiled code.

[racs4]

An indicator of the code-smell in this area is the #[allow(clippy::too_many_arguments)]

Yes, that was embarrassing.

Given that Node is a huge struct and is central to everything, I think it is worth making usage as ergonomic and idiomatic as possible.

It's worth noting that at the moment the node is only actually created in our code in the kindelia/main.rs file in response to the cli command node start. Despite that, I think it is good to have a library mentality with the Node so others in the future can build projects on top of this Node implementation. Not long ago I needed to do something analogous with another library with a similar structure (some libs and 1 binary) and it wasn't the best of experiences. That's why I'm in favor of this change.

[dan-da]

So I'm not totally happy, with this derive_builder impl either. Example usage in this PR:

   let (node_query_sender, node) = NodeBuilder::default()
    .network_id(network_id)
    .comm(comm)
    .miner_comm(miner_comm)
    .addr(addr)
    .storage(file_writer)
    .genesis_code(data_path, genesis_code)?
    .build(
      &initial_peers, 
      #[cfg(feature = "events")]
      Some(event_tx),
    )?;

What I would like to see instead is:

   let (node_query_sender, node) = NodeBuilder::default()
    .network_id(network_id)
    .comm(comm)
    .miner_comm(miner_comm)
    .addr(addr)
    .storage(file_writer)
    .data_path(data_path)
    .genesis_code(genesis_code)
    .peers(initial_peers)
    .event_tx(event_tx)
    .build()?;

Notice that data_path() and genesis_code() are now separate items that do not return any result and that build() accepts no params. So all fallible work is delayed until ::build() when all necessary inputs have been set. The whole thing is just cleaner.

Since derive_builder() doesn't support this usage yet, I think I will add a new commit to this PR with a custom builder that works per above -- for your consideration. It might take a day or two though given the holidays.

Despite that, I think it is good to have a library mentality with the Node so others in the future can build projects on top of this Node implementation.

This is exactly where I'm coming from. I think Kindelia is and will be an important project/ecosystem and that lots of people will be using it in the future after mainnet, so I consider these crates to be libraries and not just internal components of kindelia cli. Thus it's good to to polish and clearly doc the public API.

[dan-da]

marking this as draft. #274 is preferred.