lnworker: pass PaySession to route building methods.

[accumulator]

Simplifies method signatures and groups quite a large set of parameters that are mostly passed down along the call stack but functionally belong together. Also removes bolt11 specific parameters in method signatures, e.g. r_tags, which are not relevant in e.g. a bolt12 context.

[ecdsa]

Note that the fields of PaySession are mutable. We are passing them to a function that is run asynchronously (await run_in_thread). This happens to be fine in that case, because the fields we are passing (invoice_pubkey, invoice, r_tags, min_final_cltv_delta) happen to not be mutated during the lifetime of the session. However, this seems to be lucky. What would happen if we had been passing paysession._amount_inflight, or some other variable field? Then the value at the time it is evaluated in the coroutine might differ from the value passed as parameter.

[accumulator]

We could store the relevant (immutable) fields in a NamedTuple within PaySession and pass that instead. This way there is no access to the mutable fields of PaySession in the routing methods

[SomberNight]

Note that the fields of PaySession are mutable

Good point.

store the relevant (immutable) fields in a NamedTuple within PaySession

(btw besides NamedTuple, there is also @dataclass(frozen=True) in the stdlib. It is similar to attrs, a tiny bit less flexible, but attrs I would eventually like to remove from the dependencies)

[accumulator]

(btw besides NamedTuple, there is also @dataclass(frozen=True) in the stdlib. It is similar to attrs, a tiny bit less flexible, but attrs I would eventually like to remove from the dependencies)

Yes I have looked at dataclasses, but this is too inflexible, as there are mutable fields in PaySession that should remain mutable in certain cases, and I haven't seen an option to selectively un-immute (heh) a subset of fields. Subclassing also doesn't allow mutability as the restriction propagates down the hierarchy.

[accumulator]

I added a method to return an immutable copy and pass that instead. See addcbc3

There's probably nicer ways to do this, e.g. externalize the __setattr__ override.

e.g. a generic method def immutable(object) that returns an immutable copy of the object

[accumulator]

Hmm.. I'm not really happy with the last commit. If we copy the PaySession it does not really need to be immutable, and the added code is.. not very nice.

Maybe it is nicer to add a NamedTuple within PaySession, and just pass that immutable tuple down to the routing functions. If anyone has a nice naming suggestion?

[ecdsa]

indeed I would prefer not to pass paysession to that method, but a namedtuple that can be a field of paysession.

The other call (create_trampoline_route_and_onion) needs to pass a mutable field (failed_trampoline_routes) in addition to immutable fields. That mutable field should be passed explicitly, in addition to the namedtuple.

[accumulator]

as I'm reworking the patch to pass a NamedTuple, it looks like there are too many exceptions to really benefit from this encapsulation :( (e.g. use_two_trampolines is mutable and part of PaySession, but must be passed explicitly to create_trampoline_route())

passing the NamedTuple and extra fields of PaySession along is not really an improvement of the old situation IMO.

Unless we find a more elegant solution, I propose to abandon this PR