discovery/graph: move business logic out of CRUD layer #9522
Conversation
ellemouton
added
database
|
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 (
|
| @@ -1947,7 +1941,7 @@ func TestFilterKnownChanIDs(t *testing.T) { | |||
| {ShortChannelID: scid2}, | |||
| {ShortChannelID: scid3}, | |||
| } | |||
| filteredIDs, err := graph.FilterKnownChanIDs(preChanIDs, isZombieUpdate) | |||
| filteredIDs, _, err := graph.FilterKnownChanIDs(preChanIDs) | |||
There was a problem hiding this comment.
Looks like our test coverage is decreased? why are we not testing it?
There was a problem hiding this comment.
im not sure it makes sense that the coverage decreased - anything that was tested, is still tested right?
There was a problem hiding this comment.
added a commit to cover more
In our mission to make the graph CRUD layer be pure CRUD, we move some business logic out of the layer into a higher one. In this specific example, the FilterKnownChanIDs DB call would both filter known channel IDs but would also use the passed in channel updates to decide if the given channel should be removed from the zombie index. This logic, however, really is a separate piece of business logic and so should not be at the CRUD layer. Moving it up a layer will also allow us to plug in a different CRUD layer (such as a SQL store for gossip 1.75) without needing to re-implement the same business logic in that CRUD layer.
| // more recent. During the querying of the gossip msg | ||
| // verification happens as usual. However we should start | ||
| // punishing peers when they don't provide us honest data ? | ||
| isStillZombie := isZombieChan( |
There was a problem hiding this comment.
One thing I don't see here is an update in unit tests to target this shift. Before this change, from the PoV of the method (assuming it has no intimate knowledge about the referenced interface), the logic didn't affect any DB records, but it does now.
I understand the motive to move some of the logic out from the channel graph, but perhaps an intermediate step would be to extract those methods related to gossip queries into a new struct/interface, which would accomplish the same goal, but via a layer of indirection.
Otherwise, you've eliminated responsibilities from one area, only to add some entirely new responsibilities in another area.
There was a problem hiding this comment.
yes good point.
The next set of PRs is going to separate out the graph cache from the ChannelGraph (ie, a new layer of abstraction there). So i think it will actually just be better to move this logic to that layer 👍
|
Putting this on ice for now - I think with the next set of PRs that add a layer of indirection between the cache calls and the CRUD code, we can actually move this logic to that new layer instead. |
|
replaced by #9529 |
In our mission to make the graph CRUD layer be pure CRUD, we move some business logic
out of the layer into a higher one.
In this specific example, the
FilterKnownChanIDsDB call would both filter known channel IDs but wouldalso use the passed in channel updates to decide if the given channel should be removed from the zombie
index.
This logic, however, really is a separate piece of business logic and so should not be at the CRUD layer.
Moving it up a layer will also allow us to plug in a different CRUD layer (such as a SQL store for gossip 1.75)
without needing to re-implement the same business logic in that CRUD layer.
This does, however, mean that these steps will take place over more than 1 DB transaction.
This is part of the #9494 and will help to move the graph cache out of the CRUD layer.