Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a Lightning Network liquidity manager proof-of-concept with authentication via Keycloak, database persistence, and multiple swap strategies for moving Lightning channel balances to Liquid Bitcoin.
Changes:
- Added new liquidity manager backend and frontend applications with full OIDC authentication
- Extracted LND service into a shared crypto-clients package for reuse across applications
- Integrated Keycloak for authentication with PostgreSQL session storage
Reviewed changes
Copilot reviewed 72 out of 300 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server-backend/tsconfig.json | Added crypto-clients package path reference |
| server-backend/src/* | Updated imports to use crypto-clients package for LND service |
| server-backend/package.json | Added crypto-clients dependency, removed gRPC dependencies |
| liquidity-manager-frontend/* | New frontend application with SolidJS for managing channel liquidity |
| liquidity-manager-backend/* | New backend API with swap execution, history tracking, and Keycloak auth |
| crypto-clients/* | New shared package containing LND service extracted from server-backend |
| docker/* | Added Keycloak service and PostgreSQL initialization for auth databases |
| package.json | Added new workspace packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (swap.chain !== cryptoCode || swap.contractAddress == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The null check for swap.contractAddress was added as a guard, but this change may indicate a logic issue. If contractAddress can be null at this point in the process, the code after line 198 (specifically line 212) will still throw an error when trying to use it. Consider adding an early return with a clear error message if contractAddress is null, rather than allowing execution to continue to line 212 where it will fail.
| if (swap.chain !== cryptoCode || swap.contractAddress == null) { | |
| return; | |
| } | |
| if (swap.chain !== cryptoCode) { | |
| // Ignore events for other chains | |
| return; | |
| } | |
| if (swap.contractAddress == null) { | |
| this.logger.error( | |
| `Swap ${swap.id} on chain ${swap.chain} has null contractAddress in processNewTransaction`, | |
| ); | |
| return; | |
| } |
| async run(): Promise<void> { | ||
| const swap = await this.repository.findOneByOrFail({ id: this.id }); | ||
| if (swap.status !== 'CREATED') { | ||
| this.logger.error(`[swap:${swap.id}] Swap is alreay in progress but is not resumable. Failing...`); |
There was a problem hiding this comment.
Corrected spelling of 'alreay' to 'already'.
| this.logger.error(`[swap:${swap.id}] Swap is alreay in progress but is not resumable. Failing...`); | |
| this.logger.error(`[swap:${swap.id}] Swap is already in progress but is not resumable. Failing...`); |
| # opening channels one way | ||
| wait_for_chain_sync |
There was a problem hiding this comment.
A duplicate call to wait_for_chain_sync was removed on line 47, but this appears to be intentional based on the comment '# opening channels one way' on line 48. The removed call may have been serving a purpose before opening channels. Verify that removing this synchronization check doesn't cause timing issues during channel opening.
| const userInfo = await this.oidcService.getUserInfo(tokenSet.access_token!); | ||
|
|
There was a problem hiding this comment.
Using the non-null assertion operator (!) on tokenSet.access_token assumes it will always be present. Consider adding explicit validation and error handling if the access_token is missing instead of asserting its presence.
| const userInfo = await this.oidcService.getUserInfo(tokenSet.access_token!); | |
| if (!tokenSet.access_token) { | |
| this.logger.error('No access token received from token endpoint'); | |
| return res.redirect(this.getBaseUrl() + '?error=no_access_token'); | |
| } | |
| const userInfo = await this.oidcService.getUserInfo(tokenSet.access_token); |
|
|
||
| const tokenSet = await response.json(); | ||
| this.logger.log('Token exchange successful'); | ||
| return tokenSet as any; // TODO |
There was a problem hiding this comment.
The as any cast with a TODO comment indicates incomplete type handling. Define a proper interface for the token set response instead of using any, which bypasses type safety.
| if (!strategy) { | ||
| throw new BadRequestException(`Unknown strategy: ${strategy}. Available strategies: ${this.getAvailableStrategies().join(', ')}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
The condition if (!strategy) will never be true because the strategy parameter is validated by Zod in the DTO (line 14 of SwapController.ts checks it's a valid enum value). This check is redundant and the error message on line 51 would never be reached. Consider removing this check or revising the validation logic.
| if (!strategy) { | |
| throw new BadRequestException(`Unknown strategy: ${strategy}. Available strategies: ${this.getAvailableStrategies().join(', ')}`); | |
| } |
| this.logger.log(`[swap:${swapId}] Starting background execution`); | ||
| void runner!.run(); |
There was a problem hiding this comment.
Using the non-null assertion operator (!) on runner and then voiding the promise is risky. The switch statement should guarantee runner is defined through the satisfies never check, but consider either removing the assertion and handling the case explicitly, or adding proper error handling for the voided promise.
| this.logger.log(`[swap:${swapId}] Starting background execution`); | |
| void runner!.run(); | |
| if (!runner) { | |
| this.logger.error(`[swap:${swapId}] No liquidity swap runner found for strategy "${strategy}"`); | |
| return; | |
| } | |
| this.logger.log(`[swap:${swapId}] Starting background execution`); | |
| void runner.run().catch((error) => { | |
| const message = | |
| error instanceof Error | |
| ? `${error.message}` | |
| : String(error); | |
| const stack = error instanceof Error ? error.stack : undefined; | |
| this.logger.error(`[swap:${swapId}] Error during background execution: ${message}`, stack); | |
| }); |
|
|
||
| auth: | ||
| keycloak: | ||
| url: http://localhost:7085 |
There was a problem hiding this comment.
The Keycloak URL port 7085 doesn't match the port 8080 mentioned in the AUTHENTICATION.md documentation (line 9 and 16). This inconsistency could cause confusion. Consider documenting that 7085 is the external mapped port while 8080 is the internal container port, or update the documentation to consistently use 7085.
| url: http://localhost:7085 | |
| url: http://localhost:8080 |
7c947f6 to
9feeb9f
Compare
No description provided.