graph+autopilot: remove autopilot access to raw graphdb.ChannelGraph
#9480
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ellemouton
force-pushed
the
autopilotRefactor
branch
2 times, most recently
from
a42d65f to
f0d4d08
Compare
| @@ -30,6 +30,11 @@ type testGraph interface { | |||
| addRandNode() (*btcec.PublicKey, error) | |||
| } | |||
|
|
|||
| type testDBGraph struct { | |||
There was a problem hiding this comment.
nit: typo in commit message: does to make
There was a problem hiding this comment.
Looking good, think we need to fix a few typos, plus a question about where to put the interface.
Reviewed 2 of 2 files at r1, 1 of 2 files at r3, 7 of 8 files at r4, all commit messages.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @bhandras and @ellemouton)
-- commits line 31 at r4:
typo: needing
autopilot/interface.go line 223 at r4 (raw file):
// GraphSource represents read access to the channel graph. type GraphSource interface {
Why is this defined in autopilot but not in graph? Does it mean for other subsystems accessing the graph we also need to define it again in packages like routing(for payments), invoice, or gossip?
@yyforyongyu - I believe it is a good pattern to let each individual subsystem define the interface it needs. Otherwise we provide it with way more than it needs. I believe this is a better pattern because it lends itself more towards making things more modular: for example, here, if we wanted to split autopilot out into its own binary/process, this interface goes along with it and it is very clear what the dependency is. So this makes it very clear (and this pattern is used elsewhere in the code base). Later on, when we will have 1 big interface for the remote graph (for example) to implement, it will embed these kinds of interfaces
yes those packages will have interfaces with a similar/same name but they require quite different methods |
And from various interfaces where it is not needed.
This is a pure code move commit where we move any code that is only ever used by tests to test files. Many of the calls to the graphdb.ChannelGraph pointer are only coming from tests code.
Introduce a new type for testing code so the main databaseChannelGraph type does not need to make various write calls to the `graphdb.ChannelGraph` but the new testDBGraph type still can for tests.
ellemouton
force-pushed
the
autopilotRefactor
branch
from
f0d4d08 to
a0d1362
Compare
In this commit, a new NodeRTx interface is added which represents consistent access to a persisted models.LightningNode. The ForEachChannel method of the interface gives the caller access to the node's channels under the same read transaction (if any) that was used to fetch the node in the first place. The FetchNode method returns another NodeRTx which again will have the same underlying read transaction. The main point of this interface is to provide this consistent access without needing to expose the `kvdb.RTx` type as a method parameter. This will then make it much easier in future to add new implementations of this interface that are backed by other databases (or RPC connections) where the `kvdb.RTx` type does not apply. We will make use of the new interface in the `autopilot` package in upcoming commits in order to remove the `autopilot`'s dependence on the pointer to the `*graphdb.ChannelGraph` which it has today.
ellemouton
force-pushed
the
autopilotRefactor
branch
from
a0d1362 to
a8c6812
Compare
ellemouton
force-pushed
the
autopilotRefactor
branch
from
a8c6812 to
c910ee5
Compare
Agree that using the minimum viable interface for injection is usually a better choice. |
Which passes a NodeRTx to the call-back instead of a `kvdb.RTx`.
Define a new GraphSource interface that describes the access required by the autopilot server. Let its constructor take this interface instead of a raw pointer to the graphdb.ChannelGraph.
ellemouton
force-pushed
the
autopilotRefactor
branch
from
c910ee5 to
3d0ae96
Compare
There was a problem hiding this comment.
hmm feels like having a similar conversation from #5067 again...
Agree that using the minimum viable interface for injection is usually a better choice.
Could you elaborate a bit? Agree that having a smaller interface is always desirable, which means the interface is deep and hides all the implementation details. However I don't think have a concrete implementation and then extract a subset of its methods to make an interface inside a different package means the "actual" interface is small. Ideally it should be a bottom-up approach, we first properly define what the interface provides, and other packages import and use it. Otherwise, does it mean every package needs to have a small subset of other modules' interface whenever they wanna interact?
The effective go has a similar topic, it even suggests there's no need to export the concrete type but only the interface.
for example, here, if we wanted to split autopilot out into its own binary/process, this interface goes along with it and it is very clear what the dependency is.
If it imports graph.GraphSource I think it's the same? Or put it this way, moving to the next step, when we decide to have an interface for the Builder, we seem to have two options,
- import all the small interfaces, embed them into a big interface, and assert the
Builderimplements it, which means there is an import cycle. - re-define all the interface methods again in the big interface, which means there's code repetition, and harder to maintain.
I think the fact that the Builder (or ChannelGraph doesn't have compile-time assertion says something here, and we need to manually check ChannelGraph to see if the interface autopilot.GraphSource is implemented is a clear abstraction violation.
Reviewed 1 of 10 files at r5, 1 of 2 files at r6, 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ellemouton and @yyforyongyu)
no it would not ever need to implement the larger interface. It just keeps what it needs, ie: the interface it defines. Autopilot here does not need the graph source provided to it to ever implement anything other than the methods it defines.
tbh, i've always found the compile-time assertions strange since even without the assertions, compilations will still fail if we pass a pointer through that doesnt properly implement the interface. To me, the compile time assertions have always been misplaced as in my mind an implementation should not need to care about where it is being used (so here: the graph.ChannelGraph should not care or know about the fact that the autopilot server users it). If something changes in one of the graph.ChannelGraph methods that the autopilot needs, then compilation will anyways fail when we try to pass a ChannelGraph pointer to init the autopilot.
cool cool - yeah let's continue with this for now and I can always change things around when more pieces start coming together :) |
While working with a single, monolithic interface might seem simpler, I believe that reducing interfaces to only the necessary methods required by the depending package, type, or function leads to better design. This approach promotes clearer dependencies, makes code potentially more testable by allowing more precise mocking, and also minimizes unnecessary coupling between components. We actually follow this principle quite often in our codebase by injecting function types as arguments to other functions or as struct members (composition), thereby reducing the dependency surface. |
In this PR, we remove the
autopilot's direct access to thegraphdb.ChannelGraphpointer and replaceit with a new
GraphSourceinterface defined in theautopilotpackage.This will allow us in future to more easily switch out the implementation provided to the autopilot code.
This is a pure refactor.
Part of #9494