Skip to content

Conversation

@jamaljsr
Copy link
Member

@jamaljsr jamaljsr commented Nov 20, 2025

Summary

This PR moves almost all WASM-related lifecycle, connection, and RPC logic out of the main LNC class into a dedicated WasmManager, while keeping the public LNC API the same. The goal is to keep LNC small and focused as it grows with upcoming session-based auth and passkey features, so new behavior can be added without turning LNC into an unmaintainable “god class.” These changes build on the groundwork from #130 and #131 as part of the Session-Based Authentication & Passkeys work.

Technical Notes

  • Keep LNC focused and maintainable: As we add more auth/session logic, the main LNC class would otherwise accumulate a lot of low-level WASM, connection, and RPC details; pushing those concerns into WasmManager keeps LNC closer to a thin façade over the APIs.
  • Support future auth features with clearer seams: With a dedicated manager that already handles connection state and credential providers, upcoming session-based auth and passkey features have a clear extension point instead of threading new logic through an already-large core class.

Steps to Test

  1. Run the unit tests (including the new WasmManager tests and updated LNC tests):

    yarn test
  2. Build the library to ensure the production bundle still compiles correctly:

    yarn build

Related Issues & Pull Requests

Depends on:

@jamaljsr jamaljsr self-assigned this Nov 20, 2025
@jamaljsr jamaljsr force-pushed the auth-03-wasm branch 2 times, most recently from 2d53e9d to 8d6504a Compare November 20, 2025 20:13
@jamaljsr jamaljsr changed the base branch from auth-02-config to auth-02b-eslint November 20, 2025 20:13
@jamaljsr jamaljsr force-pushed the auth-03-wasm branch 2 times, most recently from 96711f0 to 763f15d Compare November 23, 2025 13:22
@jamaljsr jamaljsr force-pushed the auth-02b-eslint branch 2 times, most recently from 41df1c5 to f2f808f Compare November 25, 2025 00:54
@jamaljsr jamaljsr requested a review from Copilot November 25, 2025 15:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extracts WASM-related lifecycle, connection, and RPC logic from the LNC class into a new WasmManager class. The refactoring maintains the existing public API while improving maintainability and preparing for future session-based authentication features.

Key Changes:

  • Created WasmManager class to encapsulate all WASM operations, connection management, and RPC communication
  • Updated LNC class to delegate WASM operations to WasmManager while preserving its public interface
  • Moved lncGlobal reference from lnc.ts to wasmManager.ts for better encapsulation

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/wasmManager.ts New class implementing WASM lifecycle, connection logic, and RPC communication
lib/wasmManager.test.ts Comprehensive test suite for the new WasmManager class
lib/lnc.ts Refactored to delegate WASM operations to WasmManager while maintaining public API
lib/lnc.test.ts Updated tests to reflect delegation pattern and removed implementation detail assertions
test/utils/test-helpers.ts Updated import path for lncGlobal reference
lib/index.ts Exported WasmManager for external use
lib/index.test.ts Added test coverage for WebAssembly polyfill functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@jamaljsr jamaljsr requested a review from jbrill November 25, 2025 15:47
@jamaljsr jamaljsr force-pushed the auth-03-wasm branch 2 times, most recently from da2316c to 5bf01d8 Compare November 25, 2025 22:58
Base automatically changed from auth-02b-eslint to main November 26, 2025 16:08
Copy link

@jbrill jbrill left a comment

Choose a reason for hiding this comment

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

Great work! I've left quite a few comments (some of which may or may not be helpful). The code reads much cleaner 👏

/**
* Waits until the WASM client is ready
*/
async waitTilReady(): Promise<void> {
Copy link

Choose a reason for hiding this comment

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

Some minor issues:

  1. No immediate check — It always waits 500ms before the first check, even if isReady is already true.
  2. Magic numbers — 500 and 20 are hardcoded.
  3. If the callback takes time to execute, this might not actually measure the 10 seconds.
  4. No cancellation possible

Since waitForConnection has the same pattern, we could maybe abstract some of this:

private async pollUntil(
  condition: () => boolean,
  errorMessage: string,
  timeout = 10_000
): Promise<void> {
  const start = Date.now();
  while (!condition()) {
    if (Date.now() - start > timeout) {
      throw new Error(errorMessage);
    }
    await new Promise(r => setTimeout(r, 500));
  }
}

async waitTilReady(): Promise<void> {
  await this.pollUntil(() => this.isReady, 'Failed to load the WASM client');
  log.info('The WASM client is ready');
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored this a bit:

  1. If isReady is already true, then waitTilReady won't be called.
  2. I extracted them into constants. I don't think they should be exposed via the public API as they are internal details
  3. The intent isn't to be exactly 10secs
  4. I don't think cancellation is necessary in this case

/**
* Connects to the LNC proxy server
*/
async connect(credentialProvider?: CredentialProvider): Promise<void> {
Copy link

Choose a reason for hiding this comment

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

I wonder if it makes sense to store our connections to ensure there are no race conditions when this is called twice:

private connectionPromise?: Promise<void>;

async connect(credentialProvider?: CredentialProvider): Promise<void> {
  if (this.isConnected) return;
  
  // Return existing connection attempt if in progress
  if (this.connectionPromise) {
    return this.connectionPromise;
  }
  
  this.connectionPromise = this.doConnect(credentialProvider);
  try {
    await this.connectionPromise;
  } finally {
    this.connectionPromise = undefined;
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible/supported to connect to multiple mailboxes simultaneously. Trying to add detection for duplicate connections would require a fair amount of code and complexity. I lean towards leaving it up to the caller to ensure they only connect once to a node. It will log a "mailbox in use" error in the console when that happens.

*/
async run(): Promise<void> {
// Make sure the WASM client binary is downloaded first
if (!this.isReady) {
Copy link

Choose a reason for hiding this comment

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

Would the check for downloaded be if (!this.result) {?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using this.isReady ensures that the wasm was downloaded and is accessible to the JS runtime as well. This is more reliable than just checking this.result.

* Downloads the WASM client binary
*/
async preload(): Promise<void> {
this.result = await WebAssembly.instantiateStreaming(
Copy link

Choose a reason for hiding this comment

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

Should we check if this.result already exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a _preloadPromise to prevent simultaneous preloads. We have had this issue in Terminal when React executes useEffects multiple times.


this.go.argv = [
'wasm-client',
'--debuglevel=debug,GOBN=info,GRPC=info',
Copy link

Choose a reason for hiding this comment

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

Would it make sense to have the debug level configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

We haven't had any requests for this or needed it ourselves, so I'd prefer to leave it as-is for now.

}
if (!this.wasm.onAuthData) {
this.wasm.onAuthData = (keyHex: string) => {
log.debug('auth data received: ' + keyHex);
Copy link

Choose a reason for hiding this comment

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

Are we doing anything on auth data? Is this just for debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

This callback is required by the LNC wasm but we've never needed this data for anything.

@lightninglabs-deploy
Copy link

@jamaljsr, remember to re-request review from reviewers when ready

@jamaljsr
Copy link
Member Author

jamaljsr commented Dec 9, 2025

@jbrill Thanks for the thorough review. I've addressed all of your feedback. All of that was pre-existing code that I just moved from one file to another. It did need some cleaning up, so thanks for the nudge.

@jamaljsr jamaljsr requested a review from jbrill December 9, 2025 23:55
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