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

lnc+util: assign WASM callbacks to a namespaced scope #79

Merged
merged 2 commits into from
Mar 6, 2023

Conversation

jamaljsr
Copy link
Member

@jamaljsr jamaljsr commented Mar 1, 2023

This PR updates the LNC class to no longer use the JS global object for the callbacks from the WASM client. The callbacks functions will now be nested under an object using the namespace as a name. So if the app uses new LNC({ namespace: 'myapp' });, the callbacks will be created on the global.myapp object instead of previously being on global.

This depends on lightninglabs/lightning-node-connect#72 which has been merged into the master branch.

Steps to test with the kitchen-sink demo:

  1. Clone the lightning-node-connect repo
  2. Run make wasm to produce the WASM binary
  3. Checkout this PR branch in the lnc-web repo
  4. Copy lightning-node-connect/example/wasm-client.wasm into lnc-web/demos/kitchen-sink/public/
  5. Update kitchen-sink/src/App.js to use the new WASM binary and your own pairing phrase and namespace
      const lnc = new LNC({
         namespace: 'myapp',
         pairingPhrase: 'iron boss window awful list bag crucial mixture useless piano',
         password: 'Dm9lfaWmo92Q#m9f',
         wasmClientCode: '/lnc-5903579.wasm',
      });
  6. Ensure that the kitchen-sink demo is using this branch of the lnc-web code. Then start the app
    $ cd demos/kitchen-sink/
    $ yarn
    $ yarn add ../..
    $ yarn start
  7. In the browser, click on the "Connect" button and confirm the connection is successful by clicking on getInfo
  8. In the console, confirm that window.onLocalPrivCreate is undefined and that typeof window.myapp.onLocalPrivCreate is function

Note: This PR also fixes an issue in the credentialStore where it was overwriting the data on every page reload in the kitchen-sink demo. This was due to the password being provided in the constructor.

Copy link
Contributor

@itsrachelfish itsrachelfish left a comment

Choose a reason for hiding this comment

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

Read over the code and everything looks good. Used yarn link to test this package locally with my test script. I was able to get 3 connections working simultaneously but for some reason when I tried with 4 nodes at once the last node was still being silently dropped. 🤔

Screenshot from 2023-03-02 03-06-57

lib/lnc.ts Show resolved Hide resolved
lib/lnc.ts Show resolved Hide resolved
@jamaljsr
Copy link
Member Author

jamaljsr commented Mar 2, 2023

but for some reason when I tried with 4 nodes at once the last node was still being silently dropped. 🤔

I'm not sure why that's happening. Can you confirm that the callbacks for all four connections are all being called? It should be logged in the console. You can also check that the local/remote keys are set for all 4 nodes in localStorage.

@itsrachelfish
Copy link
Contributor

@jamaljsr after doing some further testing it looks like we might be hitting some kind of rate limiting / DOS protection in Firefox.

You can see in this first screenshot (firefox) only three of the WASM binaries are loaded -

Screenshot from 2023-03-02 03-10-35

However in Chrome it works fine with 4 simultaneous connections -

image

@jamaljsr
Copy link
Member Author

jamaljsr commented Mar 2, 2023

@jamaljsr after doing some further testing it looks like we might be hitting some kind of rate limiting / DOS protection in Firefox.

Interesting. I was not aware of that limitation, but it's definitely something we need to consider when handling multiple WASM instances. I wonder how the other browsers (Safari, Edge, Brave) handle it. Also mobile browsers may have strict limitations as well.

Copy link
Contributor

@itsrachelfish itsrachelfish left a comment

Choose a reason for hiding this comment

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

Approving this since my test script was able to connect 4 clients simultaneously in different namespaces using Chrome. Seems like we're hitting some other edge case in Firefox loading the WASM files. 🤔

Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

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

tACK. Great job, Jamal!

@jamaljsr jamaljsr merged commit f6d9d1f into main Mar 6, 2023
@jamaljsr jamaljsr deleted the namespace-scoped-callbacks branch March 6, 2023 15:07
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.

3 participants