-
Notifications
You must be signed in to change notification settings - Fork 246
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
refactor: extract libwaku calls into WakuNode struct #6027
base: feature/nwaku-in-status
Are you sure you want to change the base?
Conversation
We require commits to follow the Conventional Commits, but with
|
Jenkins Builds
|
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! Thanks for it! 🙌
I just added some nitpick comments that I hope you find useful
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if len(infoArr) > 1 { | ||
return nil, errors.New("only a single") |
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.
Maybe we could elaborate more the error description. e.g.:
return nil, errors.New("only a single") | |
return nil, errors.New("only a single addr is expected in AddPeer") |
func (self *Waku) WakuVersion() (string, error) { | ||
var resp = C.allocResp() | ||
defer C.freeResp(resp) | ||
go func() { |
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.
Maybe interesting to add a comment explaining that this go routine is in charge of hosting and running the nwaku node and we communicate with it through channel ( some comment like that but well explained ;P .)
type WakuNode struct { | ||
wakuCtx unsafe.Pointer | ||
cancel context.CancelFunc | ||
newCh chan *request | ||
} |
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.
Something to consider in upcoming PRs.
Strictly speaking, the wakuCtx
contains a WakuNode; discovery; and other items.
type WakuNode struct { | |
wakuCtx unsafe.Pointer | |
cancel context.CancelFunc | |
newCh chan *request | |
} | |
type Waku struct { | |
wakuCtx unsafe.Pointer | |
cancel context.CancelFunc | |
newCh chan *request | |
} |
We will also need to remove the current type Waku struct
. See:
Line 435 in 6c76356
type Waku struct { |
var resp = C.allocResp() | ||
defer C.freeResp(resp) | ||
C.cGoWakuStopDiscV5(self.wakuCtx, resp) | ||
func NewWakuNode(ctx context.Context, config *WakuConfig) (*WakuNode, error) { |
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.
It seems not needed to be public
func NewWakuNode(ctx context.Context, config *WakuConfig) (*WakuNode, error) { | |
func newWakuNode(ctx context.Context, config *WakuConfig) (*WakuNode, error) { |
// Notice that the events for self node are handled by the 'MyEventCallback' method | ||
C.cGoWakuSetEventCallback(self.wakuCtx) | ||
func (n *WakuNode) processLoop(ctx context.Context) { | ||
|
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 think it would be better to create both the request and response channel at this point and then avoid creating the response channel on every postTask
call.
Another suggestion would be to handle all the req/resp channels logic in this func
and make the Waku
's functions just return "ok" or "error" without dealing with channels
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.
Amazingg, thank you!
This WakuNode struct will have a process loop executed in its own goroutine that will do the calls to libwaku. This is done this way so it's possible to call waku functions from other goroutines. Operations are being done sequentially and will be optimized later to be async