Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds full Firo (and FiroTest) network support with Spark privacy integration: new network configs, Firo API and Electrum client, WASM-based Spark tooling, wallet/keyring hooks, background sync (worker + IndexedDB), Spark-aware send/verify UI and signing flows, and related types/props across the extension. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Send UI<br/>(Spark Tab)
participant Verify as Verify<br/>Component
participant Signer as Firo Transaction<br/>Signer
participant Wallet as BaseFiroWallet
participant API as Firo API / Electrum
User->>UI: Enter Spark address & amount, confirm
UI->>Verify: Navigate with tx payload
Verify->>Wallet: Request keypairs / secret (getSecret / address mapping)
Verify->>Signer: Build & sign PSBT
Signer->>Wallet: Obtain per-input keypairs
Signer->>Signer: Finalize tx
Verify->>API: broadcastTx(signedTx)
API->>Electrum: Submit / relay
Electrum-->>API: txid
API-->>Verify: { txid }
Verify->>UI: Success, update activity
sequenceDiagram
participant App as Vue App
participant Sync as useSynchronizeSparkState
participant Worker as SparkCoinInfoWorker
participant WASM as Spark WASM
participant DB as IndexedDBHelper
participant Electrum as FiroElectrum
App->>Sync: Network becomes Firo
Sync->>Worker: Spawn worker to analyze coins
Worker->>WASM: Load module & derive view keys
Worker->>DB: Read anonymity sets
Worker->>WASM: getSparkCoinInfo per coin
WASM-->>Worker: coin metadata & value
Worker->>DB: Save deduplicated coins
Worker-->>Sync: Post processed coin list
Sync->>Electrum: Fetch tx details for unused coins
Electrum-->>Sync: Transactions
Sync->>DB: Persist used flags & balance
Sync->>App: Update sparkBalance & activities
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
narekpetrosyan
force-pushed
the
feature/add-firo-network
branch
from
2a1b0bc to
a79031b
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: 8
🧹 Nitpick comments (36)
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (3)
11-31: Use a consistent fallback strategy when Spark account is unavailable.This newly introduced tab-based interface is conditionally rendered for Firo and Spark. The logic is straightforward, but consider providing a clear fallback mechanism when Spark is unavailable or the user rapidly toggles between tabs. This can help avoid edge cases and potential confusion for users when they do not have a Spark account set up.
78-78: Typed approach for selectedSendTab is clean.Defining "transparent" | "spark" in a ref is succinct and ensures type safety. Consider documenting the default "transparent" state if you foresee adding more tabs down the line.
115-153: Styling is cohesive and ensures good UX.The added CSS for .send-transaction__tabs and .send-transaction__tabs-active is consistent with the project’s style. If the design evolves to include additional states, consider using BEM or a theming approach for easier maintainability.
packages/extension/src/providers/bitcoin/libs/api-firo.ts (3)
27-29: Address derivation looks correct.getBitcoinAddress is used correctly. Consider optional chaining if there's any chance pubkey conversions might fail, but it seems safe here.
31-32: init stub left empty.You might consider adding logs or clarifying comments if future initialization steps are planned.
74-87: Balance computation is straightforward.Adding confirmed and unconfirmed satoshi values is correct. Catching network fetch errors by returning "0" is safe, though consider logging partial errors for debugging.
packages/extension/src/providers/bitcoin/networks/index.ts (1)
11-12: Export object consistency.Listing firoTest and firo among existing networks is logical. Double-check naming for uniformity, as you import firoTestnet but export it as firoTest.
packages/extension/src/libs/spark-handler/callRPC.ts (1)
3-3: Avoid hardcoding RPC endpoint.
Storing the RPC URL within the source code can be problematic for future environment changes or sensitive endpoint usage. Consider externalizing this URL (e.g., environment variable or config file) for better flexibility and maintainability.packages/extension/src/libs/spark-handler/index.ts (2)
22-28: Add targeted error handling.
The function calls callRPC and directly returns its promise. If the RPC fails or returns an invalid result, the caller won't perform specific handling. Consider adding logs or structured error responses for easier debugging.
30-43: Consider centralizing transaction logic.
Both sendToSparkAddress and sendFromSparkAddress share a similar pattern of calling RPC methods with a transaction object. If additional transaction flows arise or more robust orchestration is needed, centralizing shared logic or error handling in one place may improve maintainability.packages/extension/src/providers/bitcoin/networks/firo.ts (1)
3-6: Confirm naming consistency.
The file references “BitcoinNetwork” and “BitcoinNetworkOptions” even though they represent a Firo configuration. This is likely a shared interface for all Bitcoin-like networks, but ensure the naming remains clear to future maintainers.packages/extension/src/providers/bitcoin/networks/firo-testnet.ts (1)
3-6: Explicitly differentiate from mainnet.
While it is clear from the variable names and URLs that this is a testnet configuration, consider documenting any differences in chain parameters compared to mainnet for clarity (e.g., purpose, usage disclaimers, etc.).packages/extension/src/ui/action/components/accounts-header/index.vue (1)
31-31: Confirm event handling in deposit component.
This second emission of "action:generate-new-spark" is consistent, but ensure that deposit properly handles all scenarios if the spark address is regenerated multiple times.packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts (2)
20-23: Avoid assignment within an expression.The assignment to "tokenPrice" within your .then() chain may trigger linter or readability warnings (as highlighted by static analysis). It's clearer to separate this assignment from the return expression:
Proposed snippet:
- .then((mdata) => (tokenPrice = mdata || "0")) + .then((mdata) => { + tokenPrice = mdata || "0"; + })🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
15-36: Consider using async/await syntax for readability.Currently, fetch and subsequent transformations rely on nested .then() calls. Although functionally correct, switching to async/await often achieves better readability and easier error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (1)
44-47: Use the async Clipboard API for paste operations.The use of "document.execCommand('paste')" is deprecated. For improved reliability and forward-compatibility, consider leveraging the modern asynchronous clipboard API (navigator.clipboard.readText()).
- document.execCommand("paste"); + if (navigator.clipboard?.readText) { + navigator.clipboard.readText().then(text => { + addressInput.value!.value = text; + address.value = text; + }); + }packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (1)
34-45: Consolidate dust threshold checks and negative balance checks.Multiple conditions check either "nativeBalanceAfterTransaction.isNeg()" or that "sendAmount < dust". Consider unifying or wrapping these validations into one helper function for better readability and maintainability.
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (2)
153-156: Validate address & amount before sending.Currently, the code calls sendFromSparkAddress unconditionally. Consider adding stronger pre-checks for valid addresses, amounts, and potential out-of-range values to prevent accidental failures or partial transactions.
189-205: Improve user-facing error messages.When an error occurs, only raw JSON is displayed. Provide a more user-friendly message or parse the API error details so users can better understand and correct the issue, rather than just seeing raw JSON strings.
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue (1)
189-207: Surface clearer error messages to users.Similar to the "verify-send-from-spark-transaction" file, the catch block logs an error but sets errorMsg to JSON. Provide human-readable explanations or parse error details for improved clarity.
packages/extension/src/ui/action/views/deposit/index.vue (8)
16-28: Introduce tab-based UI for transparent vs. Spark address.
Adding the tab buttons for "Transparent address" and "Spark address" provides a clearer user experience. This is a good approach, but ensure that any transitions or toggles between these tabs are tested for edge cases (e.g., switching tabs rapidly while data is loading).
72-116: Add Spark address functionality.
The new Spark address logic cleanly parallels the existing transparent address section, including QR codes and copy functionality. Recommend verifying that the “Generate new address” flow is disabled while a new address is being fetched to prevent duplicate requests.
Line range hint
123-133: Check for potential unused imports.
While importing “SparkAccount” and other types is crucial, confirm whether all imports are actually in use. If not, remove them to keep the code clean.
185-187: toggleGeneratedNotification.
Simple toggling approach is fine. Consider a short timeout or some user-friendly acknowledgement if the action is repeated quickly.
238-243: deposit__generate style block.
Centering the generate button is good for UX. Double-check styling across various screen resolutions to ensure the layout remains consistent.
275-279: deposit__tabs container styling.
Proper spacing (gap: 10px) is beneficial for readability. Consider media queries if the tabs might overflow on smaller containers.
281-312: Hover state for deposit__tabs-active.
Ensuring an obvious hover state improves UX. No clear issues here, but be mindful of any conflicts with focus styles for accessibility.
446-448: Remove forced max-width override if no longer needed.
Be cautious with "!important" usage. If another part of the code depends on limiting max-width, consider a more localized approach.packages/extension/src/ui/action/App.vue (2)
283-286: updateGradient for app menu.
The gradient update approach is effective. If theming changes become more robust in the future, consider a centralized theme manager rather than direct style mutations.
297-301: getSparkAccountState method.
Fetching the Spark account data on demand is solid. Confirm that any additional networks requiring similar logic do not require duplicative code.packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (5)
25-56: Conditional rendering for Spark vs. standard address.
This is a neat approach. If the user changes the network from Firo to another halfway through, ensure the UI updates correctly and discards the Firo-specific components to avoid any leftover data.
64-71: NFT selection logic.
Hiding NFT components if sending Spark is appropriate. Check if a user might want to handle an NFT while also having a Spark address in the same transaction flow. Currently you separate them, which likely simplifies logic.
82-122: Input amount, fee select, and final alert checks.
Combining these fields is good for user clarity. Ensure minimal rendering overhead by controlling watchers properly, since many reactive computations (balance checks, dust checks) run simultaneously.
143-575: Core script logic for transaction preparation.
Overall, the composition of advanced transaction flows (UTXOs, NFTs, Spark addresses) is well-organized. Watch for potential performance hits as the codebase grows—some of these computations (like scanning all UTXOs on every field update) may need caching or memoization.
577-662: Scoped styles for the send tab.
The layered class naming is good. Validate that any new color variables (e.g., “@error”) match the global theme to maintain consistent brand identity.README.md (1)
111-111: Consider adding Firo network detailsWhile adding Firo to the list is correct, consider providing more context about its unique features (e.g., privacy features, Spark addresses) to help users understand its capabilities.
Consider expanding the entry like this:
- - Firo + - Firo (Privacy-focused blockchain with Spark private payment support)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/extension/src/providers/bitcoin/networks/icons/firo.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)packages/extension/src/libs/keyring/public-keyring.ts(1 hunks)packages/extension/src/libs/spark-handler/callRPC.ts(1 hunks)packages/extension/src/libs/spark-handler/index.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/activity-handlers/index.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/api-firo.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/utils.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo-testnet.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/index.ts(1 hunks)packages/extension/src/providers/bitcoin/types/bitcoin-network.ts(2 hunks)packages/extension/src/providers/bitcoin/types/index.ts(3 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue(4 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue(1 hunks)packages/extension/src/ui/action/App.vue(7 hunks)packages/extension/src/ui/action/components/accounts-header/index.vue(3 hunks)packages/extension/src/ui/action/router/index.ts(2 hunks)packages/extension/src/ui/action/types/account.ts(1 hunks)packages/extension/src/ui/action/views/deposit/index.vue(8 hunks)packages/extension/src/ui/action/views/network-activity/index.vue(1 hunks)packages/extension/src/ui/action/views/network-assets/index.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue(1 hunks)packages/hw-wallets/src/ledger/bitcoin/configs.ts(1 hunks)packages/types/src/networks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- packages/hw-wallets/src/ledger/bitcoin/configs.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts
[error] 23-23: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/extension/src/providers/bitcoin/libs/api-firo.ts
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (56)
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (4)
40-45: Spark address tab usage looks solid.
The spark-send-tab usage with fallback to null for sparkAccount appears robust. Confirm that error-handling logic for Spark account operations is in place, particularly if sparkAccount fails to load or is in an invalid state.
52-61: Imports align well with project structure.
TransparentSendTab and SparkSendTab imports are straightforward and follow naming conventions. No issues found here.
87-88: Tab-switching method is clear and concise.
setSelectedSendTab cleanly updates the reactive ref. No concurrency or side-effect concerns.
33-37: Ensure consistent validation for transparent addresses.
The transparent-send-tab is correctly gated by selectedSendTab. However, ensure that any address validation logic for transparent addresses is consistently applied (e.g., input checks, format checks).
✅ Verification successful
Let me search for address validation logic in the codebase to verify the consistency of transparent address validation.
Let me search for the address validation components and their implementation to verify the consistency.
Based on the analysis of the code, I can now provide a final response regarding the address validation concern:
Address validation is consistently implemented for transparent addresses
The codebase shows a robust and consistent implementation of address validation for transparent addresses through:
- The
send-address-inputcomponent which consistently validates addresses and provides visual feedback (red color for invalid addresses) - The
isAddressutility function inbitcoin/libs/utils.tsthat properly validates addresses against the network info - The transparent-send-tab component correctly uses these validation mechanisms before allowing transactions
The validation is properly integrated into the UI flow with:
- Visual feedback for invalid addresses
- Consistent validation before enabling the send button
- Network-specific address format validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for address validation related code in transparent send components
ast-grep --pattern 'function $FUNC($$$) {
$$$
address $$$
$$$
}'
# Search for specific validation patterns
rg -i "address.*validation|validate.*address" -A 3
# Look for the transparent-send-tab component implementation
fd "transparent-send-tab" --exec cat {}
Length of output: 23978
Script:
#!/bin/bash
# Look for the send-address-input component implementation
fd "send-address-input.vue" --exec cat {}
# Search for address validation functions
ast-grep --pattern 'function isAddress($$$) {
$$$
}'
# Look for any other address validation related code
rg "isAddress|validateAddress" -A 2
Length of output: 49875
packages/extension/src/providers/bitcoin/libs/api-firo.ts (7)
14-21: Constructor logic is straightforward.
The node URL and networkInfo assignments are standard and easy to maintain.
23-25: API accessor is well-defined.
Returning this from the getter is a tidy way to align with ProviderAPIInterface.
43-72: Comprehensive transaction status retrieval.
Mapping FiroTxType into BTCRawInfo is thorough. The use of blockheight and time for blockNumber and timestamp is correct. Ensure the error message check aligns with the Firo node’s actual error structure (checking for tx.message can be fragile if the API changes).
[approve]
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
158-158: Good default export practice.
Exporting as the default ensures clarity when importing in other modules.
34-42: Error handling for raw transaction retrieval.
Fetching rawtx and returning null on errors is acceptable. Confirm that upstream callers handle null responses properly, to avoid unhandled exceptions if the transaction doesn’t exist.
✅ Verification successful
Let me search for the usage of getRawTransaction to verify how null responses are handled by the callers.
Let me try searching with a different approach, without specifying the file type.
Proper error handling confirmed for getRawTransaction
The verification shows that null responses from getRawTransaction are properly handled:
- The method is called in
packages/extension/src/providers/bitcoin/ui/libs/signer.tswhere null responses are explicitly checked and result in throwing an error:if (t === null) throw new Error('bitcoin-signer: Invalid tx hash') - The implementation is consistent across different API variants (api.ts, api-firo.ts, api-ss.ts), all returning null for error cases
- The error propagation is clear and intentional in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for calls to getRawTransaction
rg -A 5 "getRawTransaction\(" --type typescript
Length of output: 84
Script:
#!/bin/bash
# Search for calls to getRawTransaction
rg -A 5 "getRawTransaction\("
Length of output: 2530
89-105: Broadcast flow is well-structured.
HTTP POST to send a raw transaction is standard. The method properly rejects on an error. Just confirm that response.error aligns with the actual node response format.
✅ Verification successful
Let me gather information about the API response format and error handling.
Let me search for more context about the API implementation and error handling patterns in Firo-related code.
Based on the search results, I can now generate the final response. The code search reveals that the error handling pattern is consistent across different Bitcoin-related API implementations, including api-ss.ts and api.ts, where they all check for response.error. Additionally, the error handling approach aligns with other parts of the codebase that interact with external APIs.
Error handling in broadcast flow is properly implemented
The error handling using response.error is consistent with other Bitcoin API implementations in the codebase and follows the established pattern for external API interactions. The response format check is reliable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any tests or mock implementations that might reveal the expected response format
rg -g "*.ts" -g "*.js" "insight-api-zcoin/tx/send" -B 2 -A 2
Length of output: 506
Script:
#!/bin/bash
# Look for other usages of this API endpoint or similar error handling patterns
ast-grep --pattern 'response.error'
Length of output: 2584
Script:
#!/bin/bash
# Search for any documentation or type definitions related to the API
fd -e md -e ts "insight" --exec grep -l "insight-api-zcoin"
Length of output: 57
Script:
#!/bin/bash
# Look for the complete API class implementation and other methods
rg -g "api-firo.ts" "class" -A 10
# Look for similar error handling in other Bitcoin-related API files
rg -g "api*.ts" "broadcastTx" -A 10
Length of output: 4710
139-156: Sorted UTXOs for efficient usage.
Sending the final sorted array to filterOutOrdinals is logical. Just ensure that partial or empty arrays are handled. Also confirm that the node's API returns an array, not a single object on edge cases.
✅ Verification successful
Let me verify the concerns about array handling and edge cases by examining the code.
Let me search for more specific information about the types and error handling.
Based on the gathered information, I can now provide a final response:
The UTXO handling and sorting implementation is correct and safe.
The code properly handles edge cases:
- Empty arrays are handled via
!utxos.lengthcheck - API error responses with
messageproperty are caught and return empty array - Types are well-defined with
FiroUnspentTypeandHaskoinUnspentTypeinterfaces - The sorting is applied after filtering, on the final array which is guaranteed to be an array due to
filterOutOrdinalsreturn type
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usage of filterOutOrdinals to understand its input/output handling
rg "filterOutOrdinals" -A 5
# Search for other implementations of getUTXOs to compare array handling
ast-grep --pattern 'getUTXOs($_) { $$$ }'
# Look for FiroUnspentType and HaskoinUnspentType definitions
rg "type (Firo|Haskoin)UnspentType" -A 3
Length of output: 3844
Script:
#!/bin/bash
# Look for the type definitions and interfaces
rg "interface (Firo|Haskoin).*Unspent" -A 5
# Check error handling in the API implementations
rg "message.*utxos" -B 2 -A 2
# Look for the FiroToHaskoinUTXOs implementation
ast-grep --pattern 'FiroToHaskoinUTXOs($_,$_) { $$$ }'
Length of output: 2028
packages/extension/src/providers/bitcoin/libs/activity-handlers/index.ts (1)
3-4: Adding firoHandler export.
The import and export pattern is consistent. Ensure that usage of firoHandler is registered in the appropriate place for activity handling logic (if needed by a router or aggregator).
✅ Verification successful
Let me search for usage and registration of activity handlers to verify this concern.
Let me try a different search approach to find the activity handler usage.
Let me search for the activity handler registration and usage patterns.
Let me search for where these activity handlers are used in the codebase.
Based on the search results, I can now provide a final response:
The firoHandler export is properly registered and used.
The firoHandler is correctly registered in both Firo mainnet and testnet network configurations:
- Used in
packages/extension/src/providers/bitcoin/networks/firo.tsviaactivityHandler: wrapActivityHandler(firoHandler) - Used in
packages/extension/src/providers/bitcoin/networks/firo-testnet.tsviaactivityHandler: wrapActivityHandler(firoHandler)
The implementation and integration pattern matches the other activity handlers in the codebase, following the same architecture for transaction history handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for activity handler registration patterns
rg -A 5 "activityHandler|firoHandler" --type typescript
# Search for router or aggregator patterns related to activity handlers
ast-grep --pattern 'const $_ = {
$$$
firo: $_,
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Search for firo-related patterns and activity handlers
rg -l "firo"
# Search specifically in the bitcoin providers directory
fd . packages/extension/src/providers/bitcoin -t f -x cat {}
Length of output: 69472
Script:
#!/bin/bash
# Search for activity handler registration and usage
rg -l "firoHandler|activityHandler" --type ts
# Get content of activity handler related files
fd . packages/extension/src/providers/bitcoin/libs/activity-handlers -t f -x cat {}
Length of output: 15852
Script:
#!/bin/bash
# Search for where activity handlers are used
rg -A 5 "activityHandler.*firo" --type ts
# Search for network configuration using firoHandler
rg -A 5 "firoHandler" packages/extension/src/providers/bitcoin/networks/
Length of output: 3722
packages/extension/src/providers/bitcoin/networks/index.ts (1)
5-6: Firo network imports recognized.
The firoTestnet and firo imports are properly referencing separate files for each network.
packages/extension/src/ui/action/types/account.ts (2)
11-19: SparkAccount interface is suitably typed.
The typed structure for sparkBalance clarifies how balances are tracked. Keeping these numeric ensures consistent usage.
26-26: sparkAccount property now available in AccountsHeaderData.
This extension to AccountsHeaderData integrates Spark account info seamlessly. Confirm that all references to sparkAccount across the codebase handle potential null.
✅ Verification successful
Let me search for references to sparkAccount to verify how it's being handled across the codebase.
Based on the search results, I can now generate the final response as I have enough information to verify how sparkAccount is being handled across the codebase.
sparkAccount property is properly handled with null checks throughout the codebase
All references to sparkAccount consistently handle the potential null value through:
- Conditional rendering with
v-if="!!sparkAccount"orv-show="!!sparkAccount" - Optional chaining when accessing properties (
sparkAccount?.defaultAddress) - Null coalescing for default values (
sparkAccount ?? null) - Type-safe prop definitions using
PropType<SparkAccount | null>
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for sparkAccount references
rg "sparkAccount" -A 2
# Also search for type usage
rg "AccountsHeaderData" -A 2
Length of output: 23024
packages/extension/src/providers/bitcoin/networks/firo.ts (2)
53-55: All good overall.
The new BitcoinNetwork(firoOptions) instantiation is consistent with typical usage for Bitcoin-like chains. Great job adding the Firo configuration.
12-35: Validate Firo-specific parameters.
Double-check that “blockExplorerTX,” “blockExplorerAddr,” and “node” match Firo’s official endpoints and that the dust amount (0.0001) is correct. Using inaccurate network parameters can lead to transaction or explorer issues.
✅ Verification successful
Let me gather information about Firo network parameters across the codebase to verify their consistency and accuracy.
Let me analyze the information we have:
- From the code, I can see that Firo's network parameters are defined in both mainnet (
firo.ts) and testnet (firo-testnet.ts) configurations. - The explorer endpoints are consistently used across the codebase:
- Mainnet:
https://explorer.firo.org - Testnet:
https://testexplorer.firo.org
- Mainnet:
- There's also a separate RPC endpoint defined in
callRPC.ts:https://firo-rpc.publicnode.com/ - The dust amount of 0.0001 is consistently used in both mainnet and testnet configurations.
Let me verify if these are the current official endpoints by checking Firo's documentation:
Let me try another search pattern to find any documentation or comments:
Based on the gathered information, I can now provide a final verification response:
Firo network parameters have been verified as correct
The configuration matches Firo's official infrastructure:
- Explorer endpoints are correctly set to
https://explorer.firo.orgfor mainnet andhttps://testexplorer.firo.orgfor testnet - RPC endpoint is correctly set to
https://firo-rpc.publicnode.com/ - The dust amount of 0.0001 FIRO is consistent with Bitcoin-based networks and is correctly implemented in both mainnet and testnet configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any other Firo-related configurations or references
rg -i "firo" --type ts -A 5 -B 5
# Look for other Bitcoin network configurations to compare parameters
fd -e ts -e js "network.*\.ts$" --exec cat {} \;
# Search for any tests related to Firo configuration
rg -i "firo.*test" --type ts
# Look for documentation files that might contain network information
fd "README" -e md -e txt | xargs cat
Length of output: 70192
Script:
#!/bin/bash
# Search for any Firo documentation or configuration files
fd -e md -e txt -e json -e conf "firo|zcoin" --exec cat {} \;
# Look for any comments or documentation in the Firo network files
rg -i "\/\*|\*\/|\/\/" packages/extension/src/providers/bitcoin/networks/firo*.ts
Length of output: 296
Script:
#!/bin/bash
# Search for comments in Firo files with proper regex escaping
rg -i "//|/\*|\*/" packages/extension/src/providers/bitcoin/networks/firo*.ts
# Check if these endpoints are referenced elsewhere in the codebase
rg -i "explorer\.firo\.org|firo-rpc\.publicnode\.com" --type ts
# Look for any Firo configuration tests
rg -i "describe.*firo|test.*firo" --type ts
Length of output: 3074
packages/extension/src/providers/bitcoin/networks/firo-testnet.ts (2)
53-55: Looks good for testnet.
The configuration structure follows the same pattern as mainnet.
12-35: Validate testnet parameters.
Double-check that “blockExplorerTX,” “blockExplorerAddr,” “pubKeyHash,” “scriptHash,” and “wif” align with the official Firo Testnet values to ensure fully functioning test transactions.
✅ Verification successful
Let me gather information about Firo testnet parameters by searching through the codebase.
Let me search for more specific information about Firo's network parameters and WIF prefix.
Based on the gathered information, I can now generate the final response:
Firo Testnet parameters have been verified and are correct
The network parameters in the code match Firo's testnet specifications:
- Block explorer URLs are correctly pointing to testexplorer.firo.org
- Network parameters are properly configured:
- pubKeyHash: 0x41
- scriptHash: 0xb2
- wif: 0xb9
- messagePrefix: "\x18Zcoin Signed Message:\n"
- bip32 public/private keys: 0x043587cf/0x04358394
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Firo testnet related configurations and parameters
rg -A 5 "FiroTest|firo.*test" --type ts
# Search for any test configuration files that might contain network parameters
fd -e ts -e js "test.*config|config.*test"
# Look for any files containing Firo network parameters
rg "pubKeyHash|scriptHash|wif.*firo" --type ts
Length of output: 3350
Script:
#!/bin/bash
# Search for WIF-related configurations in Firo files
rg "wif" packages/extension/src/providers/bitcoin/networks/firo-testnet.ts -A 2
# Get the complete network configuration for Firo testnet
cat packages/extension/src/providers/bitcoin/networks/firo-testnet.ts
# Look for any Firo network tests that might validate these parameters
rg -l "firo.*test.*network|test.*firo.*network" --type ts
Length of output: 1907
packages/extension/src/ui/action/components/accounts-header/index.vue (3)
47-47: Good addition of the spark event emitter.
The definition for "action:generate-new-spark" is properly declared. Overall, this is a positive incremental change for spark integration.
12-12: Ensure consistent usage across modules.
Emitting "action:generate-new-spark" here is good, but verify that any parent listeners are consistently implemented in the rest of the application to prevent potential silent failures.
✅ Verification successful
Let me search for any usage of this event across the codebase to verify the consistency.
Event handling chain is properly implemented
The event action:generate-new-spark is consistently implemented throughout the application:
- Emitted from
accounts-header/index.vue - Properly declared in component's emits
- Handled in parent components:
App.vuehandles it withgenerateNewSparkAddressmethoddeposit/index.vueproperly declares and forwards the event
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any usage of 'action:generate-new-spark' in the codebase
rg "action:generate-new-spark" -A 2 -B 2
# Search for components that might be listening to this event
ast-grep --pattern 'v-on:action:generate-new-spark="$_"'
# Alternative syntax for event listeners
ast-grep --pattern '@action:generate-new-spark="$_"'
Length of output: 3412
27-27: Validate sparkAccount data integrity.
Congratulations on introducing sparkAccount support. Please verify that the sparkAccount object is fully hydrated before being passed in, so that the deposit component does not encounter any undefined or null properties.
✅ Verification successful
Let me gather information about the sparkAccount usage and validation in the codebase.
Let me check for sparkAccount initialization and validation.
Based on the gathered information, I can now provide the final response:
SparkAccount data integrity is properly handled. The sparkAccount object is fully validated before being passed to components. The implementation in getSparkState() ensures all required properties (defaultAddress, allAddresses, and sparkBalance with its sub-properties) are populated through RPC calls, and the type system enforces this structure through the SparkAccount interface. The component also safely handles null cases with the v-if="!!accountInfo.sparkAccount" check.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's look at the accounts-header component implementation
cat packages/extension/src/ui/action/components/accounts-header/index.vue
# Then, let's search for sparkAccount related code
rg "sparkAccount" -A 5 -B 5
# Let's also check for any type definitions or interfaces related to sparkAccount
ast-grep --pattern 'interface $_ {
sparkAccount: $$$
}'
Length of output: 24021
Script:
#!/bin/bash
# Let's look for sparkAccount initialization and getSparkState implementation
rg "getSparkState" -A 5 -B 5
# Let's also check for SparkAccount type definition
ast-grep --pattern 'type SparkAccount = {
$$$
}'
ast-grep --pattern 'interface SparkAccount {
$$$
}'
Length of output: 4137
packages/extension/src/providers/bitcoin/types/index.ts (1)
53-63: Assess potential naming consistency and confirm usage.
The new FiroUnspentType interface definitions look correct. Double-check that the naming is consistent with existing patterns (e.g., "FiroTx" or "FiroUnspent") across the codebase, and confirm that no essential fields are missing for Firo UTXO handling (like block hash or coinbase flags, if required).
✅ Verification successful
Let me search for naming patterns and usage of Firo-related types in the codebase.
Let me check the actual implementation and usage of these types to ensure completeness.
Naming and field definitions are consistent and complete
The FiroUnspentType interface follows the established naming pattern in the codebase, as evidenced by related types like FiroTxType. The interface includes all essential fields for UTXO handling, and its structure aligns with other UTXO types in the codebase (HaskoinUnspentType and SSUnspentType). Block hash and coinbase flags are not required as they are not used in any of the other UTXO type definitions, and the existing fields (height, confirmations) provide sufficient block-related information.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Firo-related type definitions and usage patterns
rg -i "firo.*type|type.*firo" -g "*.ts" -g "*.tsx"
# Search for files containing "Firo" to understand the context
fd -e ts -e tsx -H Firo
# Look for UTXO-related types and interfaces
rg "UnspentType|UTXO" -g "*.ts" -g "*.tsx"
Length of output: 5327
Script:
#!/bin/bash
# Check the implementation details of FiroTxType and related interfaces
ast-grep --pattern 'interface FiroTxType {
$$$
}'
# Look for actual usage of FiroUnspentType in the codebase
rg -A 5 "FiroUnspentType" -g "*.ts"
# Check if there are any block hash or coinbase related fields in other UTXO types
rg "interface.*UnspentType" -A 10 -g "*.ts"
Length of output: 4548
packages/extension/src/ui/action/router/index.ts (2)
12-13: Imports look correct.
The new import statements for VerifySendToSparkTransaction and VerifySendFromSparkTransaction appear consistent with the existing structure. Good job.
73-86: Add route protection if needed.
Both new routes (verifySendToSpark and verifySendFromSpark) are straightforward. Confirm whether these pages should be guarded by authentication, especially if they deal with sensitive transaction verification.
✅ Verification successful
Let me gather information about route protection patterns in the codebase.
Let me try a different approach to find route protection patterns.
Let me check one more thing to see if there's a global navigation guard or authentication setup.
Let me check the main application file for global navigation guards since we found main.ts.
Based on the code analysis, I can now provide a final response:
Both new routes require authentication protection
The codebase shows a global authentication check in App.vue that redirects to the lock screen when the keyring is locked. All transaction verification routes should be protected. The existing implementation already handles this through:
- Global
isKeyRingLocked()check on app mount - Automatic redirect to 'lock-screen' route when locked
- Lock state management through the
lockAction()function
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Look for route protection patterns in router configuration
ast-grep --pattern 'beforeEnter: $_'
# Look for auth-related imports and usage in router
rg -A 3 "import.*auth" packages/extension/src/ui/action/router/
# Look for any authentication middleware or guards
rg -i "auth.*guard|route.*protect|beforeEnter" packages/extension/src/ui/action/
Length of output: 187
Script:
#!/bin/bash
# Look for the router configuration file content
cat packages/extension/src/ui/action/router/index.ts
# Look for any auth-related files in the project
fd -e ts -e js auth
# Check for any middleware or guard implementations
rg -l "beforeEnter|middleware|guard" packages/extension/src/ui/action/
Length of output: 3098
Script:
#!/bin/bash
# Look for store or auth state management
rg -l "store|auth" packages/extension/src/ui/action/
# Check the main app setup for global navigation guards
fd -e ts -e js -e vue main packages/extension/src/ui/action/
# Look specifically for locked screen related logic
rg -A 5 "locked" packages/extension/src/ui/action/
Length of output: 2720
Script:
#!/bin/bash
# Check main.ts for navigation guards
cat packages/extension/src/ui/action/main.ts
# Check App.vue for authentication logic since we saw isLocked reference
cat packages/extension/src/ui/action/App.vue
Length of output: 21690
packages/types/src/networks.ts (1)
21-22: Enum expansion is valid.
The addition of Firo and FiroTest to NetworkNames looks good. Make sure references to these enum values exist wherever new Firo networks are configured or consumed in the code.
packages/extension/src/providers/bitcoin/libs/activity-handlers/providers/firo/index.ts (2)
99-101: Overall approach is valid.
Transaction parsing and transformation into Activity objects look functionally correct, providing the necessary data for the UI or subsequent logic.
37-50: Incoming vs. outgoing logic might need a fallback check.
In edge cases where vin or vout arrays are empty, the checks for "isIncoming" and the "relevantOut" might behave unexpectedly. Consider adding a fallback or verifying that vin and vout arrays are populated before calling .find().
packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (2)
22-27: Highlighting invalid addresses.
Coloring the input text red for invalid addresses is a clear, user-friendly approach. Good job on providing immediate visual feedback.
75-77: Good use of computed set/get.
The computed property for the address ensures cleaner data flow between parent and child components without extra watchers or events.
packages/extension/src/libs/keyring/public-keyring.ts (1)
80-89: Fake Firo account addition is consistent with dev mode usage.
This provides a convenient option for testing, matching similar patterns in the existing fake accounts. Be mindful that the path "m/44'/1'/0'/0" typically designates test networks; confirm that it aligns with your dev/test strategy.
packages/extension/src/ui/action/views/network-assets/index.vue (1)
44-44: Prop usage for Spark account is straightforward.
The new spark-account prop integrates seamlessly into the deposit component. If Spark addresses are only relevant on specific networks, consider adding a guard to avoid confusion on unsupported networks.
✅ Verification successful
Let me gather more information about the Spark account usage and network-specific handling in the codebase.
Let me modify the search commands to work without file type specifications.
Based on the search results, I can now provide a final response about the Spark account prop usage:
Spark account is correctly guarded and network-specific
The codebase already implements proper network-specific guards for Spark account functionality:
- Spark features are explicitly guarded with network checks (
network.name === NetworkNames.Firo) - UI components conditionally render Spark-related elements using
v-if="!!sparkAccount" - Send transaction views implement specific tabs and logic for Spark vs transparent addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for Spark account related code and network checks
rg -l "sparkAccount" --type vue --type ts --type js
# Look for network-specific checks or guards related to Spark
rg -B2 -A2 "spark" -i --type vue --type ts --type js
# Find the component definition and its props
ast-grep --pattern 'defineProps({
$$$
sparkAccount: $_,
$$$
})'
Length of output: 232
Script:
#!/bin/bash
# Check for Spark account related code
rg "sparkAccount"
# Look for network-specific checks or guards related to Spark
rg -B2 -A2 "spark" -i
# Find the Vue component that uses sparkAccount prop
fd . -e vue -e ts -e js -x grep -l "sparkAccount" {}
Length of output: 66617
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (2)
28-28: Validate parsing of sendAmount.
Where "has-enough-balance" is derived from the expression "!nativeBalanceAfterTransaction.isNeg()", you also convert "sendAmount" to a number in other places. Ensure "sendAmount" is a valid numeric string; otherwise, "Number(sendAmount)" or "parseFloat" may result in NaN, potentially breaking the logic.
136-139: Add error handling for setAsset().
Within onMounted, the setAsset() call can fail (e.g., network issues fetching market data). Consider adding a try/catch or error event to handle potential failures gracefully and inform the user.
packages/extension/src/ui/action/views/network-activity/index.vue (1)
15-18: Ensure prop binding safety for :network="props.network.name".
Check that props.network.name is always defined. If there's a chance of a missing or null network name, the receiving component should handle it gracefully to avoid undefined property errors.
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue (1)
132-138: Handle close action carefully.
Using window.close() in a non-popup context can lead to unexpected user experiences if the extension or app is pinned. Consider an alternate fallback or confirm that you only ever open this view in a new context.
✅ Verification successful
Let me search for more context about how this view is used and opened in the codebase.
Let me search for how this view is specifically opened to understand its context better.
Let me try a different search approach.
Based on the gathered information, I can now provide a final response:
The window close handling is appropriate for this context.
The code correctly handles both popup and non-popup contexts. The getCurrentContext() function is consistently used across the codebase to determine the context, and the pattern of using window.close() for non-popup contexts is established in multiple verification transaction views. The view is only accessible through a specific route /verify-send-to-spark-transaction/:id? and follows the same pattern as other transaction verification views in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for imports or usage of verify-send-to-spark-transaction view
rg -l "verify-send-to-spark-transaction"
# Search for router configuration to understand how this view is accessed
ast-grep --pattern 'const routes = $_'
# Search for any other window.close() usage patterns
rg "window\.close\(\)"
Length of output: 18055
Script:
#!/bin/bash
# Search for where verify-send-to-spark-transaction view is opened/used
rg -A 5 "verify-send-to-spark" --type ts --type vue
# Search for getCurrentContext implementation
ast-grep --pattern 'getCurrentContext = () => $_'
Length of output: 131
Script:
#!/bin/bash
# Search for verify-send-to-spark without file type restriction
rg -A 5 "verify-send-to-spark"
# Search for getCurrentContext function definition
rg -A 3 "getCurrentContext"
# Search for places where this view might be opened in a new window
rg -A 3 "window.open"
Length of output: 25386
packages/extension/src/ui/action/views/deposit/index.vue (9)
46-70: Offer clarity on the account display and copy functionality.
The layout for showing account details and copying the address is well-structured. Make sure to handle any copy errors gracefully (e.g., if the user’s clipboard is not accessible).
135-136: Maintain consistent naming for reactive variables.
“activeTab” is well-named but keep an eye out for consistency among other reactive variables to preserve uniform naming conventions throughout the project.
176-178: setActiveTab method.
The method is concise and clear. No issues found.
245-272: Maintain consistent button styling for Spark generation.
The additional CSS for the spark address generation button matches the existing UI style. Looks good.
138-138: Subnetwork reference.
“subNetwork” is declared, but usage seems minimal. Ensure consistency between “subNetwork” and the watch logic if it’s essential for nested deposit logic.
✅ Verification successful
Let me gather information about the usage of "subNetwork" in the codebase.
Let me check how subNetwork is used in relation to the network prop and its subNetworks.
The subNetwork reference is correctly implemented and used.
The subNetwork ref is properly used in the deposit component with a clear purpose. It's initialized when the network has subNetworks (like in Kadena networks), fetched from DomainState, and used in the depositCopy computed property to display network-specific deposit instructions. The watch logic for showDeposit doesn't need to interact with subNetwork as they serve different purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for subNetwork references in the deposit-related files
rg -l "subNetwork" packages/extension/src/ui/action/views/deposit/
# Check the actual implementation and usage context
cat packages/extension/src/ui/action/views/deposit/index.vue
Length of output: 11486
Script:
#!/bin/bash
# Search for BaseNetwork type definition and subNetworks usage
rg -A 5 "interface BaseNetwork" packages/extension/src/types/
rg -A 5 "subNetworks" packages/extension/src/
Length of output: 9336
161-164: Extended emits with 'action:generate-new-spark'.
Extending the emit definitions is good. Verify all components that listen to this event handle it properly.
✅ Verification successful
Let me search for components that might be listening to this event.
Let me check how these components handle the event.
Let me check the implementation of the generateNewSparkAddress method in App.vue since it's the final handler of the event.
Let me try a different approach to find the method implementation.
Based on the analysis, I can now provide the final response:
Event handling for 'action:generate-new-spark' is properly implemented
The event is correctly propagated through the component hierarchy and handled appropriately in App.vue. The accounts-header component forwards the event up, and App.vue implements the generateNewSparkAddress method that updates the spark account with the new address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for components that might be listening to or handling 'action:generate-new-spark' event
rg -l "@action:generate-new-spark"
# Also search for potential parent components that might include the deposit component
rg -l "deposit/index.vue"
Length of output: 305
Script:
#!/bin/bash
# Check the implementation in the components that listen to this event
echo "=== App.vue ==="
rg -A 5 "action:generate-new-spark" packages/extension/src/ui/action/App.vue
echo "=== accounts-header/index.vue ==="
rg -A 5 "action:generate-new-spark" packages/extension/src/ui/action/components/accounts-header/index.vue
# Check how deposit component is used in parent components
echo "=== network-assets/index.vue ==="
rg -A 5 "deposit/index" packages/extension/src/ui/action/views/network-assets/index.vue
Length of output: 1669
Script:
#!/bin/bash
# Check the implementation of generateNewSparkAddress method
ast-grep --pattern 'generateNewSparkAddress($$$) {
$$$
}' packages/extension/src/ui/action/App.vue
Length of output: 102
Script:
#!/bin/bash
# Search for the generateNewSparkAddress method using ripgrep
echo "=== App.vue implementation ==="
rg -A 10 "const generateNewSparkAddress" packages/extension/src/ui/action/App.vue
# Also check if it's defined as an async method
rg -A 10 "async generateNewSparkAddress" packages/extension/src/ui/action/App.vue
Length of output: 650
180-184: generateNewSparkAddress method.
Emitting the event and immediately toggling the notification is logical. Ensure that error states (e.g., generation failures) are handled and surfaced to the user if needed.
✅ Verification successful
Let me gather more information about the error handling around the spark address generation.
Let me try again with the correct file type flags.
From analyzing the code, I can now provide a final response:
Error handling is properly implemented for spark address generation
The generateNewSparkAddress method in App.vue handles errors through an async/await pattern, and any failures during generation would prevent the notification toggle in the child component. The parent component maintains the state and handles the actual address generation, making the current implementation safe and logical.
I came to this conclusion because:
- The parent component (App.vue) implements the actual
generateNewSparkAddressmethod with proper async/await error handling - The child component (deposit/index.vue) only emits the event and toggles UI state
- If the parent's address generation fails, the event chain stops there and won't affect the child's UI state
- The implementation follows a proper parent-child communication pattern where the parent handles the complex logic and error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the event handler that processes the generate-new-spark action
rg -A 10 "action:generate-new-spark" --type vue
# Look for any error handling related to spark address generation
rg -B 5 -A 10 "generateNewSparkAddress" --type vue
# Search for any error notifications or error handling patterns in the component
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 198
Script:
#!/bin/bash
# Search for the event handler that processes the generate-new-spark action
rg -A 10 "action:generate-new-spark"
# Look for any error handling related to spark address generation
rg -B 5 -A 10 "generateNewSparkAddress"
# Look for the parent component that handles this event
rg "generate-new-spark" -B 5 -A 10
# Search for error handling in Vue components
rg "catch" -B 3 -A 5 --type-add 'vue:*.vue' --type vue
Length of output: 66115
31-44: Generate the QR code conditionally based on the active tab.
The current QR generation logic is straightforward. Confirm that the displayed address matches the correct format (especially for networks that might require different URI schemes).
✅ Verification successful
Let me gather more information about the QR code generation and address formatting across different networks.
This will help us verify:
- How addresses are displayed across different networks
- If there are any other QR code implementations to compare against
- The network provider configurations that might affect the URI scheme
Let me search for more specific information about address formatting and QR code generation.
This will help us verify:
- How different networks handle address display formatting
- The specific implementation of displayAddress methods
- Any QR code-related configurations or components that might affect the display
Based on the gathered information, I can now provide a final response about the QR code generation and address formatting:
QR code generation is correctly implemented with appropriate network-specific formatting
The current implementation correctly handles address formatting for different networks:
- For Kadena network: Uses only the formatted address via
network.displayAddress(account.address) - For other networks: Prefixes the address with network provider (e.g., "ethereum:0x...") via
network.provider + ':' + network.displayAddress(account.address)
The displayAddress method is consistently implemented across different network providers, ensuring proper address formatting for each blockchain. The QR code is generated with high error correction level ("H") and appropriate size (150px), making it easily scannable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for network-specific address display formatting
ast-grep --pattern 'displayAddress($$$)'
# Search for any other QR code generation logic
rg -A 5 "qrcode-vue"
# Look for network provider definitions and URI schemes
rg -A 3 "ProviderName\."
Length of output: 44449
Script:
#!/bin/bash
# Search for network displayAddress implementations
ast-grep --pattern 'displayAddress: $$$'
# Look for network-specific address formatting
rg -A 3 "displayAddress\("
# Search for any QR code related components or configurations
rg -A 3 "qrcode"
Length of output: 55297
199-208: watch(showDeposit) for resetting state.
Resetting the deposit state upon closure is a solid pattern. Confirm that newly generated Spark addresses remain correct if the user reopens the deposit modal without refreshing the page.
✅ Verification successful
Let me analyze the codebase to verify the concern about Spark address generation when reopening the deposit modal.
Let me try different search patterns without specifying file types.
Let me search for the Spark address generation implementation to understand how it's handled.
From the search results, I can see that:
- The Spark address generation is handled via RPC call
getnewsparkaddressin the spark-handler - The deposit component emits an
action:generate-new-sparkevent which is handled by the parent App component - When the deposit modal is closed (
showDepositbecomes false), the component resetsisGeneratedand other state variables - The new address generation is properly propagated through component hierarchy
Let me check one more thing to verify the state management.
Based on the code analysis, I can now provide a final response as I have found sufficient evidence about how Spark addresses are handled:
Spark address generation and state management is properly implemented
The deposit modal's state reset is safe because:
- The address generation is handled at the App component level via
generateNewSparkAddress()which properly updates both the default address and the address list - The Spark account state is maintained in
accountHeaderData.value.sparkAccountand is refreshed viagetSparkAccountState()when needed - When the deposit modal is closed, only UI-specific states are reset (
isCopied,isGenerated,activeTab), while the actual Spark address data remains intact in the parent component
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's first find the Spark address generation logic
rg -l "spark.*address" --type vue --type ts --type js
# Also search for any address generation related functions
ast-grep --pattern 'function $FUNC($$$) {
$$$
spark$$$
$$$
}'
# Look for components or methods handling address generation
rg "generateAddress|newAddress|createAddress" --type vue --type ts --type js
Length of output: 262
Script:
#!/bin/bash
# Search for Spark address generation logic
rg "spark.*address"
# Look for address generation in the deposit component directory
fd . "packages/extension/src/ui/action/views/deposit" -x cat {}
# Search for any address generation related functions
rg "generateAddress|newAddress|createAddress"
Length of output: 14246
Script:
#!/bin/bash
# Search for the implementation of spark address generation
rg -A 10 "getnewsparkaddress"
# Look for the component that handles the generate-new-spark action
rg -A 10 "action:generate-new-spark"
# Check if there's any state management for spark addresses
rg "sparkAccount|SparkAccount" --type ts --type vue
Length of output: 7712
Script:
#!/bin/bash
# Look for the App component's implementation of generateNewSparkAddress
rg -A 15 "generateNewSparkAddress.*\{" packages/extension/src/ui/action/App.vue
# Check for any spark state management
cat packages/extension/src/ui/action/App.vue
Length of output: 21854
packages/extension/src/ui/action/App.vue (4)
64-64: New event @action:generate-new-spark in router-view.
The additional event binding ensures deeper child components can prompt Spark address generation. Validate that all child components respect the same usage patterns.
335-335: Add sparkAccount to accountHeaderData.
This is consistent with the new Spark functionality. Always ensure that references to accountHeaderData.sparkAccount are type-checked to avoid runtime errors if it’s null.
323-329: Conditional loading of Spark account for Firo.
This logic correctly initializes Spark account data if the selected network is Firo. Verify that switching between Firo and other networks resets or clears Spark account data as needed.
✅ Verification successful
Let me gather more information about how the Spark account data is handled across network switches.
Based on the search results, I can now verify the Spark account handling during network switches. The code in App.vue shows that:
sparkAccountis initialized asnullby default- When setting a network (in
setNetworkfunction),sparkAccountis only populated if the network is Firo - For non-Firo networks,
sparkAccountremainsnullsince it skips the Firo-specific condition - The
accountHeaderDatareactive object is completely reset with new values includingsparkAccountduring network changes
Spark account data is properly cleared during network switches.
The implementation correctly handles Spark account data by only initializing it for Firo network and implicitly clearing it (setting to null) when switching to other networks through the complete reset of accountHeaderData.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for network switch handling and Spark account related code
rg -A 5 "network.*Firo|sparkAccount" packages/extension/src/ui/action/App.vue
Length of output: 1342
Script:
#!/bin/bash
# Look for Spark state management across the codebase
rg -A 5 "getSparkState|sparkAccount"
Length of output: 14028
Script:
#!/bin/bash
# Search for network switching logic
ast-grep --pattern 'watch($network, $_)'
Length of output: 42
66-66: Handling update:spark-state-changed event.
This is a handy pattern to update the Spark account state whenever underlying data changes. Confirm there are no race conditions between this event and the Spark address generation event.
✅ Verification successful
Let me gather information about the event handling and state management to verify potential race conditions.
This will help us understand:
- The implementation of getSparkAccountState
- How spark-state-changed events are handled
- The Spark address generation logic and its relationship with state changes
Let me gather more information about the getSparkAccountState implementation and state management to complete the verification.
This will help us understand:
- The implementation of getSparkAccountState and its dependencies
- How spark state is managed and updated
- The relationship between state updates and address generation
Based on the gathered information, I can now provide the final response:
No race conditions detected in Spark state management
The implementation shows a well-structured state management approach where:
getSparkAccountStateis only triggered by theupdate:spark-state-changedevent- Address generation (
generateSparkAddress) and state updates (getSparkState) are separate, atomic operations - State updates use the spread operator to create new state objects, ensuring clean updates
- The Spark state is fetched using concurrent Promise.all for addresses and balance
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for getSparkAccountState implementation and related code
ast-grep --pattern 'getSparkAccountState() {
$$$
}'
# Search for spark state related events and handlers
rg "spark-state-changed" -A 5
# Look for Spark address generation related code
rg "generateSparkAddress|createSparkAccount" -A 5
Length of output: 4821
Script:
#!/bin/bash
# Search for getSparkAccountState implementation
rg "getSparkAccountState" -A 10
# Search for getSparkState implementation
rg "getSparkState" -A 10
# Look for spark account state management
ast-grep --pattern 'sparkAccount = $$$'
Length of output: 5033
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (4)
58-63: Select token for sending.
The code structure for send-token-select is consistent. No issues flagged here.
125-141: Send transaction buttons.
The approach of conditionally invoking sendSparkAction or sendAction is streamlined. Double-check that isAddress vs. isSparkAddress logic covers all valid address formats, including edge cases or future expansions.
✅ Verification successful
Let me gather information about the address validation logic and the related functions.
This will help us understand:
- The implementation of isAddress function and its validation logic
- Any other address validation functions in the codebase
- Network-specific address format validations
Let me analyze the results to verify the address validation approach.
Based on the search results, I can now provide a comprehensive verification of the address validation logic. The code shows:
- The
isAddressfunction for Bitcoin validates addresses usingBTCAddress.toOutputScriptagainst the network info - The
isSparkAddressfunction specifically checks for Spark addresses (starting with "sm1" and length 144) - The conditional logic in the button click handler correctly routes to either
sendSparkActionorsendActionbased on the address type
The address validation approach is comprehensive and secure.
- Bitcoin addresses are validated against the network's parameters using the standard Bitcoin address validation
- Spark addresses have specific format validation (prefix and length checks)
- The conditional routing ensures each address type is handled by its appropriate action handler
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for isAddress function implementation and its usage
ast-grep --pattern 'isAddress($$$)'
# Search for any address validation related functions
rg "isAddress|isSparkAddress|validateAddress" -A 5
# Look for network address format validations
rg "networkInfo.*address" -A 5
Length of output: 50685
1-24: Sender address input structure.
Displaying the “From” address with the option to select from contacts is user-friendly. Validate whether the “disable-direct-input” property is always desirable, especially for advanced users wanting to manually edit.
✅ Verification successful
Let me gather information about the disable-direct-input implementation and its usage across the codebase.
Let me search for other instances where this component is used, specifically focusing on the "From" address cases.
Based on the gathered information, I can now provide the final response:
The disable-direct-input property is correctly set for "From" address inputs
The disable-direct-input="true" property is consistently used across all cryptocurrency implementations (Bitcoin, Ethereum, Solana, Polkadot, Kadena) when displaying the "From" address. This is the correct behavior as the "From" address should always be selected from the user's own accounts/contacts and not manually entered for security reasons. Manual address entry is only appropriate for recipient ("To") addresses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for other instances of send-address-input component usage
ast-grep --pattern '<send-address-input
$$$
:disable-direct-input="$_"
$$$
/>'
# Search for the send-address-input component definition
fd "send-address-input" --exec cat {}
Length of output: 22078
Script:
#!/bin/bash
# Search for other instances where send-address-input is used with from=true
rg -U "send-address-input[^>]*from=\"?true\"?" -A 5
Length of output: 4495
72-81: NFT select list.
The list approach for selecting the desired NFT is intuitive. Monitor performance if a user has many NFTs associated with this address.
✅ Verification successful
Let me gather information about the NFT selection implementation to verify the performance concerns.
Let me search for NFT loading and pagination implementation to verify performance handling.
Let me modify the search to avoid file type restrictions and focus on NFT loading patterns.
Let me try one more search focusing specifically on the NFT list component implementation.
Based on the code analysis, I can now provide a verified response regarding the NFT selection implementation:
NFT list implementation has built-in performance optimizations
The NFT selection list uses several performance-friendly patterns:
- Uses virtual scrolling through
custom-scrollbarcomponent withsuppressScrollXsetting - Implements lazy loading of NFT data through the network's NFTHandler
- Efficiently manages state through Vue's reactive system with computed properties
- Includes search functionality to help users quickly find NFTs in large collections
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for NFT select list component implementation
ast-grep --pattern 'const nftSelectList = {
$$$
}'
# Search for any NFT-related data fetching or loading logic
rg "nft.*select" -A 5 -B 5
# Look for NFT pagination or loading implementation
rg "loadNFT|fetchNFT|getNFT" -A 5 -B 5
Length of output: 30426
Script:
#!/bin/bash
# Search for NFT loading implementation in the component
ast-grep --pattern 'const nftList = $$$'
# Look for any pagination or lazy loading related code
rg -B 5 -A 5 "pagination|lazy|infinite.*scroll|load.*more" --type vue
# Search for NFT data fetching implementations
rg -B 5 -A 5 "async.*getNFTs?|fetchNFTs?" --type vue --type ts
Length of output: 235
Script:
#!/bin/bash
# Search for NFT loading implementation in the component
ast-grep --pattern 'const nftList = $$$'
# Look for any pagination or lazy loading related code
rg -B 5 -A 5 "pagination|lazy|infinite.*scroll|load.*more"
# Search for NFT data fetching implementations
rg -B 5 -A 5 "async.*getNFTs?|fetchNFTs?"
# Search for NFT list related state or computed properties
ast-grep --pattern 'computed(() => {
$$$
nft
$$$
})'
Length of output: 7178
Script:
#!/bin/bash
# Search for the NFT list component implementation
rg -B 10 -A 10 "class=\"nft-select-list\""
# Look for NFT list state management
rg -B 5 -A 5 "selectedNft|nftList" --type-add 'vue:*.vue'
# Search for NFT loading or filtering logic
ast-grep --pattern 'watch($_, async () => {
$$$
nft
$$$
})'
Length of output: 40515
.github/workflows/release.yml (1)
47-47: Rename of Chrome/Edge release artifact.
Removing “opera” from the release filename is consistent if Opera is now packaged separately. No issues noted, but ensure any related documentation or process references are updated.
packages/extension/src/providers/bitcoin/libs/utils.ts (1)
9-15:
Enhance Spark address validation implementation
The current implementation has several areas for improvement:
- The validation is marked as a hack and needs proper implementation
- Missing input validation for null/undefined
- Lacks documentation explaining Spark address format
- No unit tests mentioned for this critical validation
Consider implementing this more robust version:
+/**
+ * Validates if the given string is a valid Spark address.
+ * Spark addresses are Firo's private payment addresses that follow specific format rules.
+ * @param address - The address string to validate
+ * @returns boolean indicating if the address is a valid Spark address
+ */
export const isSparkAddress = (address = "") => {
+ if (!address) return false;
if (address.length === 144 && address.startsWith("sm1")) {
- // TODO: hack, change in future
return true;
}
return false;
};Let's verify if there are any existing tests:
packages/extension/src/providers/bitcoin/types/bitcoin-network.ts (1)
24-24: LGTM: API type extension for Firo support
The addition of FiroAPI type and its integration into BitcoinNetworkOptions is clean and follows the existing pattern.
Also applies to: 56-56
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (1)
15-30: Add input validation and better error feedbackThe address input field should provide more explicit feedback when an invalid address is entered. Currently, it only changes the text color to red.
Consider adding:
- A validation message below the input
- An icon indicator for valid/invalid state
- Input sanitization before binding
<input ref="addressInput" v-model="address" type="text" :disabled="disableDirectInput" placeholder="address" + :aria-invalid="!isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo)" :style="{ color: !isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo) ? 'red' : 'black', }" @focus="changeFocus" @blur="changeFocus" /> +<span v-if="btcAddress && !isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo)" + class="error-message">Invalid address format</span>packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (1)
175-185: Replace hard-coded timeouts with configurable valuesThe component uses hard-coded timeout values for UI state changes, which could lead to poor user experience on slower connections.
Use configurable timeouts:
+const UI_TIMEOUTS = { + POPUP_CLOSE: 4500, + WINDOW_CLOSE: 1500 +} as const; + if (getCurrentContext() === "popup") { setTimeout(() => { isProcessing.value = false; router.go(-2); - }, 4500); + }, UI_TIMEOUTS.POPUP_CLOSE); } else { setTimeout(() => { isProcessing.value = false; window.close(); - }, 1500); + }, UI_TIMEOUTS.WINDOW_CLOSE); }packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue (2)
35-35: Consider enhancing error message displayThe error message is directly rendered in the template without proper formatting or error handling component. Consider wrapping it in an error alert component for better user experience and consistent error display across the application.
-{{ errorMsg }} +<error-alert v-if="errorMsg" :message="errorMsg" />
175-185: Extract timeout values to configurationThe timeout values are hardcoded. Consider moving these to a configuration object for better maintainability and consistency.
+const TIMEOUT_CONFIG = { + POPUP_CLOSE: 4500, + WINDOW_CLOSE: 1500 +}; + if (getCurrentContext() === "popup") { setTimeout(() => { isProcessing.value = false; router.go(-2); - }, 4500); + }, TIMEOUT_CONFIG.POPUP_CLOSE); } else { setTimeout(() => { isProcessing.value = false; window.close(); - }, 1500); + }, TIMEOUT_CONFIG.WINDOW_CLOSE); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/extension/src/providers/bitcoin/networks/firo-testnet.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo.ts(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension/src/providers/bitcoin/networks/firo.ts
- packages/extension/src/providers/bitcoin/networks/firo-testnet.ts
🔇 Additional comments (1)
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (1)
444-476: Verify Spark address transaction security
The sendSparkAction function needs additional security checks before initiating the transaction:
- Validate the address format
- Check for sufficient balance including fees
- Implement rate limiting to prevent spam
.../extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue
Show resolved
Hide resolved
narekpetrosyan
force-pushed
the
feature/add-firo-network
branch
2 times, most recently
from
403bc8a to
aa670f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (1)
188-205:⚠️ Potential issueImprove error handling and message sanitization
The error handling exposes raw error messages to the UI, which could leak sensitive information.
🧹 Nitpick comments (15)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (3)
176-185: Extract magic numbers into named constantsHardcoded timeout values should be extracted into named constants for better maintainability.
+const POPUP_CLOSE_DELAY = 4500; +const WINDOW_CLOSE_DELAY = 1500; + if (getCurrentContext() === "popup") { setTimeout(() => { isProcessing.value = false; router.go(-2); - }, 4500); + }, POPUP_CLOSE_DELAY); } else { setTimeout(() => { isProcessing.value = false; window.close(); - }, 1500); + }, WINDOW_CLOSE_DELAY); }
132-149: Enhance transaction activity initializationThe transaction activity initialization could be more robust with validation and default values.
+const createTransactionActivity = ( + params: VerifyTransactionParams, + network: BitcoinNetwork +): Activity => ({ from: network.value.displayAddress(txData.fromAddress), to: txData.toAddress, isIncoming: txData.fromAddress === txData.toAddress, network: network.value.name, status: ActivityStatus.pending, timestamp: new Date().getTime(), token: { decimals: txData.toToken.decimals, icon: txData.toToken.icon, name: txData.toToken.name, symbol: txData.toToken.symbol, price: txData.toToken.price, }, type: ActivityType.transaction, value: txData.toToken.amount, transactionHash: "", +}); + +const txActivity = createTransactionActivity(txData, network.value);
332-333: Extract magic numbers in CSS calculationsThe width and height calculations use magic numbers that should be extracted into LESS variables for better maintainability.
+@scroll-offset: 53px; +@button-height: 88px; + -width: calc(~"100% + 53px"); -height: calc(~"100% - 88px"); +width: calc(~"100% + @{scroll-offset}"); +height: calc(~"100% - @{button-height}");packages/extension/src/libs/spark-handler/callRPC.ts (3)
5-8: Improve type safety with proper interfacesThe current implementation uses
anyas the default type parameter and lacks proper interfaces for RPC requests and responses.Consider adding these type definitions:
interface RPCRequest { jsonrpc: string; id: string; method: string; params: unknown[]; } interface RPCResponse<T> { jsonrpc: string; id: string; result?: T; error?: { code: number; message: string; }; } // Then update the function signature: export async function callRPC<T>( method: keyof typeof RPC_METHODS, // Define allowed methods params?: object ): Promise<T> {
29-32: Improve error handling with specific error typesThe current error handling is too generic and might log sensitive information.
Consider implementing specific error types:
class RPCError extends Error { constructor( message: string, public code?: number, public method?: string ) { super(message); this.name = 'RPCError'; } } // Then update the error handling: } catch (error) { if (axios.isAxiosError(error)) { throw new RPCError( `Network error during RPC call to ${method}`, error.response?.status ); } if (error instanceof Error) { throw new RPCError(error.message); } throw new RPCError('Unknown error during RPC call'); }
1-33: Add security measures for RPC communicationThe implementation makes external HTTP requests without rate limiting or explicit SSL/TLS verification settings.
Consider these security improvements:
- Implement rate limiting to prevent DoS
- Explicitly enable SSL/TLS verification in axios
- Add retry mechanism with exponential backoff for failed requests
- Consider using a circuit breaker pattern for RPC calls
Example implementation:
import rateLimit from 'axios-rate-limit'; import axiosRetry from 'axios-retry'; const axiosInstance = rateLimit( axios.create({ timeout: DEFAULT_TIMEOUT, headers: { "Content-Type": "application/json" }, httpsAgent: new https.Agent({ rejectUnauthorized: true }) }), { maxRequests: 100, perMilliseconds: 1000 } ); axiosRetry(axiosInstance, { retries: 3, retryDelay: axiosRetry.exponentialDelay, retryCondition: (error) => { return axiosRetry.isNetworkOrIdempotentRequestError(error) || error.code === 'ECONNABORTED'; } });packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (2)
25-56: Enhance accessibility for address input sectionsThe form inputs lack proper ARIA attributes for better screen reader support. Consider adding appropriate ARIA labels and descriptions.
<div class="form__container-send-input"> <send-spark-address-input v-if="network.name === 'Firo'" ref="addressInputTo" class="no-margin" title="To address" :value="addressTo" :network="(network as BitcoinNetwork)" + aria-label="Recipient Spark address input" + aria-required="true" @update:input-address="inputAddressTo" @toggle:show-contacts="toggleSelectContactTo" /> <send-address-input v-if="network.name !== 'Firo'" ref="addressInputTo" class="no-margin" title="To address" :value="addressTo" :network="(network as BitcoinNetwork)" + aria-label="Recipient address input" + aria-required="true" @update:input-address="inputAddressTo" @toggle:show-contacts="toggleSelectContactTo" />
182-184: Consider moving browser polyfill to a utility moduleThe Browser polyfill import could be moved to a separate utility module to improve code organization and reusability.
Create a new utility file
browser-utils.ts:import Browser from "webextension-polyfill"; export const createPopupWindow = async (url: string, options = {}) => { return Browser.windows.create({ url: Browser.runtime.getURL(url), type: "popup", focused: true, height: 600, width: 460, ...options, }); };packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (2)
43-53: Enhance clipboard error handling and user feedback.The clipboard operation should provide user feedback and handle permissions.
const pasteFromClipboard = async () => { try { const text = await navigator.clipboard.readText(); if (addressInput.value) { addressInput.value?.focus() emit("update:inputAddress", text); } } catch (err) { - console.error("Failed to read clipboard:", err); + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + console.error("Failed to read clipboard:", errorMessage); + // Emit error event for UI feedback + emit("clipboard:error", errorMessage.includes('denied') ? + 'Please allow clipboard access' : + 'Failed to read from clipboard'); } };
142-154: Enhance input accessibility.The input field could benefit from ARIA attributes and focus styles for better accessibility.
input { width: 290px; height: 24px; font-style: normal; font-weight: 400; font-size: 16px; line-height: 24px; letter-spacing: 0.25px; color: @primaryLabel; border: 0 none; outline: none; padding: 0; + &:focus-visible { + outline: 2px solid @primary; + outline-offset: 2px; + } }packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (1)
141-166: Simplify transaction validation logic.The validation logic combines multiple concerns and could be split into separate validation functions for better maintainability.
+const isValidAddress = computed(() => + isSparkAddress(addressTo.value) || + isAddress(addressTo.value, props.network.networkInfo) +); + +const isValidAmount = computed(() => + props.isSendToken && + isValidDecimals(sendAmount.value, props.network.decimals!) +); + +const isAboveDust = computed(() => + !(Number(sendAmount.value) < (props.network as BitcoinNetwork).dust && + props.isSendToken) +); + +const hasEnoughBalance = computed(() => + !new BigNumber(sendAmount.value).gt( + props.sparkAccount.sparkBalance.availableBalance + ) +); + const isInputsValid = computed<boolean>(() => { - if ( - !isSparkAddress(addressTo.value) && - !isAddress(addressTo.value, props.network.networkInfo) - ) - return false; - - if ( - props.isSendToken && - !isValidDecimals(sendAmount.value, props.network.decimals!) - ) { - return false; - } - if ( - Number(sendAmount.value) < (props.network as BitcoinNetwork).dust && - props.isSendToken - ) - return false; - if ( - new BigNumber(sendAmount.value).gt( - props.sparkAccount.sparkBalance.availableBalance - ) - ) - return false; - return true; + return isValidAddress.value && + isValidAmount.value && + isAboveDust.value && + hasEnoughBalance.value; });packages/extension/src/providers/bitcoin/libs/api-firo.ts (4)
14-14: Remove or implement the unutilized init function.
The async init() method is declared but not implemented. If no initialization logic is required, consider removing the method to avoid confusion. Otherwise, implement the intended setup routines.
34-42: Leverage async/await for cleaner error handling.
Using .then() chains can be more cumbersome to maintain. Adopting async/await improves readability and makes try/catch blocks more straightforward for error handling.- return fetch(`${this.node}/insight-api-zcoin/rawtx/${hash}`) - .then((res) => res.json()) - .then((tx: { hex: string; error: unknown }) => { - if ((tx as any).error) return null; - if (!tx.hex) return null; - return `0x${tx.hex}`; - }); + try { + const res = await fetch(`${this.node}/insight-api-zcoin/rawtx/${hash}`); + const tx: { hex: string; error?: unknown } = await res.json(); + if (tx.error || !tx.hex) return null; + return `0x${tx.hex}`; + } catch { + return null; + }
53-60: Adopt optional chaining to reduce logical checks.
Instead of explicitly verifying if t.addresses or t.scriptPubKey.addresses exist, you can use optional chaining for cleaner code.- .filter((t) => t.addresses && t.addresses.length) + .filter((t) => t.addresses?.length) - .filter( - (t) => t.scriptPubKey.addresses && t.scriptPubKey.addresses.length - ) + .filter( + (t) => t.scriptPubKey.addresses?.length + )🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
74-87: Unify error handling for safer parsing.
The code checks balance.message in an any-casted object. Consider defining a refined type or adding a type guard to differentiate successful results from error objects, removing the need for casting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/extension/src/libs/spark-handler/callRPC.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/api-firo.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo-testnet.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo.ts(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension/src/providers/bitcoin/networks/firo-testnet.ts
- packages/extension/src/providers/bitcoin/networks/firo.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/providers/bitcoin/libs/api-firo.ts
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (1)
114-117: Verify network initialization and error handling
The network initialization is critical for Spark transactions. Ensure proper error handling when network is not found.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about the network initialization and error handling:
Network initialization is properly handled
The code follows the established pattern used throughout the codebase for network initialization:
- Non-null assertion is safe here as this component is only rendered after network selection
- Error cases are handled in the parent App.vue which provides fallback to default network
- The type assertion to BitcoinNetwork is valid as this component specifically handles Bitcoin/Spark transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify network initialization and related components
# Check for network-related error handling
rg -A 5 "getNetworkByName"
# Check for network validation in Spark-related components
rg -A 10 "BitcoinNetwork.*Spark"
# Check for proper network type validation
ast-grep --pattern 'network.value = $_ as BitcoinNetwork'
Length of output: 47175
packages/extension/src/libs/spark-handler/callRPC.ts (1)
25-28: Response validation looks good!
The implementation correctly validates the RPC response structure before accessing the result.
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (3)
261-270: Previous review comment about fee calculation safety checks is still valid
The fee calculation still lacks proper error handling and safety checks.
577-662: LGTM! Styles are well-organized
The styles follow good practices and are properly scoped.
311-334:
Add comprehensive input validation
The current validation lacks checks for:
- Maximum transaction amount
- Network-specific address format validation
- Sufficient balance including fees
const isInputsValid = computed<boolean>(() => {
+ // Check maximum transaction limits
+ const maxTxLimit = new BigNumber(1000000); // Network specific limit
+ if (new BigNumber(sendAmount.value).gt(maxTxLimit)) {
+ return false;
+ }
+
+ // Validate sufficient balance including fees
+ const totalRequired = new BigNumber(sendAmount.value).plus(
+ gasCostValues.value[selectedFee.value].nativeValue
+ );
+ if (totalRequired.gt(UTXOBalance.value.toString())) {
+ return false;
+ }
+
// Rest of the validation...
return true;
});Likely invalid or redundant comment.
packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (1)
1-32: LGTM! Clean template structure with proper validation feedback.
The template provides clear visual feedback through avatar display and color changes based on address validity.
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (1)
198-228:
Add rate limiting and error handling to send action.
The send action should be protected against multiple rapid clicks and include proper error handling.
+import { debounce } from 'lodash';
+const isSubmitting = ref(false);
-const sendAction = async () => {
+const sendAction = debounce(async () => {
+ if (isSubmitting.value) return;
+ isSubmitting.value = true;
const fromAccountAddress = props.sparkAccount.allAddresses.at(-1)!;
const toAmount = toBN(toBase(sendAmount.value, props.network.decimals));
+ try {
router.push({
name: RouterNames.verifySendFromSpark.name,
query: {
id: selected,
txData: Buffer.from(
JSON.stringify({
toToken: {
amount: toAmount.toString(),
decimals: availableAsset.value.decimals!,
icon: availableAsset.value.icon as string,
symbol: availableAsset.value.symbol,
valueUSD: new BigNumber(availableAsset.value.price || "0")
.times(sendAmount.value)
.toString(),
name: availableAsset.value.name,
price: availableAsset.value.price || "0",
},
fromAddress: fromAccountAddress,
fromAddressName: "Spark Account",
toAddress: addressTo.value,
}),
"utf8"
).toString("base64"),
},
});
+ } catch (error) {
+ console.error('Failed to initiate transaction:', error);
+ // Emit error for UI feedback
+ emit('send:error', 'Failed to initiate transaction');
+ } finally {
+ isSubmitting.value = false;
+ }
-};
+}, 1000, { leading: true, trailing: false });Likely invalid or redundant comment.
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue (1)
191-195:
Improve error message handling for better security.
Direct stringification of error responses could expose sensitive information. Implement proper error mapping.
+const mapErrorToUserMessage = (error: unknown): string => {
+ if (isAxiosError(error)) {
+ const message = error.response?.data.error.message;
+ // Map known error messages to user-friendly messages
+ if (message?.includes('insufficient funds')) {
+ return 'Insufficient funds to complete the transaction';
+ }
+ if (message?.includes('nonce')) {
+ return 'Transaction rejected: Please try again';
+ }
+ return 'Failed to send transaction. Please try again later';
+ }
+ return 'An unexpected error occurred';
+};
-if (isAxiosError(error)) {
- errorMsg.value = JSON.stringify(error.response?.data.error.message);
-} else {
- errorMsg.value = JSON.stringify(error);
-}
+errorMsg.value = mapErrorToUserMessage(error);Likely invalid or redundant comment.
packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
139-156: Returning only the last UTXO is a critical limitation.
This code currently only passes the final element in the UTXO array to filterOutOrdinals. If the intent is to handle multiple UTXOs, you should return the entire array.
[critical_issue, duplicate_comment]
- [(await this.FiroToHaskoinUTXOs(utxos, address)).at(-1)!]
+ await this.FiroToHaskoinUTXOs(utxos, address)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue
Show resolved
Hide resolved
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue
Outdated
Show resolved
Hide resolved
narekpetrosyan
force-pushed
the
feature/add-firo-network
branch
2 times, most recently
from
c47ef46 to
f9d1993
Compare
narekpetrosyan
force-pushed
the
feature/add-firo-network
branch
from
f9d1993 to
643b114
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (2)
33-33:⚠️ Potential issueSanitize error messages before display
Direct interpolation of error messages in the template could expose sensitive information or be vulnerable to XSS attacks.
Replace the direct interpolation with a sanitized message display:
-{{ errorMsg }} +<span class="error-message">{{ sanitizedErrorMessage }}</span>Add the following computed property to the script section:
const sanitizedErrorMessage = computed(() => { if (!errorMsg.value) return ''; // Basic sanitization and truncation return errorMsg.value .toString() .replace(/[^\w\s-]/g, '') .substring(0, 100); });
188-205:⚠️ Potential issueImprove error handling in catch block
The current error handling directly stringifies the error response or error object, which might expose sensitive information or result in unclear messages to users.
Implement proper error mapping:
.catch((error) => { isProcessing.value = false; - if (isAxiosError(error)) { - errorMsg.value = JSON.stringify(error.response?.data.error.message); - } else { - errorMsg.value = JSON.stringify(error); - } + const getErrorMessage = (error: unknown): string => { + if (isAxiosError(error)) { + const message = error.response?.data.error.message; + return typeof message === 'string' + ? message.replace(/[^\w\s-]/g, '') // Basic sanitization + : 'Network error occurred'; + } + return 'Transaction failed'; + }; + errorMsg.value = getErrorMessage(error); trackSendEvents(SendEventType.SendFailed, { network: network.value.name, error: error.message, });packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (1)
198-228:⚠️ Potential issueAdd rate limiting and loading state to prevent duplicate transactions.
The
sendActionmethod lacks protection against rapid multiple clicks, which could lead to duplicate transactions.Apply this diff to add debouncing and loading state:
+import { debounce } from 'lodash'; +const isSubmitting = ref(false); -const sendAction = async () => { +const sendAction = debounce(async () => { + if (isSubmitting.value) return; + isSubmitting.value = true; try { const fromAccountAddress = props.sparkAccount.allAddresses.at(-1)!; const toAmount = toBN(toBase(sendAmount.value, props.network.decimals)); await router.push({ name: RouterNames.verifySendFromSpark.name, query: { id: selected, txData: Buffer.from( JSON.stringify({ toToken: { amount: toAmount.toString(), decimals: availableAsset.value.decimals!, icon: availableAsset.value.icon as string, symbol: availableAsset.value.symbol, valueUSD: new BigNumber(availableAsset.value.price || "0") .times(sendAmount.value) .toString(), name: availableAsset.value.name, price: availableAsset.value.price || "0", }, fromAddress: fromAccountAddress, fromAddressName: "Spark Account", toAddress: addressTo.value, }), "utf8" ).toString("base64"), }, }); + } catch (error) { + console.error('Failed to send transaction:', error); + } finally { + isSubmitting.value = false; + } -}; +}, 1000, { leading: true, trailing: false });packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
107-137:⚠️ Potential issueAdd error handling for API calls in FiroToHaskoinUTXOs method.
The method makes multiple API calls without proper error handling.
async FiroToHaskoinUTXOs( FiroUTXOs: FiroUnspentType[], address: string ): Promise<HaskoinUnspentType[]> { const ret: HaskoinUnspentType[] = []; + try { for (const utx of FiroUTXOs) { - const rawTxRes = (await cacheFetch({ - url: `${this.node}/insight-api-zcoin/rawtx/${utx.txid}`, - })) as { rawtx: string }; - const res = (await cacheFetch({ - url: `${this.node}/insight-api-zcoin/tx/${utx.txid}`, - })) as FiroTxType; + try { + const rawTxRes = (await cacheFetch({ + url: `${this.node}/insight-api-zcoin/rawtx/${utx.txid}`, + })) as { rawtx: string }; + const res = (await cacheFetch({ + url: `${this.node}/insight-api-zcoin/tx/${utx.txid}`, + })) as FiroTxType; - ret.push({ - address, - block: { - height: res.blockheight, - position: 0, - }, - index: utx.vout, - pkscript: res.vout[utx.vout].scriptPubKey.hex, - txid: utx.txid, - value: Number(utx.satoshis), - raw: rawTxRes.rawtx, - }); + ret.push({ + address, + block: { + height: res.blockheight, + position: 0, + }, + index: utx.vout, + pkscript: res.vout[utx.vout].scriptPubKey.hex, + txid: utx.txid, + value: Number(utx.satoshis), + raw: rawTxRes.rawtx, + }); + } catch (error) { + console.error(`Failed to process UTXO ${utx.txid}:`, error); + // Skip this UTXO and continue with others + continue; + } } ret.sort((a, b) => { return a.value - b.value; }); return ret; + } catch (error) { + console.error('Failed to process UTXOs:', error); + throw error; + } }
🧹 Nitpick comments (6)
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (1)
176-185: Add cleanup for timeoutsTimeouts should be cleaned up when the component is unmounted to prevent memory leaks.
Add timeout cleanup:
+const timeoutRef = ref<number>(); if (getCurrentContext() === "popup") { - setTimeout(() => { + timeoutRef.value = window.setTimeout(() => { isProcessing.value = false; router.go(-2); }, 4500); } else { - setTimeout(() => { + timeoutRef.value = window.setTimeout(() => { isProcessing.value = false; window.close(); }, 1500); } +onBeforeUnmount(() => { + if (timeoutRef.value) { + window.clearTimeout(timeoutRef.value); + } +});packages/extension/src/libs/spark-handler/callRPC.ts (1)
40-43: Consider adding specific error handlingThe current error handling is generic. Consider adding specific handling for common RPC errors.
Example implementation:
} catch (error) { + if (axios.isAxiosError(error)) { + if (error.response?.status === 401) { + throw new Error('Unauthorized: Check RPC credentials'); + } + if (error.code === 'ECONNABORTED') { + throw new Error('RPC request timed out'); + } + } console.error("RPC Error:", error); throw error; }packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (2)
4-11: Add alt text for accessibilityThe avatar image lacks meaningful alt text for screen readers.
<img v-if=" isSparkAddress(btcAddress) || isAddress(btcAddress, props.network.networkInfo) " :src="network.identicon(btcAddress)" - alt="" + :alt="`${btcAddress} identicon`" />
15-30: Consider adding input validation feedbackThe input validation only shows red text color for invalid addresses. Consider adding a tooltip or helper text to explain why the address is invalid.
<input ref="addressInput" v-model="address" type="text" :disabled="disableDirectInput" placeholder="address" + :aria-invalid="!isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo)" :style="{ color: !isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo) ? 'red' : 'black', }" @focus="changeFocus" @blur="changeFocus" /> + <span v-if="btcAddress && !isSparkAddress(btcAddress) && !isAddress(btcAddress, props.network.networkInfo)" + class="invalid-feedback"> + Invalid address format + </span>packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (1)
87-89: Consider adding analytics for tab switchesSince the PR adds new UI interactions, consider tracking tab switches for analytics.
const setSelectedSendTab = (activeTab: "transparent" | "spark") => { selectedSendTab.value = activeTab; + trackSendEvents(SendEventType.SendTabSwitch, { + network: props.network.name, + tab: activeTab + }); };packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
31-32: Implement or document the empty init() method.The
init()method is currently empty. If it's intended to be implemented by consumers, consider adding a comment explaining its purpose or marking it as optional.- async init(): Promise<void> {} + /** + * Optional initialization method for API setup. + * Override this method if additional setup is needed. + */ + async init(): Promise<void> {}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
packages/extension/src/libs/spark-handler/callRPC.ts(1 hunks)packages/extension/src/libs/spark-handler/index.ts(1 hunks)packages/extension/src/providers/bitcoin/libs/api-firo.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo-testnet.ts(1 hunks)packages/extension/src/providers/bitcoin/networks/firo.ts(1 hunks)packages/extension/src/providers/bitcoin/types/bitcoin-network.ts(2 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue(4 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue(1 hunks)packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue(1 hunks)packages/extension/src/ui/action/App.vue(7 hunks)packages/extension/src/ui/action/router/index.ts(2 hunks)packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/extension/src/providers/bitcoin/networks/firo-testnet.ts
- packages/extension/src/libs/spark-handler/index.ts
- packages/extension/src/providers/bitcoin/types/bitcoin-network.ts
- packages/extension/src/ui/action/router/index.ts
- packages/extension/src/providers/bitcoin/networks/firo.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/providers/bitcoin/libs/api-firo.ts
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
packages/extension/src/libs/spark-handler/callRPC.ts (3)
36-39: LGTM: Response validation looks good
The implementation properly validates the RPC response structure before accessing the result.
5-7:
Add testnet URL configuration
The PR objectives mention support for both Mainnet and Testnet environments, but only the mainnet URL is configured.
Add testnet URL:
const RPC_URLS = {
mainnet: "https://firo-rpc.publicnode.com/",
+ testnet: "https://testnet-firo-rpc.publicnode.com/", // Replace with actual testnet URL
};Likely invalid or redundant comment.
21-34: 🛠️ Refactor suggestion
Update JSON-RPC implementation and remove duplicate headers
Several improvements needed in the RPC request implementation:
- Using outdated JSON-RPC 1.0
- Static request ID could cause issues with concurrent requests
- Headers are defined both in axios instance and request config
Apply these changes:
const response = await axiosInstance.post(
RPC_URLS['mainnet'],
{
- jsonrpc: "1.0",
+ jsonrpc: "2.0",
- id: "js-client",
+ id: crypto.randomUUID(), // Generate unique ID for each request
method: method,
params: params ?? [],
- },
- {
- headers: {
- "Content-Type": "application/json",
- },
}
);Likely invalid or redundant comment.
packages/extension/src/providers/bitcoin/ui/send-transaction/components/send-spark-address-input.vue (1)
43-53: LGTM: Modern Clipboard API implementation
The implementation correctly uses the modern Clipboard API with proper error handling.
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (1)
40-45: Verify sparkAccount null check
The component renders the Spark tab conditionally but still passes potentially null sparkAccount.
✅ Verification successful
The null check in the parent component is sufficient
The current implementation is correct for the following reasons:
- The parent component (
index.vue) has a proper guard conditionv-if="selectedSendTab === 'spark' && !!accountInfo.sparkAccount"that ensures the component is only rendered whensparkAccountexists - The
??operator in:spark-account="accountInfo?.sparkAccount ?? null"is redundant but harmless since the v-if condition already guaranteessparkAccountexists - The SparkSendTab component actively uses the sparkAccount prop throughout its implementation (e.g.,
sparkAccount.sparkBalance.availableBalance,sparkAccount.allAddresses) without any null checks because it relies on the parent's guard
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if sparkAccount is properly handled in SparkSendTab
ast-grep --pattern 'sparkAccount: {
type: $_,
required: true
}'
Length of output: 70
Script:
#!/bin/bash
# Let's try to find the SparkSendTab component definition and check its props
fd "SparkSendTab.vue" --exec cat {}
Length of output: 37
Script:
#!/bin/bash
# Let's try a broader search for the component file
fd -e vue --exec grep -l "SparkSendTab" {}
Length of output: 117
Script:
#!/bin/bash
# Let's check the SparkSendTab component import and its usage
rg -A 5 "SparkSendTab" ./packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue
Length of output: 287
Script:
#!/bin/bash
# Let's examine the spark-send-tab.vue component
cat ./packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue
Length of output: 8340
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/spark-send-tab.vue (3)
1-62: LGTM! Well-structured template with proper user feedback.
The template provides a clear user interface with:
- Proper input validation through disabled states
- Clear feedback through alerts
- Good separation of concerns between components
189-196: Improve precision handling in amount input.
The amount input handling could lead to precision loss when converting between string and number.
Apply this diff to use BigNumber consistently:
const inputAmount = (inputAmount: string) => {
if (inputAmount === "") {
inputAmount = "0";
}
const inputAmountBn = new BigNumber(inputAmount);
isMaxSelected.value = false;
- amount.value = inputAmountBn.lt(0) ? "0" : inputAmount;
+ amount.value = inputAmountBn.lt(0) ? "0" : inputAmountBn.toString();
};268-302: LGTM! Clean and well-organized styles.
The styles follow good practices with:
- Proper use of LESS
- Clear class naming
- Good organization of layout styles
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (4)
189-202: 🛠️ Refactor suggestion
Strengthen props validation to prevent runtime errors.
The props lack proper validation which could lead to runtime errors. Consider adding required flags and type validation.
const props = defineProps({
network: {
type: Object as PropType<BitcoinNetwork>,
- default: () => ({}),
+ required: true,
+ validator: (value: BitcoinNetwork) => {
+ return Boolean(value.name && value.decimals && value.networkInfo);
+ }
},
accountInfo: {
type: Object as PropType<AccountsHeaderData>,
- default: () => ({}),
+ required: true,
+ validator: (value: AccountsHeaderData) => {
+ return Boolean(value.selectedAccount);
+ }
},
isSendToken: {
type: Boolean,
default: () => false,
},
});Likely invalid or redundant comment.
311-334: 🛠️ Refactor suggestion
Enhance input validation for better security.
The current validation logic could be improved to prevent transactions with unreasonable fees and ensure proper amount validation.
const isInputsValid = computed<boolean>(() => {
+ // Check for minimum transaction amount
+ const minAmount = new BigNumber(props.network.dust);
+ if (new BigNumber(sendAmount.value).lt(minAmount)) {
+ return false;
+ }
+
+ // Check for maximum gas fee (e.g., 10% of balance)
+ const maxGasFee = new BigNumber(UTXOBalance.value).multipliedBy(0.1);
+ const currentGasFee = new BigNumber(gasCostValues.value[selectedFee.value].nativeValue);
+ if (currentGasFee.gt(maxGasFee)) {
+ return false;
+ }
+
if (
!isSparkAddress(addressTo.value) &&
!isAddress(addressTo.value, (props.network as BitcoinNetwork).networkInfo)
) {
return false;
}
// ... rest of the validation logic
});Likely invalid or redundant comment.
261-289: 🛠️ Refactor suggestion
Add error handling for fee calculation and UTXO updates.
The fee calculation and UTXO update logic lack proper error handling which could lead to silent failures.
const setTransactionFees = (byteSize: number) => {
+ if (byteSize <= 0) {
+ console.error('Invalid byte size for fee calculation');
+ return;
+ }
const nativeVal = selectedAsset.value.price || "0";
- getGasCostValues(
+ return getGasCostValues(
props.network as BitcoinNetwork,
byteSize,
nativeVal,
props.network.decimals,
props.network.currencyName
- ).then((val) => (gasCostValues.value = val));
+ ).then((val) => (gasCostValues.value = val))
+ .catch((error) => {
+ console.error('Failed to calculate gas costs:', error);
+ gasCostValues.value = defaultGasCostVals;
+ });
};
const updateUTXOs = async () => {
const api = (await props.network.api()) as BitcoinAPI;
- return api.getUTXOs(addressFrom.value).then((utxos) => {
+ try {
+ const utxos = await api.getUTXOs(addressFrom.value);
accountUTXOs.value = utxos;
const txSize = calculateSizeBasedOnType(
accountUTXOs.value.length + (props.isSendToken ? 0 : 1),
2,
(props.network as BitcoinNetwork).networkInfo.paymentType
);
setTransactionFees(Math.ceil(txSize));
- });
+ } catch (error) {
+ console.error('Failed to update UTXOs:', error);
+ accountUTXOs.value = [];
+ }
};Likely invalid or redundant comment.
478-524: 🛠️ Refactor suggestion
Enhance transaction security measures.
The transaction building process needs additional security checks to prevent potential issues with output values and change addresses.
const sendAction = async () => {
+ // Validate total output value doesn't exceed input value
+ const totalOutput = toAmount.add(currentFee);
+ if (totalOutput.gt(UTXOBalance.value)) {
+ throw new Error('Insufficient funds including fees');
+ }
+
// ... existing code ...
if (remainder.gtn(0)) {
+ // Check if remainder is above dust limit
+ if (remainder.lt(props.network.dust)) {
+ throw new Error('Change amount below dust limit');
+ }
+ // Validate change address
+ if (!isAddress(addressFrom.value, props.network.networkInfo)) {
+ throw new Error('Invalid change address');
+ }
txInfo.outputs.push({
address: props.network.displayAddress(addressFrom.value),
value: remainder.toNumber(),
});
}
};Likely invalid or redundant comment.
packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
74-87: LGTM! Good error handling in getBalance method.
The method properly handles errors and edge cases, including the conversion of balance to string.
packages/extension/src/ui/action/App.vue (2)
323-329: LGTM! Proper initialization of Spark account for Firo network.
The implementation correctly initializes the Spark account state when the Firo network is selected.
287-296: 🛠️ Refactor suggestion
Add concurrency protection to generateNewSparkAddress.
The method lacks protection against concurrent executions, which could lead to race conditions if triggered rapidly.
+let isGeneratingSparkAddress = false;
const generateNewSparkAddress = async () => {
+ if (isGeneratingSparkAddress) return;
+ try {
+ isGeneratingSparkAddress = true;
const newSparkAddressResponse = await generateSparkAddress();
if (accountHeaderData.value.sparkAccount) {
accountHeaderData.value.sparkAccount.defaultAddress =
newSparkAddressResponse;
accountHeaderData.value.sparkAccount.allAddresses.push(
newSparkAddressResponse
);
}
+ } finally {
+ isGeneratingSparkAddress = false;
+ }
};Likely invalid or redundant comment.
packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue
Outdated
Show resolved
Hide resolved
narekpetrosyan
force-pushed
the
feature/add-firo-network
branch
from
d819296 to
ce6ed47
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue (1)
324-352:⚠️ Potential issue | 🟡 MinorAdd explicit status labels for
spark_transaction.
With the new type, the current logic labels Spark activity as “Swapped/Swapping”. If Spark should read as a transfer (Sent/Received), add an explicit branch.💡 One way to align labels with transfer semantics
onMounted(() => { date.value = moment(props.activity.timestamp).fromNow(); + const isTransfer = + props.activity.type === ActivityType.transaction || + props.activity.type === ActivityType.spark_transaction; if ( props.activity.status === ActivityStatus.success && props.activity.isIncoming ) { - status.value = - props.activity.type === ActivityType.transaction ? 'Received' : 'Swapped'; + status.value = isTransfer ? 'Received' : 'Swapped'; } else if ( props.activity.status === ActivityStatus.success && !props.activity.isIncoming ) { - status.value = - props.activity.type === ActivityType.transaction ? 'Sent' : 'Swapped'; + status.value = isTransfer ? 'Sent' : 'Swapped'; } else if ( props.activity.status === ActivityStatus.pending && props.activity.isIncoming ) { - status.value = - props.activity.type === ActivityType.transaction - ? 'Receiving' - : 'Swapping'; + status.value = isTransfer ? 'Receiving' : 'Swapping'; } else if ( props.activity.status === ActivityStatus.pending && !props.activity.isIncoming ) { - status.value = - props.activity.type === ActivityType.transaction ? 'Sending' : 'Swapping'; + status.value = isTransfer ? 'Sending' : 'Swapping'; } else if (props.activity.status === ActivityStatus.dropped) { status.value = 'Dropped'; } else { status.value = 'Failed'; } });
🤖 Fix all issues with AI agents
In `@packages/extension/src/libs/spark-handler/getFee.ts`:
- Around line 6-12: The getFee function currently trusts
firoElectrum.estimateFee (called in getFee) and uses a forced "as number"
assertion; modify getFee to validate the returned feeRate from
firoElectrum.estimateFee: do not use the "as number" cast, treat the result as
unknown/number | null, check for -1 (or non-number) and either throw a clear
error or substitute a safe fallback fee rate (e.g., a configurable MIN_FEE_RATE
constant) before computing with BigNumber; ensure the rest of the function
(tempTx.virtualSize() and SATOSHI multiplication) uses the validated/fallback
numeric feeRate.
In
`@packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts`:
- Around line 250-279: The multiGetTransactionByTxid method assumes responses
are FullTransactionModel but when verbose is false Electrum returns raw hex
strings; update the function signature and internal types to reflect that
returned values are FullTransactionModel | string (change Promise<{ [txId:
string]: FullTransactionModel }> to Promise<{ [txId: string]:
FullTransactionModel | string }>) and adjust the local ret declaration to the
same union type; ensure any place referencing multiGetTransactionByTxid or the
ret object (including the mapping in the res.forEach) treats values as possibly
string (or narrows via typeof) so callers handle both verbose=true and
verbose=false cases.
In
`@packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue`:
- Around line 297-306: The computed sendButtonTitle currently uses
parseInt(sendAmount.value) which truncates decimals; change it to use
parseFloat(sendAmount.value) (or Number(sendAmount.value)) and guard against NaN
so decimal amounts like "0.5" are detected; update the logic in the
sendButtonTitle computed property (referencing sendAmount, selectedAsset, and
formatFloatingPointValue) to check parseFloat(sendAmount.value) > 0 and fall
back to 'Send' when the parsed value is not a positive number.
- Around line 275-295: Wrap both async operations in try/catch (or
try/catch/finally) to handle API failures: in updateUTXOs, await
props.network.api() and the subsequent getUTXOs call inside a try block, set
accountUTXOs.value on success and in catch log the error and set
accountUTXOs.value = [] (or a safe default) and setTransactionFees(0) to avoid
stale fees; ensure any loading flags or derived state updates still occur in a
finally block if used. For fetchAssets, set selectedAsset.value = loadingAsset
and isLoadingAssets.value = true before the call, then wrap
props.network.getAllTokens(addressFrom.value) in try/catch/finally—on success
assign selectedAsset.value = allAssets[0] as BTCToken, on error log it and set
selectedAsset.value to a safe default (e.g., null or loadingAsset), and always
set isLoadingAssets.value = false in finally. Use function/variable names
updateUTXOs, fetchAssets, accountUTXOs, selectedAsset, isLoadingAssets,
setTransactionFees, props.network.api, props.network.getAllTokens, and
calculateSizeBasedOnType to locate changes.
- Around line 502-522: The code assumes getTransactionStatus returned a value
and uses ordinalTx! which can be null; update the block that calls
api.getTransactionStatus(txid) (and subsequent uses of ordinalTx,
ordinalTx.outputs, ordinalTx.blockNumber) to explicitly check for null/undefined
and handle it (e.g., throw a clear error, return early, or show a user-facing
message) before accessing properties, and remove the non-null assertions; ensure
getBTCTxInfo and the rest of the flow only run when ordinalTx is valid and
include a descriptive error/log referencing the txid and selectedNft.value.id.
In `@packages/extension/src/types/activity.ts`:
- Around line 104-108: The polling logic in the network activity view only
checks for ActivityType.transaction and ActivityType.swap so
ActivityType.spark_transaction never gets status updates; update the condition
in packages/extension/src/ui/action/views/network-activity/index.vue where
ActivityType is checked (the block that currently references
ActivityType.transaction and ActivityType.swap) to also include
ActivityType.spark_transaction so spark transactions are polled, and change the
status label mapping used for spark_transaction from the swap labels
("Swapped"/"Swapping") to transaction-appropriate text (e.g., use the same
labels as ActivityType.transaction or a custom pair like
"Confirmed"/"Confirming") so UI displays correct messages.
In `@packages/extension/src/ui/action/composables/synchronize-spark-state.ts`:
- Around line 68-79: Remove the production debug console logs that leak
transaction data: eliminate the two console.log statements that reference
txIdsDecoded and sparkUnusedTxDetails in the synchronize-spark-state flow;
instead either delete them or wrap them behind a dev-only check using the
existing environment/dev flag (or a logger.isDebugEnabled) so that only
non-production builds output these values, keeping the logic that calls
firoElectrum.multiGetTransactionByTxid and sets sparkUnusedTxDetails.value
intact.
In
`@packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue`:
- Around line 214-226: The current async computed uses status.value (which is
display text) so comparisons to ActivityStatus.failed/dropped never match;
update the logic in the useAsyncComputed callback to compare the real enum
status (e.g., props.activity.status or a computed rawStatus that returns the
enum) instead of status.value, leaving the rest (isSparkAddress checks and
return expression in activityLinkDisabled) unchanged so the link disables when
both addresses are Spark and the actual enum status is failed or dropped.
🧹 Nitpick comments (6)
packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue (3)
119-128: Fragile click handler logic for send button.The ternary condition
!isAddress(addressTo, network.networkInfo) ? sendSparkAction : sendActioncould incorrectly route tosendSparkActionfor any invalid address, not just Spark addresses. WhileisInputsValidguards the button's disabled state, the logic should explicitly check for Spark addresses for clarity and safety.♻️ Proposed fix
<base-button :title="sendButtonTitle" - :click=" - !isAddress(addressTo, network.networkInfo) - ? sendSparkAction - : sendAction - " + :click="isSparkAddress(addressTo) ? sendSparkAction : sendAction" :disabled="!isInputsValid" />
443-475: Add error handling to sendSparkAction.The function lacks try-catch handling. If
keyring.getAccountor amount conversion fails, the user receives no feedback. Consider wrapping in try-catch with user-facing error notification.🛡️ Proposed fix
const sendSparkAction = async () => { + try { const keyring = new PublicKeyRing(); const fromAccountInfo = await keyring.getAccount(addressFrom.value); const toAmount = toBN(toBase(sendAmount.value, selectedAsset.value.decimals)); router.push({ name: RouterNames.verifySendToSpark.name, query: { // ... existing query params }, }); + } catch (error) { + console.error('Failed to prepare Spark transaction:', error); + // Consider emitting an error event or showing a toast notification + } };
23-41: Avoid hardcoded network name strings.The checks
network.name === 'Firo'andnetwork.name !== 'Firo'use a hardcoded string that could easily be misspelled or become inconsistent. Consider using a constant (e.g.,NetworkNames.firo) or a network type property for safer comparisons.♻️ Suggested approach
+import { NetworkNames } from '@/types/provider'; // or wherever defined - <send-spark-address-input - v-if="network.name === 'Firo'" + <send-spark-address-input + v-if="network.name === NetworkNames.firo"packages/extension/src/libs/spark-handler/getFee.ts (1)
9-11: Consider more granular fee calculation.Using
Math.ceil(tempTx.virtualSize() / 1000)rounds up to the nearest kilobyte, which means a 100-byte transaction pays the same fee as a 1000-byte transaction. This is a conservative approach but may result in overpayment for smaller transactions.If this is intentional for safety margin, consider adding a brief comment explaining the rationale. Otherwise, a more precise calculation would be:
♻️ More precise fee calculation
return new BigNumber(feeRate) - .times(Math.ceil(tempTx.virtualSize() / 1000)) + .times(tempTx.virtualSize()) + .dividedBy(1000) .times(SATOSHI);packages/extension/src/ui/action/composables/synchronize-spark-state.ts (1)
93-105: Terminate the worker after completion to avoid leaks.Each sync creates a new Worker and never terminates it. Terminate on completion (and handle errors) or reuse a shared worker.
♻️ Proposed fix
- worker.onmessage = () => { - coinFetchDone.value = true; - }; + worker.onmessage = () => { + coinFetchDone.value = true; + worker.terminate(); + }; + + worker.onerror = err => { + console.error('[synchronizeWorker] Worker error', err); + worker.terminate(); + };packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue (1)
7-12: Add explicit disabled semantics on the anchor.
Consider addingaria-disabledand disabling tab focus when the link is not actionable so assistive tech gets the same state.♿ Suggested accessibility tweak
<a :class="{ 'network-activity__activity-link-disabled': !isPending && activityLinkDisabled, }" :href="!isPending && activityLinkDisabled ? undefined : transactionURL" + :aria-disabled="!isPending && activityLinkDisabled" + :tabindex="!isPending && activityLinkDisabled ? -1 : 0" target="_blank" class="network-activity__transaction" >
| export const getFee = async (tempTx: bitcoin.Transaction) => { | ||
| const feeRate = (await firoElectrum.estimateFee(2)) as number; | ||
|
|
||
| return new BigNumber(feeRate) | ||
| .times(Math.ceil(tempTx.virtualSize() / 1000)) | ||
| .times(SATOSHI); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electrum estimateFee return value protocol behavior when estimation fails
💡 Result:
In the Electrum JSON-RPC / Stratum protocol, blockchain.estimatefee returns:
- On success: a floating-point number = estimated fee rate in coin units per kB (e.g., BTC/kB). (electrum.readthedocs.io)
- When fee estimation fails (not enough data): the call still succeeds at the protocol level, but the
resultis the integer-1(i.e., not an"error"object). (electrum.readthedocs.io)
Also note: blockchain.estimatefee is deprecated since protocol 1.4.2 (ElectrumX docs), so clients should be prepared for servers to discourage/replace its usage and instead use newer fee-estimation mechanisms (e.g., fee histogram). (electrumx.readthedocs.io)
Citations:
- 1: https://electrum.readthedocs.io/en/latest/protocol.html?utm_source=openai
- 2: https://electrum.readthedocs.io/en/latest/protocol.html?utm_source=openai
- 3: https://electrumx.readthedocs.io/en/latest/protocol-methods.html?utm_source=openai
🏁 Script executed:
# Search for firoElectrum implementation and usage
fd -t f -e ts -e js | xargs grep -l "firoElectrum\|estimateFee" | head -20Repository: enkryptcom/enKrypt
Length of output: 446
🏁 Script executed:
# Look at the actual file being reviewed
cat -n packages/extension/src/libs/spark-handler/getFee.tsRepository: enkryptcom/enKrypt
Length of output: 627
🏁 Script executed:
# Check firoElectrum implementation
cat -n packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts | head -100Repository: enkryptcom/enKrypt
Length of output: 3323
🏁 Script executed:
# Search for getFee usage
rg -n "getFee" packages/extension/src --type ts --type js -B 2 -A 2Repository: enkryptcom/enKrypt
Length of output: 10932
🏁 Script executed:
# Search for estimateFee method definition in the FiroElectrum class
rg -n "estimateFee" packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts -A 5Repository: enkryptcom/enKrypt
Length of output: 253
🏁 Script executed:
# Check the entire electrum-client.ts file to find estimateFee
wc -l packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.tsRepository: enkryptcom/enKrypt
Length of output: 146
🏁 Script executed:
# Get more context around the estimateFee implementation
sed -n '315,330p' packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.tsRepository: enkryptcom/enKrypt
Length of output: 504
🏁 Script executed:
# Check how feeBn is used after getFee is called
rg -n "await getFee" packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue -A 10Repository: enkryptcom/enKrypt
Length of output: 360
🏁 Script executed:
# Check the full context around feeBn usage in verify-send-to-spark-transaction
sed -n '190,210p' packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vueRepository: enkryptcom/enKrypt
Length of output: 566
🏁 Script executed:
# Also check the other file's usage
sed -n '165,175p' packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vueRepository: enkryptcom/enKrypt
Length of output: 206
🏁 Script executed:
# Check if there's any try-catch around getFee calls
rg -n "try|catch" packages/extension/src/ui/action/views/verify-send-to-spark-transaction/index.vue -B 5 -A 5 | grep -A 10 "getFee"Repository: enkryptcom/enKrypt
Length of output: 44
Add validation for failed fee estimation from Electrum.
Electrum's estimateFee method returns -1 when fee estimation fails (insufficient data). The current code passes this through without validation, resulting in a negative fee that causes incorrect balance checks and miscalculated change amounts.
Additionally, the as number type assertion bypasses TypeScript's type checking and could mask unexpected response types.
Suggested fix
export const getFee = async (tempTx: bitcoin.Transaction) => {
- const feeRate = (await firoElectrum.estimateFee(2)) as number;
+ const feeRate = await firoElectrum.estimateFee(2);
+
+ if (typeof feeRate !== 'number' || feeRate <= 0) {
+ throw new Error('Failed to estimate fee rate');
+ }
return new BigNumber(feeRate)
.times(Math.ceil(tempTx.virtualSize() / 1000))
.times(SATOSHI);
};Alternatively, use a fallback minimum fee rate when estimation fails.
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/spark-handler/getFee.ts` around lines 6 - 12, The
getFee function currently trusts firoElectrum.estimateFee (called in getFee) and
uses a forced "as number" assertion; modify getFee to validate the returned
feeRate from firoElectrum.estimateFee: do not use the "as number" cast, treat
the result as unknown/number | null, check for -1 (or non-number) and either
throw a clear error or substitute a safe fallback fee rate (e.g., a configurable
MIN_FEE_RATE constant) before computing with BigNumber; ensure the rest of the
function (tempTx.virtualSize() and SATOSHI multiplication) uses the
validated/fallback numeric feeRate.
| async multiGetTransactionByTxid( | ||
| txids: Array<string>, | ||
| batchsize: number = 45, | ||
| verbose?: boolean, | ||
| ): Promise<{ [txId: string]: FullTransactionModel }> { | ||
| verbose = verbose !== false; | ||
|
|
||
| const ret: { [txId: string]: FullTransactionModel } = {}; | ||
| txids = [...new Set(txids)]; | ||
|
|
||
| const chunks = splitIntoChunks(txids, batchsize); | ||
| for (const chunk of chunks) { | ||
| if (!this.mainClient) { | ||
| throw new Error('Electrum client is not connected'); | ||
| } | ||
|
|
||
| const res = await Promise.all( | ||
| chunk.map(el => | ||
| this.mainClient!.blockchain_transaction_get(el, verbose), | ||
| ), | ||
| ); | ||
|
|
||
| res.forEach((el: any, index: number) => { | ||
| const txid = chunk[index]; | ||
| ret[txid] = el; | ||
| }); | ||
| } | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the FullTransactionModel definition
rg "FullTransactionModel" -A 10 -B 2 packages/extension/src/providers/bitcoin/libs/Repository: enkryptcom/enKrypt
Length of output: 8189
🏁 Script executed:
# Examine the electrum-client file around the method
head -300 packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts | tail -100Repository: enkryptcom/enKrypt
Length of output: 3224
🏁 Script executed:
# Search for blockchain_transaction_get method to understand what it returns
rg "blockchain_transaction_get" -A 5 -B 2 packages/extension/src/providers/bitcoin/libs/Repository: enkryptcom/enKrypt
Length of output: 3548
🏁 Script executed:
# Find usages of multiGetTransactionByTxid to see how it's called
rg "multiGetTransactionByTxid" -B 3 -A 5 packages/extension/src/Repository: enkryptcom/enKrypt
Length of output: 1913
🏁 Script executed:
# Search for any documentation or comments about blockchain_transaction_get behavior
rg "blockchain_transaction_get" -B 5 -A 10 packages/extension/src/providers/bitcoin/Repository: enkryptcom/enKrypt
Length of output: 6181
🏁 Script executed:
# Look for mainClient type definition to understand blockchain_transaction_get signature
rg "mainClient" -A 3 packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts | head -40Repository: enkryptcom/enKrypt
Length of output: 1219
🏁 Script executed:
# Check if there are any tests or examples showing what blockchain_transaction_get returns
fd "\.test\.|\.spec\." packages/extension/src/providers/bitcoin/ | head -20Repository: enkryptcom/enKrypt
Length of output: 143
🏁 Script executed:
# Search for any type definitions or interfaces for the Electrum client
rg "interface.*blockchain_transaction_get|class.*mainClient|type.*mainClient" packages/extension/src/Repository: enkryptcom/enKrypt
Length of output: 44
🏁 Script executed:
# Find ElectrumClient class definition
fd "ElectrumClient" packages/extension/src/ -type f | grep -v node_modulesRepository: enkryptcom/enKrypt
Length of output: 232
🏁 Script executed:
# Search for ElectrumClient class definition
rg "class ElectrumClient|interface ElectrumClient" packages/extension/src/Repository: enkryptcom/enKrypt
Length of output: 44
🏁 Script executed:
# Check if ElectrumClient is imported from an external package
rg "from.*ElectrumClient|import.*ElectrumClient" packages/extension/src/Repository: enkryptcom/enKrypt
Length of output: 196
🏁 Script executed:
# Look for blockchain_transaction_get method definition
rg "blockchain_transaction_get" packages/extension/src/ -B 3 -A 5 | head -50Repository: enkryptcom/enKrypt
Length of output: 3858
🌐 Web query:
electrum-client-browser blockchain_transaction_get verbose parameter return type
💡 Result:
In the Electrum/ElectrumX protocol, blockchain.transaction.get(tx_hash, verbose=false) (often exposed by JS clients as blockchain_transaction_get(tx_hash, verbose?)) returns different types depending on verbose:
verbose = false(default): returns the raw transaction as a hex string. [1][2]verbose = true: returns a JSON object / dictionary (coin-daemon-specific), i.e., whatever the underlying coin daemon returns for a “verbose” raw transaction (for Bitcoin this typically resemblesbitcoind’sgetrawtransactionwithverbose=true). [1][2][3]
So the practical return type is a union like: Promise<string | Record<string, any>>. [1][2]
Sources: [1] ElectrumX protocol docs, blockchain.transaction.get return description. [2] Electrum protocol docs, same method/return behavior. [3] Example of verbose decoded-object behavior (getrawtransaction verbose=true).
🌐 Web query:
electrum RPC blockchain.transaction.get verbose=false returns hex string
💡 Result:
blockchain.transaction.get(tx_hash, verbose=false) is supposed to return the raw transaction as a hex string when verbose is false (this is the Electrum/ElectrumX protocol definition). [1][2][6]
If you want a decoded/verbose JSON object instead, you must call it with verbose=true (supported since protocol v1.2), e.g.:
{"id":0,"method":"blockchain.transaction.get","params":["<txid>", true]}and the result becomes a coin-daemon-style dictionary (decoded transaction). [1][2]
If you’re calling via JSON-RPC, make sure verbose is actually passed as the second element in the params array; otherwise many clients will effectively use the default (false) and you’ll only see the hex string. [5]
Sources: ElectrumX protocol docs for blockchain.transaction.get [1][2], Electrum protocol docs [6], StackExchange usage example [5].
🏁 Script executed:
# Check how getTxRaw is used (since it calls blockchain_transaction_get with verbose parameter)
rg "getTxRaw" -B 2 -A 5 packages/extension/src/Repository: enkryptcom/enKrypt
Length of output: 2538
🏁 Script executed:
# Verify the line that converts verbose parameter
sed -n '255,265p' packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.tsRepository: enkryptcom/enKrypt
Length of output: 379
Return type mismatches when verbose is false.
The method accepts an optional verbose parameter that can be set to false. When verbose=false, the Electrum protocol returns raw transaction hex strings instead of structured objects. The current return type only specifies FullTransactionModel and doesn't account for this case.
🔧 Suggested fix
async multiGetTransactionByTxid(
txids: Array<string>,
batchsize: number = 45,
verbose?: boolean,
-): Promise<{ [txId: string]: FullTransactionModel }> {
+): Promise<{ [txId: string]: FullTransactionModel | string }> {🤖 Prompt for AI Agents
In
`@packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts`
around lines 250 - 279, The multiGetTransactionByTxid method assumes responses
are FullTransactionModel but when verbose is false Electrum returns raw hex
strings; update the function signature and internal types to reflect that
returned values are FullTransactionModel | string (change Promise<{ [txId:
string]: FullTransactionModel }> to Promise<{ [txId: string]:
FullTransactionModel | string }>) and adjust the local ret declaration to the
same union type; ensure any place referencing multiGetTransactionByTxid or the
ret object (including the mapping in the res.forEach) treats values as possibly
string (or narrows via typeof) so callers handle both verbose=true and
verbose=false cases.
| const updateUTXOs = async () => { | ||
| const api = (await props.network.api()) as BitcoinAPI; | ||
| return api.getUTXOs(addressFrom.value).then(utxos => { | ||
| accountUTXOs.value = utxos; | ||
| const txSize = calculateSizeBasedOnType( | ||
| accountUTXOs.value.length + (props.isSendToken ? 0 : 1), | ||
| 2, | ||
| (props.network as BitcoinNetwork).networkInfo.paymentType, | ||
| ); | ||
| setTransactionFees(Math.ceil(txSize)); | ||
| }); | ||
| }; | ||
|
|
||
| const fetchAssets = () => { | ||
| selectedAsset.value = loadingAsset; | ||
| isLoadingAssets.value = true; | ||
| return props.network.getAllTokens(addressFrom.value).then(allAssets => { | ||
| selectedAsset.value = allAssets[0] as BTCToken; | ||
| isLoadingAssets.value = false; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add error handling to async operations.
Both updateUTXOs and fetchAssets lack error handling. If the API calls fail, the component will be left in an inconsistent state (loading indicators stuck, stale data, incorrect balances).
🛡️ Proposed fix for updateUTXOs
const updateUTXOs = async () => {
const api = (await props.network.api()) as BitcoinAPI;
- return api.getUTXOs(addressFrom.value).then(utxos => {
- accountUTXOs.value = utxos;
- const txSize = calculateSizeBasedOnType(
- accountUTXOs.value.length + (props.isSendToken ? 0 : 1),
- 2,
- (props.network as BitcoinNetwork).networkInfo.paymentType,
- );
- setTransactionFees(Math.ceil(txSize));
- });
+ return api.getUTXOs(addressFrom.value)
+ .then(utxos => {
+ accountUTXOs.value = utxos;
+ const txSize = calculateSizeBasedOnType(
+ accountUTXOs.value.length + (props.isSendToken ? 0 : 1),
+ 2,
+ (props.network as BitcoinNetwork).networkInfo.paymentType,
+ );
+ setTransactionFees(Math.ceil(txSize));
+ })
+ .catch(error => {
+ console.error('Failed to fetch UTXOs:', error);
+ accountUTXOs.value = [];
+ });
};📝 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 updateUTXOs = async () => { | |
| const api = (await props.network.api()) as BitcoinAPI; | |
| return api.getUTXOs(addressFrom.value).then(utxos => { | |
| accountUTXOs.value = utxos; | |
| const txSize = calculateSizeBasedOnType( | |
| accountUTXOs.value.length + (props.isSendToken ? 0 : 1), | |
| 2, | |
| (props.network as BitcoinNetwork).networkInfo.paymentType, | |
| ); | |
| setTransactionFees(Math.ceil(txSize)); | |
| }); | |
| }; | |
| const fetchAssets = () => { | |
| selectedAsset.value = loadingAsset; | |
| isLoadingAssets.value = true; | |
| return props.network.getAllTokens(addressFrom.value).then(allAssets => { | |
| selectedAsset.value = allAssets[0] as BTCToken; | |
| isLoadingAssets.value = false; | |
| }); | |
| }; | |
| const updateUTXOs = async () => { | |
| const api = (await props.network.api()) as BitcoinAPI; | |
| return api.getUTXOs(addressFrom.value) | |
| .then(utxos => { | |
| accountUTXOs.value = utxos; | |
| const txSize = calculateSizeBasedOnType( | |
| accountUTXOs.value.length + (props.isSendToken ? 0 : 1), | |
| 2, | |
| (props.network as BitcoinNetwork).networkInfo.paymentType, | |
| ); | |
| setTransactionFees(Math.ceil(txSize)); | |
| }) | |
| .catch(error => { | |
| console.error('Failed to fetch UTXOs:', error); | |
| accountUTXOs.value = []; | |
| }); | |
| }; | |
| const fetchAssets = () => { | |
| selectedAsset.value = loadingAsset; | |
| isLoadingAssets.value = true; | |
| return props.network.getAllTokens(addressFrom.value).then(allAssets => { | |
| selectedAsset.value = allAssets[0] as BTCToken; | |
| isLoadingAssets.value = false; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
`@packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue`
around lines 275 - 295, Wrap both async operations in try/catch (or
try/catch/finally) to handle API failures: in updateUTXOs, await
props.network.api() and the subsequent getUTXOs call inside a try block, set
accountUTXOs.value on success and in catch log the error and set
accountUTXOs.value = [] (or a safe default) and setTransactionFees(0) to avoid
stale fees; ensure any loading flags or derived state updates still occur in a
finally block if used. For fetchAssets, set selectedAsset.value = loadingAsset
and isLoadingAssets.value = true before the call, then wrap
props.network.getAllTokens(addressFrom.value) in try/catch/finally—on success
assign selectedAsset.value = allAssets[0] as BTCToken, on error log it and set
selectedAsset.value to a safe default (e.g., null or loadingAsset), and always
set isLoadingAssets.value = false in finally. Use function/variable names
updateUTXOs, fetchAssets, accountUTXOs, selectedAsset, isLoadingAssets,
setTransactionFees, props.network.api, props.network.getAllTokens, and
calculateSizeBasedOnType to locate changes.
| const sendButtonTitle = computed(() => { | ||
| let title = 'Send'; | ||
| if (parseInt(sendAmount.value) > 0) | ||
| title = | ||
| 'Send ' + | ||
| formatFloatingPointValue(sendAmount.value).value + | ||
| ' ' + | ||
| selectedAsset.value?.symbol!.toUpperCase(); | ||
| return title; | ||
| }); |
There was a problem hiding this comment.
Use parseFloat instead of parseInt for decimal amounts.
parseInt(sendAmount.value) truncates decimal values, so an amount like "0.5" evaluates to 0, preventing the dynamic button title from showing the amount.
🐛 Proposed fix
const sendButtonTitle = computed(() => {
let title = 'Send';
- if (parseInt(sendAmount.value) > 0)
+ if (parseFloat(sendAmount.value) > 0)
title =
'Send ' +
formatFloatingPointValue(sendAmount.value).value +
' ' +
selectedAsset.value?.symbol!.toUpperCase();
return title;
});📝 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 sendButtonTitle = computed(() => { | |
| let title = 'Send'; | |
| if (parseInt(sendAmount.value) > 0) | |
| title = | |
| 'Send ' + | |
| formatFloatingPointValue(sendAmount.value).value + | |
| ' ' + | |
| selectedAsset.value?.symbol!.toUpperCase(); | |
| return title; | |
| }); | |
| const sendButtonTitle = computed(() => { | |
| let title = 'Send'; | |
| if (parseFloat(sendAmount.value) > 0) | |
| title = | |
| 'Send ' + | |
| formatFloatingPointValue(sendAmount.value).value + | |
| ' ' + | |
| selectedAsset.value?.symbol!.toUpperCase(); | |
| return title; | |
| }); |
🤖 Prompt for AI Agents
In
`@packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue`
around lines 297 - 306, The computed sendButtonTitle currently uses
parseInt(sendAmount.value) which truncates decimals; change it to use
parseFloat(sendAmount.value) (or Number(sendAmount.value)) and guard against NaN
so decimal amounts like "0.5" are detected; update the logic in the
sendButtonTitle computed property (referencing sendAmount, selectedAsset, and
formatFloatingPointValue) to check parseFloat(sendAmount.value) > 0 and fall
back to 'Send' when the parsed value is not a positive number.
| const api = (await props.network.api()) as BitcoinAPI; | ||
| const [txid, index] = selectedNft.value.id.split(':'); | ||
| const ordinalTx = await api.getTransactionStatus(txid); | ||
| const ordinalOutput = ordinalTx!.outputs[parseInt(index)]; | ||
| txInfo = getBTCTxInfo(accountUTXOs.value, { | ||
| address: ordinalOutput.address, | ||
| block: { | ||
| height: ordinalTx!.blockNumber, | ||
| position: -1, // not needed | ||
| }, | ||
| index: parseInt(index), | ||
| pkscript: ordinalOutput.pkscript, | ||
| txid, | ||
| value: ordinalOutput.value, | ||
| }); | ||
| toAmount = toBN(ordinalOutput.value); | ||
| txInfo.outputs.push({ | ||
| address: addressTo.value, | ||
| value: ordinalOutput.value, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle potential null return from getTransactionStatus.
The non-null assertion ordinalTx!.outputs on line 505 will throw if getTransactionStatus returns null or undefined. Add explicit null checking for safer error handling.
🛡️ Proposed fix
} else {
const api = (await props.network.api()) as BitcoinAPI;
const [txid, index] = selectedNft.value.id.split(':');
const ordinalTx = await api.getTransactionStatus(txid);
+ if (!ordinalTx) {
+ throw new Error('Failed to fetch ordinal transaction');
+ }
- const ordinalOutput = ordinalTx!.outputs[parseInt(index)];
+ const ordinalOutput = ordinalTx.outputs[parseInt(index)];
+ if (!ordinalOutput) {
+ throw new Error('Invalid ordinal output index');
+ }
txInfo = getBTCTxInfo(accountUTXOs.value, {🤖 Prompt for AI Agents
In
`@packages/extension/src/providers/bitcoin/ui/send-transaction/tabs/transparent-send-tab.vue`
around lines 502 - 522, The code assumes getTransactionStatus returned a value
and uses ordinalTx! which can be null; update the block that calls
api.getTransactionStatus(txid) (and subsequent uses of ordinalTx,
ordinalTx.outputs, ordinalTx.blockNumber) to explicitly check for null/undefined
and handle it (e.g., throw a clear error, return early, or show a user-facing
message) before accessing properties, and remove the non-null assertions; ensure
getBTCTxInfo and the rest of the flow only run when ordinalTx is valid and
include a descriptive error/log referencing the txid and selectedNft.value.id.
| enum ActivityType { | ||
| spark_transaction = 'spark_transaction', | ||
| transaction = 'transaction', | ||
| swap = 'swap', | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'ActivityType\.' --type ts --type tsx -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 89
🏁 Script executed:
rg -n 'activity\.type' --type ts --type tsx -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 89
🏁 Script executed:
rg -n 'switch.*ActivityType|case.*spark_transaction|case.*transaction|case.*swap' --type ts --type tsx -A 3 -B 1Repository: enkryptcom/enKrypt
Length of output: 89
🏁 Script executed:
fd -e ts -e tsx | xargs grep -l 'ActivityType' | head -20Repository: enkryptcom/enKrypt
Length of output: 1435
🏁 Script executed:
rg -n 'ActivityType\.' -t ts -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 9615
🏁 Script executed:
rg -n 'activity\.type' -t ts -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 44
🏁 Script executed:
rg -n 'switch.*\(.*type|case.*transaction|case.*swap|case.*spark' -t ts -A 3 -B 1Repository: enkryptcom/enKrypt
Length of output: 4470
🏁 Script executed:
# Search for specific ActivityType enum values being handled
rg -n "(ActivityType\.spark_transaction|ActivityType\.transaction|ActivityType\.swap)" -t tsRepository: enkryptcom/enKrypt
Length of output: 2031
🏁 Script executed:
rg -n 'ActivityType\.swap' -t tsRepository: enkryptcom/enKrypt
Length of output: 44
🏁 Script executed:
rg -n 'ActivityType\[|spark_transaction|swap.*activity|activity.*swap' -t tsRepository: enkryptcom/enKrypt
Length of output: 149
🏁 Script executed:
# Search for activity type filtering or matching logic
rg -n 'type.*===|type.*==|includes.*type|filter.*type' -t ts -A 2 -B 2 | head -100Repository: enkryptcom/enKrypt
Length of output: 6535
🏁 Script executed:
# Look for the activity type definition and all its usages
rg -n 'enum ActivityType|type: ActivityType' -t ts | head -50Repository: enkryptcom/enKrypt
Length of output: 2163
🏁 Script executed:
# Search for activity list/view components and filtering
rg -n 'activity' -t ts | grep -i 'list\|view\|render\|display\|filter' | head -30Repository: enkryptcom/enKrypt
Length of output: 2591
🏁 Script executed:
# Look for activity-related UI components
fd -name '*activity*' -type f -t tsRepository: enkryptcom/enKrypt
Length of output: 292
🏁 Script executed:
# Search for where Activity type is consumed/filtered
rg -n 'Activity\[|activity\[|\.type.*===\|\.type.*==\|switch.*activity' -t ts -A 2 -B 2 | head -80Repository: enkryptcom/enKrypt
Length of output: 6993
🏁 Script executed:
# Check the activity.ts file to see full enum definition and interface
cat -n packages/extension/src/types/activity.ts | head -150Repository: enkryptcom/enKrypt
Length of output: 3990
🏁 Script executed:
# Search for swap activity creation or swap type handling
rg -n 'swap' packages/extension/src/types/activity.ts -iRepository: enkryptcom/enKrypt
Length of output: 180
🏁 Script executed:
# Search in swap-related files for activity type usage
rg -n 'ActivityType\.swap|type.*swap|activity.*swap' -t ts -i | head -30Repository: enkryptcom/enKrypt
Length of output: 1690
🏁 Script executed:
# Look for activity list or activity rendering components
fd activity packages/extension/src/ui -type fRepository: enkryptcom/enKrypt
Length of output: 232
🏁 Script executed:
# Search for where activity types might be filtered or matched
rg -n 'activity\.type|\.type.*===|\.type.*==|\[.*type\]' packages/extension/src/ui -t ts -A 2 -B 2 | head -100Repository: enkryptcom/enKrypt
Length of output: 2797
🏁 Script executed:
# Find activity-related components in UI
fd -type f -path "*/ui/*" -name "*activity*"Repository: enkryptcom/enKrypt
Length of output: 232
🏁 Script executed:
# Search for activity rendering or list components
rg -n 'Activity' packages/extension/src/ui -t ts | grep -i 'component\|view\|render\|list' | head -20Repository: enkryptcom/enKrypt
Length of output: 2564
🏁 Script executed:
# Look for the activity list component
find packages/extension/src/ui -name "*activity*" -o -name "*history*"Repository: enkryptcom/enKrypt
Length of output: 559
🏁 Script executed:
# Search for potential type-based filtering or matching in activity handling
rg -n "swap.*type|transaction.*type|ActivityType\.|activity.*filter" packages/extension/src/ui -t ts -B 2 -A 2 | head -60Repository: enkryptcom/enKrypt
Length of output: 1087
🏁 Script executed:
# Examine the activity transaction rendering component
cat packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vueRepository: enkryptcom/enKrypt
Length of output: 13238
🏁 Script executed:
# Check the activity action component
cat packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vueRepository: enkryptcom/enKrypt
Length of output: 2863
🏁 Script executed:
# Look for activity type related logic in network activity components
rg -n 'type.*===|type.*==|swap|spark|transaction' packages/extension/src/ui/action/views/network-activity -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 42439
🏁 Script executed:
# Search for any filtering or switch statements on activity type in the entire UI
rg -n 'activity\.type|activity\[.type.\]|ActivityType\.' packages/extension/src/ui -A 2 -B 2Repository: enkryptcom/enKrypt
Length of output: 10214
Update activity status polling to include spark_transaction type and fix status message text.
The new spark_transaction enum value is handled correctly in UI rendering (all three types have explicit template sections), but there's a critical gap in activity status polling. In packages/extension/src/ui/action/views/network-activity/index.vue (lines 306–317), only ActivityType.transaction and ActivityType.swap activities trigger status checks. Activities with type spark_transaction won't receive status updates from the network, leaving them permanently in pending state.
Additionally, spark transactions currently use the swap status messages ("Swapped"/"Swapping") instead of transaction-appropriate text. Add spark_transaction to the polling condition and consider custom status labels for spark transactions.
🤖 Prompt for AI Agents
In `@packages/extension/src/types/activity.ts` around lines 104 - 108, The polling
logic in the network activity view only checks for ActivityType.transaction and
ActivityType.swap so ActivityType.spark_transaction never gets status updates;
update the condition in
packages/extension/src/ui/action/views/network-activity/index.vue where
ActivityType is checked (the block that currently references
ActivityType.transaction and ActivityType.swap) to also include
ActivityType.spark_transaction so spark transactions are polled, and change the
status label mapping used for spark_transaction from the swap labels
("Swapped"/"Swapping") to transaction-appropriate text (e.g., use the same
labels as ActivityType.transaction or a custom pair like
"Confirmed"/"Confirming") so UI displays correct messages.
packages/extension/src/ui/action/composables/synchronize-spark-state.ts
Outdated
Show resolved
Hide resolved
| const { value: activityLinkDisabled, isPending } = useAsyncComputed( | ||
| async () => { | ||
| const isStatusFailed = | ||
| status.value === ActivityStatus.failed || | ||
| status.value === ActivityStatus.dropped; | ||
|
|
||
| const [isFromSpark, isToSpark] = await Promise.all([ | ||
| isSparkAddress(props.activity.from), | ||
| isSparkAddress(props.activity.to), | ||
| ]); | ||
|
|
||
| return isFromSpark && isToSpark && isStatusFailed; | ||
| }, |
There was a problem hiding this comment.
Fix activityLinkDisabled to use the real status (current logic never matches).
status.value is display text (“Failed/Dropped”), so the comparison to ActivityStatus.failed/dropped is always false. This prevents the link from ever disabling.
🐛 Suggested fix
const { value: activityLinkDisabled, isPending } = useAsyncComputed(
async () => {
- const isStatusFailed =
- status.value === ActivityStatus.failed ||
- status.value === ActivityStatus.dropped;
+ const isStatusFailed =
+ props.activity.status === ActivityStatus.failed ||
+ props.activity.status === ActivityStatus.dropped;
+
+ if (props.activity.type !== ActivityType.spark_transaction) return false;
const [isFromSpark, isToSpark] = await Promise.all([
isSparkAddress(props.activity.from),
isSparkAddress(props.activity.to),
]);
return isFromSpark && isToSpark && isStatusFailed;
},
false,
);📝 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 { value: activityLinkDisabled, isPending } = useAsyncComputed( | |
| async () => { | |
| const isStatusFailed = | |
| status.value === ActivityStatus.failed || | |
| status.value === ActivityStatus.dropped; | |
| const [isFromSpark, isToSpark] = await Promise.all([ | |
| isSparkAddress(props.activity.from), | |
| isSparkAddress(props.activity.to), | |
| ]); | |
| return isFromSpark && isToSpark && isStatusFailed; | |
| }, | |
| const { value: activityLinkDisabled, isPending } = useAsyncComputed( | |
| async () => { | |
| const isStatusFailed = | |
| props.activity.status === ActivityStatus.failed || | |
| props.activity.status === ActivityStatus.dropped; | |
| if (props.activity.type !== ActivityType.spark_transaction) return false; | |
| const [isFromSpark, isToSpark] = await Promise.all([ | |
| isSparkAddress(props.activity.from), | |
| isSparkAddress(props.activity.to), | |
| ]); | |
| return isFromSpark && isToSpark && isStatusFailed; | |
| }, |
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/network-activity-transaction.vue`
around lines 214 - 226, The current async computed uses status.value (which is
display text) so comparisons to ActivityStatus.failed/dropped never match;
update the logic in the useAsyncComputed callback to compare the real enum
status (e.g., props.activity.status or a computed rawStatus that returns the
enum) instead of status.value, leaving the rest (isSparkAddress checks and
return expression in activityLinkDisabled) unchanged so the link disables when
both addresses are Spark and the actual enum status is failed or dropped.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/extension/src/ui/action/db/indexedDB.ts`:
- Around line 150-151: The code accesses data[index] without checking whether
data is undefined — update the method that calls this.readData<Record<number,
any>>(key) (same pattern used in appendSetData) to first verify the returned
value: if (!data) throw a clear Error (e.g., 'No data for key') or handle the
missing case, then check if (data[index] === undefined) throw new Error('Invalid
index'); ensure you reference this.readData, the local variables key and index,
and mirror the same null/undefined guard you added in appendSetData.
- Around line 108-109: The code calls this.readData<Record<number, any>>(key)
and then accesses data[index] without ensuring data is not null/undefined;
update the logic in the function that contains the lines with readData and the
subsequent check so you first verify data is present (e.g., data ===
null/undefined) and only then validate the index exists (e.g., using index in
data or checking data[index] !== undefined) before throwing the existing new
Error('Invalid index'); this ensures no TypeError occurs when readData returns
undefined.
In
`@packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue`:
- Around line 107-110: isWindowPopup is hardcoded to false so SendProcess never
sees window-popup mode; initialize it from context instead. Replace the static
ref(false) for isWindowPopup with a context-based initialization using
getCurrentContext()/the existing isPopup value (i.e., set isWindowPopup to
ref(getCurrentContext() === 'new-window') or ref(isPopup)) so SendProcess
receives the correct window-popup flag; update any usages that expect
isWindowPopup accordingly (symbols: isWindowPopup, isPopup, getCurrentContext,
SendProcess).
- Around line 114-116: The current onBeforeMount uses a non-null assertion on
getNetworkByName which can return undefined and later cause network.value.api()
to throw; update the onBeforeMount handler to explicitly check the result of
getNetworkByName(selectedNetwork) (instead of using !) and if it is undefined,
call the component's close/exit path (e.g., close dialog, emit cancel, or
navigate back), log or surface an error, and return early so that
trackSendEvents and any later uses of network.value (like network.value.api())
only run when network is defined.
🧹 Nitpick comments (1)
packages/extension/src/ui/action/App.vue (1)
261-280: Remove commented-out code block.This dead code should be removed. If needed later, it can be retrieved from version control.
- // const activityAddress = network.displayAddress(selectedAccount.address); - // const token = { - // name: network.name_long, - // symbol: network.currencyName, - // icon: network.icon, - // decimals: network.decimals, - // price: '0', - // }; - // const activities = details.map(tx => - // mapFiroExplorerTxToActivity( - // tx, - // activityAddress, - // network.name, - // token, - // ), - // ); - // void activityState.addActivities(activities, { - // address: activityAddress, - // network: network.name, - // });
| const data = await this.readData<Record<number, any>>(key); | ||
| if (!data[index]) throw new Error('Invalid index'); |
There was a problem hiding this comment.
Missing null check before accessing data[index].
If readData returns undefined (key not found), accessing data[index] throws a TypeError before the "Invalid index" check runs.
🛡️ Proposed fix
async appendSetData(
key: string,
index: number,
input: {
coins: string[][];
setHash: string;
blockHash: string;
},
): Promise<void> {
const data = await this.readData<Record<number, any>>(key);
- if (!data[index]) throw new Error('Invalid index');
+ if (!data || !data[index]) throw new Error('Invalid index');
data[index].blockHash = input.blockHash;🤖 Prompt for AI Agents
In `@packages/extension/src/ui/action/db/indexedDB.ts` around lines 108 - 109, The
code calls this.readData<Record<number, any>>(key) and then accesses data[index]
without ensuring data is not null/undefined; update the logic in the function
that contains the lines with readData and the subsequent check so you first
verify data is present (e.g., data === null/undefined) and only then validate
the index exists (e.g., using index in data or checking data[index] !==
undefined) before throwing the existing new Error('Invalid index'); this ensures
no TypeError occurs when readData returns undefined.
| const data = await this.readData<Record<number, any>>(key); | ||
| if (!data[index]) throw new Error('Invalid index'); |
There was a problem hiding this comment.
Missing null check before accessing data[index].
Same issue as appendSetData - readData may return undefined.
🛡️ Proposed fix
async removeSector(
key: string,
index: number,
startIndex: number,
endIndex: number,
): Promise<void> {
const data = await this.readData<Record<number, any>>(key);
- if (!data[index]) throw new Error('Invalid index');
+ if (!data || !data[index]) throw new Error('Invalid index');📝 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 data = await this.readData<Record<number, any>>(key); | |
| if (!data[index]) throw new Error('Invalid index'); | |
| async removeSector( | |
| key: string, | |
| index: number, | |
| startIndex: number, | |
| endIndex: number, | |
| ): Promise<void> { | |
| const data = await this.readData<Record<number, any>>(key); | |
| if (!data || !data[index]) throw new Error('Invalid index'); |
🤖 Prompt for AI Agents
In `@packages/extension/src/ui/action/db/indexedDB.ts` around lines 150 - 151, The
code accesses data[index] without checking whether data is undefined — update
the method that calls this.readData<Record<number, any>>(key) (same pattern used
in appendSetData) to first verify the returned value: if (!data) throw a clear
Error (e.g., 'No data for key') or handle the missing case, then check if
(data[index] === undefined) throw new Error('Invalid index'); ensure you
reference this.readData, the local variables key and index, and mirror the same
null/undefined guard you added in appendSetData.
| const isPopup: boolean = getCurrentContext() === 'new-window'; | ||
| const verifyScrollRef = ref<ComponentPublicInstance<HTMLElement>>(); | ||
| const isWindowPopup = ref(false); | ||
| const errorMsg = ref(''); |
There was a problem hiding this comment.
Initialize isWindowPopup from context.
It’s always false right now, so SendProcess never receives window-popup mode even when the view is opened as a new window.
🛠️ Suggested init
-const isWindowPopup = ref(false);
+const isWindowPopup = ref(getCurrentContext() === 'new-window');📝 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 isPopup: boolean = getCurrentContext() === 'new-window'; | |
| const verifyScrollRef = ref<ComponentPublicInstance<HTMLElement>>(); | |
| const isWindowPopup = ref(false); | |
| const errorMsg = ref(''); | |
| const isPopup: boolean = getCurrentContext() === 'new-window'; | |
| const verifyScrollRef = ref<ComponentPublicInstance<HTMLElement>>(); | |
| const isWindowPopup = ref(getCurrentContext() === 'new-window'); | |
| const errorMsg = ref(''); |
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue`
around lines 107 - 110, isWindowPopup is hardcoded to false so SendProcess never
sees window-popup mode; initialize it from context instead. Replace the static
ref(false) for isWindowPopup with a context-based initialization using
getCurrentContext()/the existing isPopup value (i.e., set isWindowPopup to
ref(getCurrentContext() === 'new-window') or ref(isPopup)) so SendProcess
receives the correct window-popup flag; update any usages that expect
isWindowPopup accordingly (symbols: isWindowPopup, isPopup, getCurrentContext,
SendProcess).
| onBeforeMount(async () => { | ||
| network.value = (await getNetworkByName(selectedNetwork)!) as BitcoinNetwork; | ||
| trackSendEvents(SendEventType.SendVerify, { network: network.value.name }); |
There was a problem hiding this comment.
Handle unknown network IDs instead of relying on non-null assertion.
If getNetworkByName returns undefined, later calls like network.value.api() will throw. Add an explicit guard and exit/close cleanly.
✅ Safer guard
- network.value = (await getNetworkByName(selectedNetwork)!) as BitcoinNetwork;
+ const resolved = await getNetworkByName(selectedNetwork);
+ if (!resolved) {
+ errorMsg.value = `Unknown network: ${selectedNetwork}`;
+ router.go(-1);
+ return;
+ }
+ network.value = resolved as BitcoinNetwork;📝 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.
| onBeforeMount(async () => { | |
| network.value = (await getNetworkByName(selectedNetwork)!) as BitcoinNetwork; | |
| trackSendEvents(SendEventType.SendVerify, { network: network.value.name }); | |
| onBeforeMount(async () => { | |
| const resolved = await getNetworkByName(selectedNetwork); | |
| if (!resolved) { | |
| errorMsg.value = `Unknown network: ${selectedNetwork}`; | |
| router.go(-1); | |
| return; | |
| } | |
| network.value = resolved as BitcoinNetwork; | |
| trackSendEvents(SendEventType.SendVerify, { network: network.value.name }); |
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue`
around lines 114 - 116, The current onBeforeMount uses a non-null assertion on
getNetworkByName which can return undefined and later cause network.value.api()
to throw; update the onBeforeMount handler to explicitly check the result of
getNetworkByName(selectedNetwork) (instead of using !) and if it is undefined,
call the component's close/exit path (e.g., close dialog, emit cancel, or
navigate back), log or surface an error, and return early so that
trackSendEvents and any later uses of network.value (like network.value.api())
only run when network is defined.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts`:
- Around line 57-69: Replace the literal 'myCoins' string with the
DB_DATA_KEYS.myCoins constant inside getMyCoinHashes: locate the getMyCoinHashes
function and change the db.readData call to use DB_DATA_KEYS.myCoins so it
matches other usages (e.g., DB_DATA_KEYS.sets) and prevents key mismatches;
ensure the imported DB_DATA_KEYS is used and the rest of the function logic
remains unchanged.
- Around line 213-217: The run() function currently calls scheduleNext()
immediately when it detects an overlap via isRunning, but the finally block also
calls scheduleNext(), creating redundant scheduling; remove the early
scheduleNext() inside the isRunning check so the function simply returns early
when isRunning is true and let the existing finally block (in run) handle
scheduling the next execution after the current run completes.
- Around line 158-166: The merge currently uses differenceSets (variable
updatedCoinsSet) which compares array elements by reference and always fails;
also the merge runs before checking isFullReplacement so full replacements get
incorrect logic. Change the logic in updateCoinSet to: if isFullReplacement, set
localSets[index].coins = newCoins (no merge); otherwise merge existing
localSets[index].coins with newCoins by mapping each coin to its stable id
coin[0], deduplicating by that hash, and then assign the deduplicated array back
to localSets[index] (update blockHash/setHash as done now). Replace the
differenceSets usage with this hash-based merge/dedupe and ensure you reference
updatedCoinsSet, localSets, newCoins, and isFullReplacement when editing the
code.
In `@packages/extension/src/ui/action/composables/synchronize-spark-state.ts`:
- Around line 72-79: The call to firoElectrum.multiGetTransactionByTxid in the
block that uses txIdsDecoded can throw and leave state inconsistent because
markCoinsAsUsed is fire-and-forget; wrap the await
firoElectrum.multiGetTransactionByTxid(txIdsDecoded) call in a try/catch, catch
and log the error (or surface to a logger), and ensure
sparkUnusedTxDetails.value is set to an empty array on failure so state remains
consistent; update the code references in this block (txIdsDecoded,
firoElectrum.multiGetTransactionByTxid, sparkUnusedTxDetails) accordingly and
avoid letting the rejection escape.
- Around line 95-107: synchronizeWorker currently allocates a new Worker each
call and never cleans it up; store the Worker instance in a module/local ref
(e.g., activeWorker) so you can terminate any existing worker before creating a
new one, call activeWorker.terminate() inside the worker.onmessage handler after
setting coinFetchDone.value = true to stop the worker when done, and register a
Vue lifecycle cleanup (onBeforeUnmount) to terminate the active worker when the
component unmounts; also remove or replace event handlers on the old worker
before discarding it to avoid retained references.
| const getMyCoinHashes = async (): Promise<Set<string>> => { | ||
| try { | ||
| const myCoins = await db.readData<StoredCoin[]>('myCoins'); | ||
| if (!Array.isArray(myCoins)) { | ||
| return new Set(); | ||
| } | ||
| return new Set( | ||
| myCoins | ||
| .map(entry => entry?.coin?.[0]) | ||
| .filter( | ||
| (hash): hash is string => typeof hash === 'string' && hash.length > 0, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DB_DATA_KEYS definition and usage
rg -n "DB_DATA_KEYS" --type ts --type tsx -A 3 -B 1 | head -100Repository: enkryptcom/enKrypt
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for 'myCoins' usage patterns
rg -n "'myCoins'" --type ts --type tsx -C 2Repository: enkryptcom/enKrypt
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for the specific file mentioned in the review
cat -n "packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts" 2>/dev/null | sed -n '50,75p'Repository: enkryptcom/enKrypt
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Search for DB_DATA_KEYS definition without type filter
rg -n "DB_DATA_KEYS" -A 5 | head -50Repository: enkryptcom/enKrypt
Length of output: 4650
🏁 Script executed:
#!/bin/bash
# Search for 'myCoins' usage pattern
rg -n "myCoins" -C 2 | head -80Repository: enkryptcom/enKrypt
Length of output: 6560
🏁 Script executed:
#!/bin/bash
# Look for constants file or DB keys definitions
find . -type f -name "*DB*" -o -name "*constants*" -o -name "*keys*" | grep -E "\.(ts|js)$" | head -20Repository: enkryptcom/enKrypt
Length of output: 318
Use the DB_DATA_KEYS.myCoins constant instead of a literal string.
The file already imports DB_DATA_KEYS and consistently uses it for other keys like DB_DATA_KEYS.sets (lines 41, 169). The constant is defined in packages/extension/src/ui/action/db/indexedDB.ts (line 8) and should be used here for consistency and to prevent silent mismatches if the key is ever renamed.
Suggested change
- const myCoins = await db.readData<StoredCoin[]>('myCoins');
+ const myCoins = await db.readData<StoredCoin[]>(DB_DATA_KEYS.myCoins);📝 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 getMyCoinHashes = async (): Promise<Set<string>> => { | |
| try { | |
| const myCoins = await db.readData<StoredCoin[]>('myCoins'); | |
| if (!Array.isArray(myCoins)) { | |
| return new Set(); | |
| } | |
| return new Set( | |
| myCoins | |
| .map(entry => entry?.coin?.[0]) | |
| .filter( | |
| (hash): hash is string => typeof hash === 'string' && hash.length > 0, | |
| ), | |
| ); | |
| const getMyCoinHashes = async (): Promise<Set<string>> => { | |
| try { | |
| const myCoins = await db.readData<StoredCoin[]>(DB_DATA_KEYS.myCoins); | |
| if (!Array.isArray(myCoins)) { | |
| return new Set(); | |
| } | |
| return new Set( | |
| myCoins | |
| .map(entry => entry?.coin?.[0]) | |
| .filter( | |
| (hash): hash is string => typeof hash === 'string' && hash.length > 0, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` around
lines 57 - 69, Replace the literal 'myCoins' string with the
DB_DATA_KEYS.myCoins constant inside getMyCoinHashes: locate the getMyCoinHashes
function and change the db.readData call to use DB_DATA_KEYS.myCoins so it
matches other usages (e.g., DB_DATA_KEYS.sets) and prevents key mismatches;
ensure the imported DB_DATA_KEYS is used and the rest of the function logic
remains unchanged.
| const updatedCoinsSet = differenceSets( | ||
| new Set(localSets?.[index]?.coins ?? []), | ||
| new Set(newCoins), | ||
| ); | ||
| localSets[index] = { | ||
| blockHash: remoteMeta.blockHash, | ||
| setHash: remoteMeta.setHash, | ||
| coins: Array.from(updatedCoinsSet), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the set-utils file with differenceSets
find . -type f -name "*set-utils*" -o -name "*set*.ts" | head -20Repository: enkryptcom/enKrypt
Length of output: 1569
🏁 Script executed:
#!/bin/bash
# Read the updateCoinSet.ts file
cat -n packages/extension/src/libs/utils/updateAndSync/updateCoinSet.tsRepository: enkryptcom/enKrypt
Length of output: 8043
🏁 Script executed:
#!/bin/bash
# Search for differenceSets implementation
rg "differenceSets" --type ts -B 2 -A 5Repository: enkryptcom/enKrypt
Length of output: 5476
🏁 Script executed:
#!/bin/bash
# Verify differenceSets implementation details
rg "const differenceSets" -A 12 packages/extension/src/ui/action/utils/set-utils.tsRepository: enkryptcom/enKrypt
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or usage patterns showing expected behavior
rg "differenceSets" -B 3 -A 3 packages/extension/src/ui/action/workers/sparkCoinInfoWorker.tsRepository: enkryptcom/enKrypt
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Verify how coins are structured and accessed
grep -n "coin\[0\]" packages/extension/src/libs/utils/updateAndSync/updateCoinSet.tsRepository: enkryptcom/enKrypt
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there's any special handling or serialization of coins
rg "StoredAnonymitySet|coins:" packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts -B 1 -A 1Repository: enkryptcom/enKrypt
Length of output: 870
🏁 Script executed:
#!/bin/bash
# Look at how coins are processed and what coin[0] represents
rg "coin\?" packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts -B 2 -A 2Repository: enkryptcom/enKrypt
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Check matchedCoins filtering to understand coin structure
sed -n '176,178p' packages/extension/src/libs/utils/updateAndSync/updateCoinSet.tsRepository: enkryptcom/enKrypt
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Examine the full context of how isFullReplacement is used
sed -n '145,175p' packages/extension/src/libs/utils/updateAndSync/updateCoinSet.tsRepository: enkryptcom/enKrypt
Length of output: 941
🏁 Script executed:
#!/bin/bash
# Check if there are similar merge patterns in updateTagsSet.ts for comparison
cat packages/extension/src/libs/utils/updateAndSync/updateTagsSet.ts | head -100Repository: enkryptcom/enKrypt
Length of output: 2490
Fix coin-set merging: differenceSets with array reference comparison breaks merge logic and risks data loss.
When merging coin sets with differenceSets, the function compares Set<string[]> elements by reference equality. Since localSets[index].coins and newCoins are different array instances, the Set comparison always fails, rendering the difference operation semantically incorrect. Additionally, the merge is applied unconditionally before checking isFullReplacement (line 168), so both full replacements and incremental syncs use the wrong logic. For incremental updates, this should combine old and new coins; for full replacements, use only new coins.
Merge and deduplicate by coin hash (the stable identifier at coin[0]) instead:
🛠️ Proposed fix (merge by coin hash)
- const updatedCoinsSet = differenceSets(
- new Set(localSets?.[index]?.coins ?? []),
- new Set(newCoins),
- );
- localSets[index] = {
- blockHash: remoteMeta.blockHash,
- setHash: remoteMeta.setHash,
- coins: Array.from(updatedCoinsSet),
- };
+ const baseCoins = isFullReplacement ? [] : (localSets[index]?.coins ?? []);
+ const dedupedByHash = new Map<string, string[]>();
+ for (const coin of [...baseCoins, ...newCoins]) {
+ const hash = coin?.[0];
+ if (hash) dedupedByHash.set(hash, coin);
+ }
+ localSets[index] = {
+ blockHash: remoteMeta.blockHash,
+ setHash: remoteMeta.setHash,
+ coins: Array.from(dedupedByHash.values()),
+ };📝 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 updatedCoinsSet = differenceSets( | |
| new Set(localSets?.[index]?.coins ?? []), | |
| new Set(newCoins), | |
| ); | |
| localSets[index] = { | |
| blockHash: remoteMeta.blockHash, | |
| setHash: remoteMeta.setHash, | |
| coins: Array.from(updatedCoinsSet), | |
| }; | |
| const baseCoins = isFullReplacement ? [] : (localSets[index]?.coins ?? []); | |
| const dedupedByHash = new Map<string, string[]>(); | |
| for (const coin of [...baseCoins, ...newCoins]) { | |
| const hash = coin?.[0]; | |
| if (hash) dedupedByHash.set(hash, coin); | |
| } | |
| localSets[index] = { | |
| blockHash: remoteMeta.blockHash, | |
| setHash: remoteMeta.setHash, | |
| coins: Array.from(dedupedByHash.values()), | |
| }; |
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` around
lines 158 - 166, The merge currently uses differenceSets (variable
updatedCoinsSet) which compares array elements by reference and always fails;
also the merge runs before checking isFullReplacement so full replacements get
incorrect logic. Change the logic in updateCoinSet to: if isFullReplacement, set
localSets[index].coins = newCoins (no merge); otherwise merge existing
localSets[index].coins with newCoins by mapping each coin to its stable id
coin[0], deduplicating by that hash, and then assign the deduplicated array back
to localSets[index] (update blockHash/setHash as done now). Replace the
differenceSets usage with this hash-based merge/dedupe and ensure you reference
updatedCoinsSet, localSets, newCoins, and isFullReplacement when editing the
code.
| const run = async () => { | ||
| if (isRunning) { | ||
| scheduleNext(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to understand the full function context
cat -n packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts | head -250 | tail -100Repository: enkryptcom/enKrypt
Length of output: 2995
🏁 Script executed:
# Verify there are no other calls to scheduleNext that might be affected
rg -A 5 -B 5 "scheduleNext" packages/extension/src/libs/utils/updateAndSync/updateCoinSet.tsRepository: enkryptcom/enKrypt
Length of output: 510
Remove redundant scheduleNext() call in overlap case; let the finally block handle scheduling the next run.
When run() detects an overlap (isRunning is true), calling scheduleNext() schedules the next execution. However, this gets immediately overwritten when the finally block calls scheduleNext() again after the current run completes. The redundant call adds unnecessary scheduling logic without benefit. Simply return early and let the finally block schedule the next run at the proper interval after the current execution finishes.
🛠️ Proposed fix
- if (isRunning) {
- scheduleNext();
- return;
- }
+ if (isRunning) {
+ return;
+ }📝 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 run = async () => { | |
| if (isRunning) { | |
| scheduleNext(); | |
| return; | |
| } | |
| const run = async () => { | |
| if (isRunning) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` around
lines 213 - 217, The run() function currently calls scheduleNext() immediately
when it detects an overlap via isRunning, but the finally block also calls
scheduleNext(), creating redundant scheduling; remove the early scheduleNext()
inside the isRunning check so the function simply returns early when isRunning
is true and let the existing finally block (in run) handle scheduling the next
execution after the current run completes.
packages/extension/src/ui/action/composables/synchronize-spark-state.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts`:
- Line 3: Remove the unused import FiroWallet from the top of
handleCoinSetUpdates.ts; locate the import statement "import { FiroWallet }" and
delete it so lint warnings for unused imports are resolved and there are no
references left to FiroWallet in this module.
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts`:
- Line 124: Remove the leftover debug console.log in updateCoinSet.ts (the
"console.log('======>>> syncCoinSetsOnce:remoteMetas', remoteMetas)" call)
before merging; either delete the line or replace it with the project's
structured logger at debug/trace level (e.g., processLogger.debug or
logger.debug used elsewhere) so it won't spam production logs, and ensure you
reference the syncCoinSetsOnce context when adding a logger message.
- Around line 228-231: The forEach callback in the updates.filter(...).forEach
call implicitly returns the result of options?.onMyCoinDetected?.(update);
change the arrow callback to a block-bodied function (e.g., update => {
options?.onMyCoinDetected?.(update); }) so it does not return a value; update
the callback in the forEach that references update.containsMyCoins and
options?.onMyCoinDetected accordingly in updateCoinSet.ts.
- Around line 80-115: fetchNewCoinsForSet is computing a symmetric difference
instead of a union and also fetches the wrong sector offset for deltas; replace
the current differenceSets usage so you merge newCoins with localCoins (i.e.,
produce a union/merged array like updateTagsSet does by combining the
diff/updates back into the stored set) and save that merged result instead of
only the difference, and when doing delta fetch use the correct offset (start at
localCoinsCount, not 0) in the wallet.fetchAnonymitySetSector call so you
request only the newly added coins; update references in fetchNewCoinsForSet and
ensure differenceSets (or a union operation) yields the combined set before
returning isFullReplacement: false.
🧹 Nitpick comments (3)
packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts (1)
4-4:differenceSetsimport will be unused after fixing the merge logic.Once the merge on lines 160–168 is corrected (per the earlier comment), this import can be removed.
packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts (2)
27-42:mergeCoinsdeduplication viaJSON.stringifyis correct but consider usingcoin[0]as the key for consistency.
JSON.stringifyworks correctly for deduplication ofstring[]elements since all values are primitives. However, the rest of the codebase (e.g.,getMyCoinHashesandmatchedCoinsinupdateCoinSet.ts) usescoin[0](the coin hash) as the stable identifier. Usingcoin[0]here would be more efficient (avoids serializing entire arrays) and consistent.This isn't a correctness issue — just a performance/consistency note.
44-72: Sparse array holes ifsetIdexceeds the current array length.If a
setIdmaps to an index beyond the currentupdatedSets.length(e.g.,setId=5but only 2 entries exist), JavaScript creates a sparse array withundefinedholes at intermediate indices. These holes survive serialization to IndexedDB asnullentries.Downstream consumers (
getLocalSets,syncCoinSetsOnce) access by index and handle falsy values, so this is unlikely to cause runtime errors. However, pre-filling the array up to the required length would make intent explicit and avoid surprises:Optional defensive fill
normalizedUpdates.forEach(update => { const setId = update.setId; if (!setId || setId < 1) return; const setIndex = setId - 1; + // Ensure no sparse holes + while (updatedSets.length <= setIndex) { + updatedSets.push({ coins: [], blockHash: '', setHash: '' }); + } + const existingSet = updatedSets[setIndex];
packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts
Outdated
Show resolved
Hide resolved
| getMyCoinHashes(), | ||
| ]); | ||
|
|
||
| console.log('======>>> syncCoinSetsOnce:remoteMetas', remoteMetas); |
There was a problem hiding this comment.
Remove debug console.log before merging.
This looks like a leftover debug statement that will be noisy in production.
Suggested fix
- console.log('======>>> syncCoinSetsOnce:remoteMetas', remoteMetas);📝 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.
| console.log('======>>> syncCoinSetsOnce:remoteMetas', remoteMetas); |
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` at line
124, Remove the leftover debug console.log in updateCoinSet.ts (the
"console.log('======>>> syncCoinSetsOnce:remoteMetas', remoteMetas)" call)
before merging; either delete the line or replace it with the project's
structured logger at debug/trace level (e.g., processLogger.debug or
logger.debug used elsewhere) so it won't spam production logs, and ensure you
reference the syncCoinSetsOnce context when adding a logger message.
| updates | ||
| .filter(update => update.containsMyCoins) | ||
| .forEach(update => options?.onMyCoinDetected?.(update)); | ||
| } |
There was a problem hiding this comment.
forEach callback should not return a value (Biome lint).
The arrow function implicitly returns the result of the optional call. Wrap in braces to suppress the lint warning.
Suggested fix
updates
.filter(update => update.containsMyCoins)
- .forEach(update => options?.onMyCoinDetected?.(update));
+ .forEach(update => { options?.onMyCoinDetected?.(update); });📝 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.
| updates | |
| .filter(update => update.containsMyCoins) | |
| .forEach(update => options?.onMyCoinDetected?.(update)); | |
| } | |
| updates | |
| .filter(update => update.containsMyCoins) | |
| .forEach(update => { options?.onMyCoinDetected?.(update); }); | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 230-230: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` around
lines 228 - 231, The forEach callback in the updates.filter(...).forEach call
implicitly returns the result of options?.onMyCoinDetected?.(update); change the
arrow callback to a block-bodied function (e.g., update => {
options?.onMyCoinDetected?.(update); }) so it does not return a value; update
the callback in the forEach that references update.containsMyCoins and
options?.onMyCoinDetected accordingly in updateCoinSet.ts.
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@packages/extension/src/libs/spark-handler/createTempTx.ts`:
- Around line 62-74: The signing loop assumes spendableUtxos and inputs are
aligned and that addressKeyPairs[utxo.address] exists; update the code to
iterate over the actual inputs array (or explicitly validate lengths) and locate
the matching utxo for each input (e.g., by matching txid/vout or maintaining the
same mapping used when building inputs) before calling tx.signInput(index, ...).
Add a guard to check that the found utxo is defined and that
addressKeyPairs[utxo.address] returns a keyPair; if missing, throw or return a
clear error instead of calling keyPair.sign, and only call tx.signInput with the
correct input index and a Signer built from the validated keyPair.
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts`:
- Around line 73-100: The full-replacement condition is inverted: change the
logic so full-fetch happens only when there is no local data (localSet is
falsy); when localSet exists and meta.size > localCoinsCount perform a delta
fetch by calling wallet.fetchAnonymitySetSector with offset localCoinsCount and
limit meta.size - localCoinsCount (instead of offset 0), and when meta.size <=
localCoinsCount return an empty delta (coins = [], isFullReplacement = false)
rather than doing a full replacement; update the branches around localSet,
meta.size, localCoinsCount and the wallet.fetchAnonymitySetSector calls
accordingly.
In
`@packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts`:
- Around line 274-281: In getUnspentTransactionsByAddress (and similarly in
multiGetUnspentTransactions) the result of
this.mainClient?.request('blockchain.headers.subscribe', []) may be undefined so
accessing header.height (tipHeight) can throw; guard the call by checking that
header is defined and has height (e.g., if (!header || typeof header.height !==
'number') throw or handle the missing client case) or fail early when
this.mainClient is not set before calling request; update the code paths that
use header.height (tipHeight) to use the guarded value or return/throw a clear
error if the Electrum client is unavailable.
In `@packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts`:
- Around line 90-97: The mapping sets isUsed to false for every coin, overriding
the real used state from getSparkCoinInfo; change the construction of myCoins
(the finalResult.map callback) to use the actual value coinData.coin.isUsed
instead of the hardcoded false so each entry reflects the WASM-provided state
(locate the finalResult.map that produces myCoins and replace the isUsed field
accordingly).
- Around line 105-113: The code creates dedupedMyCoinsSet via removeDuplicates
but then returns Array.from(new Set([...savedMyCoins, ...myCoins])) which
reintroduces duplicates because Set does reference equality for objects; change
the return to Array.from(dedupedMyCoinsSet) so the function returns the
already-deduplicated result (refer to dedupedMyCoinsSet and removeDuplicates in
sparkCoinInfoWorker.ts).
🟠 Major comments (19)
packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts-75-93 (1)
75-93:⚠️ Potential issue | 🟠 MajorRetry guard is dead code —
mainClientis alwaysundefinedat that point.Line 84 sets
this.mainClient = undefined, so the check on line 86 (if (!this.mainClient)) is alwaystrue. The guard serves no purpose and retries fire unconditionally with no backoff or max-retry limit, risking an infinite retry loop on persistent failures.Also, the
Peertype carriestcp/sslport fields butconnectMainhardcodes port50004and protocol'wss', making those fields misleading.Suggested fix
+ private retryCount = 0; + private readonly maxRetries = 5; + async connectMain() { try { const peer = getRandomHardcodedPeer(); - this.mainClient = new ElectrumClient(peer.host, 50004, 'wss'); + const port = Number(peer.ssl) || 50002; + this.mainClient = new ElectrumClient(peer.host, port, 'wss'); await this.mainClient.connect('electrum-client-browser', '1.4'); this.wasConnectedAtLeastOnce = true; + this.retryCount = 0; } catch (e) { this.mainClient = undefined; console.error('electrum_wallet:connectMain', e); - if (!this.mainClient) { - console.warn('electrum_wallet:connectMain', 'retry'); + if (this.retryCount < this.maxRetries) { + this.retryCount++; + console.warn('electrum_wallet:connectMain', `retry ${this.retryCount}/${this.maxRetries}`); setTimeout(() => { this.connectMain(); - }, 5000); + }, 5000 * this.retryCount); } } }packages/extension/src/libs/utils/updateAndSync/getSetsFromDb.ts-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorModule-level
IndexedDBHelperinstance withoutwaitInit()— first call may silently return[].
IndexedDBHelper.initDB()is async. IfgetSetsFromDbis called before the DB opens,getObjectStorethrows"Missing db", which the catch block swallows, returning[]. While this doesn't crash, it can cause silent data loss (the caller believes there are no sets when there actually are).This same pattern (module-level
new IndexedDBHelper()withoutwaitInit()) appears inhandleCoinSetUpdates.tsand the worker file. Consider either:
- Calling
await db.waitInit()before reads, or- Exporting a shared singleton that ensures initialization.
packages/extension/src/providers/bitcoin/ui/libs/signer.ts-147-157 (1)
147-157:⚠️ Potential issue | 🟠 MajorMissing null check on
keyPair— will crash if address is not in the mapping.If
addressKeyPairMappingdoesn't containinput.address,keyPairisundefinedandkeyPair.sign(hash)throws an NPE.Proposed fix
allInputs.forEach((input, i) => { const keyPair = addressKeyPairMapping[input.address]; + if (!keyPair) { + throw new Error(`No key pair found for address: ${input.address}`); + } const Signer = {packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts-150-153 (1)
150-153:⚠️ Potential issue | 🟠 Major
allSets.length - 1is-1whenallSetsis empty, persisting an invalid index.If
getSetsFromDb()returns[], saving-1aslastCheckedSetIndexwill causeallSets.slice(-1)in subsequent runs, processing only the last element incorrectly.Proposed fix
- await db.saveData(DB_DATA_KEYS.lastCheckedSetIndex, allSets.length - 1); + await db.saveData(DB_DATA_KEYS.lastCheckedSetIndex, Math.max(0, allSets.length - 1));packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts-54-67 (1)
54-67:⚠️ Potential issue | 🟠 MajorSparse array created when
setIdexceeds current array length.If
update.setIdis larger thanupdatedSets.length, assigningupdatedSets[setIndex]createsundefinedholes in the array. When this sparse array is later persisted to IndexedDB and read back, consumers iterating it (e.g.,.map(),.reduce()) will encounterundefinedentries and crash.Consider expanding the array to the required size or validating bounds:
Proposed fix — expand array
normalizedUpdates.forEach(update => { const setId = update.setId; if (!setId || setId < 1) return; const setIndex = setId - 1; + // Ensure the array is large enough; fill gaps with empty sets + while (updatedSets.length <= setIndex) { + updatedSets.push({ coins: [], blockHash: '', setHash: '' }); + } + const existingSet = updatedSets[setIndex]; const mergedCoins = mergeCoins(existingSet?.coins ?? [], update.set.coins);packages/extension/src/libs/spark-handler/createTempTx.ts-53-60 (1)
53-60:⚠️ Potential issue | 🟠 MajorEmpty
spendableUtxoswhenchangeAmount > 0causes a runtime crash.If
changeAmountis positive butspendableUtxosis empty,spendableUtxos[0]isundefinedand accessing.addressthrows. Add a guard.Proposed fix
if (changeAmount.gt(0)) { + if (!spendableUtxos.length) { + throw new Error('No spendable UTXOs available for change output'); + } const firstUtxoAddress = spendableUtxos[0].address; tx.addOutput({ - address: firstUtxoAddress!, + address: firstUtxoAddress, value: changeAmount.toNumber(), }); }packages/extension/src/providers/bitcoin/ui/libs/signer.ts-108-111 (1)
108-111:⚠️ Potential issue | 🟠 Major
FiroTransactionSignerignoresoptions.account— hardware wallet calls will silently use software keys.Unlike
TransactionSigner, which checksaccount.isHardware,FiroTransactionSignerunconditionally usesBaseFiroWalletfor signing. If a hardware wallet account is passed, this silently bypasses the hardware signer, which is a security concern.At minimum, add an early guard:
Proposed fix
const FiroTransactionSigner = async ( options: SignerTransactionOptions, ): Promise<Transaction> => { - const { network, payload } = options; + const { account, network, payload } = options; + if (account.isHardware) { + throw new Error('Hardware wallet signing is not yet supported for Firo'); + } const wallet = new BaseFiroWallet();packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts-119-164 (1)
119-164:⚠️ Potential issue | 🟠 MajorUnhandled errors in worker event listener —
throwinsideaddEventListenercrashes the worker silently.The
throwstatements on lines 125, 131, and 137 will not be caught by any handler and will terminate the worker without posting a result. Wrap the body in a try/catch and post an error message back.Additionally,
resultcan beundefinediffetchAllCoinInfoscatches internally (line 114-116), sopostMessage(result)sendsundefinedto the host.Proposed fix sketch
addEventListener('message', async () => { + try { const Module = await wasmWorkerInstance.getInstance(); const spendKeyObj = await getSpendKeyObj(Module); if (!spendKeyObj || spendKeyObj === 0) { - throw new Error('Failed to create spendKeyObj'); + postMessage({ error: 'Failed to create spendKeyObj' }); + return; } // ... similar for other validation ... // ... rest of logic ... postMessage(result); + } catch (err) { + console.error('sparkCoinInfoWorker error:', err); + postMessage({ error: String(err) }); + } });packages/extension/src/providers/bitcoin/types/index.ts-99-104 (1)
99-104:⚠️ Potential issue | 🟠 MajorMaking
addra required field breaks the SS provider.The SS provider (lines 34, 64 in activity-handlers/providers/ss/index.ts) only uses the
addressesfield fromvinand doesn't supplyaddr. Meanwhile, the Firo provider (line 31 in activity-handlers/providers/firo/index.ts) expectsaddr. Makingaddroptional (addr?: string) allows both providers to work correctly; the Firo handler already uses optional chaining (tx.vin?.[0]?.addrat line 64) suggesting this pattern is expected.Proposed fix
vin: { txid: string; addresses?: string[]; - addr: string; + addr?: string; value: string; }[];packages/extension/src/libs/utils/updateAndSync/markCoinsAsUsed.ts-31-32 (1)
31-32:⚠️ Potential issue | 🟠 MajorRemove verbose debug logs before merging.
Multiple
console.logcalls with>>>>and===>>>prefixes leak coin tags, transaction IDs, and balance data to the console. These should be removed or gated behind a debug flag for production.Also applies to: 36-36, 43-43, 55-55, 71-71, 79-79
packages/extension/src/ui/action/composables/synchronize-spark-state.ts-128-144 (1)
128-144:⚠️ Potential issue | 🟠 MajorUnhandled rejection from
markCoinsAsUsedin watch callback.Line 141 awaits
markCoinsAsUsedbut the watch callback has no error handling. IfmarkCoinsAsUsedthrows (e.g., IndexedDB or network failure), the rejection is unhandled and the UI state becomes stale.🛡️ Proposed fix
if (updatedCoinFetchDone && updatedTagFetchDone) { coinFetchDone.value = false; tagFetchDone.value = false; - sparkUnusedTxDetails.value = await markCoinsAsUsed(onBalanceCalculated); + try { + sparkUnusedTxDetails.value = await markCoinsAsUsed(onBalanceCalculated); + } catch (err) { + console.error('markCoinsAsUsed failed:', err); + error.value = err as any; + sparkUnusedTxDetails.value = []; + } }packages/extension/src/libs/utils/updateAndSync/markCoinsAsUsed.ts-73-77 (1)
73-77:⚠️ Potential issue | 🟠 MajorMissing error handling around
multiGetTransactionByTxidprevents data persistence on failure.If
firoElectrum.multiGetTransactionByTxidthrows, execution aborts before lines 91–94, so updated coins and the computed balance are never persisted to IndexedDB. Wrap this call in try-catch so the coin state and balance are saved regardless.🛡️ Proposed fix
- if (!!txIdsDecoded?.length) { - const results = await firoElectrum.multiGetTransactionByTxid(txIdsDecoded); - - unusedTxDetails = Object.values(results); - } + if (txIdsDecoded.length) { + try { + const results = await firoElectrum.multiGetTransactionByTxid(txIdsDecoded); + unusedTxDetails = Object.values(results); + } catch (err) { + console.error('Failed to fetch Spark TX details:', err); + } + }packages/extension/src/ui/action/composables/synchronize-spark-state.ts-62-64 (1)
62-64:⚠️ Potential issue | 🟠 MajorWorker error leaves sync permanently stuck.
When the worker errors (line 62),
cleanupAndMaybeRestartis called butcoinFetchDoneis never set totrue. Since the secondwatch(line 128) waits forcoinFetchDone && tagFetchDonebefore callingmarkCoinsAsUsed, a worker error means synchronization silently stalls andisCoinFetchPendingremainstrueforever.🛡️ Proposed fix
worker.onerror = () => { + error.value = new Error('Spark coin info worker failed'); + isCoinFetchPending.value = false; cleanupAndMaybeRestart(); };packages/extension/src/providers/bitcoin/libs/firo-wallet/base-firo-wallet.ts-140-144 (1)
140-144:⚠️ Potential issue | 🟠 MajorProduction
console.error/console.logstatements leak addresses and UTXO details.
- Line 141-143: Logs expected vs. derived addresses on mismatch
- Line 170-172: Logs the address when UTXO fetch fails
- Line 176: Logs total spendable UTXO count
These should be removed or gated behind a debug flag for production builds.
- if (address !== derivedAddress) { - console.error( - `🚨 Derived address mismatch! Expected: ${address}, Got: ${derivedAddress}`, - ); - } + if (address !== derivedAddress) { + throw new Error(`Derived address mismatch at index ${index}`); + }Also applies to: 170-173, 176-176
packages/extension/src/providers/bitcoin/libs/firo-wallet/base-firo-wallet.ts-102-149 (1)
102-149:⚠️ Potential issue | 🟠 MajorAddress-to-key-pair mapping assumes exact external/internal ordering — fragile.
iterationsLimit = Math.floor(addresses.length / 2)assumes the first half of the input array is external addresses and the second half is internal. If the caller provides addresses in a different order, or if the counts aren't evenly split, derivation paths won't match andaddress !== derivedAddressmismatches will occur silently (logged but not thrown).The poorly named variables (
dd,idx) further obscure the intent. Consider either:
- Accepting structured input (separate external/internal arrays), or
- Deriving the derivation path from the address by looking it up in the cache, or
- At minimum, throwing on mismatch instead of logging.
packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue-81-86 (1)
81-86:⚠️ Potential issue | 🟠 Major
sparkAccountdefault returns{}(truthy) — sync button renders on all networks.The default factory
() => { return {}; }produces an empty object, which is truthy. Combined with thev-if="sparkAccount"check on line 25, the "Synchronized/Synchronizing" button will render on every network (not just Firo) unless the parent explicitly passesnull.Proposed fix
sparkAccount: { type: Object as PropType<SparkAccount | null>, - default: () => { - return {}; - }, + default: null, },packages/extension/src/libs/utils/updateAndSync/updateTagsSet.ts-29-34 (1)
29-34:⚠️ Potential issue | 🟠 MajorLength-only equality check will miss tag replacements.
Comparing only
.lengthmeans if the remote set has the same number of tags but different values (e.g., tags were rotated or replaced), the change goes undetected and local data becomes stale.Consider comparing content hashes or using set-based comparison:
Proposed fix
- if ( - coinTags.tags.length === localTags?.tags?.length && - txHashes.tagsandtxids.length === localTags?.txHashes?.length - ) { - return { tags: [] }; - } + const tagsUnchanged = + coinTags.tags.length === localTags?.tags?.length && + coinTags.tags.every((t, i) => t === localTags.tags[i]); + const hashesUnchanged = + txHashes.tagsandtxids.length === localTags?.txHashes?.length && + txHashes.tagsandtxids.every( + (h, i) => JSON.stringify(h) === JSON.stringify(localTags.txHashes[i]), + ); + if (tagsUnchanged && hashesUnchanged) { + return { tags: [] }; + }packages/extension/src/providers/bitcoin/libs/api-firo.ts-69-74 (1)
69-74:⚠️ Potential issue | 🟠 Major
getBalanceignores thepubkeyparameter and returns total wallet balance instead of per-address balance.The method signature accepts
pubkeyper theProviderAPIInterfacecontract, but callsthis.#wallet.getPublicBalance()which aggregates UTXOs across all wallet addresses viagetTransactionsAddresses(). This returns the total wallet balance regardless of which account's pubkey was passed. Other Bitcoin provider implementations (api.ts, api-ss.ts) correctly convert the pubkey to a specific address and fetch balance for that address only. In multi-account scenarios, this will produce incorrect results.packages/extension/src/providers/bitcoin/libs/firo-wallet/base-firo-wallet.ts-82-89 (1)
82-89:⚠️ Potential issue | 🟠 MajorFiroWallet stores the mnemonic phrase in plain text in BrowserStorage.
setSecretwrites the raw mnemonic and seed hex directly into browser extension storage without encryption. While KeyRing encrypts the mnemonic with a password before storing it (under theenkrypt_mnemonickey), FiroWallet circumvents this protection by storing the decrypted mnemonic plainly under thefiro_wallet_secretkey. If the extension storage is compromised or exported, the entire wallet is exposed. This storage pattern is inconsistent with the security practices used in the rest of enKrypt's keyring implementation.
🟡 Minor comments (7)
packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts-354-357 (1)
354-357:⚠️ Potential issue | 🟡 Minor
rawfield may beundefinedbut return type promisesUnspentTxOutputModel(which includesraw: string).
getTxRawuses optional chaining (this.mainClient?.blockchain_transaction_get), sorawTxs[index]can beundefined. The spread on line 354-357 would setraw: undefined, violating theUnspentTxOutputModelcontract.packages/extension/src/providers/bitcoin/ui/libs/signer.ts-145-145 (1)
145-145:⚠️ Potential issue | 🟡 MinorStatic analysis:
forEachcallback should not return a value.
tx.addOutput(output)returns a number, and the arrow function implicitly returns it. Wrap in braces to suppress the lint warning.Proposed fix
- payload.outputs.forEach(output => tx.addOutput(output)); + payload.outputs.forEach(output => { tx.addOutput(output); });packages/extension/src/libs/spark-handler/index.ts-343-347 (1)
343-347:⚠️ Potential issue | 🟡 Minor
BigNumber(...).toNumber()can still lose precision for large amounts.Line 345 converts the result back to a JS
numbervia.toNumber(). For amounts exceedingNumber.MAX_SAFE_INTEGERsatoshis (~0.9 BTC), this will silently lose precision. Use.integerValue(BigNumber.ROUND_FLOOR).toNumber()or ensure the amount is bounded.🛡️ Proposed fix
- value: new BigNumber(amount).multipliedBy(10 ** 8).toNumber(), + value: new BigNumber(amount).multipliedBy(1e8).integerValue(BigNumber.ROUND_FLOOR).toNumber(),packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue-24-32 (1)
24-32:⚠️ Potential issue | 🟡 MinorButton labeled "Synchronized" but triggers a modal — confusing UX.
The button text ("Synchronized" / "Synchronizing...") reads like a status indicator, but clicking it opens an "anonymize funds modal." Users may not expect a status label to be an interactive action trigger. Consider making the intent clearer, e.g., "Anonymize Funds" or adding a visual affordance (icon, underline) to indicate it's actionable.
packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue-35-36 (1)
35-36:⚠️ Potential issue | 🟡 MinorTypo:
sunchromizeState→synchronizeState.The variable name is misspelled across lines 35, 36, 95, and 120, making it harder to search/refactor.
Proposed fix
-const sunchromizeState = ref(false); +const synchronizeState = ref(false); // ... const openAnonymizeFundsModal = () => { - sunchromizeState.value = true; + synchronizeState.value = true; };And in the template:
<synchronize-state - v-if="sunchromizeState" - v-model="sunchromizeState" + v-if="synchronizeState" + v-model="synchronizeState" :isCoinSyncing="props.isCoinSyncing" :isTagSyncing="props.isTagSyncing" />Also applies to: 95-95, 119-121
packages/extension/src/providers/bitcoin/libs/firo-wallet/base-firo-wallet.ts-246-258 (1)
246-258:⚠️ Potential issue | 🟡 Minor
skipAddresshas no upper-bound check on the spark index.Each call increments the index in localStorage without limit. If triggered repeatedly (e.g., by rapid clicks on "generate new spark address"), the index can grow unbounded, potentially leading to derivation of addresses far outside any practical range.
Consider adding a reasonable ceiling or wrapping behavior.
packages/extension/src/libs/utils/updateAndSync/updateTagsSet.ts-66-69 (1)
66-69:⚠️ Potential issue | 🟡 MinorRedundant
scheduleNext()when a run is already in progress.When
isRunningis true,scheduleNext()queues a new timer, but the currently running invocation will also callscheduleNext()in itsfinallyblock (line 88). This can lead to double-scheduled timers. Simply returning early is sufficient.Proposed fix
if (isRunning) { - scheduleNext(); return; }
🧹 Nitpick comments (14)
packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts (2)
294-303: Duplicated UTXO mapping logic.The mapping from raw Electrum list-unspent results to
UnspentTxOutputModelis duplicated betweengetUnspentTransactionsByAddress(lines 294-303) andmultiGetUnspentTransactions(lines 335-344). Extract a shared helper to reduce maintenance burden and divergence risk.Also applies to: 335-344
348-348: Typo:filterred→filtered.packages/extension/src/libs/spark-handler/createTempTx.ts (1)
28-28:addressKeyPairsis typed asRecord<string, any>— loses type safety.Consider typing the value as
ECPairInterface(already imported) to get compile-time guarantees on.sign()and.publicKey.Proposed fix
- addressKeyPairs: Record<string, any>; + addressKeyPairs: Record<string, ECPairInterface>;packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts (2)
89-89:Promise.allSettledsilently swallows rejected coin-info fetches.Failed promises are ignored with no logging. Consider inspecting
allSettledresults for rejected entries and logging them for diagnostics.Proposed improvement
- await Promise.allSettled(allPromises); + const settled = await Promise.allSettled(allPromises); + const failures = settled.filter(r => r.status === 'rejected'); + if (failures.length) { + console.warn(`${failures.length} coin info fetches failed`, failures.map(f => (f as PromiseRejectedResult).reason)); + }
102-103: Debugconsole.logstatements with===>>>prefix should be removed before merging.These appear to be development-time debug logs and will pollute production console output.
Also applies to: 109-109
packages/extension/src/libs/utils/updateAndSync/handleCoinSetUpdates.ts (1)
27-42:mergeCoinsprepends new coins before existing — verify this is intentional.Line 41 returns
[...merged, ...existing], placing newly-added coins before existing ones. If downstream consumers expect coins in chronological/append order, this could cause subtle issues. If order doesn't matter, consider documenting that.packages/extension/src/libs/utils/updateAndSync/updateTagsSet.ts (3)
42-44: Remove the no-opcatchblock.
catch (error) { throw error; }adds nothing — the error propagates identically without it.Proposed fix
const syncTagsOnce = async (): Promise<SetsUpdateResult> => { - try { const localTags = await db.readData<{ tags: string[]; txHashes: string[][]; }>(DB_DATA_KEYS.usedCoinsTags); const txHashes = await wallet.getUsedCoinsTagsTxHashes(0); const coinTags = await wallet.getUsedSparkCoinsTags(0); // ... return coinTags; - } catch (error) { - throw error; - } };
78-79: Remove the emptyelseblock.The empty
else {}on lines 78-79 is dead code and should be removed.if (updates?.tags?.length) { options?.onUpdate?.(updates); - } else { }
15-16: Module-level singleton instances may cause issues in a multi-context extension environment.
walletanddbare instantiated at module load time as singletons. If this module is imported in different extension contexts (background, popup, content script), each context gets its own instance, which is fine. However, if the module is imported multiple times within the same context (e.g., through different routes), these singletons share mutable state silently. Consider injecting these dependencies or creating them insidestartTagSetSync.packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (1)
33-46: Both tab components usev-if— user input is lost on tab switch.When switching between "Transparent" and "Spark" tabs,
v-ifdestroys and recreates the component, discarding any partially entered form data. If preserving draft input across tab switches is desired, usev-show(or<keep-alive>) instead.- <transparent-send-tab - v-if="selectedSendTab === 'transparent'" + <transparent-send-tab + v-show="selectedSendTab === 'transparent'" :network="network" :account-info="accountInfo" :is-send-token="isSendToken" /> - <spark-send-tab - v-if="selectedSendTab === 'spark' && !!accountInfo.sparkAccount" + <spark-send-tab + v-show="selectedSendTab === 'spark' && !!accountInfo.sparkAccount" :network="network" :account-info="accountInfo" :spark-account="accountInfo?.sparkAccount ?? null" :is-send-token="isSendToken" />packages/extension/src/ui/action/App.vue (1)
155-156: Firo-specific objects are eagerly instantiated for all networks.
BaseFiroWalletandIndexedDBHelperare created at module scope regardless of which network the user is on.BaseFiroWalletcreates aBrowserStorageinstance in its constructor, which is unnecessary overhead and potential side effects for non-Firo users (the vast majority).Consider lazy initialization:
Proposed fix
-const wallet = new BaseFiroWallet(); -const db = new IndexedDBHelper(); +let _wallet: BaseFiroWallet | null = null; +let _db: IndexedDBHelper | null = null; +const getFiroWallet = () => (_wallet ??= new BaseFiroWallet()); +const getFiroDb = () => (_db ??= new IndexedDBHelper());packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
99-111:getUTXOsuses fragile duck-typing for error detection.Line 102:
(spendableUtxos as any).messagechecks if the result is an error object by looking for a.messageproperty. This is brittle — a legitimate UTXO could theoretically have amessagefield, and it bypasses TypeScript's type safety. Consider catching errors fromgetOnlySpendableUtxos()with a try/catch instead.Proposed fix
async getUTXOs(pubkey: string): Promise<UnspentTxOutputModel[]> { - const spendableUtxos = await this.#wallet.getOnlySpendableUtxos(); - - if ((spendableUtxos as any).message || !spendableUtxos.length) return []; + let spendableUtxos; + try { + spendableUtxos = await this.#wallet.getOnlySpendableUtxos(); + } catch { + return []; + } + if (!spendableUtxos.length) return []; spendableUtxos.sort((a, b) => { return a.amount - b.amount; });packages/extension/src/providers/bitcoin/libs/firo-wallet/base-firo-wallet.ts (2)
269-269:index = index * 1is unnecessary in TypeScript.The parameter
indexis already typed asnumber. The* 1coercion is a legacy JavaScript pattern that adds confusion without benefit.- index = index * 1; // cast to int + // index is already typed as number
231-244: Duplicated localStorage initialization pattern betweengetSparkAddressAsyncandskipAddress.Both methods repeat the same "check if sparkIndex exists, set to 1 if not, then read" pattern. Extract a shared helper to reduce duplication.
Proposed refactor
+ private getOrInitSparkIndex(): number { + const localStorage = new LocalStorageHelper(LOCAL_STORAGE_PREFIXES.wallet); + if (!localStorage.exists(LOCAL_STORAGE_KEYS.sparkIndex)) { + localStorage.set(LOCAL_STORAGE_KEYS.sparkIndex, 1); + } + return localStorage.get<number>(LOCAL_STORAGE_KEYS.sparkIndex)!; + } + async getSparkAddressAsync(): Promise<string | undefined> { - const localStorage = new LocalStorageHelper(LOCAL_STORAGE_PREFIXES.wallet); - if (!localStorage.exists(LOCAL_STORAGE_KEYS.sparkIndex)) { - localStorage.set(LOCAL_STORAGE_KEYS.sparkIndex, 1); - } - const currentIndex = localStorage.get<number>(LOCAL_STORAGE_KEYS.sparkIndex); + const currentIndex = this.getOrInitSparkIndex(); const sparkState = await getSparkState(currentIndex); return sparkState?.defaultAddress; } skipAddress(): void { - const localStorage = new LocalStorageHelper(LOCAL_STORAGE_PREFIXES.wallet); - if (!localStorage.exists(LOCAL_STORAGE_KEYS.sparkIndex)) { - localStorage.set(LOCAL_STORAGE_KEYS.sparkIndex, 1); - } - const currentIndex = localStorage.get<number>(LOCAL_STORAGE_KEYS.sparkIndex); - localStorage.set(LOCAL_STORAGE_KEYS.sparkIndex, currentIndex! + 1); + const currentIndex = this.getOrInitSparkIndex(); + const localStorage = new LocalStorageHelper(LOCAL_STORAGE_PREFIXES.wallet); + localStorage.set(LOCAL_STORAGE_KEYS.sparkIndex, currentIndex + 1); }Also applies to: 246-258
| for (let index = 0; index < spendableUtxos.length; index++) { | ||
| const utxo = spendableUtxos[index]; | ||
| const keyPair = addressKeyPairs[utxo.address]; | ||
|
|
||
| const Signer = { | ||
| sign: (hash: Uint8Array) => { | ||
| return Buffer.from(keyPair.sign(hash)); | ||
| }, | ||
| publicKey: Buffer.from(keyPair.publicKey), | ||
| } as unknown as bitcoin.Signer; | ||
|
|
||
| tx.signInput(index, Signer); | ||
| } |
There was a problem hiding this comment.
Signing loop assumes inputs and spendableUtxos are 1:1 aligned — no guard.
The loop iterates over spendableUtxos indices and calls tx.signInput(index, ...), but inputs are added from the separate inputs array. If inputs.length !== spendableUtxos.length, this will sign wrong indices or throw at runtime. Additionally, addressKeyPairs[utxo.address] can be undefined, causing an NPE on keyPair.sign.
Proposed fix — add guards
for (let index = 0; index < spendableUtxos.length; index++) {
const utxo = spendableUtxos[index];
const keyPair = addressKeyPairs[utxo.address];
+ if (!keyPair) {
+ throw new Error(`Missing key pair for address: ${utxo.address}`);
+ }
const Signer = {📝 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.
| for (let index = 0; index < spendableUtxos.length; index++) { | |
| const utxo = spendableUtxos[index]; | |
| const keyPair = addressKeyPairs[utxo.address]; | |
| const Signer = { | |
| sign: (hash: Uint8Array) => { | |
| return Buffer.from(keyPair.sign(hash)); | |
| }, | |
| publicKey: Buffer.from(keyPair.publicKey), | |
| } as unknown as bitcoin.Signer; | |
| tx.signInput(index, Signer); | |
| } | |
| for (let index = 0; index < spendableUtxos.length; index++) { | |
| const utxo = spendableUtxos[index]; | |
| const keyPair = addressKeyPairs[utxo.address]; | |
| if (!keyPair) { | |
| throw new Error(`Missing key pair for address: ${utxo.address}`); | |
| } | |
| const Signer = { | |
| sign: (hash: Uint8Array) => { | |
| return Buffer.from(keyPair.sign(hash)); | |
| }, | |
| publicKey: Buffer.from(keyPair.publicKey), | |
| } as unknown as bitcoin.Signer; | |
| tx.signInput(index, Signer); | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/libs/spark-handler/createTempTx.ts` around lines 62 -
74, The signing loop assumes spendableUtxos and inputs are aligned and that
addressKeyPairs[utxo.address] exists; update the code to iterate over the actual
inputs array (or explicitly validate lengths) and locate the matching utxo for
each input (e.g., by matching txid/vout or maintaining the same mapping used
when building inputs) before calling tx.signInput(index, ...). Add a guard to
check that the found utxo is defined and that addressKeyPairs[utxo.address]
returns a keyPair; if missing, throw or return a clear error instead of calling
keyPair.sign, and only call tx.signInput with the correct input index and a
Signer built from the validated keyPair.
| if (!localSet || meta.size <= localCoinsCount) { | ||
| const [firstChunk, secondChunk] = await Promise.all([ | ||
| wallet.fetchAnonymitySetSector( | ||
| setId, | ||
| meta.blockHash, | ||
| 0, | ||
| Math.floor(meta.size / 2), | ||
| ), | ||
| wallet.fetchAnonymitySetSector( | ||
| setId, | ||
| meta.blockHash, | ||
| Math.floor(meta.size / 2), | ||
| meta.size, | ||
| ), | ||
| ]); | ||
| const coins = [...firstChunk.coins, ...secondChunk.coins]; | ||
| return { coins, isFullReplacement: true }; | ||
| } | ||
|
|
||
| const sector = await wallet.fetchAnonymitySetSector( | ||
| setId, | ||
| meta.blockHash, | ||
| 0, | ||
| meta.size - localCoinsCount, | ||
| ); | ||
|
|
||
| const coins = sector.coins ?? []; | ||
| return { coins, isFullReplacement: false }; |
There was a problem hiding this comment.
Full-replacement condition appears inverted.
Line 73: !localSet || meta.size <= localCoinsCount triggers a full replacement when the remote set is same size or smaller than local. This discards local coins that the remote hasn't caught up to yet. Conversely, when remote is larger (i.e., there are genuinely new coins), it falls through to the delta path.
The expected behavior is typically: full-fetch when no local data exists, delta-fetch when local data is present and remote has more coins.
Suggested condition
- if (!localSet || meta.size <= localCoinsCount) {
+ if (!localSet || localCoinsCount === 0) {Also, the delta fetch (lines 92-97) uses offset 0 which likely retrieves the oldest coins rather than the newest additions. If the API uses chronological indexing, the offset should start at localCoinsCount.
#!/bin/bash
# Verify how fetchAnonymitySetSector is called elsewhere and what offset semantics are used
rg -n "fetchAnonymitySetSector\|getSparkAnonymitySetSector" --type ts -B 2 -A 5🤖 Prompt for AI Agents
In `@packages/extension/src/libs/utils/updateAndSync/updateCoinSet.ts` around
lines 73 - 100, The full-replacement condition is inverted: change the logic so
full-fetch happens only when there is no local data (localSet is falsy); when
localSet exists and meta.size > localCoinsCount perform a delta fetch by calling
wallet.fetchAnonymitySetSector with offset localCoinsCount and limit meta.size -
localCoinsCount (instead of offset 0), and when meta.size <= localCoinsCount
return an empty delta (coins = [], isFullReplacement = false) rather than doing
a full replacement; update the branches around localSet, meta.size,
localCoinsCount and the wallet.fetchAnonymitySetSector calls accordingly.
| async getUnspentTransactionsByAddress( | ||
| address: string, | ||
| ): Promise<Omit<UnspentTxOutputModel, 'raw'>[]> { | ||
| const header = await this.mainClient?.request( | ||
| 'blockchain.headers.subscribe', | ||
| [], | ||
| ); | ||
| const tipHeight = header.height; |
There was a problem hiding this comment.
header can be undefined — accessing .height will crash.
this.mainClient?.request(...) returns undefined if mainClient is not set, so header.height on line 281 throws a TypeError. Same issue at line 321 in multiGetUnspentTransactions.
Suggested guard
const header = await this.mainClient?.request(
'blockchain.headers.subscribe',
[],
);
+ if (!header) {
+ throw new Error('Electrum client is not connected');
+ }
const tipHeight = header.height;🤖 Prompt for AI Agents
In
`@packages/extension/src/providers/bitcoin/libs/electrum-client/electrum-client.ts`
around lines 274 - 281, In getUnspentTransactionsByAddress (and similarly in
multiGetUnspentTransactions) the result of
this.mainClient?.request('blockchain.headers.subscribe', []) may be undefined so
accessing header.height (tipHeight) can throw; guard the call by checking that
header is defined and has height (e.g., if (!header || typeof header.height !==
'number') throw or handle the missing client case) or fail early when
this.mainClient is not set before calling request; update the code paths that
use header.height (tipHeight) to use the guarded value or return/throw a clear
error if the Electrum client is unavailable.
| const myCoins = finalResult.map(coinData => ({ | ||
| coin: coinData.coin.originalCoin, | ||
| setId: coinData.setId, | ||
| value: coinData.coin.value, | ||
| tag: coinData.tag, | ||
| setHash: coinData.setHash, | ||
| isUsed: false, | ||
| })); |
There was a problem hiding this comment.
isUsed: false is hardcoded, ignoring the actual value from getSparkCoinInfo.
coinData.coin.isUsed carries the real used-state from the WASM module, but it's overridden here. This means all coins will appear unused.
Proposed fix
const myCoins = finalResult.map(coinData => ({
coin: coinData.coin.originalCoin,
setId: coinData.setId,
value: coinData.coin.value,
tag: coinData.tag,
setHash: coinData.setHash,
- isUsed: false,
+ isUsed: coinData.coin.isUsed,
}));📝 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 myCoins = finalResult.map(coinData => ({ | |
| coin: coinData.coin.originalCoin, | |
| setId: coinData.setId, | |
| value: coinData.coin.value, | |
| tag: coinData.tag, | |
| setHash: coinData.setHash, | |
| isUsed: false, | |
| })); | |
| const myCoins = finalResult.map(coinData => ({ | |
| coin: coinData.coin.originalCoin, | |
| setId: coinData.setId, | |
| value: coinData.coin.value, | |
| tag: coinData.tag, | |
| setHash: coinData.setHash, | |
| isUsed: coinData.coin.isUsed, | |
| })); |
🤖 Prompt for AI Agents
In `@packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts` around lines
90 - 97, The mapping sets isUsed to false for every coin, overriding the real
used state from getSparkCoinInfo; change the construction of myCoins (the
finalResult.map callback) to use the actual value coinData.coin.isUsed instead
of the hardcoded false so each entry reflects the WASM-provided state (locate
the finalResult.map that produces myCoins and replace the isUsed field
accordingly).
| const dedupedMyCoinsSet = removeDuplicates( | ||
| new Set([...savedMyCoins, ...myCoins]), | ||
| ); | ||
|
|
||
| console.log('===>>>Deduped Spark Coins:', dedupedMyCoinsSet); | ||
|
|
||
| await db.saveData(DB_DATA_KEYS.myCoins, Array.from(dedupedMyCoinsSet)); | ||
|
|
||
| return Array.from(new Set([...savedMyCoins, ...myCoins])); |
There was a problem hiding this comment.
Set uses reference equality — the return value on line 113 is not actually deduplicated.
Line 111 correctly saves dedupedMyCoinsSet to the DB. However, line 113 creates a new Set from the raw savedMyCoins and myCoins arrays. Since Set compares objects by reference (not by value), no deduplication occurs for the returned data. You likely intended to return the already-deduped result.
Proposed fix
await db.saveData(DB_DATA_KEYS.myCoins, Array.from(dedupedMyCoinsSet));
- return Array.from(new Set([...savedMyCoins, ...myCoins]));
+ return Array.from(dedupedMyCoinsSet);📝 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 dedupedMyCoinsSet = removeDuplicates( | |
| new Set([...savedMyCoins, ...myCoins]), | |
| ); | |
| console.log('===>>>Deduped Spark Coins:', dedupedMyCoinsSet); | |
| await db.saveData(DB_DATA_KEYS.myCoins, Array.from(dedupedMyCoinsSet)); | |
| return Array.from(new Set([...savedMyCoins, ...myCoins])); | |
| const dedupedMyCoinsSet = removeDuplicates( | |
| new Set([...savedMyCoins, ...myCoins]), | |
| ); | |
| console.log('===>>>Deduped Spark Coins:', dedupedMyCoinsSet); | |
| await db.saveData(DB_DATA_KEYS.myCoins, Array.from(dedupedMyCoinsSet)); | |
| return Array.from(dedupedMyCoinsSet); |
🤖 Prompt for AI Agents
In `@packages/extension/src/ui/action/workers/sparkCoinInfoWorker.ts` around lines
105 - 113, The code creates dedupedMyCoinsSet via removeDuplicates but then
returns Array.from(new Set([...savedMyCoins, ...myCoins])) which reintroduces
duplicates because Set does reference equality for objects; change the return to
Array.from(dedupedMyCoinsSet) so the function returns the already-deduplicated
result (refer to dedupedMyCoinsSet and removeDuplicates in
sparkCoinInfoWorker.ts).
…ture/add-firo-network
Fix get balance
…ture/add-firo-network
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts`:
- Around line 142-153: The getUTXOs function's outer fetch/JSON chain lacks a
.catch handler so network or parse errors reject the promise; update getUTXOs
(and its fetch chain that calls this.FiroToHaskoinUTXOs) to append a .catch that
logs or swallows the error and returns an empty array ([]) to match getBalance's
behavior, ensuring any fetch or res.json() failure results in [] instead of an
unhandled rejection.
- Around line 44-73: The getTransactionStatus function lacks error handling for
network/JSON errors; wrap the fetch chain in a try/catch (or append a .catch)
inside getTransactionStatus so any fetch/res.json errors are caught and handled,
returning null (or a resolved value) and logging the error via the same logging
mechanism used elsewhere; update the function name getTransactionStatus and
mirror the pattern used in getRawTransaction for consistent behavior.
- Around line 132-134: Replace the debug console.log(123, error) in the catch
block with proper error handling: either call console.error with a clear
contextual message plus the error (e.g., "api-firo: <operation> failed" and
error.stack) or rethrow/propagate the error so callers can handle it; update the
catch surrounding the failing operation in api-firo.ts (the catch that currently
references the variable error) to perform one of these actions instead of the
numeric debug log.
- Line 127: The pkscript field is set to the literal string
"'res.vout[utx.vout].scriptPubKey.hex'" instead of the evaluated value; locate
the object construction that sets pkscript (look for the symbol pkscript in
api-firo.ts near where res and utx are used) and replace the quoted string with
the actual expression res.vout[utx.vout].scriptPubKey.hex so pkscript reads the
UTXO's scriptPubKey hex value (ensure utx.vout and res.vout exist and are
accessed as expressions, not strings).
In
`@packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue`:
- Around line 132-168: The style block in network-activity-total.vue is unscoped
causing .btn__anonymize (and other rules) to leak globally and duplicates rules
from synchronize-state.vue; fix by either adding the scoped attribute to the
<style> tag so .network-activity-total-wrapper and .btn__anonymize only apply to
this component, or extract the shared .btn__anonymize rules into a common LESS
partial/mixin (e.g., _buttons.less) and import that partial into both
network-activity-total.vue and synchronize-state.vue, then remove the duplicate
block from this file.
- Around line 23-26: The template is rendering
sparkAccount?.sparkBalance?.availableBalance raw; update the
NetworkActivityTotal component to format that value the same way as the existing
cryptoAmount display: convert the raw satoshi/high-precision value by dividing
by the token decimals (use sparkAccount.sparkBalance.decimals or the Firo
decimals constant) and apply locale-aware/crypto formatting (e.g.,
Intl.NumberFormat or the component's existing crypto formatting helper). Locate
the h6 block checking NetworkNames.Firo and replace the direct binding with the
formatted value, or add a computed property/method (e.g.,
formattedSparkAvailable or formatCryptoAmount) that returns the adjusted,
localized string for use in the template.
- Around line 89-94: The sparkAccount prop in network-activity-total.vue is
typed as PropType<SparkAccount | null> but its default is an empty object ({}),
which is truthy and can cause runtime errors when code assumes a real
SparkAccount or null; change the prop's default to null (e.g., default: () =>
null or default: null) so it matches the declared type and downstream checks (in
components/methods reading sparkAccount properties) behave correctly.
- Around line 77-80: The prop declaration for network in
network-activity-total.vue incorrectly types network as BitcoinNetwork; change
its PropType to BaseNetwork (update the type on the network prop) so it matches
actual data passed from parent components (network-activity and network-assets)
and remove the unsafe casts in those parents that coerce to BitcoinNetwork;
ensure the default remains an empty object cast appropriately to BaseNetwork if
needed and update any imports to reference BaseNetwork instead of
BitcoinNetwork.
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`:
- Around line 264-297: The broadcastTx call is currently fire-and-forget; wrap
api.broadcastTx(rawTx) in an awaited try/catch (or try/finally) so the enclosing
async function waits for completion and ensure isTransferLoading.value is reset
on both success and failure; inside the try await api.broadcastTx(rawTx) then
call trackSendEvents(SendEventType.SendComplete,...), update txActivity with the
txid and call activityState.addActivities(...), and in the catch call
trackSendEvents(SendEventType.SendFailed,...) set txActivity.status =
ActivityStatus.failed and call activityState.addActivities(...) and set
errorMsg.value, and finally always set isTransferLoading.value = false to avoid
permanently locking the button.
- Around line 198-199: Remove the two debug console.log calls that print
sensitive transaction details (the one referencing feeBn and firstUtxoAddress
and the one printing rawTxHex) from synchronize-state.vue; either delete them or
wrap them behind a non-production/debug guard (e.g., if (process.env.NODE_ENV
!== 'production' || this.debugMode) { ... }) or replace with a logger.debug call
that is disabled in prod, and ensure no rawTxHex or address/fee values are
logged in production.
- Around line 295-296: The assignment errorMsg.value = JSON.stringify(error)
produces "{}" for Error objects; update the handler that sets errorMsg (look for
errorMsg.value assignment near the synchronize-state component) to use the
error's readable text such as error.message or String(error) and include
error.stack if available, and keep the console.error call
(console.error('ERROR', error)) for full details; ensure the displayed message
is a plain string (e.g., error.message || String(error) + (error.stack ? '\n' +
error.stack : '')) so users see useful information.
- Around line 155-161: The txid is being converted to a Buffer using the default
UTF-8 encoding which yields wrong bytes; in the block building utxos (where
address: props.sparkAccount.defaultAddress, amount: amountToSendBN.toString(),
and utxos: spendableUtxos.map(...)) change the conversion to decode hex (use
Buffer.from(txid, 'hex')) and set txHashLength to the resulting byte length
(e.g., 32) or use Buffer.from(txid, 'hex').length so the txHash and txHashLength
passed into getMintTxData are the actual raw bytes and correct byte length.
- Around line 102-107: The prop sparkAccount is declared as type SparkAccount |
null but its default returns an empty object ({}), which is truthy and breaks
guards and template checks (e.g., usages that do if (sparkAccount) or access
sparkAccount.defaultAddress); change the prop default to null so it matches the
declared type and preserves null-check behavior — update the sparkAccount
PropType default factory from returning {} to returning null and keep the type
as PropType<SparkAccount | null> so runtime checks and template bindings behave
correctly.
- Around line 134-145: anonymizeFunds currently sets isTransferLoading.value =
true but lacks a top-level try/catch/finally so early returns or thrown errors
(e.g., the `return` when !props.sparkAccount or the `throw` on empty UTXOs)
leave the loader stuck; wrap the function body in try { ... } catch (err) {
errorMsg.value = err?.message || String(err); /* optionally log */ } finally {
isTransferLoading.value = false; } or alternatively move the early guard
(props.sparkAccount) before setting isTransferLoading and ensure any thrown
errors are caught and assigned to errorMsg.value, with isTransferLoading.value
reset in finally to guarantee the button is never permanently loading.
🧹 Nitpick comments (4)
packages/extension/src/providers/bitcoin/libs/api-firo.ts (1)
106-106: Method nameFiroToHaskoinUTXOsuses PascalCase.Instance methods should use camelCase (
firoToHaskoinUTXOs). PascalCase is conventionally reserved for classes/constructors/types.packages/extension/src/ui/action/views/verify-send-from-spark-transaction/index.vue (1)
162-162: Remove debugconsole.log.This looks like a development artifact. Consider removing or gating behind a debug flag.
packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue (2)
119-119: InstantiatingBaseFiroWalletat module scope runs on every component mount.
new BaseFiroWallet()is invoked unconditionally in<script setup>, regardless of whether the user ever clicks "Anonymize funds." If the constructor is heavyweight (wallet initialization, key derivation, etc.), this adds unnecessary overhead. Consider lazy-initializing it insideanonymizeFunds.
169-181: Hardcoded500satoshi fee placeholder in temp transaction.Line 170 uses
.minus(new BigNumber(500))as a placeholder fee for building the temporary transaction. This magic number is undocumented. Consider extracting it as a named constant with a comment, or using a minimum relay fee estimate.
| async getTransactionStatus(hash: string): Promise<BTCRawInfo | null> { | ||
| return fetch(`${this.node}/insight-api-zcoin/tx/${hash}`) | ||
| .then(res => res.json()) | ||
| .then((tx: FiroTxType) => { | ||
| if ((tx as any).message) return null; | ||
| if (tx.blockheight < 0) return null; | ||
| const rawInfo: BTCRawInfo = { | ||
| blockNumber: tx.blockheight, | ||
| fee: Number(tx.fees), | ||
| inputs: tx.vin | ||
| .filter(t => t.addresses && t.addresses.length) | ||
| .map(input => ({ | ||
| address: input.addresses![0], | ||
| value: Number(input.value), | ||
| })), | ||
| outputs: tx.vout | ||
| .filter( | ||
| t => t.scriptPubKey.addresses && t.scriptPubKey.addresses.length, | ||
| ) | ||
| .map(output => ({ | ||
| address: output.scriptPubKey.addresses![0], | ||
| value: Number(output.value), | ||
| pkscript: output.scriptPubKey.hex, | ||
| })), | ||
| transactionHash: tx.txid, | ||
| timestamp: tx.time * 1000, | ||
| }; | ||
| return rawInfo; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing error handling on getTransactionStatus fetch call.
Same concern as getRawTransaction — a network or JSON parse error will result in an unhandled rejection.
Proposed fix
return rawInfo;
- });
+ })
+ .catch(() => null);
}📝 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.
| async getTransactionStatus(hash: string): Promise<BTCRawInfo | null> { | |
| return fetch(`${this.node}/insight-api-zcoin/tx/${hash}`) | |
| .then(res => res.json()) | |
| .then((tx: FiroTxType) => { | |
| if ((tx as any).message) return null; | |
| if (tx.blockheight < 0) return null; | |
| const rawInfo: BTCRawInfo = { | |
| blockNumber: tx.blockheight, | |
| fee: Number(tx.fees), | |
| inputs: tx.vin | |
| .filter(t => t.addresses && t.addresses.length) | |
| .map(input => ({ | |
| address: input.addresses![0], | |
| value: Number(input.value), | |
| })), | |
| outputs: tx.vout | |
| .filter( | |
| t => t.scriptPubKey.addresses && t.scriptPubKey.addresses.length, | |
| ) | |
| .map(output => ({ | |
| address: output.scriptPubKey.addresses![0], | |
| value: Number(output.value), | |
| pkscript: output.scriptPubKey.hex, | |
| })), | |
| transactionHash: tx.txid, | |
| timestamp: tx.time * 1000, | |
| }; | |
| return rawInfo; | |
| }); | |
| } | |
| async getTransactionStatus(hash: string): Promise<BTCRawInfo | null> { | |
| return fetch(`${this.node}/insight-api-zcoin/tx/${hash}`) | |
| .then(res => res.json()) | |
| .then((tx: FiroTxType) => { | |
| if ((tx as any).message) return null; | |
| if (tx.blockheight < 0) return null; | |
| const rawInfo: BTCRawInfo = { | |
| blockNumber: tx.blockheight, | |
| fee: Number(tx.fees), | |
| inputs: tx.vin | |
| .filter(t => t.addresses && t.addresses.length) | |
| .map(input => ({ | |
| address: input.addresses![0], | |
| value: Number(input.value), | |
| })), | |
| outputs: tx.vout | |
| .filter( | |
| t => t.scriptPubKey.addresses && t.scriptPubKey.addresses.length, | |
| ) | |
| .map(output => ({ | |
| address: output.scriptPubKey.addresses![0], | |
| value: Number(output.value), | |
| pkscript: output.scriptPubKey.hex, | |
| })), | |
| transactionHash: tx.txid, | |
| timestamp: tx.time * 1000, | |
| }; | |
| return rawInfo; | |
| }) | |
| .catch(() => null); | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts` around lines 44 -
73, The getTransactionStatus function lacks error handling for network/JSON
errors; wrap the fetch chain in a try/catch (or append a .catch) inside
getTransactionStatus so any fetch/res.json errors are caught and handled,
returning null (or a resolved value) and logging the error via the same logging
mechanism used elsewhere; update the function name getTransactionStatus and
mirror the pattern used in getRawTransaction for consistent behavior.
| position: 0, | ||
| }, | ||
| index: utx.vout, | ||
| pkscript: 'res.vout[utx.vout].scriptPubKey.hex', |
There was a problem hiding this comment.
Bug: pkscript is a string literal instead of the actual expression.
'res.vout[utx.vout].scriptPubKey.hex' is assigned as a raw string rather than evaluating the property access. This means every UTXO will have a nonsensical pkscript value, breaking downstream transaction construction.
🐛 Proposed fix
- pkscript: 'res.vout[utx.vout].scriptPubKey.hex',
+ pkscript: res.vout[utx.vout].scriptPubKey.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.
| pkscript: 'res.vout[utx.vout].scriptPubKey.hex', | |
| pkscript: res.vout[utx.vout].scriptPubKey.hex, |
🤖 Prompt for AI Agents
In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts` at line 127, The
pkscript field is set to the literal string
"'res.vout[utx.vout].scriptPubKey.hex'" instead of the evaluated value; locate
the object construction that sets pkscript (look for the symbol pkscript in
api-firo.ts near where res and utx are used) and replace the quoted string with
the actual expression res.vout[utx.vout].scriptPubKey.hex so pkscript reads the
UTXO's scriptPubKey hex value (ensure utx.vout and res.vout exist and are
accessed as expressions, not strings).
| } catch (error) { | ||
| console.log(123, error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace debug logging with proper error handling.
console.log(123, error) is a debug artifact. Use console.error with a meaningful message, or propagate the error if appropriate.
Proposed fix
} catch (error) {
- console.log(123, error);
+ console.error('FiroToHaskoinUTXOs: failed to fetch tx details for', utx.txid, error);
}📝 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.
| } catch (error) { | |
| console.log(123, error); | |
| } | |
| } catch (error) { | |
| console.error('FiroToHaskoinUTXOs: failed to fetch tx details for', utx.txid, error); | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts` around lines 132 -
134, Replace the debug console.log(123, error) in the catch block with proper
error handling: either call console.error with a clear contextual message plus
the error (e.g., "api-firo: <operation> failed" and error.stack) or
rethrow/propagate the error so callers can handle it; update the catch
surrounding the failing operation in api-firo.ts (the catch that currently
references the variable error) to perform one of these actions instead of the
numeric debug log.
| async getUTXOs(pubkey: string): Promise<HaskoinUnspentType[]> { | ||
| const address = pubkey.length < 64 ? pubkey : this.getAddress(pubkey); | ||
|
|
||
| return fetch(`${this.node}/insight-api-zcoin/addr/${address}/utxo`) | ||
| .then(res => { | ||
| return res.json(); | ||
| }) | ||
| .then(async (utxos: FiroUnspentType[]) => { | ||
| if ((utxos as any).message || !utxos.length) return []; | ||
| return await this.FiroToHaskoinUTXOs(utxos); | ||
| }); | ||
| } |
There was a problem hiding this comment.
getUTXOs missing .catch() on the outer fetch chain.
If the fetch or JSON parsing fails, the promise rejects without a fallback. Consider returning [] on error for consistency with getBalance.
Proposed fix
.then(async (utxos: FiroUnspentType[]) => {
if ((utxos as any).message || !utxos.length) return [];
return await this.FiroToHaskoinUTXOs(utxos);
- });
+ })
+ .catch(() => []);
}📝 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.
| async getUTXOs(pubkey: string): Promise<HaskoinUnspentType[]> { | |
| const address = pubkey.length < 64 ? pubkey : this.getAddress(pubkey); | |
| return fetch(`${this.node}/insight-api-zcoin/addr/${address}/utxo`) | |
| .then(res => { | |
| return res.json(); | |
| }) | |
| .then(async (utxos: FiroUnspentType[]) => { | |
| if ((utxos as any).message || !utxos.length) return []; | |
| return await this.FiroToHaskoinUTXOs(utxos); | |
| }); | |
| } | |
| async getUTXOs(pubkey: string): Promise<HaskoinUnspentType[]> { | |
| const address = pubkey.length < 64 ? pubkey : this.getAddress(pubkey); | |
| return fetch(`${this.node}/insight-api-zcoin/addr/${address}/utxo`) | |
| .then(res => { | |
| return res.json(); | |
| }) | |
| .then(async (utxos: FiroUnspentType[]) => { | |
| if ((utxos as any).message || !utxos.length) return []; | |
| return await this.FiroToHaskoinUTXOs(utxos); | |
| }) | |
| .catch(() => []); | |
| } |
🤖 Prompt for AI Agents
In `@packages/extension/src/providers/bitcoin/libs/api-firo.ts` around lines 142 -
153, The getUTXOs function's outer fetch/JSON chain lacks a .catch handler so
network or parse errors reject the promise; update getUTXOs (and its fetch chain
that calls this.FiroToHaskoinUTXOs) to append a .catch that logs or swallows the
error and returns an empty array ([]) to match getBalance's behavior, ensuring
any fetch or res.json() failure results in [] instead of an unhandled rejection.
| <h6 v-if="!!sparkAccount && network.name === NetworkNames.Firo"> | ||
| <span>{{ sparkAccount?.sparkBalance?.availableBalance }}</span> Private | ||
| {{ symbol }} | ||
| </h6> |
There was a problem hiding this comment.
Raw availableBalance displayed without formatting.
sparkAccount?.sparkBalance?.availableBalance is rendered directly. If this is a satoshi value or a high-precision decimal, it may display as an unformatted long number. Consider formatting it (e.g., dividing by 10^decimals, applying locale-aware number formatting) to match the cryptoAmount display above.
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue`
around lines 23 - 26, The template is rendering
sparkAccount?.sparkBalance?.availableBalance raw; update the
NetworkActivityTotal component to format that value the same way as the existing
cryptoAmount display: convert the raw satoshi/high-precision value by dividing
by the token decimals (use sparkAccount.sparkBalance.decimals or the Firo
decimals constant) and apply locale-aware/crypto formatting (e.g.,
Intl.NumberFormat or the component's existing crypto formatting helper). Locate
the h6 block checking NetworkNames.Firo and replace the direct binding with the
formatted value, or add a computed property/method (e.g.,
formattedSparkAvailable or formatCryptoAmount) that returns the adjusted,
localized string for use in the template.
| const anonymizeFunds = async () => { | ||
| isTransferLoading.value = true; | ||
| errorMsg.value = undefined; | ||
| if (!props.sparkAccount) return; | ||
|
|
||
| const wasmModule = await wasmInstance.getInstance(); | ||
| const address2Check = await wallet.getTransactionsAddresses(); | ||
|
|
||
| const { spendableUtxos, addressKeyPairs } = | ||
| await wallet.getSpendableUtxos(address2Check); | ||
|
|
||
| if (spendableUtxos.length === 0) throw new Error('No UTXOs available!'); |
There was a problem hiding this comment.
Missing try/catch — unhandled throws leave isTransferLoading stuck at true.
anonymizeFunds is async but has no top-level try/catch. Lines 137 and 145 both have early-exit paths (return / throw) that never reset isTransferLoading.value = false. Any exception (WASM failure, wallet errors, PSBT signing, etc.) will leave the button in a permanent loading state and surface as an unhandled promise rejection.
Wrap the body in try/catch/finally:
Proposed structural fix
const anonymizeFunds = async () => {
isTransferLoading.value = true;
errorMsg.value = undefined;
- if (!props.sparkAccount) return;
+ if (!props.sparkAccount) {
+ isTransferLoading.value = false;
+ return;
+ }
+ try {
...
- api
- .broadcastTx(rawTx)
+ const { txid } = await api.broadcastTx(rawTx);
+ trackSendEvents(SendEventType.SendComplete, { network: props.network.name });
+ activityState.addActivities(
+ [{ ...txActivity, transactionHash: txid }],
+ { address: props.network.displayAddress(fromAddress), network: props.network.name },
+ );
+ } catch (error: any) {
+ trackSendEvents(SendEventType.SendFailed, { network: props.network.name, error: error.message });
+ txActivity.status = ActivityStatus.failed;
+ activityState.addActivities([txActivity], { address: fromAddress, network: props.network.name });
+ errorMsg.value = error.message ?? String(error);
+ console.error('anonymizeFunds error', error);
+ } finally {
+ isTransferLoading.value = false;
+ }
};🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`
around lines 134 - 145, anonymizeFunds currently sets isTransferLoading.value =
true but lacks a top-level try/catch/finally so early returns or thrown errors
(e.g., the `return` when !props.sparkAccount or the `throw` on empty UTXOs)
leave the loader stuck; wrap the function body in try { ... } catch (err) {
errorMsg.value = err?.message || String(err); /* optionally log */ } finally {
isTransferLoading.value = false; } or alternatively move the early guard
(props.sparkAccount) before setting isTransferLoading and ensure any thrown
errors are caught and assigned to errorMsg.value, with isTransferLoading.value
reset in finally to guarantee the button is never permanently loading.
| address: props.sparkAccount.defaultAddress, | ||
| amount: amountToSendBN.toString(), | ||
| utxos: spendableUtxos.map(({ txid, vout }) => ({ | ||
| txHash: Buffer.from(txid), | ||
| vout, | ||
| txHashLength: txid.length, | ||
| })), |
There was a problem hiding this comment.
Buffer.from(txid) uses UTF-8, not hex encoding.
Bitcoin transaction IDs are hex strings. Buffer.from(txid) defaults to UTF-8 encoding, producing a buffer of the literal ASCII characters rather than the decoded bytes. This will yield incorrect txHash values passed to getMintTxData.
Proposed fix
utxos: spendableUtxos.map(({ txid, vout }) => ({
- txHash: Buffer.from(txid),
+ txHash: Buffer.from(txid, 'hex'),
vout,
- txHashLength: txid.length,
+ txHashLength: txid.length / 2,
})),Note: txHashLength should also likely reflect the byte length (32 bytes for a txid), not the hex string character length (64).
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`
around lines 155 - 161, The txid is being converted to a Buffer using the
default UTF-8 encoding which yields wrong bytes; in the block building utxos
(where address: props.sparkAccount.defaultAddress, amount:
amountToSendBN.toString(), and utxos: spendableUtxos.map(...)) change the
conversion to decode hex (use Buffer.from(txid, 'hex')) and set txHashLength to
the resulting byte length (e.g., 32) or use Buffer.from(txid, 'hex').length so
the txHash and txHashLength passed into getMintTxData are the actual raw bytes
and correct byte length.
| console.log( | ||
| `🔹 Sending Change (${feeBn.toNumber() / 1e8} FIRO) to ${firstUtxoAddress}`, |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
Line 198 logs the fee/change address, and line 228 logs the full raw transaction hex. Logging raw transactions in production is a privacy concern and exposes implementation details. These should be removed or gated behind a debug flag.
Also applies to: 228-228
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`
around lines 198 - 199, Remove the two debug console.log calls that print
sensitive transaction details (the one referencing feeBn and firstUtxoAddress
and the one printing rawTxHex) from synchronize-state.vue; either delete them or
wrap them behind a non-production/debug guard (e.g., if (process.env.NODE_ENV
!== 'production' || this.debugMode) { ... }) or replace with a logger.debug call
that is disabled in prod, and ensure no rawTxHex or address/fee values are
logged in production.
| api | ||
| .broadcastTx(rawTx) | ||
| .then(({ txid }) => { | ||
| trackSendEvents(SendEventType.SendComplete, { | ||
| network: props.network.name, | ||
| }); | ||
| activityState.addActivities( | ||
| [ | ||
| { | ||
| ...txActivity, | ||
| ...{ transactionHash: txid }, | ||
| }, | ||
| ], | ||
| { | ||
| address: props.network.displayAddress(fromAddress), | ||
| network: props.network.name, | ||
| }, | ||
| ); | ||
| isTransferLoading.value = false; | ||
| }) | ||
| .catch(error => { | ||
| trackSendEvents(SendEventType.SendFailed, { | ||
| network: props.network.name, | ||
| error: error.message, | ||
| }); | ||
| txActivity.status = ActivityStatus.failed; | ||
| activityState.addActivities([txActivity], { | ||
| address: fromAddress, | ||
| network: props.network.name, | ||
| }); | ||
|
|
||
| errorMsg.value = JSON.stringify(error); | ||
| console.error('ERROR', error); | ||
| }); |
There was a problem hiding this comment.
broadcastTx promise is not awaited — function completes before broadcast finishes.
Because api.broadcastTx(rawTx).then(...).catch(...) is fire-and-forget (no await), the async function returns immediately. The .catch handler also never resets isTransferLoading.value = false, so a broadcast failure permanently locks the button in loading state. Use await (see the try/catch refactor above).
🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`
around lines 264 - 297, The broadcastTx call is currently fire-and-forget; wrap
api.broadcastTx(rawTx) in an awaited try/catch (or try/finally) so the enclosing
async function waits for completion and ensure isTransferLoading.value is reset
on both success and failure; inside the try await api.broadcastTx(rawTx) then
call trackSendEvents(SendEventType.SendComplete,...), update txActivity with the
txid and call activityState.addActivities(...), and in the catch call
trackSendEvents(SendEventType.SendFailed,...) set txActivity.status =
ActivityStatus.failed and call activityState.addActivities(...) and set
errorMsg.value, and finally always set isTransferLoading.value = false to avoid
permanently locking the button.
| errorMsg.value = JSON.stringify(error); | ||
| console.error('ERROR', error); |
There was a problem hiding this comment.
JSON.stringify(error) on an Error object produces "{}" — use error.message instead.
Error objects have non-enumerable properties, so JSON.stringify yields an empty object. This means errorMsg will always be "{}" on failure, providing no useful information to the user.
Proposed fix
- errorMsg.value = JSON.stringify(error);
+ errorMsg.value = error?.message ?? String(error);🤖 Prompt for AI Agents
In
`@packages/extension/src/ui/action/views/network-activity/components/synchronize-state.vue`
around lines 295 - 296, The assignment errorMsg.value = JSON.stringify(error)
produces "{}" for Error objects; update the handler that sets errorMsg (look for
errorMsg.value assignment near the synchronize-state component) to use the
error's readable text such as error.message or String(error) and include
error.stack if available, and keep the console.error call
(console.error('ERROR', error)) for full details; ensure the displayed message
is a plain string (e.g., error.message || String(error) + (error.stack ? '\n' +
error.stack : '')) so users see useful information.
Revert "Fix get balance"
Fix activity state and add reset DB on reset
…ture/add-firo-network
4 participants
Summary by CodeRabbit
New Features
Chores