-
Notifications
You must be signed in to change notification settings - Fork 249
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_: move waku interface out of eth-node #6244
Conversation
Jenkins BuildsClick to see older builds (8)
|
5c78c3a
to
6dc3ce7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6244 +/- ##
========================================
Coverage 61.55% 61.56%
========================================
Files 842 842
Lines 110604 110604
========================================
+ Hits 68086 68096 +10
- Misses 34562 34569 +7
+ Partials 7956 7939 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Again, definitely need an approval from @richard-ramos
wakuinterface/api.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if wakuinterface
is a good naming. If it's always waku, then what's the point describing it as interface, we can use it directly, right?
Perhaps transport
would be a better name?
wakuinterface/api.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently I started to add new files into ./internal
directory, as a step to follow Standard Go Project Layout.
Do you think we could do the same for this PR? It could maybe fall under ./pkg
directory though, not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will postpone using ./internal for now, if you don't mind. I plan to consolidate everything once I have resolved all the waku <-> eth-node dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of moving the waku interface out of eth-node but not the new name wakuinterface
. I just wonder if we could instead rename the package waku
to wakuv1
instead, and use waku
instead of wakuinterface
.
In any case, i'm approving it since the change is good, and my suggestion is just nitpicking :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6dc3ce7
to
572bc4f
Compare
Waku interface does not belong to eth-node. It has been moved to newly created
wakuinterface
package.