feat(TXL-435): turn smart transactions on by default for new users #27885
Conversation
|
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. |
Builds ready [8624a66]
Page Load Metrics (2132 ± 134 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const isSmartTransaction = | ||
| currentSmartTransactionsEnabled && smartTransactionsOptInStatus; | ||
| useSelector(getSmartTransactionsPreferenceEnabled) && | ||
| currentSmartTransactionsEnabled; |
There was a problem hiding this comment.
What's the reason for not using getIsSmartTransaction from the selectors here?
There was a problem hiding this comment.
Even though the variable is called isSmartTransaction, the logic behind it (useSelector(getSmartTransactionsPreferenceEnabled) && useSelector(getCurrentSmartTransactionsEnabled)) isn’t obviously equivalent to useSelector(getIsSmartTransaction) at first glance.
This PR was mainly intended as a simple rename, but if you have any suggestions, I’d be happy to include them!
| /** | ||
| * Returns the user's explicit opt-in status for the smart transactions feature. | ||
| * | ||
| * Do not export this selector. It should only be used internally within this file. |
There was a problem hiding this comment.
Would you mind please adding to this comment a bit to help readers understand why it shouldn't be exported?
| @@ -280,7 +281,8 @@ export default function ReviewQuote({ setReceiveToAmount }) { | |||
| const unsignedTransaction = usedQuote.trade; | |||
| const { isGasIncludedTrade } = usedQuote; | |||
| const isSmartTransaction = | |||
| currentSmartTransactionsEnabled && smartTransactionsOptInStatus; | |||
| useSelector(getSmartTransactionsPreferenceEnabled) && | |||
| currentSmartTransactionsEnabled; | |||
There was a problem hiding this comment.
I think there are a few references on this page where we should now replace usage of smartTransactionsOptInStatus with isSmartTransaction because they have to do with whether or not a smart transaction will occur rather than the opt-in status for metrics purposes:
There was a problem hiding this comment.
Hi @martahj – good catch(es). I've replaced all of these with smartTransactionsPreferenceEnabled.
|
Builds ready [6d371dc]
Page Load Metrics (1817 ± 122 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-12.7.0
gauthierpetetin
added
release-12.6.0
Description
This PR:
To enable stx by default, we needed to replace the
getSmartTransactionsOptInStatusselector with two distinct selectors:getSmartTransactionsOptInStatusForMetricsgetSmartTransactionsPreferenceEnabledPreviously, the
getSmartTransactionsOptInStatusselector was doing double duty for the user's opt-in status and deciding whether the preference was enabled. Since the feature was disabled by default, and a user that has not opted-in or out of smart transactions has an opt-in status of null, this did not pose a problem.However, since we decided to enable smart transactions by default, we needed a separate selector for checking the preference for deciding to enable stx.
Why not keep the
getSmartTransactionsOptInStatusselector as-is and just add a new one?We considered keeping the
getSmartTransactionsOptInStatusselector and adding a new one. However, by renaming it togetSmartTransactionsOptInStatusForMetrics, we avoid any confusion and make it clear that it is used for metrics collection and not for user preference handling.Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/TXL-435
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist