-
Notifications
You must be signed in to change notification settings - Fork 5.2k
feat: add setting to dismiss prompt to enable smart contract #31609
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
Changes from 3 commits
2bed795
75144bb
bb98225
b44aef5
9d3afbf
d17b0d6
5f644e5
33a4883
8d1bf81
1786b55
c0e5a5f
79078c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import React from 'react'; | ||
import { useSelector } from 'react-redux'; | ||
|
||
import { useIsUpgradeTransaction } from '../../info/hooks/useIsUpgradeTransaction'; | ||
import { getDismissSmartAccountSuggestionEnabled } from '../../../../../../selectors'; | ||
import { useI18nContext } from '../../../../../../hooks/useI18nContext'; | ||
import { Checkbox } from '../../../../../../components/component-library'; | ||
import { AlignItems } from '../../../../../../helpers/constants/design-system'; | ||
|
@@ -13,8 +16,11 @@ export function Acknowledge(props: AcknowledgeProps) { | |
const t = useI18nContext(); | ||
const isUpgradeTransaction = useIsUpgradeTransaction(); | ||
const { isAcknowledged, onAcknowledgeToggle } = props; | ||
const dismissSmartAccountSuggestionEnabled = useSelector( | ||
getDismissSmartAccountSuggestionEnabled, | ||
); | ||
|
||
if (!isUpgradeTransaction) { | ||
if (!isUpgradeTransaction || dismissSmartAccountSuggestionEnabled) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately we can't just hide the acknowledge checkbox as the transaction would still perform the upgrade once approved. If we instead check this here: Then the middleware would reject the request outright since the user has said they never want to upgrade. Plus technically we should also check if the user is already upgraded and skip this check, but that can be done in a separate ticket as I have pending changes in the transaction controller to get that information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh my bad, I fixed this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me create task for
|
||
return null; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1998,6 +1998,11 @@ export function getShouldShowAggregatedBalancePopover(state) { | |
return shouldShowAggregatedBalancePopover; | ||
} | ||
|
||
export function getDismissSmartAccountSuggestionEnabled(state) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
const { dismissSmartAccountSuggestionEnabled } = getPreferences(state); | ||
return dismissSmartAccountSuggestionEnabled; | ||
} | ||
|
||
export const getConnectedSnapsList = createDeepEqualSelector( | ||
getSnapsList, | ||
(snapsData) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the
accountUpgradeDisabledChains
also, should this be combined into that?Do we also want to display those in the settings UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep I was also thinking about re-using this - and I think this will be more a user level thing and not account level, additionally it will reflect across the app