filecoin-project / lotus

Reference implementation of the Filecoin protocol, written in Go
https://lotus.filecoin.io/
Other
2.81k stars 1.24k forks source link

Do not expose node builder internals to the consumer #5370

Open schomatis opened 3 years ago

schomatis commented 3 years ago

This basically means not allowing (or at least disincentivizing) direct manipulation of the DI graph through node.Override() as we currently do:

https://github.com/filecoin-project/lotus/blob/476df99179157c9d0019d2bcc9a9a4fb6b31d876/cmd/lotus/daemon.go#L280-L305

In the previous example this would mean (among others):

I'm fine with:

The objective is to decouple the node/ logic from the rest of the code to start cleaning it up and provide a more fine-grained but standardized interface to initialize only the subsystems needed during testing.

raulk commented 3 years ago

I'm onboard with not leaking internals via the API, and obsoleting the need to directly manpulate the DI graph for simple configuration changes.

In practice, I think we do need an escape hatch so that code that needs "privileged" access to the node construction can achieve so.

Passing in an optional callback function to perform Overrides and such, should be good enough for that. An examle of such a privileged user would be testground test plans.

raulk commented 3 years ago

Replacing generic Override calls by explicit configuration options for the bootstrapper, genesis and other usually modified dependencies (i.e., white-listing what the consumer is allowed to change).

I'm a bit +1 on this, as long as we preserve the escape hatch I mentioned.

raulk commented 3 years ago

Not needing to explicitly provide the api.FullNode to "extract" the created artifact of interest but having the API returning it directly.

One nice feature of the current model is that you can extract components that are not necessarily exposed through the public API. We need to preserve that capability, either by exposing those components through getters, or by supplying an extractor callback.

schomatis commented 3 years ago

One nice feature of the current model is that you can extract components that are not necessarily exposed through the public API. We need to preserve that capability, either by exposing those components through getters, or by supplying an extractor callback.

I think we are aiming at the same thing. Just to clarify, I'm not sure we can freely extract stuff at the moment, not the way the current DI model is architectured. I don't see people being able to instantiate for example an isolated state manager or chain storage for a test (that is exactly my motivation for this issue in the first place, https://github.com/filecoin-project/lotus/pull/5313/). If there is something like what you are describing please link it, I might be able to use it as a model and expand on that.