fix: set is_smart_transaction=true for cancelled/dropped smart transactions#26479
Conversation
…ctions Cancelled or dropped smart transactions never get mined, so getSmartTransactionByMinedTxHash() returns nothing and is_smart_transaction was never set. This caused "Smart transaction failed" errors to appear under is_smart_transaction=false in metrics. Since this function is only called when smart transactions are enabled for the chain, return is_smart_transaction: true even when the mined hash lookup fails.
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
| // smart transactions are enabled for the chain. Cancelled/dropped smart | ||
| // transactions won't have a mined tx hash, so the lookup above returns | ||
| // nothing, but the transaction still went through the smart transaction flow. | ||
| return { is_smart_transaction: true }; |
There was a problem hiding this comment.
Non-smart txs may be misclassified
Medium Severity
getSmartTransactionMetricsProperties() now returns { is_smart_transaction: true } when getSmartTransactionByMinedTxHash() returns nothing. If this function is called for any finalized transaction while smart transactions are merely enabled/opted-in (but the specific transactionMeta did not actually go through the smart transaction flow, or controller state was lost), metrics will incorrectly label it as a smart transaction.
There was a problem hiding this comment.
Bugbot's concern doesn't hold up. Looking at stx.ts:26-31, getSmartTransactionMetricsProperties is only called when selectShouldUseSmartTransaction returns true for the transaction's chain.
The gate is chain-level, not per-transaction, but if STX is enabled for a chain, all transactions on that chain go through the smart transaction publish hook in transaction-controller-init.ts:247-259. There's no per-transaction opt-out.
The only exception would be relay deposit child transactions (created with requireApproval: false in the relay-submit strategy), but those get their own "Transaction Finalized" events with their own types.
So the scenario described -- a transaction on an STX-enabled chain that somehow skips the STX flow -- doesn't happen in practice.
There was a problem hiding this comment.
Renaming smartTransaction to may be successfulSmartTransaction so it is more clear.
There was a problem hiding this comment.
but isn't success/failed handled by status and error fields? I feel like is_smart_transaction should indicate whether or not it was broadcasted to the STX system, not whether or not the STX system succeeded
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: This is purely a metrics/analytics tracking fix that:
The change is well-contained and the unit tests adequately validate the fix. No E2E tests are needed as this doesn't affect any user-visible functionality or transaction flows. Performance Test Selection: |
| // smart transactions are enabled for the chain. Cancelled/dropped smart | ||
| // transactions won't have a mined tx hash, so the lookup above returns | ||
| // nothing, but the transaction still went through the smart transaction flow. | ||
| return { is_smart_transaction: true }; |
There was a problem hiding this comment.
Agreed, I'd go even further and align with extension which just sets it to true if the preference is enabled, the chain is supported, and the flags are enabled.
There is no additional checks based on the individual transaction itself and the resulting hash etc.
We have dedicated properties in extension for the other states such as smart_transaction_timed_out.
I'm also concerned why we're awaiting transaction events while just generating metrics, that could risk delaying or omitting portions of the metrics.
Ideally we'd have a single function that generates all the STX properties for a given transaction that encapsulates the preference and chain ID and feature flag checks, so we don't have to assume the caller has done any checks.
There was a problem hiding this comment.
Yes agreed I think that makes sense for a future refactor.
Description
Cancelled or dropped smart transactions never get mined, so
getSmartTransactionByMinedTxHash()returns nothing andis_smart_transactionwas never set. This caused "Smart transaction failed" errors to appear underis_smart_transaction=falsein metrics.Since
getSmartTransactionMetricsProperties()is only called when smart transactions are enabled for the chain (gated byselectShouldUseSmartTransactioninstx.ts), we can safely return{ is_smart_transaction: true }even when the mined hash lookup fails — the transaction still went through the smart transaction flow.Context
In PR #21027,
return {}was intentionally introduced for the!smartTransactioncase to fix a bug whereundefinedsmart transactions were being misclassified asis_smart_transaction: true(due to!smartTransaction?.statusMetadataevaluating totrueforundefined).That fix was correct for the general case, but it didn't account for cancelled/dropped smart transactions — these go through the STX flow but never produce a mined hash, so
getSmartTransactionByMinedTxHash()returns nothing. Since this function is only reachable when STX is enabled for the chain, returning{ is_smart_transaction: true }here is safe and gives us accurate metrics for failed STX.Changelog
CHANGELOG entry: null
Related issues
N/A
Manual testing steps
This is a metrics-only change.
is_smart_transactionis not used in any business logic, UI rendering, or transaction routing — it is purely an analytics property on the "Transaction Finalized" event.Verification:
app/util/smart-transactions/index.test.tsis_smart_transaction: truein the "Transaction Finalized" event in MixpanelScreenshots/Recordings
N/A — no UI changes.
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Low risk metrics-only change: adjusts analytics properties when a smart transaction record can’t be found, with a unit test update to lock in the new behavior.
Overview
getSmartTransactionMetricsPropertiesnow returns{ is_smart_transaction: true }(instead of{}) when no smart transaction is found by mined hash, ensuring cancelled/dropped smart transactions are still classified as smart transactions in analytics.Updates the corresponding unit test to expect
is_smart_transaction: truein this no-record/no-wait scenario.Written by Cursor Bugbot for commit 8184363. This will update automatically on new commits. Configure here.