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

chore: decouple sub-systems #1897

Closed
SionoiS opened this issue Aug 8, 2023 · 2 comments
Closed

chore: decouple sub-systems #1897

SionoiS opened this issue Aug 8, 2023 · 2 comments

Comments

@SionoiS
Copy link
Contributor

SionoiS commented Aug 8, 2023

Background

I've notice some problems with the nwaku code architecture. I'm opening this to track the various issues.
What can we do to not just to fix the problems we have right now but design the code in a way that makes it easier to maintain?

With my 🦀 background the first thing I would use is channels but Nim is not Rust and so IDK what would be best.
We might need some input from Nim experts.

I'm not sure if this should be a milestone or not @fryorcraken

Details

#1828 -> Discv5 should not know or care what a peer manager is!
#1843 -> RPC handlers should not know or care what a wakunode is!

Have you seen other examples of tightly coupled systems in the code? Leave them below!

@fryorcraken
Copy link
Collaborator

fryorcraken commented Aug 9, 2023

So far it seems that the focus of this issue is tightly coupled systems, and I am guessing, lack of using a defined API between modules, should this be reflected in the title?

In term of refactoring, I usually see the following reason to do one:

  1. Make future change easier (this seems to fall in this bucket)
  2. Make debugging easier
  3. Improve execution (less bugs, better performance, etc).

I'd even say that usually 2/3 go together: if debugging is hard and one has to debug it means there are bugs, and hence it also impact 3.

For (1), I suggest to adopt the make the change easy, then make the easy change strategy. Meaning that the refactoring should not happen by itself but instead, be done before a change in the area is needed.
In the instance of tightly coupled system: A uses B, but B does not expose a clean API so everytime B is changed, A needs to be changed. Then the refactoring should be done before B is changed or when A changes the way it uses B.

So while we can use this issue to identify and list needed refactoring, I would not track work on it.
Instead, I would schedule the work when it is the most impactful.

Some the case of Discv5 should not know or care what a peer manager is then I would plan the work before the next change in peer manager or next in discv5. This work would then be a Task under a Milestone that plans to change peer manager or discv5 (whatever would be most likely to benefit from it).

tl;dr: for refactoring that makes the code more maintainable, refactoring should be done in preparation of maintaining said code, but not ad hoc/out of context.

@SionoiS SionoiS changed the title chore: Nwaku Architecture Update chore: decouple sub-systems Aug 9, 2023
@vpavlin
Copy link
Member

vpavlin commented Aug 22, 2023

As discussed in a meeting, closing this for now, we will create a more comprehensive issue if/when we decide to follow up with the refactoring

@vpavlin vpavlin closed this as completed Aug 22, 2023
@github-project-automation github-project-automation bot moved this to Done in Waku Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants