fix(stellar-wallet-snap): display transaction error message + skip security scanning when failed during the background validation in confirmation dialog - #282
Conversation
| */ | ||
| export function getTxnErrorMessageKey( | ||
| error: TransactionValidationException, | ||
| error: unknown, |
There was a problem hiding this comment.
change to unknown to accept any error
| transactionService.createValidatedSendTransaction.mockRejectedValueOnce( | ||
| new Error('insufficient balance'), | ||
| ); | ||
| it.each([ |
There was a problem hiding this comment.
add all coverage to the test
There was a problem hiding this comment.
🟡 Changes recommended
Writing securityScanRequest: null can invalidate the scan refresher context shape and cause the refresh handler to skip all subsequent refresh cycles when scan refresh is enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Stellar wallet snap’s background confirmation re-validation flow so that when the pending transaction becomes invalid during periodic refresh, the UI can display a mapped (localized) transaction error message.
Changes:
- Add
errorMessage(mapped viagetTxnErrorMessageKey) to the transaction refresher’s error result when background re-validation fails. - Expand transaction refresher tests to cover multiple transaction-validation error subclasses and assert the expected message keys.
- Widen
getTxnErrorMessageKey’s input type tounknownand record the behavior change in the package changelog.
File summaries
| File | Description |
|---|---|
| packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts | Adds mapped transaction error message to the refresh result when re-validation fails; attempts to suppress scan work on failure. |
| packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts | Converts the single failure test into a parameterized suite covering specific validation exceptions and expected message keys. |
| packages/stellar-wallet-snap/src/handlers/clientRequest/utils.ts | Broadens getTxnErrorMessageKey’s parameter type from a specific exception to unknown. |
| packages/stellar-wallet-snap/CHANGELOG.md | Adds a “Fixed” entry describing the new error banner mapping behavior and scan-skip behavior on failure. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expect(result).toStrictEqual({ | ||
| result: { | ||
| transactionsFetchStatus: FetchStatus.Error, | ||
| errorMessage, | ||
| securityScanRequest: null, | ||
| }, | ||
| reschedule: false, | ||
| }); |
There was a problem hiding this comment.
it is not valid
local Transaction validation always go first, and set securityScanRequest to null if any error
in security scan there is a method shouldFetch, if securityScanRequest is null, the whole scan will be skip
shouldFetch(ctx: ConfirmationDataContext): boolean {
const scanCtx = ctx as SecurityScanContext;
if (scanCtx.scanFetchStatus === FetchStatus.Error) {
return false;
}
if (!scanCtx.securityScanRequest) {<---this will skip the scan
return false;
}
return this.#getScanOptions(scanCtx).length > 0;
}
| return { | ||
| result: { transactionsFetchStatus: FetchStatus.Error }, | ||
| result: { | ||
| transactionsFetchStatus: FetchStatus.Error, | ||
| errorMessage: getTxnErrorMessageKey(error, accountAddress), | ||
| // Skip the security scan request to avoid wasting time on a failed transaction. | ||
| securityScanRequest: null, | ||
| }, |
There was a problem hiding this comment.
same as above, this is not a valid comment
| const { request, accountId, scope, securityScanRequest, origin } = | ||
| validationCtx; | ||
| // Use the scan request address as Default if it is present. | ||
| let accountAddress = securityScanRequest?.accountAddress ?? ''; |
There was a problem hiding this comment.
on a re-scan process, address will be already extracted in securityScanRequest?.accountAddress
this is just best effort
| }); | ||
| accountAddress = onChainAccount.accountId; | ||
|
|
||
| // TODO(follow-up): this validates a rebuilt draft as a proxy for the stored |
There was a problem hiding this comment.
comment not valid anymore
| const { securityScanRequest, origin } = validationCtx; | ||
| const rebuiltTransactionXdr = rebuiltTransaction.getRaw().toXDR(); | ||
|
|
||
| // Always feed the rebuilt envelope to the scan refresher. The user-facing |
There was a problem hiding this comment.
comment not valid anymore
| transactionsFetchStatus: FetchStatus.Error, | ||
| errorMessage: getTxnErrorMessageKey(error, accountAddress), | ||
| // Skip the security scan request to avoid wasting time on a failed transaction. | ||
| securityScanRequest: null, |
There was a problem hiding this comment.
stop the security scanning for whatever reason that we cant scan locally
stanleyyconsensys
changed the title
|
Explanation
This PR:
References
Ticket: https://consensyssoftware.atlassian.net/browse/WPN-2027
https://www.loom.com/share/7a961d45b97b4bc5ba1fce2a98f6307f
Checklist