waku-org / waku-rust-bindings

Rust wrapper over go-waku ffi
14 stars 6 forks source link

Fix #32: add discoveryV5 functions #33

Closed al8n closed 1 year ago

al8n commented 1 year ago

Please review this PR.

cc: @richard-ramos @Zeegomo

danielSanchezQ commented 1 year ago

@al8n Could we also add so this methods are used in the example? Mostly to bump up the code coverage.

danielSanchezQ commented 1 year ago

@al8n It seems there is something going on with the submodule. Did you update it and push the changes pointing to the new main commit containing the discv5 changes in go-waku?

al8n commented 1 year ago

@al8n It seems there is something going on with the submodule. Did you update it and push the changes pointing to the new main commit containing the discv5 changes in go-waku?

My bad, forget it.

codecov-commenter commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b26eb9b). Click here to learn what that means. Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #33 +/- ## ========================================= Coverage ? 55.27% ========================================= Files ? 19 Lines ? 1764 Branches ? 0 ========================================= Hits ? 975 Misses ? 789 Partials ? 0 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

danielSanchezQ commented 1 year ago

@al8n Could we also add so this methods are used in the example? Mostly to bump up the code coverage.

@al8n is this done? Cannot see it in the changes 🤔

al8n commented 1 year ago

@al8n is this done? Cannot see it in the changes 🤔

Sorry, a little bit sleepy, committed but forgot to push.

al8n commented 1 year ago

Hi, @richard-ramos, the failed CI says "DiscV5 is not mounted", do I need to change the CI environment to make it pass?

adv-bacv commented 1 year ago

@al8n you can update the submodule commit and use the discv5.stop again: waku-org/go-waku#402 (comment)

The referenced link does not resolve this: https://github.com/waku-org/waku-rust-bindings/issues/32#issuecomment-1361776414, discv5.stop is called internally on node.stop().

danielSanchezQ commented 1 year ago

@al8n you can update the submodule commit and use the discv5.stop again: waku-org/go-waku#402 (comment)

The referenced link does not resolve this: #32 (comment), discv5.stop is called internally on node.stop().

Aaaah, didn't saw that. Imo it should be either responsibility of the caller to start/end the service, or completely automated and internally handled when starting/stopping the "node".

danielSanchezQ commented 1 year ago

@al8n , the issue of double closing is now fixed. We just need to keep the settings and remove the api calls as they do not exist anymore (it is handled when starting/stopping the node). Please do update this PR accordingly :)

al8n commented 1 year ago

Hi @richard-ramos, it seems that when enabling the discv5 in settings, the start function will report Error: "listen udp 0.0.0.0:9000: bind: address already in use". The details are in the failing CI.

richard-ramos commented 1 year ago

it seems that when enabling the discv5 in settings, the start function will report Error: "listen udp 0.0.0.0:9000: bind: address already in use". The details are in the failing CI.

Try setting a random port instead of the default 9000 set in discV5UDPPort. Perhaps the CI is already using 9000 for something?

al8n commented 1 year ago

it seems that when enabling the discv5 in settings, the start function will report Error: "listen udp 0.0.0.0:9000: bind: address already in use". The details are in the failing CI.

Try setting a random port instead of the default 9000 set in discV5UDPPort. Perhaps the CI is already using 9000 for something?

I also tried some random ports and got the same error.

danielSanchezQ commented 1 year ago

it seems that when enabling the discv5 in settings, the start function will report Error: "listen udp 0.0.0.0:9000: bind: address already in use". The details are in the failing CI.

Try setting a random port instead of the default 9000 set in discV5UDPPort. Perhaps the CI is already using 9000 for something?

I also tried some random ports and got the same error.

Doesn't it work on your local machine as well? It does work for me (but I am getting other unrelated errors that we may have to take a look as well).

bacv commented 1 year ago

Test fails because waku-bindings::waku_stop() method calls waku_start() from go-waku internally. Not sure when this was introduced, but I'm testing a fix. There are other errors that are reported if you run a test with --nocapture. They do not panic even though it's expected to.