feat: enhance transaction alert scanning#138
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Enhances the snap’s transaction scanning and confirmation UX by adding more robust error localization/mapping (including new simulation error codes), improving estimated-change rendering for Stellar assets (native/classic/SEP-41), and introducing a local preflight check for expired transactions before calling the Security Alerts API.
Changes:
- Added
TransactionScanErrorId+ expanded localized scan error coverage (e.g., trustline missing, transaction expired) and updated simulation error copy. - Refactored scan response mapping to output string display balances (no scientific notation) and derive icons/metadata for native/classic/SEP-41 assets.
- Added local preflight validation for expired transactions and introduced Security Alerts API response fixtures for more realistic tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/snap/src/ui/confirmation/components/TransactionAlert.tsx | Adds localized error IDs for new scan error codes used in the alert banner. |
| packages/snap/src/ui/confirmation/components/TransactionAlert.test.tsx | Updates simulation title assertion and adds coverage for the new expired-tx message path. |
| packages/snap/src/ui/confirmation/components/EstimatedChanges/EstimatedChanges.tsx | Stops formatting numeric values locally; renders the scan-provided display string. |
| packages/snap/src/ui/confirmation/components/EstimatedChanges/EstimatedChanges.test.tsx | Updates tests to expect string-valued asset changes. |
| packages/snap/src/services/transaction-scan/TransactionScanService.ts | Adds preflight timebound check, refactors asset mapping, and normalizes error codes for localization. |
| packages/snap/src/services/transaction-scan/TransactionScanService.test.ts | Reworks tests around realistic fixtures; adds preflight validation coverage and asset-mapping scenarios. |
| packages/snap/src/services/transaction-scan/SecurityAlertsApiClient.ts | Renames/aligns request typing for the scan endpoint payload. |
| packages/snap/src/services/transaction-scan/api.ts | Introduces TransactionScanErrorId, new asset structs (native/classic/SEP-41), and changes estimated-change value to `string |
| packages/snap/src/services/transaction-scan/mocks/security-alerts-api-response.fixture.ts | Adds realistic canned Blockaid responses to use in service tests. |
| packages/snap/src/handlers/keyring/signTransaction.ts | Removes fee computation/simulation step before confirmation/signing; relies on dapp-provided fee. |
| packages/snap/src/handlers/keyring/signTransaction.test.ts | Updates handler setup and removes the fee-simulation failure test. |
| packages/snap/src/handlers/cronjob/refreshConfirmationContext/scanRefresher.test.ts | Updates expected estimated-change value fields to strings. |
| packages/snap/src/handlers/clientRequest/confirmSend.ts | Seeds estimated changes using the string amount directly (no Number(...)). |
| packages/snap/src/handlers/clientRequest/confirmSend.test.ts | Updates expected seeded estimated-change values to strings. |
| packages/snap/src/context.ts | Removes transactionService injection from SignTransactionHandler wiring. |
| packages/snap/messages.json | Updates simulation error title copy and adds new scan error messages. |
| packages/snap/locales/es.json | Mirrors messages.json updates for the ES locale file. |
| packages/snap/locales/en.json | Mirrors messages.json updates for the EN locale file. |
| packages/snap/.env.example | Updates the example static API base URL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+74
to
+76
| // We do not process RPC simulation here, we trust the fee that provided by the dapp. | ||
| // If the transaction is invalid, the security scan will output the error. | ||
| if (!(await this.#confirmation(request, transaction, account))) { |
Collaborator
Author
There was a problem hiding this comment.
this is expected, we trust the dapp
| { | ||
| type: AssetChangeDirection.Out, | ||
| value: Number(amount), | ||
| value: amount, |
Collaborator
Author
There was a problem hiding this comment.
we use string instead of number to display
|
|
||
| import { KnownCaip2ChainId, XdrStruct } from '../../api'; | ||
|
|
||
| /** |
Collaborator
Author
There was a problem hiding this comment.
re structure the api struct
| options: TransactionScanOption[]; | ||
| }): Promise<TransactionScanResult | null> { | ||
| try { | ||
| const preflightValidationErrorResult = options.includes( |
Collaborator
Author
There was a problem hiding this comment.
add preflight check to handle some check that blockaids not supported
| // failures. | ||
| const error = | ||
| simulationError ?? | ||
| preflightValidationError ?? |
Collaborator
Author
There was a problem hiding this comment.
preflightValidationError should display if simulationError does not hv error
| ): StellarAssetDiff[] { | ||
| return ( | ||
| simulation.assets_diffs?.[accountAddress] ?? | ||
| simulation.account_summary.account_assets_diffs ?? |
Collaborator
Author
There was a problem hiding this comment.
simulation.account_summary.account_assets_diffs is useless
| * @param value - The human-readable asset amount, or null. | ||
| * @returns The amount as a plain decimal string. | ||
| */ | ||
| function formatValue(value: number | null): string { |
Collaborator
Author
There was a problem hiding this comment.
no use anymore
|
|
||
| # Static API Base URL | ||
| STATIC_API_BASE_URL=https://static.api.cx.metamask.io | ||
| STATIC_API_BASE_URL=https://static.cx.metamask.io |
Collaborator
Author
There was a problem hiding this comment.
previous url is incorrect
| // Map them to a known error id. | ||
| // - "insufficient balance" to "insufficientbalance". | ||
| // - "no trustline" to "notrustline". | ||
| if (message.toLowerCase().includes('insufficient balance')) { |
Collaborator
Author
There was a problem hiding this comment.
try our best to map the error into some expected error
so instead of show a branch of error code, we show some readable error
| return TransactionScanErrorId.NoTrustline; | ||
| } | ||
| return ( | ||
| this.#getErrorCode(message) ?? TransactionScanErrorId.InvalidTransaction |
Collaborator
Author
There was a problem hiding this comment.
hide the error message , and using invalid transaction
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Enhances the snap’s transaction scanning and confirmation UX by adding more robust error localization/mapping (including new simulation error codes), improving estimated-change rendering for Stellar assets (native/classic/SEP-41), and introducing a local preflight check for expired transactions before calling the Security Alerts API.
References
Checklist