bot/modules/nostr: convert to TS #627
Conversation
There was a problem hiding this comment.
Hi @webwarrior-ws when I type /nostr have an Error
and when type /npub with a correct npub this the bot says:
⚠️ ERROR Not a valid NIP-19 (https://github.com/nostr-protocol/nips/blob/master/19.md) npub. Please check your input.
webwarrior-ws
force-pushed
the
convert-to-ts-final-5
branch
from
d627989 to
7e0587c
Compare
|
@Catrya take a look, I added a commit that should fix the error. Assuming |
webwarrior-ws
force-pushed
the
convert-to-ts-final-5
branch
from
5adbdc2 to
5e7ad23
Compare
|
Hi, @webwarrior-ws it keep getting the same 2 errors: with Anyway, what do you think @grunch about removing the |
webwarrior-ws
force-pushed
the
convert-to-ts-final-5
branch
from
5e7ad23 to
d356b5b
Compare
WalkthroughThe pull request introduces a comprehensive modernization of the Nostr module in the bot project. The changes primarily focus on transitioning from CommonJS to ES module syntax, adding TypeScript type annotations, and improving module structure. The modifications span across multiple files in the Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Convert bot/modules/nostr to TypeScript. Specified "lib" property in tsconfig.json because otherwise TypeScript compiler complains about usage of `Promise.any` in bot/modules/nostr/index.ts: ``` Property 'any' does not exist on type 'PromiseConstructor'. Do you need to change your target library? Try changing the lib compiler option to 'es2021' or later. ```
Decode nostr secret key from environment variable using hex encoding. Also replace requires with imports.
webwarrior-ws
force-pushed
the
convert-to-ts-final-5
branch
from
ba12c3a to
0e0b0b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
bot/modules/nostr/index.ts (1)
Line range hint
26-36: Improve error handling for event publishingThe Promise.any usage needs better error handling as it will reject if all publishes fail.
OrderEvents.onOrderUpdated(async (order: IOrder) => { try { const event = await createOrderEvent(order); if (event) { - await Promise.any(Config.pool.publish(Config.getRelays(), event)); + const relays = Config.getRelays(); + if (relays.length === 0) { + throw new Error('No relays configured'); + } + try { + await Promise.any(Config.pool.publish(relays, event)); + } catch (publishError) { + throw new Error('Failed to publish to any relay: ' + publishError.message); + } } return event; } catch (err) { - logger.error(err); + logger.error('Failed to process order update:', err); + return undefined; } });bot/modules/nostr/events.ts (1)
Line range hint
57-82: Add error handling and return type annotation.The function should handle potential undefined private key and have an explicit return type.
-export const createOrderEvent = async (order: IOrder) => { +export const createOrderEvent = async (order: IOrder): Promise<{ [key: string]: any } | undefined> => { const myPrivKey = Config.getPrivateKey(); + if (!myPrivKey) { + console.error('Private key not configured'); + return; + } if (order.is_public === false) { return; }
🧹 Nitpick comments (7)
bot/modules/nostr/index.ts (3)
1-1: Convert require to import statementFor consistency with ES modules:
-require('websocket-polyfill'); +import 'websocket-polyfill';
9-9: Convert CommunityEvents require to importFor consistency with ES modules:
-const CommunityEvents = require('../events/community'); +import * as CommunityEvents from '../events/community';
20-22: Add type for community object and implement notification logicThe TODO comment should be addressed and proper typing added for the community object.
Would you like me to help implement the notification logic for community updates? I can also help define the proper type for the community object instead of using
any.tsconfig.json (1)
7-7: Consider removing the DOM library.The inclusion of "DOM" in the lib array seems unnecessary for a bot module that doesn't interact with browser APIs. The "ES2021" library is sufficient for
Promise.anysupport.- "lib":["ES2021", "DOM"], + "lib":["ES2021"],bot/modules/nostr/events.ts (3)
1-6: Consider organizing imports by category.For better maintainability, consider grouping imports into categories:
- External dependencies
- Internal config/utils
- Models/Types
import { finalizeEvent, verifyEvent } from 'nostr-tools'; + import * as Config from './config'; - -import { Community } from '../../../models'; -import { toKebabCase, removeAtSymbol } from '../../../util'; import { IOrder } from '../../../models/order'; +import { Community } from '../../../models'; + +import { toKebabCase, removeAtSymbol } from '../../../util';
12-18: Improve error message and number parsing.Consider the following improvements:
- Make error messages more descriptive by including the expected format/purpose
- Add radix parameter to parseInt for explicit base-10 parsing
const orderToTags = async (order: IOrder) => { const orderPublishedExpirationWindow = process.env.ORDER_PUBLISHED_EXPIRATION_WINDOW; if(orderPublishedExpirationWindow === undefined) - throw new Error("Environment variable ORDER_PUBLISHED_EXPIRATION_WINDOW is not defined"); + throw new Error("Environment variable ORDER_PUBLISHED_EXPIRATION_WINDOW is not defined. Expected: number of seconds for order expiration window"); const expiration = Math.floor(Date.now() / 1000) + - parseInt(orderPublishedExpirationWindow); + parseInt(orderPublishedExpirationWindow, 10);
20-24: Simplify fiat_amount array construction.The current implementation can be simplified using array spread operator.
- const fiat_amount = ['fa']; - if (order.fiat_amount === undefined) { - fiat_amount.push(order.min_amount.toString(), order.max_amount.toString()); - } else { - fiat_amount.push(order.fiat_amount.toString()); - } + const fiat_amount = [ + 'fa', + ...(order.fiat_amount === undefined + ? [order.min_amount.toString(), order.max_amount.toString()] + : [order.fiat_amount.toString()]) + ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bot/modules/nostr/commands.ts(1 hunks)bot/modules/nostr/config.js(0 hunks)bot/modules/nostr/config.ts(1 hunks)bot/modules/nostr/events.ts(3 hunks)bot/modules/nostr/index.ts(2 hunks)bot/modules/nostr/lib.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- bot/modules/nostr/config.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
bot/modules/nostr/commands.ts (1)
Line range hint
6-22: LGTM! Well-structured command implementation.The code has proper:
- Error handling with logging
- Type annotations
- HTML escaping for relay URLs
bot/modules/nostr/index.ts (1)
24-24: 🛠️ Refactor suggestionMove OrderEvents import to top level
The require statement should be moved to the top level with other imports:
- const OrderEvents = require('../events/orders');Add to top-level imports:
+import * as OrderEvents from '../events/orders';Likely invalid or redundant comment.
bot/modules/nostr/events.ts (1)
Line range hint
57-82: Verify event creation functionality.Given the concerns about Nostr notifications not working properly, let's verify the event creation and publication flow.
✅ Verification successful
Event creation and publication flow is properly implemented ✅
The code correctly handles the complete flow from event creation to publication:
- Creates events only for public orders
- Verifies events before publication
- Publishes to configured relays using the pool
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of createOrderEvent and verify the event publication flow echo "Searching for createOrderEvent usage..." rg -A 5 "createOrderEvent" --type typescript echo "\nSearching for event publication code..." rg "publish.*Event" --type typescript echo "\nChecking for any error logging related to Nostr events..." rg "console\.(error|warn).*nostr" -i --type typescriptLength of output: 557
Script:
#!/bin/bash # Search with correct file extensions echo "Searching for createOrderEvent usage..." rg -A 5 "createOrderEvent" -t ts -t js echo "\nSearching for event publication code..." rg "publish.*Event" -t ts -t js echo "\nChecking for Nostr configuration..." rg "Config\.get.*(?i)nostr" -t ts -t js echo "\nChecking for any error logging related to Nostr..." rg "console\.(error|warn).*nostr" -i -t ts -t js echo "\nLooking for Nostr-related files..." fd -e ts -e js . | grep -i nostrLength of output: 1899
bot/modules/nostr/lib.ts
Outdated
| export const encodeNpub = (hex: string) => { | ||
| return Nostr.nip19.npubEncode(hex); | ||
| }; |
There was a problem hiding this comment.
Add error handling and return type annotation for encodeNpub
The function should handle invalid hex strings and include return type annotation.
-export const encodeNpub = (hex: string) => {
+export const encodeNpub = (hex: string): string => {
+ if (!/^[0-9a-f]{64}$/i.test(hex)) {
+ throw new Error('Invalid hex string');
+ }
return Nostr.nip19.npubEncode(hex);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const encodeNpub = (hex: string) => { | |
| return Nostr.nip19.npubEncode(hex); | |
| }; | |
| export const encodeNpub = (hex: string): string => { | |
| if (!/^[0-9a-f]{64}$/i.test(hex)) { | |
| throw new Error('Invalid hex string'); | |
| } | |
| return Nostr.nip19.npubEncode(hex); | |
| }; |
bot/modules/nostr/lib.ts
Outdated
| export const decodeNpub = (npub: string) => { | ||
| try { | ||
| const { type, data } = Nostr.nip19.decode(npub); | ||
| if (type === 'npub') return data; | ||
| } catch (err) {} | ||
| }; |
There was a problem hiding this comment.
Add error handling and return type annotation for decodeNpub
The function has the following issues:
- Empty catch block silently fails
- Missing return type annotation
- Implicit undefined return when type !== 'npub'
-export const decodeNpub = (npub: string) => {
+export const decodeNpub = (npub: string): string | undefined => {
try {
const { type, data } = Nostr.nip19.decode(npub);
if (type === 'npub') return data;
+ return undefined;
} catch (err) {
+ logger.error('Failed to decode npub:', err);
+ return undefined;
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const decodeNpub = (npub: string) => { | |
| try { | |
| const { type, data } = Nostr.nip19.decode(npub); | |
| if (type === 'npub') return data; | |
| } catch (err) {} | |
| }; | |
| export const decodeNpub = (npub: string): string | undefined => { | |
| try { | |
| const { type, data } = Nostr.nip19.decode(npub); | |
| if (type === 'npub') return data; | |
| return undefined; | |
| } catch (err) { | |
| console.error('Failed to decode npub:', err); | |
| return undefined; | |
| } | |
| }; |
| export const pool = new SimplePool(); | ||
| const relays = (env => { | ||
| if (!env.RELAYS) return []; | ||
| return env.RELAYS.split(','); | ||
| })(process.env); |
There was a problem hiding this comment.
Add type annotation for relays array and validate relay URLs
The relays array should be typed and URLs should be validated.
export const pool = new SimplePool();
+const isValidRelayUrl = (url: string): boolean => {
+ try {
+ return new URL(url).protocol === 'wss:';
+ } catch {
+ return false;
+ }
+};
+
+const relays: string[] = ((env: NodeJS.ProcessEnv) => {
if (!env.RELAYS) return [];
- return env.RELAYS.split(',');
+ return env.RELAYS.split(',').filter(isValidRelayUrl);
})(process.env);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const pool = new SimplePool(); | |
| const relays = (env => { | |
| if (!env.RELAYS) return []; | |
| return env.RELAYS.split(','); | |
| })(process.env); | |
| export const pool = new SimplePool(); | |
| const isValidRelayUrl = (url: string): boolean => { | |
| try { | |
| return new URL(url).protocol === 'wss:'; | |
| } catch { | |
| return false; | |
| } | |
| }; | |
| const relays: string[] = ((env: NodeJS.ProcessEnv) => { | |
| if (!env.RELAYS) return []; | |
| return env.RELAYS.split(',').filter(isValidRelayUrl); | |
| })(process.env); |
| const nostrSkEnvVar = process.env.NOSTR_SK; | ||
| const sk = nostrSkEnvVar ? Buffer.from(nostrSkEnvVar, 'hex') : generateSecretKey(); | ||
| const pk = nostrGetPublicKey(sk); |
There was a problem hiding this comment.
Add validation for NOSTR_SK environment variable
The hex format of NOSTR_SK should be validated before usage.
const nostrSkEnvVar = process.env.NOSTR_SK;
-const sk = nostrSkEnvVar ? Buffer.from(nostrSkEnvVar, 'hex') : generateSecretKey();
+const sk = nostrSkEnvVar
+ ? (/^[0-9a-f]{64}$/i.test(nostrSkEnvVar)
+ ? Buffer.from(nostrSkEnvVar, 'hex')
+ : generateSecretKey())
+ : generateSecretKey();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const nostrSkEnvVar = process.env.NOSTR_SK; | |
| const sk = nostrSkEnvVar ? Buffer.from(nostrSkEnvVar, 'hex') : generateSecretKey(); | |
| const pk = nostrGetPublicKey(sk); | |
| const nostrSkEnvVar = process.env.NOSTR_SK; | |
| const sk = nostrSkEnvVar | |
| ? (/^[0-9a-f]{64}$/i.test(nostrSkEnvVar) | |
| ? Buffer.from(nostrSkEnvVar, 'hex') | |
| : generateSecretKey()) | |
| : generateSecretKey(); | |
| const pk = nostrGetPublicKey(sk); |
| export const addRelay = (relay: string) => { | ||
| relays.push(relay); | ||
| relays.map(relay => pool.ensureRelay(relay)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve relay management
The current implementation has potential issues:
- Duplicate relays aren't prevented
- No validation of relay URL format
- No cleanup of disconnected relays
-export const addRelay = (relay: string) => {
+export const addRelay = (relay: string): boolean => {
+ if (!isValidRelayUrl(relay) || relays.includes(relay)) {
+ return false;
+ }
relays.push(relay);
- relays.map(relay => pool.ensureRelay(relay));
+ pool.ensureRelay(relay);
+ return true;
};Committable suggestion skipped: line range outside the PR's diff.
Using `Nostr` from `nostr-tools` as before resulted in error: ``` Cannot read properties of undefined (reading 'nip19') TypeError: Cannot read properties of undefined (reading 'nip19') ```
webwarrior-ws
force-pushed
the
convert-to-ts-final-5
branch
from
a776738 to
10c646c
Compare
|
@Catrya I fixed both errors in the last commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bot/modules/nostr/lib.ts (2)
3-8:⚠️ Potential issueAdd error handling and return type annotation for decodeNpub
The function has the following issues:
- Empty catch block silently fails
- Missing return type annotation
- Implicit undefined return when type !== 'npub'
-export const decodeNpub = (npub: string) => { +export const decodeNpub = (npub: string): string | undefined => { try { const { type, data } = nip19.decode(npub); if (type === 'npub') return data; + return undefined; } catch (err) { + logger.error('Failed to decode npub:', err); + return undefined; } };
9-11:⚠️ Potential issueAdd error handling and return type annotation for encodeNpub
The function should handle invalid hex strings and include return type annotation.
-export const encodeNpub = (hex: string) => { +export const encodeNpub = (hex: string): string => { + if (!/^[0-9a-f]{64}$/i.test(hex)) { + throw new Error('Invalid hex string'); + } return nip19.npubEncode(hex); };
🧹 Nitpick comments (1)
bot/modules/nostr/commands.ts (1)
Line range hint
6-22: Consider adding input validation for publicKeyThe function has good error handling and type annotations. However, consider validating the publicKey format before encoding it to npub.
export const info = async (ctx: MainContext) => { try { const publicKey = Config.getPublicKey(); if (!publicKey) return; + if (!/^[0-9a-f]{64}$/i.test(publicKey)) { + logger.error('Invalid public key format'); + return ctx.reply('Error: Invalid public key format'); + } const info = { publicKey, npub: nip19.npubEncode(publicKey),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bot/modules/nostr/commands.ts(1 hunks)bot/modules/nostr/lib.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci_to_main
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
bot/modules/nostr/commands.ts (1)
1-4: LGTM! Clean module importsThe imports are well-organized, using specific imports where possible and namespace imports where appropriate. The type imports are correctly separated.
| import { nip19 } from 'nostr-tools'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add logger import
Import the logger module to support error logging in the decodeNpub function.
import { nip19 } from 'nostr-tools';
+import { logger } from '../../../logger';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { nip19 } from 'nostr-tools'; | |
| import { nip19 } from 'nostr-tools'; | |
| import { logger } from '../../../logger'; |
Convert bot/modules/nostr to TypeScript.
Specified "lib" property in tsconfig.json because otherwise TypeScript compiler complains about usage of
Promise.anyin bot/modules/nostr/index.ts:Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates