feat: enable STX by default with migration and notification #28854
+1,792
−48
Conversation
This file contains 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
…es-controller with `smartTransactionsOptInStatus` in `controllerMetadata`
…ler and migration's state transformation.
|
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. |
…transaction flows that occur when experimental/Improved transaction requests is not toggled on.
|
I have read the CLA Document and I hereby sign the CLA |
dan437
reviewed
Dec 4, 2024
ui/pages/confirmations/components/stx-banner-alert/stx-banner-alert.js
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/transaction-alerts/transaction-alerts.js
Outdated
Show resolved
Hide resolved
…passing, update console.logs to debug in migration code for testing manually
… alert dismissal issue.
…he user has seen our message and has dismissed.
Update stx-migration ducks test.
…or without complex store mocking.
This comment was marked as outdated.
This comment was marked as outdated.
…ing message. When a user dismisses the alert (via close or "Learn more"), their preference is stored and persists across transaction sessions. The dismissal is handled through the alerts state system and uses the existing setAlertEnabledness functionality. Remove comments.
…sts for the STXBannerAlert and TransactionAlerts tests.
…ion flow. Does not let me close alert or show link.
dan437
force-pushed
the
feat/enable-stx-migration
branch
from
733d66f to
202e7d6
Compare
dan437
reviewed
Dec 19, 2024
| data: { | ||
| PreferencesController?: { | ||
| preferences?: { | ||
| smartTransactionsOptInStatus?: boolean | null; |
There was a problem hiding this comment.
Just a reminder that smartTransactionsOptInStatus is still here twice.
| describe('Swap Eth for another Token @no-mmi', function () { | ||
| // TODO: (MM-PENDING) These tests are planned for deprecation as part of swaps testing revamp | ||
| // eslint-disable-next-line mocha/no-skipped-tests | ||
| describe.skip('Swap Eth for another Token @no-mmi', function () { |
There was a problem hiding this comment.
Why is this skipped? It tests happy path for Swaps, which is the most important thing to test for Swaps. It should be only removed when a testing revamp happens and there is a replacement E2E test for this.
| const notOptedInState = { | ||
| metamask: { | ||
| alertEnabledness: { | ||
| [AlertTypes.smartTransactionsMigration]: true, |
There was a problem hiding this comment.
Why is this set to true when migration happens and STX opt in status is false? Shouldn't we set alertEnabledness to false in that case?
| ] !== false, | ||
| ); | ||
|
|
||
| const smartTransactionsOptIn = useSelector( |
There was a problem hiding this comment.
There is an existing selector for this: getSmartTransactionsOptInStatusInternal
| state.metamask.preferences?.smartTransactionsOptInStatus === true, | ||
| ); | ||
|
|
||
| const smartTransactionsMigrationApplied = useSelector( |
There was a problem hiding this comment.
We shouldn't be having selectors in React components, this could be for example in: shared/modules/selectors/smart-transactions.ts
| React.useEffect(() => { | ||
| if (alertEnabled && !smartTransactionsOptIn) { | ||
| dispatch({ | ||
| type: 'alert/dismiss', |
There was a problem hiding this comment.
Isn't there some constant for this?
|
|
||
| const handleDismiss = useCallback(() => { | ||
| dispatch({ | ||
| type: 'alert/dismiss', |
There was a problem hiding this comment.
Same comment about a constant. Also, I see that this code is duplicated. Can you have the dispatch and setAlertEnabledness calls in one function which you can reuse?
...confirmations/components/smart-transactions-banner-alert/smart-transactions-banner-alert.tsx
Show resolved
Hide resolved
| networkConfigurationsByChainId: { | ||
| [CHAIN_ID_MOCK]: { | ||
| chainId: CHAIN_ID_MOCK, | ||
| // other required network properties |
There was a problem hiding this comment.
You can remove this comment.
There was a problem hiding this comment.
will do, I will check for others too.
Builds ready [fd00aa9]
Page Load Metrics (1776 ± 108 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…have already removed in `preference-controller` and I forgot to remove from the migration. Removed unneeded comments.
…ptInStatus` from root level of PreferencesController. Also, cleaned up migration test for readability.
…ask-extension into feat/enable-stx-migration
Builds ready [20cca27]
Page Load Metrics (1787 ± 100 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…ore or without migration) as conditions are already tested individually.
- Replace inline Redux selector with `getSmartTransactionsOptInStatusInternal` - Add `getSmartTransactionsMigrationAppliedInternal` for consistent state handling - Centralize banner visibility logic in `alertConditions` for clarity - Consolidate alert dismissal logic in `dismissAlert` and remove redundant `dispatch` - Simplify state typing and remove unused types - Merge duplicate dismissal tests for shared functionality Alert state is now managed via AlertController's `setAlertEnabledness`. Selectors are reusable, logic is more maintainable, and tests better reflect component behavior.
…-transactions-alert-banner.test.tsx
Builds ready [dcf814c]
Page Load Metrics (1664 ± 86 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
matthewwalsh0
previously approved these changes
Dec 20, 2024
Builds ready [8385fce]
Page Load Metrics (1655 ± 77 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [f4b14dc]
Page Load Metrics (1906 ± 78 ms)
|
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.
Description
This PR enables Smart Transactions (STX) by default through migration number 135 for users who have either opted out or haven't interacted with the STX toggle, provided they have no recorded STX activity.
How it works:
In the case a user migrates from a previous version of the extension and the migration runs and sets STX toggle "ON" in
Settings > Advanced > Smart Transactions, they will receive an STX Banner Alert on transaction confirmation screens until dismissed through a close button, or by clicking on the "Higher success rates" link within the alert that goes to: What is 'Smart Transactions'? for more information.Edge Cases:
If a user is new and setting up a wallet for the first time, they will not receive the Banner Alert. If a user imports a new wallet during a fresh install of the extension on a new browser or recovers a wallet, it's possible they may not see the alert if STX was on in a previous install. The STX Banner Alert is dismissed and will not show again if a user is in the state to get shown the banner and toggles STX off independently even if they do not physically dismiss the STX Banner Alert.
Migration Logic:
smartTransactionsOptInStatusisnull(new/never interacted)UI Components:
The notification system bridges the migration changes with the UI, ensuring users are informed of the STX enablement while maintaining their ability to opt out through settings.
Target release: TBD
Affected user base: ~5.7M users who previously opted out of STX but have no STX activity.
Related issues
Fixes: N/A
Running Unit Tests
Migration Test:
Smart Transaction Banner Component Test:
Confirm Transaction base Test:
Preferences Controller Test:
Transaction Alerts Component Test:
Manual testing steps
Test Migration (using a wallet/account with no STX Transactions)
git checkout tags/v12.5.0and run:dist/chromedirectorySettings > Advanced > Smart Transactionsgit checkout feat/enable-stx-migrationSettings > Advanced > Smart TransactionsTest STX Banner Alert that it shows on Transaction Confirmations and not Sign Confirmations)
(using new confirmations flow)
Improved transaction requestsis ON inSettings > Experimental0.0001ETHTest STX Banner Alert that it shows on Transaction Confirmations and not Sign Confirmations)
(using old confirmations flow)
Improved transaction requestsis OFF inSettings > Experimental0.0001ETHImproved transaction requestsback to ON inSettings > Experimental0.0001ETHCongrats, you have manually tested the happy path, now we just need to test the edge cases:
0.0001ETHSettings > Advanced > Smart Transactions0.0001ETHTest edge case that after STX migration runs and Banner is being shown that clicking on "Higher success rates" link:
0.0001ETHSettings > Advanced > Smart Transactions0.0001ETHBecause the NEW confirmation flow does not support alerts using hooks that are dismissible, we have used the old style Banner Alert, and it is normal for their to be some variation on where the alert shows up and it's surroundings. But overall they should look similar.
Screenshots/Recordings
Before
After
Pre-merge author checklist
nullopt-in statusfalseopt-in status with no STX activityfalseopt-in status with existing STX activitytrueopt-in statusPre-merge reviewer checklist