Move Builder to own module, allow for shared runtime reference.

[tnull]

Firstly, as lib.rs got quite big and Builder made up a good chunk of it, we now move it to a dedicated submodule.

We then split the actual logic of build_with_store from the API method, which is needed as VSS in the
future will need to share the Runtime, i.e., will need to hand in a Arc<Rwlock<Runtime>>> parameter that we'll set Some in Node::start.

Finally, we do exactly that: we allow build_with_store_internal to take a runtime parameter.

[tnull]

Now added a commit that re-introduces a paradigmatic Builder for the Rust API that takes &mut and returns a Node instead of an Arc<Node>. For uniffi we now export an ArcedNodeBuilder as Builder that wraps the former and returns an Arc<Node>.

I think it's preferable to have a clean Rust API here and have changed my mind regarding the necessary maintenance overhead (shouldn't be too bad now that we have the uniffi feature, we just have to deal with it). As this partly reverts #88, excuse the unnecessary churn! :(

[tnull]

Rebased on main.

[jkczyz]

Any reason we can't use move semantics now (i.e., take self and return Self)? That way you can chain mutators.

[tnull]

I think move semantics still wouldn't work seamlessly as the ArcedNodeBuilder wrapper needs to own the inner NoderBuilder and we couldn't move/modify the RwLock'ed value in place? I guess we could work around that by ArcedNodeBuilder holding an inner: RwLock<Option<NodeBuilder>> and taking and re-setting the value upon each call. Not sure this is preferable to taking &mut and returning &mut Self?

Generally, the lack of mutator chaining is a good point: I simply had forgotten to add the &mut Self return values, now added.

[jkczyz]

I think move semantics still wouldn't work seamlessly as the ArcedNodeBuilder wrapper needs to own the inner NoderBuilder and we couldn't move/modify the RwLock'ed value in place? I guess we could work around that by ArcedNodeBuilder holding an inner: RwLock<Option<NodeBuilder>> and taking and re-setting the value upon each call. Not sure this is preferable to taking &mut and returning &mut Self?

Right, it would require using an Option, I believe.

Generally, the lack of mutator chaining is a good point: I simply had forgotten to add the &mut Self return values, now added.

One reason I'd prefer returning Self over &mut Self is the former makes for a cleaner way of chaining when you need to do intermediary calculations before using the builder again. The latter requires initializing a variable with the builder before doing any chaining. Otherwise, you'd return a reference to a temporary, which the compiler will complain about:

creates a temporary which is freed while still in use

[tnull]

One reason I'd prefer returning Self over &mut Self is the former makes for a cleaner way of chaining when you need to do intermediary calculations before using the builder again. The latter requires initializing a variable with the builder before doing any chaining. Otherwise, you'd return a reference to a temporary, which the compiler will complain about:

creates a temporary which is freed while still in use

Mh, I now experimented with it and have it working in an experimental branch, see: tnull@3efa72e

Few thoughts:

1. Move semantics indeed 'feel' a bit 'cleaner' too me as well.
2. While moving indeed allow to 'quick safe' the builder state, it also requires the user to do so, i.e., if they don't want to do it all in a single line, they'd have to do

let builder = Builder::new().set_X()

rather than just

let mut builder = Builder.new();
builder.set_X();

This is a bit unfortunate also as the latter corresponds to the syntax they'd have to use in the bindings, i.e., our Rust docs would be a bit less transferable/relevant to the binding languages.

3. Consuming restricts the use to a 1:1 relationship between Builder and Node. However, AFAIK there is no reason we shouldn't allow creation of multiple nodes with the same builder. Having the Builder be non-reusable also means that in bindings we would panic on a second build() command for now (https://github.com/tnull/ldk-node/blob/3efa72e1354bda556518873358f452ae29b3af9b/src/builder.rs#L314) and would need to introduce a dedicated BuildError::AlreadyBuilt type that's only returned for that case in Prefer returning errors over panicking where possible #119.

Not entirely convinced one way or the other yet tbh. Moving/consuming 'feels' more paradigmatic, but &mut self would allow us to keep the APIs closer together.

[jkczyz]

Mh, I now experimented with it and have it working in an experimental branch, see: tnull@3efa72e

Few thoughts:

1. Move semantics indeed 'feel' a bit 'cleaner' too me as well.
2. While moving indeed allow to 'quick safe' the builder state, it also requires the user to do so, i.e., if they don't want to do it all in a single line, they'd have to do

let builder = Builder::new().set_X()

rather than just

let mut builder = Builder.new();
builder.set_X();

Actually, this will cause a compilation error when the builder is later used since set_X consumed it but never reassigned it to builder. But maybe I'm misunderstanding what version each of the above is demonstrating...

This is a bit unfortunate also as the latter corresponds to the syntax they'd have to use in the bindings, i.e., our Rust docs would be a bit less transferable/relevant to the binding languages.

That's a fair point.

3. Consuming restricts the use to a 1:1 relationship between Builder and Node. However, AFAIK there is no reason we shouldn't allow creation of multiple nodes with the same builder. Having the Builder be non-reusable also means that in bindings we would panic on a second build() command for now (https://github.com/tnull/ldk-node/blob/3efa72e1354bda556518873358f452ae29b3af9b/src/builder.rs#L314) and would need to introduce a dedicated BuildError::AlreadyBuilt type that's only returned for that case in Prefer returning errors over panicking where possible #119.

Alternatively, build functions don't necessarily need to consume if we want reuse. But if it consumes, then we could implement Clone to support building additional nodes. If not, then note that we are already internally cloning config fields to pass to the node. The consuming variation is a bit more efficient as those fields could be moved instead and only cloned when the user explicitly clones the builder.

The bindings version could internally clone the config to avoid needing a BuildError::AlreadyBuilt.

Not entirely convinced one way or the other yet tbh. Moving/consuming 'feels' more paradigmatic, but &mut self would allow us to keep the APIs closer together.

I don't feel very strongly if you prefer to keep the APIs similar. The common case of building a node in one statement looks the same either way. So I suppose it's a matter of whether API similarity is more important than how uncommon cases are formulated.

[tnull]

Actually, this will cause a compilation error when the builder is later used since set_X consumed it but never reassigned it to builder. But maybe I'm misunderstanding what version each of the above is demonstrating...

Yes, this wouldn't work for the moving/consuming variant, which is exactly my point.

I don't feel very strongly if you prefer to keep the APIs similar. The common case of building a node in one statement looks the same either way.

Right the &mut variant is a bit more flexible as in that it allows for either the chaining pattern or the "setter" pattern. While the former would be mandatory for the moving variant, it doesn't work for the ArcedNodeBuilder exposed in Uniffi, and the "setter" pattern needed there doesn't work for NodeBuilder. This leads to an API incompatibility that's not only unfortunate, it for example also breaks the crate's doc test (see above linked commit), requiring a fix like: tnull@b0e2813. While the feature gating fix makes the doctest pass, it doesn't actually mask the docs, so they show both variants.. I'm not sure there is a good way working around this short of ignoreing this doctest, which I'd like to avoid as it functions as the main usage example.

However, I think I might take this example as a first sign that the slight Rust-only benefits of the moving variant might not be worth dealing with the increased complexity of API incompatibility going forward.

TLDR: I see moving/consuming could have some minor benefits and feels more paradigmatic, but I'm not sure it's worth having to deal with two API-incompatible builders going forward. At least with the &mut variant there is some common ground, also having the benefit that the Rust-only docs stay more relevant to the bindings.

[jkczyz]

LGTM. Please squash fix-ups.

[tnull]

LGTM. Please squash fix-ups.

Squashed without further changes.