Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TECH_DEBT] - Remove or Refactor CommunicationChannel #2572

Closed
bfish713 opened this issue Feb 13, 2024 · 0 comments · Fixed by #2578
Closed

[TECH_DEBT] - Remove or Refactor CommunicationChannel #2572

bfish713 opened this issue Feb 13, 2024 · 0 comments · Fixed by #2578
Assignees

Comments

@bfish713
Copy link
Collaborator

What is this task and why do we need to work on it?

Context from zulip:
"
What are peoples thoughts on the future or the CommunicationChannel trait? Right now I'm looking at how we should properly handle DA and VID with libp2p and they will need to be handled in a specific way. For DA we probably want to do direct messages to everyone in the comittee for DA proposals. And for VID a first pass will probably be direct message based as well. I think neither of these fit into the orginal CommunicationChannel idea. It is supposed to be an abstraction of a subset of a network where broadcast would send to all members of the subset. For libp2p I don't think that abastraction works nicely because Gossiping to a subset only works if well if you are always a part of that subset. With small DA committees it will mean leaders will have to join the DA committee for one view then leave (except of the rarer cases where the leader is on the comittee). It's even less viable when the comitte starts rotating

Right now all the implementations just wrap ConnectedNetwork, more or less. The one exception is CombinedNetwork which wraps the 2 different ConnectedNetworks. What we end up doing anyway is passing the membership in to broadcast anyway and using that list to decide who to broadcast two. With that pattern the CommunicationChannel does literally nothing. The webserver as well ignores the membership anyway as it's just posting all it's outgoing broadcast to whichever server it's connected to.

The reason this is coming up now is because we need to add an interface for VID share distribution at some point soon, and I don't want to have to change CommunicationChannel trait and add a new function to every impl that is just a wrapper to ConnectedNetwork.

I propose we do either:

  1. Remove the trait
  2. Somehow incorporate the membership into the channel so that it at least abstracts that part of broadcasting from us.

I prefer removing it, but I'm curious on other thoughts or if it's maybe useful for something I'm missing
"

What work will need to be done to complete this task?

Decide to remove or refactor the trait and then complete the work.

Are there any other details to include?

No response

What are the acceptance criteria to close this issue?

The refactor is done without effecting test behavior

Branch work will be merged to (if not the default branch)

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant