search

Fantom-foundation / lachesis-rs

Lachesis BFT consensus for permission-less networks, in Rust
MIT License
34 stars 5 forks source link

Replace traits with concrete types if possible #47

Closed rishflab closed 5 years ago

rishflab commented 5 years ago

We have a lot of traits which are implemented with by only one concrete type. We should replace these traits with concrete types. Excessive use of traits makes it difficult to integrate and extend the codebase and requires use of lifetimes.

For example, Peer could be a concrete type making the Lachesis struct simpler and easier to integrate into Actix state.

use crate::event::event_hash::EventHash;

pub type PeerId = Vec<u8>;

pub trait Peer<H>: Send + Sync {
    fn get_sync(&self, pk: PeerId, known: Option<&H>) -> (EventHash, H);
    fn id(&self) -> &PeerId;
AgustinCB commented 5 years ago

This is not a good idea. traits are easier to test around, for once, through mocking.

And they allow extensibility.

For example, the Peer trait is used to implement nodes that transfer information through processes in one side and to transfer information through tcp. It could also be implemented later to transfer through udp.So adding an extra possible implementation is trivial and requires no modification in the original code.

In general, coding through interfaces is a good idea and an idiomatic approach in rust.

rishflab commented 5 years ago

I agree that coding through interfaces allows your code to be extensible which is a good thing.

But I think it is too early to be worrying about it or designing around that right now and there is a high change the abstractions chosen right now will not be fit for the future. I am trying to integrate Lachesis with Actix and Actix has its own abstractions for network transports which makes the Peer trait redundant.

AgustinCB commented 5 years ago

Sure, but adjusting it to Actix would mean that any other type of communication will be a pain in the ass. I prefer us having a meeting next week and discuss the problem you have and working towards a solution rather than getting ride of the traits.

rishflab commented 5 years ago

Yes lets work out a time and have a meeting early next week.

Actix allows you to implement whatever type of communication you want (tcp-bincode, udp-bincode, tcp-protobuf etc..). We are using it primarily for the actor model abstraction it provides saving us from having to deal with OS level threads and a web server functionality provided in Actix-web.