feat: enable STX by default with migration and notification #12857
Conversation
- Enable STX for new users or those that have not enabled in settings - Preserves settings for users who explicitly opted out and have existing STX transactions - Adds a migration flag to track this change
…ror messages using team's established pattern.
- Mock Sentry error handling ensuring we can track when errors are captured - Setup `beforeEach()` restore/reset for all mocks - Define test cases for error handling - Each case has: invalid input state, expected error message, and description - Invalid States Test: how migration handles malformed data - Runs each invalid state test case - Verifies: Invalid state returns unchanged, Error is captured by Sentry, Error message matches expected format Also, Setup first test to ensure STX is enabled and migration flag is set when undefined opt-in status
- Null status test, similar to undefined opt-in status test - Tests migration with null status - Verifies that STX is enabled and migration flag is set
httpJunkie
added
team-confirmations
|
CLA Signature Action: Thank you for your submission, we really appreciate it. We ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:
By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository. 0 out of 1 committers have signed the CLA. |
- create opt-out test - tests that users explicit opt-out is respected when STX transactions exist - verifies migration flag is set but preference is unchanged
- create opt-out test - tests that users explicit opt-out is respected when STX transactions exist - verifies migration flag is set but preference is unchanged
- tests that opt-out is overridden when user has no transaction history - verifies migration flag is set and preference is changed
- added test for initializing preferences - added test for when STX is already enabled - use `merge` with `initialRootState` consistently across all tests
app/store/migrations/065.test.ts
Outdated
| const invalidStates = [ | ||
| { | ||
| state: null, | ||
| errorMessage: "FATAL ERROR: Migration 65: Invalid state error: 'null'", |
There was a problem hiding this comment.
nit: you can save 65 into a constant and use that one. E.g.
const migrationVersion = 65;
- uses redux selectors to access the state - checks flags we need (opt-in status and migration applied) - Returns (isEnabled, isMigrationApplied) for our banner to use - Provides proper TypeScript types
- check default states - ensure proper state reading - handle partial states
- test conditional rendering - check text content - ensure close button functionality works - ensure link functionality works
- useSmartTransactionsEnabled enhanced to include dismiss functionality - add a dispatch action to update the smartTransactionsOptInStatus - remove external onClose prop from SmartTransactionsEnabledBanner in favor of using the hook's dismiss functionality - update component and hook tests to account for new functionality These changes require more changes in the next commit to add the smartTransactionsBannerDismissed field to our PreferencesController state as well to add a reducer case for SET_SMART_TRANSACTIONS_BANNER_DISMISSED and update any types/interfaces in our codebase that reference PreferencesController.
…nitialize it. Update the migration and test to account for the new change
app/actions/settings/index.js
Outdated
| @@ -74,3 +74,10 @@ export function setTokenSortConfig(tokenSortConfig) { | |||
| tokenSortConfig, | |||
| }; | |||
| } | |||
|
|
|||
| export function setSmartTransactionsBannerDismissed(bannerDismissed) { | |||
There was a problem hiding this comment.
nit: to make it clearer, I would call it setSmartTransactionsMigrationBannerDismissed
app/actions/settings/index.js
Outdated
|
|
||
| export function setSmartTransactionsBannerDismissed(bannerDismissed) { | ||
| return { | ||
| type: 'SET_SMART_TRANSACTIONS_BANNER_DISMISSED', |
There was a problem hiding this comment.
SET_SMART_TRANSACTIONS_MIGRATION_BANNER_DISMISSED
| import { SmartTransactionsEnabledBannerProps } from './SmartTransactionsEnabledBanner.types'; | ||
| import useSmartTransactionsEnabled from '../../../../hooks/useSmartTransactionsEnabled/useSmartTransactionsEnabled'; | ||
|
|
||
| const SMART_TRANSACTIONS_LEARN_MORE = AppConstants.URLS.SMART_TXS; |
There was a problem hiding this comment.
Since this constant is only used in 1 place, you can just delete it and use it there directly. I would also rename it a little:
Linking.openURL(AppConstants.URLS.SMART_TRANSACTIONS);
|
|
||
| const SET_SMART_TRANSACTIONS_BANNER_DISMISSED = 'SET_SMART_TRANSACTIONS_BANNER_DISMISSED'; | ||
|
|
||
| const useSmartTransactionsEnabled = () => { |
There was a problem hiding this comment.
useSmartTransactionsEnabled naming is very similar to existing selectSmartTransactionsEnabled, which can lead to confusion.
If you see the selectShouldUseSmartTransaction selector, it checks 2 things:
- isSmartTransactionsEnabled, which indicates if the STX feature is even available, no matter what's in Settings
- smartTransactionsOptInStatus, which indicates if it's on or off in Settings
Both have to be true in order for the mobile app to use smart transactions.
I see that in the hooks folder not many files use the state: RootState code. Either we should rename this hook since there is another function with a very similar name and different meaning or we can create individual selectors for isMigrationApplied, isBannerDismissed, etc. and remove this hook
| @@ -0,0 +1,12 @@ | |||
| import { StyleSheet, ViewStyle } from 'react-native'; | |||
There was a problem hiding this comment.
Why don't we call this and other files SmartTransactionsMigrationBanner?
…is idea did not pan out.
…nsure that styles are applied to match the mockup design and add the Banner Alter to the SendFlow Confirmation page.
…k is viewed. Update useSmartTransactionsEnabled hook to set feature flags for smartTransactionsBannerDismissed. Remove action from app/actions/settings. Test is still failing (TBD)
- update the Engine mock to be more explicit - mock both the default export and the named exports of the Engine module
… to ensure: - mock is clean at start - mock was called exactly once - arguments are correct
- Make types in PreferencesState more accurate knowing the values will exist after migration - Prevent unnecessary null checks and TS hints and error checking - Memoize shouldShowBanner calculation to prevent unnecessary recalculations when other state changes, follows basic practices for derived state only recalculating when dependencies changes
- Move smartTransactionsBannerDismissed from PreferencesController root to featureFlags object - Add type safety improvements for feature flags initialization - Add SmartTransactionsController state type guard - Update migration test to check for banner state in featureFlags instead of root
|
- covers token transfer approval - covers dapp and contract interaction like deploy a contract and mint
Description
This PR enables Smart Transactions (STX) in MetaMask Mobile by default through migration number (TBD) 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 (if user does not have STX enabled or prior STX Transaction history:
In the case a user migrates from a previous version of the Mobile app and the migration runs and sets STX toggle "ON" in
Settings > Advanced > Smart Transactions, they will receive an Alert on transaction confirmation screens until dismissed, or by clicking on the "Higher success rates" link within the alert. If they click on the link in the banner alert they will get sent to: What is 'Smart Transactions'? for more information. When returning to the confirmation they just navigated from the banner alert should not show and should never show again.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:
TBD, still exploring the Mobile UI for implementation solution.
Running Unit Tests
Migration Test:
yarn jest "./app/store/migrations/096.test.ts" --no-cacheHook Test:
yarn jest "./app/components/hooks/useSmartTransactionsEnabled/useSmartTransactionsEnabled.test.ts" --no-cacheComponent Test:
yarn jest "./app/components/Views/confirmations/components/SmartTransactionsEnabledBanner/SmartTransactionsEnabledBanner.test.ts" --no-cacheManual testing steps
Test Migration (using a wallet/account with no STX Transactions)
Start with an older repo version:
Terminal #1
yarn setup && yarn watchTerminal #1
Import or setup a wallet without STX transactions, launch the wallet (do not enable STX if prompted), check that toggle is OFF in:
Settings > Advanced > Smart TransactionsSwitch to feature branch and run app:
Terminal #1
yarn setup && yarn watchTerminal #1
0.0001ETH7 Ensure that Smart Transactions Banner Alert IS showing
0.0001ETHScreenshots/Recordings
N/A yet...
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist