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

feat: use nwaku instead of go-waku #87

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

feat: use nwaku instead of go-waku #87

wants to merge 19 commits into from

Conversation

richard-ramos
Copy link
Member

@richard-ramos richard-ramos commented Feb 8, 2024

This PR / branch will contain all the changes related to using nwaku instead of go-waku.
Once we achieve feature parity with current master, we need to reset that branch to point to the latest commit of this PR.

  • Compile nwaku and generate bindings automagically
  • Replace function calls to invoke nwaku's libwaku.h
  • Create / Start / Stop node
  • Relay Publish / Subscribe
  • Event callback
  • Result + Error handling

In separate PRs, the following functionality needs to be implemented

  • Implement suggestions from Nwaku 2 event callback test #88
  • Use Opaque type like in
    pub struct OpaqueWakunodePtr {
    obj_ptr: *mut c_void,
    }
    unsafe impl Send for OpaqueWakunodePtr {}
    /// Handle to the underliying waku node
    pub struct WakuNodeHandle {
    ctx: Arc<Mutex<OpaqueWakunodePtr>>,
  • Destroy node
  • Add example program and docs
  • Fix TODOs
  • Expose any additional function

@richard-ramos
Copy link
Member Author

This PR so far compiles waku-sys succesfully generating binding.rs

cd waku-sys
cargo build

The first time does take a while because it's building nim and downloading the submodules, so maybe we can consider generating .so/.a for different architectures and uploading them to the releases, if build times become a problem.

@richard-ramos
Copy link
Member Author

richard-ramos commented Feb 13, 2024

waku-bindings now compiles succesfully and runs some basic tests

cd waku-bindings
cargo test

I added rln as a dependency so nwaku can run succesfully (it seems to no longer be an optional dependency). Also linked some other libraries and had to compile a libbacktrace_wrapper.o fike to link it succesfully. This has been tested on a linux machine. We should try on other architectures.

@richard-ramos
Copy link
Member Author

We can now invoke the relay functions to publish and subscribe. I've run into some trouble setting up the event handler, which seems to work, but in the tests it fails when we attempt to push a value to a tokio channel (fails on line 48).

println!("===============");
println!("ID: {}", id);
println!("Sending to channel....");
futures::executor::block_on(tx.send(Response {
id: id.to_string(),
payload,
}))
.expect("send response to the receiver");
println!("Sent!");

I'm investigating what's up with this.

@richard-ramos
Copy link
Member Author

Thanks to @danielSanchezQ 's help, the event callback now works!

cd waku-bindings
cargo test default_echo -- --nocapture --exact

I'll now proceed to improve error handling as well as including suggestions from #88

@richard-ramos
Copy link
Member Author

richard-ramos commented Feb 20, 2024

This PR is mostly complete.
Once waku-org/nwaku#2461 is merged, I'll fix the waku_flow test to use a rust node instead of using the fleet. I will not add more code to the PR, since it will get too complicated to review. Missing functionality will be done on a separate PR.

@richard-ramos richard-ramos marked this pull request as ready for review February 21, 2024 19:58
@richard-ramos
Copy link
Member Author

@danielSanchezQ @Ivansete-status @SionoiS
I've marked this PR as ready for review. It contains a lot of changes (sorry!), but most of them are just deleting code or changing function signatures, with just a small section dedicated to actually use nwaku instead of go-waku

@danielSanchezQ , don't worry too much about all the deleted code. I'm going to add it back in future PRs towards this branch once the functionality is available in nwaku c-bindings :)

Copy link

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Nice base to build upon!

waku-bindings/src/node/events.rs Outdated Show resolved Hide resolved
waku-bindings/src/node/management.rs Outdated Show resolved Hide resolved
message: &WakuMessage,
pubsub_topic: Option<WakuPubSubTopic>,
pubsub_topic: &String,
Copy link

Choose a reason for hiding this comment

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

In the future we may want to do something like pubsub_topic: impl Into<WakuPubsubTopic> and impl From<String> for WakuPubsubTopic Or even better remove pubsub topic and use shards with the appropriate type constraints.

Copy link

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

Wonderful! Thanks for it!
I just added some minor comments

pub fn waku_peer_id() -> Result<PeerId> {
/// nwaku version
#[allow(clippy::not_unsafe_ptr_arg_deref)]
pub fn waku_version(ctx: *mut c_void) -> Result<String> {

Choose a reason for hiding this comment

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

I think we can use const in this particular case. Maybe we could check other fns and use the most restrictive option in each case.

Suggested change
pub fn waku_version(ctx: *mut c_void) -> Result<String> {
pub fn waku_version(ctx: *const c_void) -> Result<String> {

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! in this case we're using bindgen to generate the bindings for libwaku.h automagically in https://github.com/waku-org/waku-rust-bindings/blob/nwaku/waku-sys/build.rs#L68-L79 and it translates *void to *mut c_void.
I could make the change and then cast the variable from *const to *mut right before interacting with nwaku, but since these functions are not exposed to the end user. We expose a friendlier API:

pub fn version(&self) -> Result<String> {
management::waku_version(&self.ctx)
}
)

I'm thinking we could accept to use a *mut in this case, just to not have to do the cast.

/// Start a Waku node mounting all the protocols that were enabled during the Waku node instantiation.
/// as per the [specification](https://rfc.vac.dev/spec/36/#extern-char-waku_start)
pub fn start(self) -> Result<WakuNodeHandle<Running>> {
management::waku_start().map(|_| WakuNodeHandle(Default::default()))
pub fn start(&self) -> Result<()> {

Choose a reason for hiding this comment

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

In a future PR we might need to add error handling ( it is actually pending to be implemented in libwaku.nim too .)

… return code (#97)

* refactor: handle return code and use an enum to handle responses
* fix: nwaku does not return an envelope hash on publish
* refactor: node handle constructor and messageId on publish
* refactor: add back typestate
* chore: rename messageId to messageHash
@richard-ramos richard-ramos linked an issue Mar 20, 2024 that may be closed by this pull request
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.

chore: pack nwaku instead of go-waku
4 participants