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

refactor: extract libwaku calls into WakuNode struct #6027

Open
wants to merge 1 commit into
base: feature/nwaku-in-status
Choose a base branch
from

Conversation

richard-ramos
Copy link
Member

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

Copy link

github-actions bot commented Nov 3, 2024

We require commits to follow the Conventional Commits, but with _ for non-breaking changes.
Please fix these commit messages:

refactor: extract libwaku calls into WakuNode struct

@status-im-auto
Copy link
Member

status-im-auto commented Nov 3, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 6c76356 #1 2024-11-03 02:15:40 ~1 min tests 📄log
6c76356 #1 2024-11-03 02:15:53 ~1 min ios 📄log
6c76356 #1 2024-11-03 02:16:08 ~2 min android 📄log
6c76356 #1 2024-11-03 02:16:38 ~2 min macos 📄log
6c76356 #1 2024-11-03 02:16:55 ~2 min linux 📄log
✖️ 6c76356 #1 2024-11-03 02:16:56 ~2 min tests-rpc 📄log
6c76356 #1 2024-11-03 02:18:12 ~3 min windows 📄log
6c76356 #1 2024-11-03 02:19:00 ~4 min macos 📄log

Copy link
Contributor

@Ivansete-status Ivansete-status left a 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")
Copy link
Contributor

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.:

Suggested change
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() {
Copy link
Contributor

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 .)

Comment on lines +2389 to +2393
type WakuNode struct {
wakuCtx unsafe.Pointer
cancel context.CancelFunc
newCh chan *request
}
Copy link
Contributor

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.

Suggested change
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:

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) {
Copy link
Contributor

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

Suggested change
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) {

Copy link
Contributor

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

Copy link

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazingg, thank you!

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

Successfully merging this pull request may close these issues.

4 participants