Brc100 wallet infra example tests - #8
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for BRC-100 wallet operations targeting a local wallet-infra service. The tests demonstrate usage of various wallet operations including cryptographic operations, actions, outputs, certificates, and blockchain interactions. Additionally, TypeScript import issues for parseWalletOutpoint are suppressed with @ts-ignore directives.
- Adds 709 lines of test coverage for wallet-infra integration testing
- Implements database cleanup utilities for nosend transaction management
- Adds @ts-ignore suppressions for parseWalletOutpoint import issues in three files
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| src/brc100.test.ts | New comprehensive test suite covering wallet operations, cryptographic functions, actions, certificates, and blockchain queries against local wallet-infra service |
| src/listChange.ts | Added @ts-ignore comment to suppress TypeScript import error for parseWalletOutpoint |
| src/janitor.ts | Added @ts-ignore comment to suppress TypeScript import error for parseWalletOutpoint |
| src/internalizeWalletPayment.ts | Added @ts-ignore comment to suppress TypeScript import error for parseWalletOutpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const messageBytes = Buffer.from(message) | ||
| const hexData = messageBytes.toString('hex') | ||
| const length = messageBytes.length | ||
| const lockingScript = `006a${length.toString(16).padStart(2, '0')}${hexData}` |
There was a problem hiding this comment.
The locking script construction logic is duplicated on lines 364, 418, and potentially elsewhere. Consider extracting this into a helper function that takes a message and returns the locking script to improve maintainability and reduce duplication.
| expect(result).toBeDefined() | ||
| expect(result.publicKey).toBeDefined() | ||
| expect(typeof result.publicKey).toBe('string') | ||
| expect(result.publicKey.length).toBeGreaterThan(60) // Public key length |
There was a problem hiding this comment.
The magic number 60 for public key length validation is unclear. Consider adding a comment explaining this is the expected minimum hex-encoded compressed public key length, or extract it to a named constant.
| if ( | ||
| err.message.includes('Insufficient funds') || | ||
| err.message.includes('insufficient') | ||
| ) | ||
| return |
There was a problem hiding this comment.
The error handling pattern that silently returns on insufficient funds is repeated multiple times (lines 399-403, 455-459). This makes tests pass when they should be skipped, which can be confusing. Consider using test.skip() with a condition or a beforeEach check to explicitly skip tests when balance is insufficient.
| expect(result).toBeDefined() | ||
| expect(result.header).toBeDefined() | ||
| expect(typeof result.header).toBe('string') | ||
| expect(result.header.length).toBeGreaterThan(100) |
There was a problem hiding this comment.
The magic number 100 for header length validation is unclear. Consider adding a comment or extracting it to a named constant to clarify what this minimum length represents for block headers.
| // Check if there are any unsigned actions we can abort | ||
| const actions = await setup.wallet.listActions({ labels: [], limit: 20 }) | ||
| const unsignedAction = actions.actions.find( | ||
| a => a.status === 'unsigned' || a.status === 'nosend' | ||
| ) | ||
|
|
||
| // listActions doesn't return references, so we can only abort | ||
| // actions we created in the same session with signableTransaction | ||
| expect(unsignedAction === undefined || unsignedAction.status).toBeTruthy() |
There was a problem hiding this comment.
This test doesn't actually test the abortAction functionality - it only checks if an unsigned action exists. The test should either create an action to abort (with cleanup) or be renamed to reflect what it actually tests (e.g., 'should check for unsigned actions').
| // Check if there are any unsigned actions we can abort | |
| const actions = await setup.wallet.listActions({ labels: [], limit: 20 }) | |
| const unsignedAction = actions.actions.find( | |
| a => a.status === 'unsigned' || a.status === 'nosend' | |
| ) | |
| // listActions doesn't return references, so we can only abort | |
| // actions we created in the same session with signableTransaction | |
| expect(unsignedAction === undefined || unsignedAction.status).toBeTruthy() | |
| // Check balance first to avoid intermittent failures | |
| const balance = await setup.wallet.balance() | |
| if (balance < 10) return | |
| try { | |
| const message = 'Abort test' | |
| const messageBytes = Buffer.from(message) | |
| const hexData = messageBytes.toString('hex') | |
| const length = messageBytes.length | |
| const lockingScript = `006a${length.toString(16).padStart(2, '0')}${hexData}` | |
| // Create an unsigned (nosend) action that we can abort | |
| const action = await setup.wallet.createAction({ | |
| description: `Abort test: ${message}`, | |
| outputs: [ | |
| { | |
| lockingScript, | |
| satoshis: 0, | |
| outputDescription: 'Abort test output', | |
| basket: 'opreturn', | |
| tags: ['test', 'abort'] | |
| } | |
| ], | |
| labels: ['test:abort'], | |
| options: { | |
| noSend: true | |
| } | |
| }) | |
| expect(action).toBeDefined() | |
| expect(action.signableTransaction).toBeDefined() | |
| expect(action.signableTransaction.reference).toBeDefined() | |
| // Abort the unsigned action to release any locked UTXOs | |
| await setup.wallet.abortAction({ | |
| reference: action.signableTransaction.reference | |
| }) | |
| } catch (err: any) { | |
| if ( | |
| err.message.includes('Insufficient funds') || | |
| err.message.includes('insufficient') | |
| ) | |
| return | |
| throw err | |
| } |
| // @ts-ignore - parseWalletOutpoint import issue | ||
| import { parseWalletOutpoint } from '@bsv/wallet-toolbox/out/src/sdk' |
There was a problem hiding this comment.
Using @ts-ignore to suppress TypeScript errors for import issues is a temporary workaround that hides potential type safety problems. Consider using @ts-expect-error instead, which will produce an error if the suppression becomes unnecessary (when the import issue is fixed), or better yet, address the underlying import issue in the @bsv/wallet-toolbox package.
| // @ts-ignore - parseWalletOutpoint import issue | |
| import { parseWalletOutpoint } from '@bsv/wallet-toolbox/out/src/sdk' |
| SetupWallet | ||
| } from '@bsv/wallet-toolbox' | ||
| import { outputBRC29 } from './brc29' | ||
| // @ts-ignore - parseWalletOutpoint import issue |
There was a problem hiding this comment.
Using @ts-ignore to suppress TypeScript errors for import issues is a temporary workaround that hides potential type safety problems. Consider using @ts-expect-error instead, which will produce an error if the suppression becomes unnecessary (when the import issue is fixed), or better yet, address the underlying import issue in the @bsv/wallet-toolbox package.
| try { | ||
| const result = await setup.wallet.revealCounterpartyKeyLinkage({ | ||
| counterparty: 'self', | ||
| verifier: '02' + 'a'.repeat(64), // demo verifier pubkey |
There was a problem hiding this comment.
The hardcoded demo verifier public key '02' + 'a'.repeat(64) is repeated on lines 238 and 254. Extract this to a constant at the top of the file to reduce duplication and make it easier to maintain.
| async function cleanupNosendTransactions(): Promise<number> { | ||
| try { | ||
| // Fail all nosend transactions and release their outputs | ||
| const result = execSync( | ||
| `docker exec mysql mysql -uroot -prootPass wallet_storage -e " | ||
| -- Get nosend transaction IDs | ||
| SET @nosend_ids = (SELECT GROUP_CONCAT(transactionId) FROM transactions WHERE status='nosend'); | ||
|
|
||
| -- Release outputs consumed by nosend transactions | ||
| UPDATE outputs SET spendable=1, spentBy=NULL | ||
| WHERE spentBy IN (SELECT transactionId FROM transactions WHERE status='nosend'); | ||
|
|
||
| -- Mark nosend transactions as failed | ||
| UPDATE transactions SET status='failed' WHERE status='nosend'; | ||
|
|
||
| -- Count how many we cleaned up | ||
| SELECT COUNT(*) as cleaned FROM transactions WHERE status='failed' AND satoshis=-1; | ||
| " 2>/dev/null`, | ||
| { encoding: 'utf-8' } | ||
| ) | ||
|
|
||
| const match = result.match(/cleaned\n(\d+)/) | ||
| return match ? parseInt(match[1]) : 0 | ||
| } catch (err) { | ||
| // Docker/MySQL not available - skip cleanup | ||
| return 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
The comment says this function 'uses direct database access for test cleanup', but it's worth adding a warning that this approach bypasses the application layer and should only be used in test environments. Consider adding an environment check or more prominent documentation about the risks.
| @@ -1,5 +1,6 @@ | |||
| import { sdk, Setup } from '@bsv/wallet-toolbox' | |||
| import { | |||
| // @ts-ignore - parseWalletOutpoint import issue | |||
There was a problem hiding this comment.
Using @ts-ignore to suppress TypeScript errors for import issues is a temporary workaround that hides potential type safety problems. Consider using @ts-expect-error instead, which will produce an error if the suppression becomes unnecessary (when the import issue is fixed), or better yet, address the underlying import issue in the @bsv/wallet-toolbox package.
| // @ts-ignore - parseWalletOutpoint import issue | |
| // @ts-expect-error - parseWalletOutpoint import issue |
Added a test file to target the wallet-infra repo locally, maybe the example code it contains might be of some use?