Page MenuHomePhabricator

[keyserver] Unifying API interface and adding Tunnelbroker types
ClosedPublic

Authored by max on Feb 10 2023, 9:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 12:31 AM
Unknown Object (File)
Mon, Dec 16, 12:50 AM
Unknown Object (File)
Sat, Dec 14, 7:15 AM
Unknown Object (File)
Wed, Dec 11, 5:44 PM
Unknown Object (File)
Tue, Dec 10, 5:55 PM
Unknown Object (File)
Tue, Dec 3, 3:35 PM
Unknown Object (File)
Tue, Dec 3, 3:35 PM
Unknown Object (File)
Tue, Dec 3, 3:34 PM

Details

Summary

This diff introduces adding the Tunnelbroker Rust binding types, and refactoring the API getter function to return the API types for all Rust bindings.
Types for the Rust API bindings are moved to a separate file.
The purpose of the getter is to call the Tunnelbroker client class and Identity service API from the nodejs codebase.

Linear task: ENG-2613

Test Plan

Passing the CI gates seems like enough for these changes.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.

Fixing nativeBinding returns.

max retitled this revision from [keyserver] Adding of Tunnelbroker client type and getter from napi to [keyserver] Adding of Tunnelbroker client type interface and getter in `rust-node-addon` index file.Feb 13 2023, 5:53 AM
max edited the summary of this revision. (Show Details)
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, ginsu.
max published this revision for review.Feb 13 2023, 5:57 AM
max added inline comments.
keyserver/addons/rust-node-addon/index.js
41–46 ↗(On Diff #22411)

I think I'll move this to the types/tunnelbroker-types.js, not switching to plan changes to not block the review process.

varun requested changes to this revision.Feb 13 2023, 2:32 PM
varun added inline comments.
keyserver/addons/rust-node-addon/index.js
41 ↗(On Diff #22411)

git grep "export interface" yields only one result in native. are we sure this is the right approach? CC @ashoat

48–69 ↗(On Diff #22411)

lot of duplicate code here. can we factor it out to a common function?

This revision now requires changes to proceed.Feb 13 2023, 2:32 PM
ashoat requested changes to this revision.Feb 13 2023, 3:56 PM
ashoat added inline comments.
keyserver/addons/rust-node-addon/index.js
41 ↗(On Diff #22411)

I'd avoid it if possible. Can we declare it using the standard export type?

48–69 ↗(On Diff #22411)

Agree

71 ↗(On Diff #22411)

Why do we need this? Doesn't appear necessary in the original code. It's probably because you're returning an object with a key named TunnelbrokerClient when you claim you're returning just the object. Try to cooperate with the typechecker – it can help you. $FlowFixMe and any should be avoided whenever possible, and should be used very intentionally (just like unsafe code in Rust)

Please update Test Plan to cover testing calling publish from keyserver JS code, NOT from a JS "test repo"

Refactoring function to be used in any provided API, refactoring types, moving types to a separate file.

Fixing type file import path.

max retitled this revision from [keyserver] Adding of Tunnelbroker client type interface and getter in `rust-node-addon` index file to [keyserver] Unifying API interface and adding Tunnelbroker types.Feb 20 2023, 5:11 AM
keyserver/addons/rust-node-addon/index.js
7 ↗(On Diff #22717)

You'll need to rebase on D6784

37–38 ↗(On Diff #22717)

I forget why we did it this way, but there may have (not sure) been a good reason. It's worth testing to make sure you're not breaking anything here. I worry about testing because I'm not sure how you were able to get this to work without D6784 (maybe you don't have D6695 either?)

lib/types/rust-binding-types.js
8 ↗(On Diff #22717)

Avoid interface, use declare type if at all possible

Rebasing on master changes.

max added a subscriber: jon.
max added inline comments.
keyserver/addons/rust-node-addon/index.js
7 ↗(On Diff #22717)

You'll need to rebase on D6784

Rebased, thanks for pointing out this.

37–38 ↗(On Diff #22717)

I forget why we did it this way, but there may have (not sure) been a good reason. It's worth testing to make sure you're not breaking anything here.

I've tested it, nativeBinding have a default object, and registerUser, TunnelbrokerClient inside it.
I'm asking @jon did he tested it in D6790 without default?
Because @varun has the same approach and used default in his diff.
I'll update testing plan on my changes as I'll make work the following diff in the stack, but simply getting the nativeBinding to the console shows that the default should be used first. cc @jon

lib/types/rust-binding-types.js
8 ↗(On Diff #22717)

Avoid interface, use declare type if at all possible

We have a Class for the Tunnelbroker client and seems that in flow we can only declare a class, but it's derived from the napi-rs bridge generation. It seems only the interface can be used here.

max marked 3 inline comments as done.

Adding @jon to take a look.

keyserver/addons/rust-node-addon/index.js
36 ↗(On Diff #23008)
  1. We definitely need to get rid of default here. You haven't tested after rebasing, and you didn't look closely enough at the merge conflicts when resolving them.
  2. I'm still not sure about returning nativeBinding directly, vs. destructuring nativeBinding and return the functions directly. If you've tested this, that's good... but I worry that you haven't tested, since otherwise you would've realized that default doesn't work.

Fix Flow types and remove .default

I tested the types and they're good. Did not test the actual bindings but I assume @max did

This revision is now accepted and ready to land.Feb 24 2023, 1:56 PM

Fix Flow types and remove .default

Thanks, @ashoat for fixing this flow type!

keyserver/addons/rust-node-addon/index.js
36 ↗(On Diff #23008)
  1. We definitely need to get rid of default here. You haven't tested after rebasing, and you didn't look closely enough at the merge conflicts when resolving them.

My fault! We no longer need .default with the recent changes from D6784.

  1. I'm still not sure about returning nativeBinding directly, vs. destructuring nativeBinding and return the functions directly. If you've tested this, that's good... but I worry that you haven't tested, since otherwise you would've realized that default doesn't work.

It can be tested easily by adding a simple code to the keyserver/src/keyserver.js:

    import { getRustAPI } from 'rust-node-addon';
...
    const rustBinding = await getRustAPI();
    const tbClientBinding = rustBinding.TunnelbrokerClient;
    const tbClient = new tbClientBinding('ksID', (error, msg) => {
      console.log(`tbClient got: ${msg}`);
      if (error) {
        console.log(`tbClient got an error: ${error.toString()}`);
      }
    });
    tbClient.publish('someOtherID', 'Hello from keyserver');

I've tested it and we can return nativeBinding directly.

max marked an inline comment as done.

Rebasing on master and parent changes.