Refactor `fuel-core` services to be abstracted and allow testing

[MitchTurner]

There are a few things preventing this code from being tested properly.

Let's look at the P2P service as an example.

FuelP2PService has quite a bit of testing already, but it's incomplete. The tests manually poll the next_event() method to get the events and then mock the internal event handling before returning some arbitrary result. The actual event handling (as well as request handling) takes place inside the Task, but these tests ignore the Task completely.

This might be okay if we could also test the the Task directly, but Task takes a direct dependency on a concrete FuelP2PService and SharedState:

pub struct Task<D> {
    p2p_service: FuelP2PService<PostcardCodec>,
    db: Arc<D>,
    next_block_height: BoxStream<BlockHeight>,
    /// Receive internal Task Requests
    request_receiver: mpsc::Receiver<TaskRequest>,
    shared: SharedState,
    max_headers_per_request: u32,
}

So we would need to abstract p2p_service so we can feed the Task faked events and requests and abstract shared to check that we are modifying state correctly, etc.

BUT even if we were to add the abstractions to Task, we wouldn't have coverage of how those Tasks are spun up and operated in the wild. That logic is exempted from the FuelP2PService and short circuited by calling next_event() directly, where normally the Task calls next_event().

What would be ideal is if one place just had full coverage. All we know is that some external party makes a request, the service does a bunch of stuff, and then gives a response and state changes. That's all we should be testing: request in, response/state change out.

One question is whether or not libp2p should be included in the test scope. We seem to have some logic in the Libp2p code and in the Service code. Ideally, everything tested should be agnostic to how or where the request came from, so if we've coupled our core logic to libp2p we might need to mock the peering somehow, if not, then we can abstract the libp2p code as an adapter to some kind of request/response port.

[MitchTurner]

I don't think this is worth working on just for better test coverage, but imo testable code is maintainable code and it will make our jobs easier in the future.

[bvrooman]

The advantages of a refactor here sound promising:

• We can achieve better, more granular testability of tasks if we can isolate them in unit tests by removing direct dependencies and using mockable traits.
• We can achieve a higher degree of maintainability through more granular architecture

If we put this refactor into a future sprint, it would be nice to see a concrete list of:

• which tasks need to be refactored
• which new traits need to be added to support the abstraction
• which test suites need to be updated

in the ticket to get an idea of what's involved. Maybe then we can break that up into smaller refactor tickets.

[Voxelot]

I agree that further separation of business logic from external dependencies a-la onion model would be ideal. The relayer crate handles this a bit better already for example, by keeping the core state machine logic disentangled from the ethereum SDK. The p2p logic is highly intertwined with the inner workings of libp2p, so I understand the complexity of testing this. However, it should still be possible with more integration-level tests in the meantime right?

We also need to balance large tech-debt refactoring efforts to modules like P2P with other critical blockers to shipping mainnet like improved gas pricing etc.

[MitchTurner]

We also need to balance large tech-debt refactoring efforts to modules like P2P with other critical blockers to shipping mainnet like improved gas pricing etc.

I completely agree.

I'm currently working on a halfway solution to test some of the peer reputation stuff here: https://github.com/FuelLabs/fuel-core/pull/1356 I felt very uncomfortable just sticking the code in there and hoping it worked. It doesn't solve for the libp2p abstraction completely, but it's insulating business logic in the correct way. I'm hopeful that the refactor work can be done incrementally and not interfere with critical stories--hopefully make them easier :crossed_fingers:

[MitchTurner]

Just some thoughts.

All of our services are abstracted as services. This is represented as the trait Service with type SharedData, etc. I think this is a counter-productive abstraction.

It's true that all the services might have similarities, e.g. they need to be run, they need to access data from other services, etc. But the problem is that it doesn't describe the relationship between the implementations. I think it would be better to have the abstractions be domains specific, there might be some redundancy on how each service is started, but the similarities are incidental from the perspective of the domain. So we shouldn't define the abstractions that way.

i.e. the smell here is SharedData. Shared with whom and what data? Instead, you can define what data each service needs. The Sync service needs access to p2p, it would just have a p2p interface it depends on. This will isolate each service and allow it to be testable and not dependent on arbitrary abstractions (trait Service)