Feat: Firo network#799
Open
kvhnuke wants to merge 115 commits into
Open
Conversation
…ture/add-firo-network # Conflicts: # packages/extension/src/libs/utils/wasmModule/spark.js
Add TX activity history after sending from Spark address
…ture/add-firo-network
…ed-spark Disable activity when spark to spark transaction is failed or dropped
…ture/add-firo-network
Revert "Fix get balance"
Fix activity state and add reset DB on reset
…ture/add-firo-network
2. Fix state init
3. In QR now shows Firo:{address}
1. Accounts list show spark addresses
…ture/add-firo-network
Release: v2.17.0
In getSparkCoinInfo, the WASM buffer for the serial context was allocated with serializedCoin.length and then populated with serialContext via HEAPU8.set(serialContext, ...). When serialContext.length exceeds serializedCoin.length, this writes past the allocated region, corrupting WASM heap memory and producing non-deterministic failures or undefined behavior in js_deserializeCoin. Use serialContext.length, matching the pattern already used in libs/spark-handler/index.ts. Co-authored-by: Reuben Yap <reuben@firo.org>
|
💼 Build Files |
WalkthroughAdds Firo mainnet/testnet and Spark privacy: wallet/Electrum, WASM bridge, IndexedDB sync/worker, provider/activity, and UI for transparent/Spark send and verification, with private balance aggregation. ChangesFiro + Spark end-to-end integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/extension/src/ui/action/views/reset-wallet/index.vue (1)
69-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure
isProcessingis reset when reset flow fails.If Line 72 or Line 73 throws, the button remains disabled indefinitely.
Proposed fix
const resetAction = async () => { - isProcessing.value = true; - const keyring = new KeyRingBase(); - await keyring.reset(); - await db.resetDB(); - emit('action:lock'); - openOnboard().then(() => { - window.close(); - }); + isProcessing.value = true; + try { + const keyring = new KeyRingBase(); + await keyring.reset(); + await db.resetDB(); + emit('action:lock'); + await openOnboard(); + window.close(); + } finally { + isProcessing.value = false; + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/views/reset-wallet/index.vue` around lines 69 - 78, The resetAction currently sets isProcessing.value = true but never resets it if KeyRingBase().reset() or db.resetDB() throws; wrap the async steps in a try/catch/finally (or use .catch/.finally) so that any thrown error is handled and isProcessing.value is set back to false in the finally block; keep the existing emit('action:lock') and openOnboard() logic but ensure isProcessing.value = false runs after success or failure, and optionally rethrow or handle/log the error in the catch to preserve behavior.packages/extension/src/ui/action/views/lock-screen/components/lock-screen-forgot.vue (1)
71-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd failure recovery for processing state in reset flow.
If Line 74 or Line 75 throws, the action stays locked because
isProcessingnever returns tofalse.Proposed fix
const resetAction = async () => { - isProcessing.value = true; - const keyring = new KeyRingBase(); - await keyring.reset(); - await db.resetDB(); - openOnboard(); + isProcessing.value = true; + try { + const keyring = new KeyRingBase(); + await keyring.reset(); + await db.resetDB(); + await openOnboard(); + } finally { + isProcessing.value = false; + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/views/lock-screen/components/lock-screen-forgot.vue` around lines 71 - 77, The resetAction currently sets isProcessing.value = true but never guarantees it is set back to false if KeyRingBase().reset() or db.resetDB() throws; wrap the async work in a try/catch/finally (or use .finally) so that isProcessing.value is reset to false in the finally block, optionally handling/logging errors from KeyRingBase.reset and db.resetDB before calling openOnboard; ensure references to isProcessing, resetAction, KeyRingBase.reset, db.resetDB and openOnboard are updated so the processing state is always cleared even on failure.packages/extension/src/ui/action/App.vue (1)
392-433:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
sparkAccounttonullfor non-Firo networks.Initializing
sparkAccountas{}makes it truthy on all networks and can bypass null checks in consumers expecting either a populated SparkAccount ornull.Suggested fix
- const sparkAccount = {} as SparkAccount; + let sparkAccount: SparkAccount | null = null; ... - if (network.name === NetworkNames.Firo) { + if (network.name === NetworkNames.Firo) { + sparkAccount = {} as SparkAccount; ... - sparkAccount['sparkBalance'] = { availableBalance }; + sparkAccount.sparkBalance = { availableBalance }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/App.vue` around lines 392 - 433, The code always initializes sparkAccount as an empty object which is truthy on non-Firo networks; change the logic so sparkAccount is only populated for NetworkNames.Firo and set to null otherwise. Specifically, update the initialization and branching around the sparkAccount variable (the const sparkAccount declaration and the if (network.name === NetworkNames.Firo) block) so that when the network is not Firo you assign null to sparkAccount, and when it is Firo you build the object as currently done; ensure accountHeaderData.value uses that null value for sparkAccount so consumers can reliably check for presence.
🟡 Minor comments (17)
packages/extension/src/libs/spark-handler/utils.ts-14-23 (1)
14-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling for invalid base64 input.
The
atobfunction on Line 15 will throw an error if the input is not valid base64. This could cause uncaught exceptions in the caller.🛡️ Proposed fix with validation
export const base64ToReversedHex = (base64: string): string => { + if (!base64) { + throw new Error('Base64 input cannot be empty'); + } + + let binary: string; + try { - const binary = atob(base64); + binary = atob(base64); + } catch (error) { + throw new Error(`Invalid base64 input: ${error instanceof Error ? error.message : 'unknown error'}`); + } + const bytes = new Uint8Array(binary.length); for (let i = 0; i < binary.length; i++) { bytes[i] = binary.charCodeAt(i); } const reversed = Array.from(bytes).reverse(); return reversed.map(b => b.toString(16).padStart(2, '0')).join(''); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/spark-handler/utils.ts` around lines 14 - 23, The function base64ToReversedHex can throw when atob receives invalid base64; wrap the decoding in a try/catch (or validate the input before calling atob) inside base64ToReversedHex so the function either returns a clear error value or throws a controlled Error with a descriptive message; ensure the catch handles malformed input and does not allow uncaught exceptions to propagate to callers.packages/extension/src/providers/bitcoin/libs/firo-wallet/utils.ts-1-19 (1)
1-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix callback signature mismatch and improve type safety.
The callback signature declares an optional
indexparameter, but Line 11 never passes it when callingcallback(data[i]). Additionally, usingany[]reduces type safety.🔧 Proposed fix
-export const chunkedEvery = ( - data: any[], +export const chunkedEvery = <T>( + data: T[], chunksize: number, - callback: (data: any, index?: number) => void, + callback: (data: T, index: number) => void, finished: () => void, ) => { let i = 0; (function chunk() { const end = Math.min(i + chunksize, data.length); for (; i < end; ++i) { - callback(data[i]); + callback(data[i], i); } if (i < data.length) { setTimeout(chunk, 0); } else { finished.call(null); } })(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/libs/firo-wallet/utils.ts` around lines 1 - 19, The function chunkedEvery has a callback signature mismatch and weak typing; change it to be generic (e.g., <T>) and type the parameters as data: T[], chunksize: number, callback: (item: T, index: number) => void, finished: () => void, and when invoking the callback inside chunk() pass both the item and its index (call callback(data[i], i)); update any other related signatures in chunkedEvery to use the new generic types for improved type safety.packages/extension/src/libs/spark-handler/utils.ts-1-12 (1)
1-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix return type and add input validation.
The function has type safety and validation issues:
- Lines 9-11 use optional chaining and can return
undefined, but the return type isn't declared asstring | undefined- No validation for negative numbers, which would produce unexpected results
- The regex
matchcan returnnullif no matches are found (though unlikely with the current logic)🔧 Proposed fix
-export const numberToReversedHex = (num: number) => { +export const numberToReversedHex = (num: number): string => { + if (num < 0) { + throw new Error('Number must be non-negative'); + } + let hex = num.toString(16); if (hex.length % 2 !== 0) { hex = '0' + hex; } - const bytes = hex.match(/.{2}/g); - const reversed = bytes?.reverse(); - - return reversed?.join(''); + const bytes = hex.match(/.{2}/g) || []; + return bytes.reverse().join(''); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/spark-handler/utils.ts` around lines 1 - 12, numberToReversedHex currently can return undefined and accepts invalid inputs; change its signature to return a string and add input validation: in numberToReversedHex validate that num is a finite, non-negative safe integer (e.g., Number.isFinite and Number.isSafeInteger and num >= 0) and throw a TypeError for invalid inputs; compute hex = Math.floor(num).toString(16), pad with a leading '0' when needed, call hex.match(/.{2}/g) and assert the result (bytes) is non-null before reversing, then join and return the resulting string so bytes/reversed never produce undefined (use local variables bytes and reversed from the function to locate the logic to change).packages/extension/src/ui/action/views/lock-screen/index.vue-59-59 (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unused
update:init-spark-stateemit event.The
update:init-spark-stateevent is declared but never emitted in this component and has no listeners in parent components. Remove it if it's not needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/views/lock-screen/index.vue` at line 59, Remove the unused emit declaration for 'update:init-spark-state' from the component's emits/type signature (the line declaring (e: 'update:init-spark-state', password: string): void;), since the event is never emitted or listened to; update the emits/type definition to exclude 'update:init-spark-state' and run type checks to ensure no other code relies on it (search for references to 'update:init-spark-state' to confirm safe removal).packages/extension/src/libs/utils/wasm-loader.ts-34-44 (1)
34-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove async Promise executor anti-pattern.
Using
asyncin a Promise executor is unnecessary and can hide errors. The static analysis tool correctly flags this.🔧 Proposed fix
async function loadWasm(): Promise<WasmModule> { - return new Promise(async (resolve, reject) => { - const wasmModule = await initSpark(); - - if (typeof wasmModule !== 'undefined') { - resolve(wasmModule); - } else { - reject(new Error('Failed to load WASM module.')); - } - }); + const wasmModule = await initSpark(); + + if (typeof wasmModule === 'undefined') { + throw new Error('Failed to load WASM module.'); + } + + return wasmModule; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/wasm-loader.ts` around lines 34 - 44, The Promise executor in loadWasm uses an unnecessary async function which can hide errors; remove the async Promise-executor anti-pattern by not passing an async function into new Promise. Instead, call initSpark() outside of a new Promise (or use a synchronous executor) — await initSpark() directly in loadWasm, then resolve or throw based on the returned wasmModule (refer to loadWasm and initSpark) so errors propagate normally instead of being swallowed by an async executor.packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue-86-88 (1)
86-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Spark validation for the initial value.
This watcher is lazy, so a prefilled or restored address renders as invalid until the user edits it. That makes valid Spark destinations start in the red/error state on first paint.
Suggested fix
-watch(btcAddress, async () => { - isValidSparkAddress.value = await isSparkAddress(btcAddress.value); -}); +watch( + btcAddress, + async () => { + isValidSparkAddress.value = await isSparkAddress(btcAddress.value); + }, + { immediate: true }, +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue` around lines 86 - 88, The watcher on btcAddress only validates on changes so prefilled/restored addresses remain unvalidated; update the logic to validate the initial value as well by either making the watcher immediate (watch(btcAddress, ..., { immediate: true })) or invoking isSparkAddress(btcAddress.value) once during component setup/onMounted and assigning the result to isValidSparkAddress.value; reference btcAddress, isValidSparkAddress, and isSparkAddress so you update the existing watcher or add the one-time initial validation call.packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue-90-93 (1)
90-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit the blur transition too.
toggle:showContactsis only emitted on focus. After a blur, the parent never receivesfalse, so any contacts UI opened from this field can stay visible with stale state.Suggested fix
const changeFocus = (val: FocusEvent) => { isFocus.value = val.type === 'focus'; - if (isFocus.value) emit('toggle:showContacts', isFocus.value); + emit('toggle:showContacts', isFocus.value); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue` around lines 90 - 93, The changeFocus handler only emits toggle:showContacts on focus, leaving the parent with stale true after blur; update the changeFocus function (changeFocus and isFocus.value) to emit('toggle:showContacts', isFocus.value) for both focus and blur (i.e., remove the conditional so the emit always runs after setting isFocus.value) so the parent receives false on blur and the contacts UI state stays in sync.packages/extension/src/providers/bitcoin/libs/api-firo.ts-76-91 (1)
76-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Errorobjects for rejected broadcast failures.On Line 88,
Promise.reject(response.message)rejects a string. Downstream handlers commonly readerror.message, which becomesundefinedand weakens telemetry/UI reporting.Suggested fix
.then(response => { if (response.error) { - return Promise.reject(response.message); + return Promise.reject( + new Error( + typeof response.message === 'string' + ? response.message + : 'Firo broadcast failed', + ), + ); } return response; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts` around lines 76 - 91, The broadcastTx function currently rejects with a raw string (Promise.reject(response.message)) which loses error.message; change the rejection to an Error object so callers can read error.message — in broadcastTx, where it checks if (response.error) replace Promise.reject(response.message) with rejecting a new Error constructed from response.message (fallback to JSON.stringify(response) if message is missing) so the rejected value is an Error instance.packages/extension/src/ui/action/views/asset-detail-view/index.vue-66-73 (1)
66-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard fiat recomputation against non-numeric values.
Line 70 can yield
NaNwhentoken.valuefis non-numeric/formatted, causing invalid USD output instead of a safe fallback.Suggested fix
{{ $filters.parseCurrency( - token.balanceTotal - ? Number(token.balanceTotal) * Number(token.valuef) - : token.balanceUSDf, + token.balanceTotal && + Number.isFinite(Number(token.balanceTotal)) && + Number.isFinite(Number(token.value)) + ? Number(token.balanceTotal) * Number(token.value) + : token.balanceUSDf, ) }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/views/asset-detail-view/index.vue` around lines 66 - 73, The template is computing fiat with Number(token.balanceTotal) * Number(token.valuef) which can produce NaN when token.valuef is non-numeric; update the expression (or move to a small computed helper) to coerce and validate both values (e.g., safeBalance = Number(token.balanceTotal), safeValue = Number(token.valuef)), check isFinite/safeValue and isFinite/safeBalance, compute fiat = isFinite(safeBalance) && isFinite(safeValue) ? safeBalance * safeValue : Number(token.balanceUSDf) || 0, and pass that fiat to $filters.parseCurrency to ensure parseCurrency never receives NaN (use the existing symbols token.balanceTotal, token.valuef, token.balanceUSDf, and $filters.parseCurrency).packages/extension/src/ui/action/components/accounts-header/components/header-accounts.vue-6-10 (1)
6-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that
sparkAddressis provided when network is Firo.When
network.name === NetworkNames.Firo, the code usessparkAddressfor the identicon and display address. However,sparkAddresshas a default value of empty string (Line 145), which could result in rendering an empty identicon and truncated empty address if the parent component doesn't provide it.🛡️ Proposed fix to add validation or fallback
<img :src=" network.identicon( - network.name === NetworkNames.Firo ? sparkAddress : address, + network.name === NetworkNames.Firo ? (sparkAddress || address) : address, ) " class="account__info-images__identicon" /><span>{{ $filters.replaceWithEllipsis( - network.name === NetworkNames.Firo ? sparkAddress : address, + network.name === NetworkNames.Firo ? (sparkAddress || address) : address, 6, 4, ) }}</span>Also applies to: 22-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/components/accounts-header/components/header-accounts.vue` around lines 6 - 10, When rendering identicons/addresses for Firo in header-accounts.vue, ensure sparkAddress is validated before use: in the conditional that calls network.identicon(network.name === NetworkNames.Firo ? sparkAddress : address) and the display/address truncation logic (also at the 22-28 block), check that sparkAddress is non-empty and falls back to address or a known placeholder (e.g., a default identicon seed or "—") when empty; if fallback is used, optionally emit a warning via console or a local logger. Update the conditional(s) to prefer sparkAddress only when truthy, e.g., use (network.name === NetworkNames.Firo && sparkAddress) ? sparkAddress : address, and ensure the displayed text similarly falls back to address so no empty identicon or truncated empty string is rendered.packages/extension/src/providers/bitcoin/types/bitcoin-network.ts-40-41 (1)
40-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
'0'instead of empty string for consistency.When no private balance resolver is found, the function returns an empty string. This is inconsistent with
calculateCurrentSparkBalance(Line 11) which returns'0'for empty/missing balances. An empty string could cause unexpected behavior in UI components expecting numeric strings.🛡️ Proposed fix for consistency
const resolvePrivateBalance = privateBalanceResolvers[networkName.toUpperCase()]; - if (!resolvePrivateBalance) return ''; + if (!resolvePrivateBalance) return '0';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/types/bitcoin-network.ts` around lines 40 - 41, The resolver lookup currently returns an empty string when no resolver is found; change this to return the string '0' to match calculateCurrentSparkBalance's behavior. Update the branch that checks resolvePrivateBalance (using privateBalanceResolvers and networkName.toUpperCase()) so that instead of returning '' it returns '0', ensuring callers and UI components always receive a numeric string.packages/extension/src/libs/utils/updateAndSync/calculateCurrentSparkBalance.ts-7-18 (1)
7-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling for IndexedDB operations.
The function performs an IndexedDB read without a try-catch block. If
db.readDatafails or if the reduce encounters unexpected data, the promise will reject without graceful handling.🛡️ Proposed fix to add error handling
export const calculateCurrentSparkBalance = async () => { + try { const myCoins = await db.readData<MyCoinModel[]>(DB_DATA_KEYS.myCoins); if (!myCoins?.length) { return '0'; } const unusedCoins = myCoins.filter(el => !el.isUsed); const balance = unusedCoins.reduce((a: bigint, c) => a + c.value, 0n); return new BigNumber(balance.toString()).toString(); + } catch (error) { + console.error('Failed to calculate Spark balance:', error); + return '0'; + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/updateAndSync/calculateCurrentSparkBalance.ts` around lines 7 - 18, Wrap the body of calculateCurrentSparkBalance in a try-catch: call db.readData(DB_DATA_KEYS.myCoins) inside the try, validate myCoins is an array before using filter/reduce, protect the reduce by ensuring each coin has a numeric/BigInt value (or coerce with BigInt) and use 0n as the initial accumulator; on any error log the error (console.error or the project logger) and return '0' to fail gracefully instead of letting the promise reject.packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts-54-67 (1)
54-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd bounds checking for
setId.The code checks
setId < 1but doesn't validate the upper bound. AccessingupdatedSets[setIndex]whensetIndex >= updatedSets.lengthwould create sparse array entries or undefined behavior.🛡️ Proposed fix to add upper bound check
normalizedUpdates.forEach(update => { const setId = update.setId; if (!setId || setId < 1) return; const setIndex = setId - 1; + + // Ensure array is large enough + while (updatedSets.length <= setIndex) { + updatedSets.push({ blockHash: '', setHash: '', coins: [] }); + } const existingSet = updatedSets[setIndex]; const mergedCoins = mergeCoins(existingSet?.coins ?? [], update.set.coins);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts` around lines 54 - 67, The loop over normalizedUpdates does not check the upper bound for setId, so setIndex may exceed updatedSets.length and create sparse entries; modify the forEach block that uses setId/setIndex (references: normalizedUpdates, setId, setIndex, updatedSets, mergeCoins) to validate that setIndex is within [0, updatedSets.length - 1] before reading/updating updatedSets—if setIndex is out of range, skip the update (return) or handle it explicitly (e.g., log/warn) so you never assign to updatedSets[setIndex] when it would extend the array unintentionally.packages/extension/src/ui/action/views/network-activity/index.vue-92-95 (1)
92-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
isSyncingprop.The
isSyncingprop is defined but never referenced in the template, script logic, or passed to any child components. The child componentnetwork-activity-totaluses its ownisSyncingcomputed property derived from different props (isCoinSyncingandisTagSyncing), making this parent prop dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/views/network-activity/index.vue` around lines 92 - 95, Remove the dead prop definition isSyncing from the props block in the parent component (index.vue) since it is never read or passed down; update the props object to no longer declare isSyncing and run a quick search for any references to isSyncing in this component to ensure nothing else relies on it (the child component network-activity-total computes its own isSyncing from isCoinSyncing/isTagSyncing so no changes are needed there).packages/extension/src/ui/action/composables/update-activity-state.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate import to use the public entrypoint
@enkryptcom/types.The import from
@enkryptcom/types/distis inconsistent with the codebase standard and breaks with export-map changes. Change toimport { NetworkNames } from '@enkryptcom/types';to match the rest of the codebase. Note:get-spark-address.ts,synchronize-state.vue,accounts-list-item.vue, andtransparent-send-tab.vuehave the same issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/composables/update-activity-state.ts` at line 2, Replace the non-public import of NetworkNames from '@enkryptcom/types/dist' with the public package entrypoint by changing occurrences like "import { NetworkNames } from '@enkryptcom/types/dist';" to "import { NetworkNames } from '@enkryptcom/types';" (apply the same change in the other affected files get-spark-address.ts, synchronize-state.vue, accounts-list-item.vue, and transparent-send-tab.vue) so the code uses the documented public export and avoids export-map breakage.packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue-296-303 (1)
296-303:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid global error styling for all
<p>in this view.This forces every paragraph in the send screen to use
@errorcolor, including non-error text. Scope it to a dedicated error class instead.Suggested fix
-p { - font-weight: 400; - font-size: 14px; - line-height: 20px; - letter-spacing: 0.25px; - color: `@error`; - margin: 0; -} +.send-transaction__error { + font-weight: 400; + font-size: 14px; + line-height: 20px; + letter-spacing: 0.25px; + color: `@error`; + margin: 0; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue` around lines 296 - 303, The stylesheet currently targets all p elements and forces the error color; change the selector to a dedicated class (e.g., .error-text or .send-error) instead of p, remove the global p color rule, and update the template in index.vue to apply that class only to the paragraph(s) that display error messages (replace <p> used for errors with <p class="error-text"> or similar); ensure the style is scoped to the component if needed so other views aren’t affected.packages/extension/src/libs/utils/updateAndSync/markCoinsAsUsed.ts-72-80 (1)
72-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn a numeric fallback value here.
SparkUnusedTxDetails.valueis typed asnumber, but this branch returns'0'as a string. That will leak a mixed type into downstream arithmetic/sorting even though the predicate later asserts the result isSparkUnusedTxDetails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/updateAndSync/markCoinsAsUsed.ts` around lines 72 - 80, The branch that handles missing coinValue returns value: '0' (a string), which conflicts with the expected numeric SparkUnusedTxDetails.value and can break downstream arithmetic/sorting; update the return in that branch to use a numeric fallback (value: 0) and ensure the object shape returned by the function (e.g., in markCoinsAsUsed) preserves the typed SparkUnusedTxDetails; verify coinValue, txId and index logic still match before returning the fallback so TypeScript and consumers receive a number not a string.
🧹 Nitpick comments (18)
packages/extension/src/libs/utils/number-formatter.ts (1)
316-317: ⚡ Quick winRemove blanket TypeScript suppression in percentage parsing.
//@ts-ignore`` on Line 316 hides real type errors on Line 317. Prefer a safe coercion path instead of suppressing checks.Suggested fix
- // `@ts-ignore` - const value = new BigNumber(_value.toString().replaceAll('%', '')); + const sanitized = String(_value).replace(/%/g, ''); + const value = new BigNumber(sanitized);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/number-formatter.ts` around lines 316 - 317, Remove the blanket "// `@ts-ignore`" before the percentage parsing and instead coerce and sanitize _value safely before constructing BigNumber: ensure _value is not null/undefined, convert it to a string using a safe coercion (e.g., String(_value)), strip percent signs using a regex-based replace (avoid replaceAll for compatibility), and then pass the sanitized string into new BigNumber(...); update the variable references (_value, value, BigNumber, replaceAll) accordingly and add a guard to handle empty or invalid sanitized input (e.g., fallback to "0" or throw a clear error).packages/name-resolution/tests/resolver.test.ts (2)
40-40: ⚡ Quick winUpdate assertions to Vitest style for consistency.
These assertions still use Chai-style
.to.be.eq()syntax. For consistency with the updates made inud.test.tsand to align with Vitest best practices, consider updating to.toBe().Example fix for line 40
- expect(address).to.be.eq("0x54fd548fFD6Ff54943d6863F1d485Cc2Fb495574"); + expect(address).toBe("0x54fd548fFD6Ff54943d6863F1d485Cc2Fb495574");Also applies to: 42-42, 45-45, 63-63, 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/name-resolution/tests/resolver.test.ts` at line 40, Replace Chai-style assertions in the resolver tests with Vitest-style matchers: for each occurrence of expect(...).to.be.eq(...) or expect(...).to.be.eql(...), update to expect(...).toBe(...) or expect(...).toEqual(...) as appropriate (e.g. the assertion using the variable address on line 40 and the similar assertions at lines 42, 45, 63, 67 in resolver.test.ts). Ensure you only change the matcher syntax (keep the same expected values and variables like address, addressEns, etc.) so tests use Vitest's .toBe/.toEqual consistently.
4-23: ⚡ Quick winRemove commented-out code or implement it properly.
Similar to
sid.test.ts, this commented-out mock code should either be properly implemented or removed. If uncommented, it would fail due to the missingviimport fromvitest.Additionally, these changes appear unrelated to the PR objective of adding Firo network support.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/name-resolution/tests/resolver.test.ts` around lines 4 - 23, The commented-out vi.mock blocks in resolver.test.ts (the mocks for createWeb3Name, createSolName, createPaymentIdName using vi.mock) should be either removed or properly implemented: either delete these commented lines since they are unrelated to the Firo work, or uncomment and add the proper Vitest import (e.g., import { vi } from 'vitest') and ensure the mock factories (createWeb3Name, createSolName, createPaymentIdName) return the mocked methods (getDomainName, getAddress) as shown; keep only the approach you choose to avoid unrelated test noise.packages/name-resolution/tests/sid.test.ts (2)
36-36: ⚡ Quick winUpdate assertions to Vitest style for consistency.
The assertions in this file still use Chai-style
.to.be.eq()syntax, whileud.test.tswas updated to use Vitest's.toBe()syntax. For consistency across the test suite, consider updating these assertions.Example fix for line 36
- expect(address).to.be.eq("0xb5932a6B7d50A966AEC6C74C97385412Fb497540"); + expect(address).toBe("0xb5932a6B7d50A966AEC6C74C97385412Fb497540");Also applies to: 50-50, 64-64, 69-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/name-resolution/tests/sid.test.ts` at line 36, Tests in sid.test.ts use Chai-style assertions (e.g., expect(address).to.be.eq(...)); replace them with Vitest-style matchers by changing calls like expect(address).to.be.eq("...") to expect(address).toBe("...") (and similarly update the other occurrences noted around the other expect calls at the same file). Locate the expect(...) usages for the variable/address assertions and update the matcher to toBe(...), ensuring you keep the same expected string values and do not reintroduce Chai chain syntax.
4-23: ⚡ Quick winRemove commented-out code or implement it properly.
Commented-out code is generally considered a code smell and should be removed. Additionally, if these mocks were uncommented, they would fail because
viis not imported fromvitest.If mocking is needed to make tests pass, either:
- Properly implement the mocks with the necessary imports, or
- Remove the commented code entirely
Also note that these changes appear unrelated to the PR objective of adding Firo network support.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/name-resolution/tests/sid.test.ts` around lines 4 - 23, Remove the large commented-out mock block in the tests (the createWeb3Name / createSolName / createPaymentIdName mocks) or implement them correctly: if you need the mocks, import vi from vitest at the top (import { vi } from 'vitest') and uncomment and adapt the mock factories so they export the mocked functions used by the tests; otherwise simply delete the commented code to avoid unrelated noise in sid.test.ts. Ensure any retained mocks reference the exact factory names createWeb3Name, createSolName, and createPaymentIdName so the test harness picks them up.packages/extension/tsconfig.app.json (1)
13-14: ⚡ Quick winDocument why
skipLibCheckandallowJsare required.These options are necessary for the current setup:
allowJs: trueis required to importspark.jsWASM modules (Emscripten-generated files inlibs/utils/wasmModule/andwasmWorkerModule/)skipLibCheck: trueis needed becausebitcoinjs-lib-firois a custom GitHub fork without TypeScript type definitionsConsider adding a comment in
tsconfig.app.jsonbriefly explaining these requirements so future maintainers understand why they cannot be safely removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/tsconfig.app.json` around lines 13 - 14, Add inline comments in tsconfig.app.json next to the "skipLibCheck" and "allowJs" compiler options explaining why they are required: note that "allowJs": true is needed to import Emscripten-generated spark.js WASM modules from libs/utils/wasmModule and wasmWorkerModule, and "skipLibCheck": true is required because we depend on a custom fork (bitcoinjs-lib-firo) that lacks TypeScript definitions; update the comment to warn maintainers not to remove these flags without adding proper typings or replacing the WASM build artifacts.packages/extension/src/libs/spark-handler/serializeMintContext.ts (1)
14-49: ⚡ Quick winConsider adding input validation.
The function doesn't validate that:
txHashLengthdoesn't exceed the actualtxHashbuffer length (Line 20)voutis within valid uint32 rangeinputsarray is not emptyInvalid inputs could produce malformed serialization that may cause issues in the WASM layer.
🛡️ Proposed validation checks
export const serializeMintContext = ( inputs: DartInputData[], ): SerializedMintContextResult => { + if (inputs.length === 0) { + throw new Error('Inputs array cannot be empty'); + } + const serialContextStream: Buffer[] = []; for (let i = 0; i < inputs.length; i++) { + if (inputs[i].txHashLength > inputs[i].txHash.length) { + throw new Error(`Invalid txHashLength at input ${i}`); + } + if (inputs[i].vout < 0 || inputs[i].vout > 0xffffffff) { + throw new Error(`Invalid vout value at input ${i}`); + } + const txHashBuffer = inputs[i].txHash.slice(0, inputs[i].txHashLength);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/spark-handler/serializeMintContext.ts` around lines 14 - 49, In serializeMintContext validate inputs before serializing: ensure inputs.length > 0 and throw/return a clear error if empty; for each input verify inputs[i].txHashLength is <= inputs[i].txHash.length and that inputs[i].txHash is a Buffer (or convert/throw if not); check inputs[i].vout is a safe unsigned 32-bit integer (0 <= vout <= 0xFFFFFFFF) and throw/return on invalid values; perform these checks at the top of the loop (referencing txHashLength, txHash, vout, and serializeMintContext) so malformed data is rejected before building serialContextStream.packages/extension/src/ui/action/utils/set-utils.ts (1)
11-19: 💤 Low valueConsider clarifying the function name or parameter order.
The
differenceSetsfunction computesupdate \ set(elements inupdatebut not inset), which is the opposite of the standard mathematical set difference operationset \ update. This could be confusing for maintainers.Consider either:
- Renaming to make the direction explicit (e.g.,
elementsNotInOriginal,newElementsIn)- Swapping parameters to match standard set difference semantics
- Adding a JSDoc comment explaining the behavior
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/utils/set-utils.ts` around lines 11 - 19, The function differenceSets(set, update) currently returns elements in update that are not in set (i.e., update \ set) which is counter‑intuitive; rename the function (e.g., newElementsIn or elementsNotInOriginal) and add a short JSDoc to clarify the behavior, then update all references to differenceSets to the new name; alternatively, if you prefer standard set difference semantics, swap the parameter order to (update, set) and update callers accordingly—refer to the function symbol differenceSets and its parameters set and update when making the change.packages/extension/package.json (1)
66-66: ⚡ Quick winDocument the purpose of the forked dependency.
The
bitcoinjs-lib-firofork is used to support Firo network functionality (Spark balance, anonymization features). The dependency is pinned to a specific verified commit (4b0c4322cb9f308cb84b2f655d16a9e9abc7b4e1), which ensures reproducible builds. Consider adding an inline comment explaining that this fork is required for Firo-specific features and documenting any blockers to upstreaming these changes to the official bitcoinjs-lib.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/package.json` at line 66, Add an inline comment and brief documentation explaining why the forked dependency "bitcoinjs-lib-firo" is used: state it provides Firo-specific functionality (Spark balance and anonymization features), note that it's pinned to commit 4b0c4322cb9f308cb84b2f655d16a9e9abc7b4e1 for reproducible builds, and summarize any known blockers to upstreaming these changes to the official bitcoinjs-lib so future maintainers know the rationale and upgrade path; update the package.json near the "bitcoinjs-lib-firo" entry and add a short note in the repo README or deps.md referencing the same commit and blockers.packages/extension/src/ui/action/composables/async-computed.ts (2)
21-21: ⚡ Quick winType error more precisely.
Casting to
as unknown as anyloses all type information. Use a more specific error type.♻️ Proposed fix
} catch (err) { - if (!cancelled) error.value = err as unknown as any; + if (!cancelled) error.value = err instanceof Error ? err : new Error(String(err)); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/composables/async-computed.ts` at line 21, The assignment "if (!cancelled) error.value = err as unknown as any" loses type information; change it to assert a specific error shape (for example cast to Error or a union like Error | string) or narrow at runtime (e.g., if (err instanceof Error) error.value = err else error.value = new Error(String(err))) so error.value keeps a meaningful type; update the code around variables "error", "cancelled", and "err" in async-computed.ts accordingly to preserve proper typing instead of using "as unknown as any".
3-3: ⚡ Quick winImprove type safety with generic types.
The function signature uses
anyfor both the async function return and initial value, which loses type information and may lead to runtime errors.♻️ Proposed fix with generic typing
-export default (asyncFn: () => any, initialValue: any) => { +export default <T>(asyncFn: () => Promise<T>, initialValue: T) => { - const value = ref(initialValue); + const value = ref<T>(initialValue); const isPending = ref(false); - const error = ref(null); + const error = ref<Error | null>(null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/ui/action/composables/async-computed.ts` at line 3, The exported default function currently types both parameters as any; change it to use a generic type parameter (e.g., <T>) so the signature becomes generic: the asyncFn parameter should produce T (or Promise<T>) and initialValue should be T, and update any internal types/returns to use T so callers retain strong typing; locate the anonymous exported function declaration starting with "export default (asyncFn: () => any, initialValue: any) =>" and replace the any types with a generic type parameter throughout.packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts (1)
89-96: ⚡ Quick winUse optional chaining for safer access.
Lines 90 and 94 use non-null assertions when mapping inputs/outputs. If the API returns unexpected data (e.g., missing
addroraddresses), this will throw during the map operation.♻️ Proposed fix
inputs: tx.vin.map(input => ({ - address: input.addr, + address: input.addr || '', value: Number(input.value), })), outputs: tx.vout.map(output => ({ - address: output.scriptPubKey.addresses![0], + address: output.scriptPubKey.addresses?.[0] || '', value: Number(output.value), pkscript: output.scriptPubKey.hex, })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts` around lines 89 - 96, The mapping over tx.vin and tx.vout uses non-null assertions and can throw if fields are missing; update the tx.vin.map and tx.vout.map handlers (the inputs and outputs objects) to use optional chaining and safe fallbacks (e.g., input?.addr ?? null, Number(input?.value ?? 0), output?.scriptPubKey?.addresses?.[0] ?? null, Number(output?.value ?? 0), and output?.scriptPubKey?.hex ?? null) so missing properties don’t cause exceptions during mapping; ensure the created inputs/outputs keep the same property names (address, value, pkscript) but tolerate undefined/null values instead of using the ! operator.packages/extension/src/libs/utils/wasm-loader.ts (1)
46-68: ⚖️ Poor tradeoffConsider exposing WASM initialization errors.
The constructor (lines 50-56) starts loading the WASM module but only logs errors to the console. Callers using
getInstance()won't know if initialization failed until they try to use the module. This can make debugging difficult.Consider either:
- Exposing an
initializationErrorproperty that callers can check- Having
getInstance()reject with the initialization error instead of retrying- Documenting that callers must handle cases where the WASM module fails to load
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/wasm-loader.ts` around lines 46 - 68, Add an initializationError property to the WasmInstance class, set it in the constructor catch block when loadWasm() fails (instead of only console.error), and update getInstance() to first check if initializationError is set and reject with that error (rather than silently retrying); reference the WasmInstance class, its instance field, the constructor's loadWasm().then/.catch, and the getInstance() method when making these changes so callers can detect initialization failures reliably.packages/extension/src/providers/bitcoin/libs/utils.ts (1)
10-29: ⚖️ Poor tradeoffClarify error handling semantics.
When the WASM module fails to load (lines 16-18), the function returns
falseand logs to console. This conflates "WASM unavailable" with "invalid Spark address," which may lead to silent failures if callers assumefalsealways means "not a Spark address."Consider either:
- Throwing an error when WASM is unavailable (forcing callers to handle initialization failures)
- Returning a more explicit result like
{ valid: boolean, error?: string }- At minimum, document that
falsecan mean either "invalid address" or "validation unavailable"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/providers/bitcoin/libs/utils.ts` around lines 10 - 29, The current isSparkAddress function conflates "WASM unavailable" with "invalid address"; change its behavior to throw a clear initialization error when wasmInstance.getInstance() returns falsy instead of returning false — in isSparkAddress replace the console.log+false path with throwing an Error (e.g., "WASM not loaded: spark address validation unavailable") so callers of isSparkAddress (and any code invoking js_isValidSparkAddress) must handle initialization failures explicitly; update call sites to catch this error or, if you prefer a non-throwing API, implement an alternative return shape ({ valid: boolean, error?: string }) consistently across usages.packages/extension/src/libs/spark-handler/createTempTx.ts (1)
28-28: ⚡ Quick winType addressKeyPairs more precisely.
Record<string, any>loses type information. Consider defining an interface for the keypair structure.♻️ Proposed improvement
+ addressKeyPairs: Record<string, ECPairInterface>; - addressKeyPairs: Record<string, any>;This provides better type safety since you're already importing
ECPairInterfaceand using it forspendableUtxos[].keyPair.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/spark-handler/createTempTx.ts` at line 28, The type declaration addressKeyPairs: Record<string, any> is too loose — define a precise type/interface (e.g., AddressKeyPair or AddressKeyPairsMap) that models the stored keypair structure and use ECPairInterface for the keyPair property so TypeScript can validate usages like spendableUtxos[].keyPair; update the addressKeyPairs declaration to use this new interface and adjust any related references (e.g., where addressKeyPairs[...] is accessed) to match the stricter shape.packages/extension/src/libs/spark-handler/createTempFromSparkTx.ts (1)
36-40: 💤 Low valueConsider BigNumber overflow when converting to satoshis.
Line 38 uses
.toNumber()after multiplying by 10^8. JavaScript'sNumbertype has a safe integer range of ±2^53, which corresponds to approximately 90 million BTC. While this is unlikely to be exceeded in practice, ifamountis extremely large or malformed,.toNumber()could produceInfinityor lose precision, leading to incorrect transaction outputs.Consider adding a bounds check or documenting the expected range.
♻️ Optional bounds check
if (!isSparkTransaction) { + const satoshis = new BigNumber(amount).multipliedBy(10 ** 8); + if (!satoshis.isFinite() || satoshis.gt(Number.MAX_SAFE_INTEGER)) { + throw new Error('Amount too large for transaction output'); + } tempTx.addOutput( bitcoin.address.toOutputScript(to, network), - new BigNumber(amount).multipliedBy(10 ** 8).toNumber(), + satoshis.toNumber(), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/spark-handler/createTempFromSparkTx.ts` around lines 36 - 40, The code converts amount to satoshis with new BigNumber(amount).multipliedBy(10 ** 8).toNumber() which can overflow or lose precision; before calling tempTx.addOutput (and symbols bitcoin.address.toOutputScript, tempTx.addOutput, BigNumber, amount), validate and sanitize amount: compute satoshis = new BigNumber(amount).multipliedBy(1e8).integerValue(BigNumber.ROUND_FLOOR), check satoshis.isFinite() and sats.lte(Number.MAX_SAFE_INTEGER) and sats.gte(0) (or throw a descriptive Error if out of bounds/malformed), then pass sats.toNumber(); alternatively document allowed range in the function comment if you prefer not to throw. Ensure you fail fast with a clear error message when amount is invalid to avoid pushing Infinity/incorrect values into tempTx.addOutput.packages/extension/src/libs/utils/wasm-worker-loader.ts (1)
46-67: ⚡ Quick winDeduplicate in-flight WASM initialization.
Line 50 preload and Line 63 fallback can race and initialize twice; cache a shared loading promise.
Refactor sketch
class WasmWorkerInstance { instance: WasmModule | null = null; + private loadingPromise: Promise<WasmModule> | null = null; constructor() { - loadWasm() + this.loadingPromise = loadWasm() .then(module => { this.instance = module; + return module; }) .catch(error => { console.error('Error loading WASM module:', error); + this.loadingPromise = null; + throw error; }); } public async getInstance(): Promise<WasmModule> { - if (this.instance) { - return Promise.resolve(this.instance); - } else { - const wasm = await loadWasm(); - this.instance = wasm; - return Promise.resolve(wasm); - } + if (this.instance) return this.instance; + if (!this.loadingPromise) { + this.loadingPromise = loadWasm().then(module => { + this.instance = module; + return module; + }); + } + return this.loadingPromise; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/wasm-worker-loader.ts` around lines 46 - 67, The constructor and getInstance of WasmWorkerInstance can race and call loadWasm twice; add a shared loading promise (e.g., a private property loadingPromise: Promise<WasmModule> | null) and assign it when starting loadWasm in the constructor, await that same promise in getInstance instead of calling loadWasm again, and on resolution set this.instance and clear loadingPromise on error set it to null while propagating/logging the error; update references in constructor and getInstance to use loadingPromise and avoid duplicate initialization.packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts (1)
27-42: ⚖️ Poor tradeoffConsider more robust coin deduplication strategy.
Using
JSON.stringifyfor deduplication (Lines 31, 35) is fragile because:
- JavaScript object property ordering is not guaranteed in all cases
- Memory-intensive for large coin arrays (creates string copies)
- Two semantically identical coins with different key orders would not be detected as duplicates
Alternative approach using stable coin identifiers
If coins have a unique identifier (e.g.,
txid+vout), use that directly:-const mergeCoins = (existing: string[][], incoming: string[][]): string[][] => { +const getCoinKey = (coin: string[]): string => { + // Adjust based on actual coin structure, e.g., coin[0] + coin[1] + return `${coin[0]}-${coin[1]}`; +}; + +const mergeCoins = (existing: string[][], incoming: string[][]): string[][] => { if (!existing?.length) return [...incoming]; if (!incoming?.length) return [...existing]; - const seen = new Set(existing.map(coin => JSON.stringify(coin))); + const seen = new Set(existing.map(getCoinKey)); const updates: string[][] = []; incoming.forEach(coin => { - const key = JSON.stringify(coin); + const key = getCoinKey(coin); if (seen.has(key)) return; seen.add(key); updates.push(coin); }); return [...updates, ...existing]; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts` around lines 27 - 42, The mergeCoins function currently deduplicates by JSON.stringify which is brittle and memory-heavy; change mergeCoins to compute a stable, lightweight key for each coin (e.g., if coin elements represent txid and vout use `${txid}:${vout}` or build a deterministic key by ordering/normalizing coin fields and joining with a fixed separator) and use a Set or Map of those keys for deduplication instead of JSON.stringify; update references in mergeCoins to generate the key (e.g., getCoinKey(coin)) when seeding seen and when checking incoming coins so semantically identical coins are recognized without expensive stringifying of whole arrays.
Comment on lines
+262
to
+267
| privateRecipientsVector = Module.ccall( | ||
| 'js_createPrivateRecipientsVectorForCreateSparkSpendTransaction', | ||
| 'number', | ||
| ['number'], | ||
| [1], // intended final size | ||
| ); |
There was a problem hiding this comment.
Spark-to-Spark sends are missing a change recipient.
For private sends, the vector is always sized to 1 and Line 311 uses requestedAmount as the only private output. When coin selection overshoots the target—which the greedy selector will often do—there is no second private recipient returning change to the sender, so the extra value has nowhere explicit to go.
Also applies to: 304-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/extension/src/libs/spark-handler/index.ts` around lines 262 - 267,
The private send code always creates privateRecipientsVector with size 1 (using
js_createPrivateRecipientsVectorForCreateSparkSpendTransaction) and then only
pushes the requestedAmount as the sole private output (around the logic that
references requestedAmount), which drops any change when coin selection
overshoots; update the logic to size the privateRecipientsVector to accommodate
a potential change recipient (e.g., size 2 or size = 1 + (changePossible ? 1 :
0)), and when the coin selector produces change, append a second private
recipient representing the sender's change (address = sender, amount = change)
before calling the native create function; ensure you only add the change
recipient when change > 0 and adjust any loops or indices that assume a single
recipient (symbols: privateRecipientsVector,
js_createPrivateRecipientsVectorForCreateSparkSpendTransaction, and the code
that inserts requestedAmount at index 0).
| if (!supportedNetworks.includes(networkName as NetworkNames)) | ||
| return Promise.resolve(utxos); | ||
| return getAllOrdinals(address, networkName, []).then(ordinals => { | ||
| console.log({ ordinals }); |
There was a problem hiding this comment.
Remove debug console.log from production code.
This debug statement will expose ordinal data to the browser console in production builds, potentially leaking user privacy information.
🐛 Proposed fix
return getAllOrdinals(address, networkName, []).then(ordinals => {
- console.log({ ordinals });
-
return utxos.filter(utxo => {📝 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.
Suggested change
| console.log({ ordinals }); | |
| return getAllOrdinals(address, networkName, []).then(ordinals => { | |
| return utxos.filter(utxo => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/extension/src/providers/bitcoin/libs/filter-ordinals.ts` at line 49,
Remove the debug console.log that prints ordinals to the browser console: locate
the console.log({ ordinals }) in filter-ordinals.ts (the place where the
ordinals variable is handled) and delete it or replace it with a secure logging
call (e.g., a debug-only logger gated by NODE_ENV or a telemetry-safe logger
that redacts sensitive ordinal data) so no raw ordinal data is emitted in
production.
Comment on lines
+34
to
+51
| private initDB(): void { | ||
| const request = indexedDB.open(this.dbName, this.dbVersion); | ||
|
|
||
| request.onupgradeneeded = () => { | ||
| const db = request.result; | ||
| if (!db.objectStoreNames.contains(this.storeName)) { | ||
| db.createObjectStore(this.storeName); | ||
| } | ||
| }; | ||
|
|
||
| request.onsuccess = () => { | ||
| this.db = request.result; | ||
| }; | ||
|
|
||
| request.onerror = () => { | ||
| console.error('Error opening IndexedDB', request.error); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file in question
find . -type f -name "indexedDB.ts" | head -20Repository: enkryptcom/enKrypt
Length of output: 113
🏁 Script executed:
# Check the repository structure to understand the context
git ls-files | grep -E "(indexedDB|db)" | head -20Repository: enkryptcom/enKrypt
Length of output: 254
🏁 Script executed:
# Read the indexedDB.ts file to examine the code
cat -n packages/extension/src/ui/action/db/indexedDB.tsRepository: enkryptcom/enKrypt
Length of output: 5662
🏁 Script executed:
# Check for test files or CI configuration that might reference this error
find . -type f \( -name "*.test.*" -o -name "*.spec.*" -o -name ".github" -o -name "ci.yml" -o -name "github-ci*" \) | grep -E "(test|spec|ci|github)" | head -20Repository: enkryptcom/enKrypt
Length of output: 1135
🏁 Script executed:
# Search for any usage of IndexedDBHelper to understand impact
rg "IndexedDBHelper" --type ts --type js -lRepository: enkryptcom/enKrypt
Length of output: 828
🏁 Script executed:
# Check how IndexedDBHelper is instantiated/used in one of the main files
head -100 packages/extension/src/ui/action/db/indexedDB.tsRepository: enkryptcom/enKrypt
Length of output: 2800
🏁 Script executed:
# Look at sparkCoinInfoWorker to see if it runs in a browser context
head -50 packages/extension/src/ui/action/workers/sparkCoinInfoWorker.tsRepository: enkryptcom/enKrypt
Length of output: 1287
Guard IndexedDB initialization for non-browser environments.
Line 35 assumes indexedDB exists. This breaks immediately when the module loads in non-browser contexts (Node.js tests, CI), since the constructor calls initDB() synchronously and indexedDB is undefined outside the browser.
Proposed fix
private initDB(): void {
+ if (typeof indexedDB === 'undefined') {
+ console.warn('IndexedDB is unavailable in this environment');
+ return;
+ }
const request = indexedDB.open(this.dbName, this.dbVersion);🧰 Tools
🪛 GitHub Actions: Test all workspaces / 0_test.txt
[error] 35-35: Test run failed with ReferenceError: indexedDB is not defined (indexedDB.open(...)).
🪛 GitHub Actions: Test all workspaces / test
[error] 35-35: ReferenceError: indexedDB is not defined at IndexedDBHelper.initDB (indexedDB.open(...)).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/extension/src/ui/action/db/indexedDB.ts` around lines 34 - 51, The
initDB method assumes indexedDB exists and is called synchronously from the
constructor, which fails in non-browser environments; modify initDB (and the
code that invokes it) to first check for the presence of globalThis.indexedDB
(e.g., if (typeof globalThis.indexedDB === 'undefined') return;) and bail out
early if missing, so no indexedDB.open is invoked; update any constructor path
that calls initDB to still work when initDB returns early (ensure this.db
remains null/undefined and consumers handle that case).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores