-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: enable STX by default with migration and notification #28854
Open
httpJunkie
wants to merge
136
commits into
main
Choose a base branch
from
feat/enable-stx-migration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+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.
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)
|
Builds ready [dac3904]
Page Load Metrics (1843 ± 70 ms)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
release-12.11.0
Issue or pull request that will be included in release 12.11.0
team-confirmations
Push issues to confirmations team
team-transactions
Transactions team
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:
smartTransactionsOptInStatus
isnull
(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.0
and run:dist/chrome
directorySettings > Advanced > Smart Transactions
git checkout feat/enable-stx-migration
Settings > Advanced > Smart Transactions
Test STX Banner Alert that it shows on Transaction Confirmations and not Sign Confirmations)
(using new confirmations flow)
Improved transaction requests
is ON inSettings > Experimental
0.0001
ETHTest STX Banner Alert that it shows on Transaction Confirmations and not Sign Confirmations)
(using old confirmations flow)
Improved transaction requests
is OFF inSettings > Experimental
0.0001
ETHImproved transaction requests
back to ON inSettings > Experimental
0.0001
ETHCongrats, you have manually tested the happy path, now we just need to test the edge cases:
0.0001
ETHSettings > Advanced > Smart Transactions
0.0001
ETHTest edge case that after STX migration runs and Banner is being shown that clicking on "Higher success rates" link:
0.0001
ETHSettings > Advanced > Smart Transactions
0.0001
ETHBecause 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
null
opt-in statusfalse
opt-in status with no STX activityfalse
opt-in status with existing STX activitytrue
opt-in statusPre-merge reviewer checklist