From 17e31e63b726ba75f64256e47a8c7c5136ba6a80 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 8 Nov 2023 16:37:40 +0100 Subject: [PATCH 01/18] feat: enable recovery flow structure --- src/components/settings/Recovery/index.tsx | 38 +++++++++ .../sidebar/SidebarNavigation/config.tsx | 4 + .../EnableRecoveryFlowIntro.tsx | 7 ++ .../EnableRecoveryFlowReview.tsx | 43 ++++++++++ .../EnableRecoveryFlowSettings.tsx | 14 +++ .../tx-flow/flows/EnableRecovery/index.tsx | 58 +++++++++++++ src/config/routes.ts | 1 + src/pages/settings/recovery.tsx | 24 ++++++ src/services/recovery/setup.ts | 85 +++++++++++++++++++ 9 files changed, 274 insertions(+) create mode 100644 src/components/settings/Recovery/index.tsx create mode 100644 src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx create mode 100644 src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx create mode 100644 src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx create mode 100644 src/components/tx-flow/flows/EnableRecovery/index.tsx create mode 100644 src/pages/settings/recovery.tsx create mode 100644 src/services/recovery/setup.ts diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx new file mode 100644 index 0000000000..7181add121 --- /dev/null +++ b/src/components/settings/Recovery/index.tsx @@ -0,0 +1,38 @@ +import { Box, Button, Chip, Grid, Paper, Typography } from '@mui/material' +import { useContext } from 'react' +import type { ReactElement } from 'react' + +import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' +import { TxModalContext } from '@/components/tx-flow' + +export function Recovery(): ReactElement { + const { setTxFlow } = useContext(TxModalContext) + + return ( + + + + + + Account recovery + + + {/* TODO: Extract when widget is merged https://github.com/safe-global/safe-wallet-web/pull/2768 */} + + + + + + + Choose a trusted guardian to recover your Safe Account, in case you should ever lose access to your Account. + Enabling the Account recovery module will require a transactions. + + + + + + + ) +} diff --git a/src/components/sidebar/SidebarNavigation/config.tsx b/src/components/sidebar/SidebarNavigation/config.tsx index 9f8dbde18f..67f870101f 100644 --- a/src/components/sidebar/SidebarNavigation/config.tsx +++ b/src/components/sidebar/SidebarNavigation/config.tsx @@ -84,6 +84,10 @@ export const settingsNavItems = [ label: 'Appearance', href: AppRoutes.settings.appearance, }, + { + label: 'Recovery', + href: AppRoutes.settings.recovery, + }, { label: 'Notifications', href: AppRoutes.settings.notifications, diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx new file mode 100644 index 0000000000..6d8296c2c8 --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx @@ -0,0 +1,7 @@ +import type { ReactElement } from 'react' + +import TxCard from '../../common/TxCard' + +export function EnableRecoveryFlowIntro(): ReactElement { + return EnableRecoveryFlowIntro +} diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx new file mode 100644 index 0000000000..4b2beab26a --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx @@ -0,0 +1,43 @@ +import { useContext, useEffect, useMemo } from 'react' +import type { ReactElement } from 'react' + +import SignOrExecuteForm from '@/components/tx/SignOrExecuteForm' +import { Errors, logError } from '@/services/exceptions' +import { createMultiSendCallOnlyTx } from '@/services/tx/tx-sender' +import { SafeTxContext } from '@/components/tx-flow/SafeTxProvider' +import { getRecoverySetup } from '@/services/recovery/setup' +import { useWeb3 } from '@/hooks/wallets/web3' +import useSafeInfo from '@/hooks/useSafeInfo' +import type { EnableRecoveryFlowProps } from '.' + +export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlowProps }): ReactElement { + const web3 = useWeb3() + const { safe } = useSafeInfo() + const { setSafeTx, safeTxError, setSafeTxError } = useContext(SafeTxContext) + + const recoverySetup = useMemo(() => { + if (!web3) { + return + } + + return getRecoverySetup({ + ...params, + safe, + provider: web3, + }) + }, [params, safe, web3]) + + useEffect(() => { + if (recoverySetup) { + createMultiSendCallOnlyTx(recoverySetup.transactions).then(setSafeTx).catch(setSafeTxError) + } + }, [recoverySetup, setSafeTx, setSafeTxError]) + + useEffect(() => { + if (safeTxError) { + logError(Errors._809, safeTxError.message) + } + }, [safeTxError]) + + return null} /> +} diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx new file mode 100644 index 0000000000..79709c965d --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx @@ -0,0 +1,14 @@ +import type { ReactElement } from 'react' + +import TxCard from '../../common/TxCard' +import type { EnableRecoveryFlowProps } from '.' + +export function EnableRecoveryFlowSettings({ + params, + onSubmit, +}: { + params: EnableRecoveryFlowProps + onSubmit: (formData: EnableRecoveryFlowProps) => void +}): ReactElement { + return EnableRecoveryFlowSettings +} diff --git a/src/components/tx-flow/flows/EnableRecovery/index.tsx b/src/components/tx-flow/flows/EnableRecovery/index.tsx new file mode 100644 index 0000000000..2f092732c7 --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/index.tsx @@ -0,0 +1,58 @@ +import type { ReactElement } from 'react' + +import TxLayout from '@/components/tx-flow/common/TxLayout' +import CodeIcon from '@/public/images/apps/code-icon.svg' +import useTxStepper from '../../useTxStepper' +import { EnableRecoveryFlowReview } from './EnableRecoveryFlowReview' +import { EnableRecoveryFlowSettings } from './EnableRecoveryFlowSettings' +import { EnableRecoveryFlowIntro } from './EnableRecoveryFlowIntro' + +export enum EnableRecoveryFlowFields { + guardians = 'guardians', + txCooldown = 'txCooldown', + txExpiration = 'txExpiration', +} + +export type EnableRecoveryFlowProps = { + [EnableRecoveryFlowFields.guardians]: Array + [EnableRecoveryFlowFields.txCooldown]: string + [EnableRecoveryFlowFields.txExpiration]: string +} + +export function EnableRecoveryFlow(): ReactElement { + const { data, step, nextStep, prevStep } = useTxStepper({ + [EnableRecoveryFlowFields.guardians]: [], + [EnableRecoveryFlowFields.txCooldown]: '0', + [EnableRecoveryFlowFields.txExpiration]: '0', + }) + + const steps = [ + , + nextStep({ ...data, ...formData })} />, + , + ] + + const isIntro = step === 0 + const isSettings = step === 1 + + const subtitle = isIntro + ? 'How does recovery work?' + : isSettings + ? 'Set up account settings' + : 'Set up account recovery' + + const icon = isIntro ? undefined : CodeIcon + + return ( + + {steps} + + ) +} diff --git a/src/config/routes.ts b/src/config/routes.ts index 6090dbb7ad..a3f245cd3b 100644 --- a/src/config/routes.ts +++ b/src/config/routes.ts @@ -28,6 +28,7 @@ export const AppRoutes = { settings: { spendingLimits: '/settings/spending-limits', setup: '/settings/setup', + recovery: '/settings/recovery', notifications: '/settings/notifications', modules: '/settings/modules', index: '/settings', diff --git a/src/pages/settings/recovery.tsx b/src/pages/settings/recovery.tsx new file mode 100644 index 0000000000..a5aa677b9f --- /dev/null +++ b/src/pages/settings/recovery.tsx @@ -0,0 +1,24 @@ +import Head from 'next/head' +import type { NextPage } from 'next' + +import SettingsHeader from '@/components/settings/SettingsHeader' +import { Recovery } from '@/components/settings/Recovery' + +// TODO: Condense to other setting section once confirmed +const RecoveryPage: NextPage = () => { + return ( + <> + + {'Safe{Wallet} – Settings – Recovery'} + + + + +
+ +
+ + ) +} + +export default RecoveryPage diff --git a/src/services/recovery/setup.ts b/src/services/recovery/setup.ts new file mode 100644 index 0000000000..6e8291468a --- /dev/null +++ b/src/services/recovery/setup.ts @@ -0,0 +1,85 @@ +import { getModuleInstance, KnownContracts, deployAndSetUpModule } from '@gnosis.pm/zodiac' +import { Interface } from 'ethers/lib/utils' +import type { Web3Provider } from '@ethersproject/providers' +import type { SafeInfo } from '@safe-global/safe-gateway-typescript-sdk' +import type { MetaTransactionData } from '@safe-global/safe-core-sdk-types' + +export function getRecoverySetup({ + txCooldown, + txExpiration, + guardians, + safe, + provider, +}: { + txCooldown: string + txExpiration: string + guardians: Array + safe: SafeInfo + provider: Web3Provider +}): { + expectedModuleAddress: string + transactions: Array +} { + const safeAddress = safe.address.value + + const setupArgs: Parameters[1] = { + types: ['address', 'address', 'address', 'uint256', 'uint256'], + values: [ + safeAddress, // address _owner + safeAddress, // address _avatar + safeAddress, // address _target + txCooldown, // uint256 _cooldown + txExpiration, // uint256 _expiration + ], + } + + const saltNonce: Parameters[4] = Date.now().toString() + + const { transaction, expectedModuleAddress } = deployAndSetUpModule( + KnownContracts.DELAY, + setupArgs, + provider, + Number(safe.chainId), + saltNonce, + ) + + const transactions: Array = [] + + // Deploy Delay Modifier + const deployDeplayModifier: MetaTransactionData = { + ...transaction, + value: transaction.value.toString(), + } + + transactions.push(deployDeplayModifier) + + const safeAbi = ['function enableModule(address module)'] + const safeInterface = new Interface(safeAbi) + + // Enable Delay Modifier on Safe + const enableDelayModifier: MetaTransactionData = { + to: safeAddress, + value: '0', + data: safeInterface.encodeFunctionData('enableModule', [expectedModuleAddress]), + } + + transactions.push(enableDelayModifier) + + const delayModifierContract = getModuleInstance(KnownContracts.DELAY, expectedModuleAddress, provider) + + // Add guardians to Delay Modifier + const enableDelayModifierModules: Array = guardians.map((guardian) => { + return { + to: expectedModuleAddress, + data: delayModifierContract.interface.encodeFunctionData('enableModule', [guardian]), + value: '0', + } + }) + + transactions.push(...enableDelayModifierModules) + + return { + expectedModuleAddress, + transactions, + } +} From 3021932136651e7b3794664e4f3fa104ea6338c8 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 8 Nov 2023 17:11:24 +0100 Subject: [PATCH 02/18] feat: intro step --- .../transactions/recovery-execution.svg | 8 +++ .../images/transactions/recovery-guardian.svg | 5 ++ .../tx-flow/common/TxLayout/index.tsx | 10 ++- .../EnableRecoveryFlowIntro.tsx | 69 ++++++++++++++++++- .../tx-flow/flows/EnableRecovery/index.tsx | 3 +- 5 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 public/images/transactions/recovery-execution.svg create mode 100644 public/images/transactions/recovery-guardian.svg diff --git a/public/images/transactions/recovery-execution.svg b/public/images/transactions/recovery-execution.svg new file mode 100644 index 0000000000..cc395ac152 --- /dev/null +++ b/public/images/transactions/recovery-execution.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/public/images/transactions/recovery-guardian.svg b/public/images/transactions/recovery-guardian.svg new file mode 100644 index 0000000000..0522763c01 --- /dev/null +++ b/public/images/transactions/recovery-guardian.svg @@ -0,0 +1,5 @@ + + + + + diff --git a/src/components/tx-flow/common/TxLayout/index.tsx b/src/components/tx-flow/common/TxLayout/index.tsx index c31b676db4..58863b306e 100644 --- a/src/components/tx-flow/common/TxLayout/index.tsx +++ b/src/components/tx-flow/common/TxLayout/index.tsx @@ -55,6 +55,7 @@ type TxLayoutProps = { txSummary?: TransactionSummary onBack?: () => void hideNonce?: boolean + hideProgress?: boolean isBatch?: boolean isReplacement?: boolean isMessage?: boolean @@ -69,6 +70,7 @@ const TxLayout = ({ txSummary, onBack, hideNonce = false, + hideProgress = false, isBatch = false, isReplacement = false, isMessage = false, @@ -117,9 +119,11 @@ const TxLayout = ({ - - - + {!hideProgress && ( + + + + )} diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx index 6d8296c2c8..6e75d7aa85 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx @@ -1,7 +1,70 @@ -import type { ReactElement } from 'react' +import { Button, CardActions, Divider, Grid, Typography } from '@mui/material' +import type { ReactElement, ReactNode } from 'react' import TxCard from '../../common/TxCard' +import RecoveryGuardians from '@/public/images/settings/spending-limit/beneficiary.svg' +import RecoveryGuardian from '@/public/images/transactions/recovery-guardian.svg' +import RecoveryDelay from '@/public/images/settings/spending-limit/time.svg' +import RecoveryExecution from '@/public/images/transactions/recovery-execution.svg' -export function EnableRecoveryFlowIntro(): ReactElement { - return EnableRecoveryFlowIntro +import commonCss from '@/components/tx-flow/common/styles.module.css' + +const RecoverySteps: Array<{ icon: ReactElement; title: string; subtitle: ReactNode }> = [ + { + icon: , + title: 'Choose a guardian and set a delay', + subtitle: + 'Only your chosen guardian can initiate the recovery process. The process can be cancelled at any time during the delay period.', + }, + { + icon: , + title: 'Lost access? Let the guardian connect', + subtitle: 'The recovery process can be initiated by a trusted guardian when connected to your Safe Account.', + }, + { + icon: , + title: 'Start the recovery process', + subtitle: ( + <> + Your guardian chooses new Safe Account owner(s) that you control and can initiates the recovery + transaction. + + ), + }, + { + icon: , + title: 'All done! The Account is yours again', + subtitle: + 'Once the delay period has passed, you can execute the recovery transaction and regain access to your Safe Account.', + }, +] + +export function EnableRecoveryFlowIntro({ onSubmit }: { onSubmit: () => void }): ReactElement { + return ( + + + {RecoverySteps.map(({ icon, title, subtitle }, index) => ( + + + {icon} + + + {title} + + {subtitle} + + + + ))} + + + + + + + + + ) } diff --git a/src/components/tx-flow/flows/EnableRecovery/index.tsx b/src/components/tx-flow/flows/EnableRecovery/index.tsx index 2f092732c7..2784c3441a 100644 --- a/src/components/tx-flow/flows/EnableRecovery/index.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/index.tsx @@ -27,7 +27,7 @@ export function EnableRecoveryFlow(): ReactElement { }) const steps = [ - , + nextStep(data)} />, nextStep({ ...data, ...formData })} />, , ] @@ -51,6 +51,7 @@ export function EnableRecoveryFlow(): ReactElement { step={step} onBack={prevStep} hideNonce={isIntro} + hideProgress={isIntro} > {steps} From 5d9ac7a2e7166b68fb4a17f31eb15a9ffedc4bf2 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 8 Nov 2023 17:24:05 +0100 Subject: [PATCH 03/18] feat: basic settings template --- public/images/common/recovery-plus.svg | 12 ++++ .../EnableRecoveryFlowSettings.tsx | 56 ++++++++++++++++++- .../tx-flow/flows/EnableRecovery/index.tsx | 6 +- 3 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 public/images/common/recovery-plus.svg diff --git a/public/images/common/recovery-plus.svg b/public/images/common/recovery-plus.svg new file mode 100644 index 0000000000..351202af8e --- /dev/null +++ b/public/images/common/recovery-plus.svg @@ -0,0 +1,12 @@ + + + + + + + + + + + + diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx index 79709c965d..dbb1b792c1 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx @@ -1,8 +1,12 @@ +import { Divider, CardActions, Button, Typography } from '@mui/material' +import { useForm, FormProvider } from 'react-hook-form' import type { ReactElement } from 'react' import TxCard from '../../common/TxCard' import type { EnableRecoveryFlowProps } from '.' +import commonCss from '@/components/tx-flow/common/styles.module.css' + export function EnableRecoveryFlowSettings({ params, onSubmit, @@ -10,5 +14,55 @@ export function EnableRecoveryFlowSettings({ params: EnableRecoveryFlowProps onSubmit: (formData: EnableRecoveryFlowProps) => void }): ReactElement { - return EnableRecoveryFlowSettings + const formMethods = useForm({ + defaultValues: params, + mode: 'onChange', + }) + + return ( + <> + +
+ + Trusted guardian + + Choosen a guardian, such as a hardware wallet or family member's wallet, that can initiate the + recovery process in the future. + + + {/* TODO: Address field */} + + {/* TODO: Info button */} + Recovery delay + + You can cancel any recovery attempt when it is not needed or wanted within the delay period. + + + {/* TODO: Delay field */} + + {/* TODO: Advanced options */} + + + + {/* TODO: Recommended badge */} + + Receive email updates + Get notified about any recovery initiations and their statuses. + + {/* TODO: Email address field */} + + {/* TODO: Tenderly logo */} + + + + + + + +
+ + + ) } diff --git a/src/components/tx-flow/flows/EnableRecovery/index.tsx b/src/components/tx-flow/flows/EnableRecovery/index.tsx index 2784c3441a..5c0065b630 100644 --- a/src/components/tx-flow/flows/EnableRecovery/index.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/index.tsx @@ -1,7 +1,7 @@ import type { ReactElement } from 'react' import TxLayout from '@/components/tx-flow/common/TxLayout' -import CodeIcon from '@/public/images/apps/code-icon.svg' +import RecoveryPlus from '@/public/images/common/recovery-plus.svg' import useTxStepper from '../../useTxStepper' import { EnableRecoveryFlowReview } from './EnableRecoveryFlowReview' import { EnableRecoveryFlowSettings } from './EnableRecoveryFlowSettings' @@ -38,10 +38,10 @@ export function EnableRecoveryFlow(): ReactElement { const subtitle = isIntro ? 'How does recovery work?' : isSettings - ? 'Set up account settings' + ? 'Set up account recovery settings' : 'Set up account recovery' - const icon = isIntro ? undefined : CodeIcon + const icon = isIntro ? undefined : RecoveryPlus return ( Date: Mon, 13 Nov 2023 15:34:13 +0100 Subject: [PATCH 04/18] feat: settings + review step --- src/components/settings/Recovery/index.tsx | 9 +- .../TxDetails/Summary/TxDataRow/index.tsx | 2 +- .../tx-flow/common/TxLayout/index.tsx | 5 + .../EnableRecoveryFlowEmailHint.tsx | 23 +++ .../EnableRecoveryFlowReview.tsx | 73 ++++++++- .../EnableRecoveryFlowSettings.tsx | 153 ++++++++++++++++-- .../tx-flow/flows/EnableRecovery/index.tsx | 41 ++++- .../flows/EnableRecovery/styles.module.css | 35 ++++ 8 files changed, 324 insertions(+), 17 deletions(-) create mode 100644 src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowEmailHint.tsx create mode 100644 src/components/tx-flow/flows/EnableRecovery/styles.module.css diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx index 7181add121..3783806a2c 100644 --- a/src/components/settings/Recovery/index.tsx +++ b/src/components/settings/Recovery/index.tsx @@ -4,9 +4,11 @@ import type { ReactElement } from 'react' import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' import { TxModalContext } from '@/components/tx-flow' +import { useDarkMode } from '@/hooks/useDarkMode' export function Recovery(): ReactElement { const { setTxFlow } = useContext(TxModalContext) + const isDarkMode = useDarkMode() return ( @@ -18,7 +20,12 @@ export function Recovery(): ReactElement { {/* TODO: Extract when widget is merged https://github.com/safe-global/safe-wallet-web/pull/2768 */} - + diff --git a/src/components/transactions/TxDetails/Summary/TxDataRow/index.tsx b/src/components/transactions/TxDetails/Summary/TxDataRow/index.tsx index 023bacae0d..472f0c2b1c 100644 --- a/src/components/transactions/TxDetails/Summary/TxDataRow/index.tsx +++ b/src/components/transactions/TxDetails/Summary/TxDataRow/index.tsx @@ -8,7 +8,7 @@ import css from './styles.module.css' import EthHashInfo from '@/components/common/EthHashInfo' type TxDataRowProps = { - title: string + title: ReactNode children?: ReactNode } diff --git a/src/components/tx-flow/common/TxLayout/index.tsx b/src/components/tx-flow/common/TxLayout/index.tsx index 58863b306e..528ca90041 100644 --- a/src/components/tx-flow/common/TxLayout/index.tsx +++ b/src/components/tx-flow/common/TxLayout/index.tsx @@ -13,6 +13,7 @@ import SafeLogo from '@/public/images/logo-no-text.svg' import { TxSecurityProvider } from '@/components/tx/security/shared/TxSecurityContext' import ChainIndicator from '@/components/common/ChainIndicator' import SecurityWarnings from '@/components/tx/security/SecurityWarnings' +import { EnableRecoveryFlowEmailHint } from '../../flows/EnableRecovery/EnableRecoveryFlowEmailHint' const TxLayoutHeader = ({ hideNonce, @@ -59,6 +60,7 @@ type TxLayoutProps = { isBatch?: boolean isReplacement?: boolean isMessage?: boolean + isRecovery?: boolean } const TxLayout = ({ @@ -74,6 +76,7 @@ const TxLayout = ({ isBatch = false, isReplacement = false, isMessage = false, + isRecovery = false, }: TxLayoutProps): ReactElement => { const [statusVisible, setStatusVisible] = useState(true) @@ -154,6 +157,8 @@ const TxLayout = ({ + + {isRecovery && } diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowEmailHint.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowEmailHint.tsx new file mode 100644 index 0000000000..8a9ad8655e --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowEmailHint.tsx @@ -0,0 +1,23 @@ +import { Box, Typography, SvgIcon, Alert } from '@mui/material' +import type { ReactElement } from 'react' + +import LightbulbIcon from '@/public/images/common/lightbulb.svg' + +import infoWidgetCss from 'src/components/new-safe/create/InfoWidget/styles.module.css' + +export function EnableRecoveryFlowEmailHint(): ReactElement { + return ( + + palette.info.main }}> + + + Security tip + + + + For security reasons, we highly recommend adding an email address. You will be notified once a Guardian + initiates recovery and be able to reject it if it's a malicious attempt. + + + ) +} diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx index 4b2beab26a..7772e437c5 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx @@ -8,6 +8,11 @@ import { SafeTxContext } from '@/components/tx-flow/SafeTxProvider' import { getRecoverySetup } from '@/services/recovery/setup' import { useWeb3 } from '@/hooks/wallets/web3' import useSafeInfo from '@/hooks/useSafeInfo' +import { SvgIcon, Tooltip, Typography } from '@mui/material' +import { EnableRecoveryFlowFields, RecoveryDelayPeriods, RecoveryExpirationPeriods } from '.' +import { TxDataRow } from '@/components/transactions/TxDetails/Summary/TxDataRow' +import InfoIcon from '@/public/images/notifications/info.svg' +import EthHashInfo from '@/components/common/EthHashInfo' import type { EnableRecoveryFlowProps } from '.' export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlowProps }): ReactElement { @@ -39,5 +44,71 @@ export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlo } }, [safeTxError]) - return null} /> + const guardian = params[EnableRecoveryFlowFields.guardians][0] + const delay = RecoveryDelayPeriods.find(({ value }) => value === params[EnableRecoveryFlowFields.txCooldown])!.label + const expiration = RecoveryExpirationPeriods.find( + ({ value }) => value === params[EnableRecoveryFlowFields.txExpiration], + )!.label + const emailAddress = params[EnableRecoveryFlowFields.emailAddress] + + return ( + null}> + This transaction will enable the Account recovery feature once executed. + + + + + + {/* TODO: Info */} + + Recovery delay + + + + + + + } + > + {delay} + + {/* TODO: Info */} + + Transaction validity + + + + + + + } + > + {expiration} + + + {emailAddress ? {emailAddress} : null} + + ) } diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx index dbb1b792c1..a404ed6e73 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx @@ -1,11 +1,31 @@ -import { Divider, CardActions, Button, Typography } from '@mui/material' -import { useForm, FormProvider } from 'react-hook-form' +import { + Divider, + CardActions, + Button, + Typography, + SvgIcon, + MenuItem, + TextField, + Collapse, + Checkbox, + FormControlLabel, +} from '@mui/material' +import ExpandLessIcon from '@mui/icons-material/ExpandLess' +import ExpandMoreIcon from '@mui/icons-material/ExpandMore' +import { useForm, FormProvider, Controller } from 'react-hook-form' +import { useState } from 'react' +import type { TextFieldProps } from '@mui/material' import type { ReactElement } from 'react' import TxCard from '../../common/TxCard' +import { EnableRecoveryFlowFields, RecoveryDelayPeriods, RecoveryExpirationPeriods } from '.' +import AddressBookInput from '@/components/common/AddressBookInput' +import CircleCheckIcon from '@/public/images/common/circle-check.svg' +import { useDarkMode } from '@/hooks/useDarkMode' import type { EnableRecoveryFlowProps } from '.' import commonCss from '@/components/tx-flow/common/styles.module.css' +import css from './styles.module.css' export function EnableRecoveryFlowSettings({ params, @@ -14,49 +34,135 @@ export function EnableRecoveryFlowSettings({ params: EnableRecoveryFlowProps onSubmit: (formData: EnableRecoveryFlowProps) => void }): ReactElement { + const [showAdvanced, setShowAdvanced] = useState(params[EnableRecoveryFlowFields.txExpiration] !== '0') + const [understandsRisk, setUnderstandsRisk] = useState(false) + const isDarkMode = useDarkMode() + const formMethods = useForm({ defaultValues: params, mode: 'onChange', }) + const emailAddress = formMethods.watch(EnableRecoveryFlowFields.emailAddress) + + const onShowAdvanced = () => setShowAdvanced((prev) => !prev) + return ( <>
Trusted guardian + - Choosen a guardian, such as a hardware wallet or family member's wallet, that can initiate the + Choose a guardian, such as a hardware wallet or family member's wallet, that can initiate the recovery process in the future. - {/* TODO: Address field */} + - {/* TODO: Info button */} Recovery delay + You can cancel any recovery attempt when it is not needed or wanted within the delay period. - {/* TODO: Delay field */} + ( + + {RecoveryDelayPeriods.map(({ label, value }, index) => ( + + {label} + + ))} + + )} + /> + + + Advanced {showAdvanced ? : } + + + + + Set a period of time after which the recovery attempt will expire and can no longer be executed. + - {/* TODO: Advanced options */} + ( + + {RecoveryExpirationPeriods.map(({ label, value }, index) => ( + + {label} + + ))} + + )} + /> + - {/* TODO: Recommended badge */} +
+ + Recommended +
Receive email updates - Get notified about any recovery initiations and their statuses. - {/* TODO: Email address field */} + + Get notified about any recovery initiations and their statuses. + + + ( + + )} + /> - {/* TODO: Tenderly logo */} + {emailAddress ? ( +
+ Powered by + Tenderly +
+ ) : ( + setUnderstandsRisk(checked)} />} + sx={{ pl: 2 }} + /> + )} - @@ -66,3 +172,26 @@ export function EnableRecoveryFlowSettings({ ) } + +function SelectField(props: TextFieldProps) { + return ( + + ) +} diff --git a/src/components/tx-flow/flows/EnableRecovery/index.tsx b/src/components/tx-flow/flows/EnableRecovery/index.tsx index 5c0065b630..376d70bead 100644 --- a/src/components/tx-flow/flows/EnableRecovery/index.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/index.tsx @@ -7,23 +7,59 @@ import { EnableRecoveryFlowReview } from './EnableRecoveryFlowReview' import { EnableRecoveryFlowSettings } from './EnableRecoveryFlowSettings' import { EnableRecoveryFlowIntro } from './EnableRecoveryFlowIntro' +const DAY_SECONDS = 60 * 60 * 24 + +export const RecoveryDelayPeriods = [ + { + label: '2 days', + value: `${DAY_SECONDS}`, + }, + { + label: '7 days', + value: `${DAY_SECONDS * 7}`, + }, + { + label: '14 days', + value: `${DAY_SECONDS * 14}`, + }, + { + label: '28 days', + value: `${DAY_SECONDS * 28}`, + }, + { + label: '56 days', + value: `${DAY_SECONDS * 56}`, + }, +] as const + +export const RecoveryExpirationPeriods = [ + { + label: 'Never', + value: '0', + }, + ...RecoveryDelayPeriods, +] as const + export enum EnableRecoveryFlowFields { guardians = 'guardians', txCooldown = 'txCooldown', txExpiration = 'txExpiration', + emailAddress = 'emailAddress', } export type EnableRecoveryFlowProps = { [EnableRecoveryFlowFields.guardians]: Array [EnableRecoveryFlowFields.txCooldown]: string [EnableRecoveryFlowFields.txExpiration]: string + [EnableRecoveryFlowFields.emailAddress]: string } export function EnableRecoveryFlow(): ReactElement { const { data, step, nextStep, prevStep } = useTxStepper({ - [EnableRecoveryFlowFields.guardians]: [], - [EnableRecoveryFlowFields.txCooldown]: '0', + [EnableRecoveryFlowFields.guardians]: [''], + [EnableRecoveryFlowFields.txCooldown]: `${60 * 60 * 24 * 28}`, // 28 days in seconds [EnableRecoveryFlowFields.txExpiration]: '0', + [EnableRecoveryFlowFields.emailAddress]: '', }) const steps = [ @@ -52,6 +88,7 @@ export function EnableRecoveryFlow(): ReactElement { onBack={prevStep} hideNonce={isIntro} hideProgress={isIntro} + isRecovery={!isIntro} > {steps} diff --git a/src/components/tx-flow/flows/EnableRecovery/styles.module.css b/src/components/tx-flow/flows/EnableRecovery/styles.module.css new file mode 100644 index 0000000000..88d891374b --- /dev/null +++ b/src/components/tx-flow/flows/EnableRecovery/styles.module.css @@ -0,0 +1,35 @@ +.advanced { + display: flex; + align-items: center; + cursor: pointer; + color: var(--color-primary-light); +} + +.recommended { + color: var(--color-text-primary); + display: flex; + padding: var(--space-1) var(--space-2); + position: absolute; + right: var(--space-4); + border-radius: 0px 0px 4px 4px; + background: var(--color-info-light); + margin-top: calc(var(--space-3) * -1); + align-items: center; +} + +[data-theme='dark'] .recommended { + color: var(--color-text-primary); + background: var(--color-info-dark); +} + +.poweredBy { + display: flex; + align-items: center; + gap: calc(var(--space-1) / 2); + color: var(--color-text-secondary); +} + +.tenderly { + width: 65px; + height: 15px; +} From 5b709d9b5232994dda35fc3c521d53e5087c0cf0 Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 13 Nov 2023 16:35:08 +0100 Subject: [PATCH 05/18] fix: add test coverage + remove comments --- package.json | 1 + .../EnableRecoveryFlowReview.tsx | 3 +- src/pages/settings/recovery.tsx | 1 - src/services/recovery/__tests__/setup.test.ts | 100 ++++++++++++++++++ yarn.lock | 33 +++++- 5 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 src/services/recovery/__tests__/setup.test.ts diff --git a/package.json b/package.json index 9882a430e1..d58949d6b9 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "@emotion/react": "^11.10.0", "@emotion/server": "^11.10.0", "@emotion/styled": "^11.10.0", + "@gnosis.pm/zodiac": "^3.4.2", "@mui/icons-material": "^5.14.3", "@mui/material": "^5.14.3", "@mui/x-date-pickers": "^5.0.12", diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx index 7772e437c5..97bb575dcb 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx @@ -59,7 +59,6 @@ export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlo - {/* TODO: Info */} @@ -83,7 +82,7 @@ export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlo > {delay} - {/* TODO: Info */} + diff --git a/src/pages/settings/recovery.tsx b/src/pages/settings/recovery.tsx index a5aa677b9f..b483293687 100644 --- a/src/pages/settings/recovery.tsx +++ b/src/pages/settings/recovery.tsx @@ -4,7 +4,6 @@ import type { NextPage } from 'next' import SettingsHeader from '@/components/settings/SettingsHeader' import { Recovery } from '@/components/settings/Recovery' -// TODO: Condense to other setting section once confirmed const RecoveryPage: NextPage = () => { return ( <> diff --git a/src/services/recovery/__tests__/setup.test.ts b/src/services/recovery/__tests__/setup.test.ts new file mode 100644 index 0000000000..258d9a1ab9 --- /dev/null +++ b/src/services/recovery/__tests__/setup.test.ts @@ -0,0 +1,100 @@ +import { getModuleInstance, KnownContracts, deployAndSetUpModule } from '@gnosis.pm/zodiac' +import { faker } from '@faker-js/faker' +import { BigNumber } from 'ethers' +import type { Web3Provider } from '@ethersproject/providers' +import type { SafeInfo } from '@safe-global/safe-gateway-typescript-sdk' + +import { getRecoverySetup } from '@/services/recovery/setup' + +jest.mock('@gnosis.pm/zodiac', () => ({ + ...jest.requireActual('@gnosis.pm/zodiac'), + getModuleInstance: jest.fn(), + deployAndSetUpModule: jest.fn(), +})) + +const mockGetModuleInstance = getModuleInstance as jest.MockedFunction +const mockDeployAndSetUpModule = deployAndSetUpModule as jest.MockedFunction + +describe('getRecoverySetup', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('should deploy Delay Modifier, enable it on Safe and add a Guardian', () => { + const txCooldown = faker.string.numeric() + const txExpiration = faker.string.numeric() + const guardians = [faker.finance.ethereumAddress()] + const safeAddress = faker.finance.ethereumAddress() + const chainId = faker.string.numeric() + const safe = { + address: { + value: safeAddress, + }, + chainId, + } as SafeInfo + const provider = {} as Web3Provider + + const expectedModuleAddress = faker.finance.ethereumAddress() + const deployDelayModifierTx = { + to: faker.finance.ethereumAddress(), + data: faker.string.hexadecimal(), + value: BigNumber.from(0), + } + mockGetModuleInstance.mockReturnValue({ + interface: { + encodeFunctionData: jest.fn().mockReturnValue(deployDelayModifierTx.data), + }, + } as any) + mockDeployAndSetUpModule.mockReturnValue({ + expectedModuleAddress, + transaction: deployDelayModifierTx, + }) + + const result = getRecoverySetup({ + txCooldown, + txExpiration, + guardians, + safe, + provider, + }) + + expect(mockDeployAndSetUpModule).toHaveBeenCalledTimes(1) + expect(mockDeployAndSetUpModule).toHaveBeenCalledWith( + KnownContracts.DELAY, + { + types: ['address', 'address', 'address', 'uint256', 'uint256'], + values: [ + safeAddress, // address _owner + safeAddress, // address _avatar + safeAddress, // address _target + txCooldown, // uint256 _cooldown + txExpiration, // uint256 _expiration + ], + }, + provider, + Number(safe.chainId), + expect.any(String), + ) + + expect(result.expectedModuleAddress).toEqual(expectedModuleAddress) + expect(result.transactions).toHaveLength(3) + + // Deploy Delay Modifier + expect(result.transactions[0]).toEqual({ + ...deployDelayModifierTx, + value: '0', + }) + // Enable Delay Modifier on Safe + expect(result.transactions[1]).toEqual({ + to: safeAddress, + data: expect.any(String), + value: '0', + }) + // Add guardian to Delay Modifier + expect(result.transactions[2]).toEqual({ + to: expectedModuleAddress, + data: expect.any(String), + value: '0', + }) + }) +}) diff --git a/yarn.lock b/yarn.lock index 978b89eee0..0f23fdb5d2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2706,6 +2706,27 @@ dependencies: tslib "^2.1.0" +"@gnosis.pm/mock-contract@^4.0.0": + version "4.0.0" + resolved "https://registry.yarnpkg.com/@gnosis.pm/mock-contract/-/mock-contract-4.0.0.tgz#eaf500fddcab81b5f6c22280a7b22ff891dd6f87" + integrity sha512-SkRq2KwPx6vo0LAjSc8JhgQstrQFXRyn2yqquIfub7r2WHi5nUbF8beeSSXsd36hvBcQxQfmOIYNYRpj9JOhrQ== + +"@gnosis.pm/safe-contracts@1.3.0": + version "1.3.0" + resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-contracts/-/safe-contracts-1.3.0.tgz#316741a7690d8751a1f701538cfc9ec80866eedc" + integrity sha512-1p+1HwGvxGUVzVkFjNzglwHrLNA67U/axP0Ct85FzzH8yhGJb4t9jDjPYocVMzLorDoWAfKicGy1akPY9jXRVw== + +"@gnosis.pm/zodiac@^3.4.2": + version "3.4.2" + resolved "https://registry.yarnpkg.com/@gnosis.pm/zodiac/-/zodiac-3.4.2.tgz#ce3e7498e39ccc3324eabc6f163bd173bf4d9aad" + integrity sha512-u7BPXsoo1ZdbmsElMbuejKNTWA3NPvFdzs3vjUSIcFfHTb9B/UE+gDQ3vMYL6bt+YLVw0F/IT5ytbiruKYQpEQ== + dependencies: + "@gnosis.pm/mock-contract" "^4.0.0" + "@gnosis.pm/safe-contracts" "1.3.0" + "@openzeppelin/contracts" "^5.0.0" + "@openzeppelin/contracts-upgradeable" "^5.0.0" + ethers "^5.7.1" + "@grpc/grpc-js@~1.9.0": version "1.9.5" resolved "https://registry.yarnpkg.com/@grpc/grpc-js/-/grpc-js-1.9.5.tgz#22e283754b7b10d1ad26c3fb21849028dcaabc53" @@ -3604,11 +3625,21 @@ "@nodelib/fs.scandir" "2.1.5" fastq "^1.6.0" +"@openzeppelin/contracts-upgradeable@^5.0.0": + version "5.0.0" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-5.0.0.tgz#859c00c55f04b6dda85b3c88bce507d65019888f" + integrity sha512-D54RHzkOKHQ8xUssPgQe2d/U92mwaiBDY7qCCVGq6VqwQjsT3KekEQ3bonev+BLP30oZ0R1U6YC8/oLpizgC5Q== + "@openzeppelin/contracts@^4.9.2": version "4.9.3" resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-4.9.3.tgz#00d7a8cf35a475b160b3f0293a6403c511099364" integrity sha512-He3LieZ1pP2TNt5JbkPA4PNT9WC3gOTOlDcFGJW4Le4QKqwmiNJCRt44APfxMxvq7OugU/cqYuPcSBzOw38DAg== +"@openzeppelin/contracts@^5.0.0": + version "5.0.0" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-5.0.0.tgz#ee0e4b4564f101a5c4ee398cd4d73c0bd92b289c" + integrity sha512-bv2sdS6LKqVVMLI5+zqnNrNU/CA+6z6CmwFXm/MzmOPBRSO5reEJN7z0Gbzvs0/bv/MZZXNklubpwy3v2+azsw== + "@polka/url@^1.0.0-next.20": version "1.0.0-next.23" resolved "https://registry.yarnpkg.com/@polka/url/-/url-1.0.0-next.23.tgz#498e41218ab3b6a1419c735e5c6ae2c5ed609b6c" @@ -8798,7 +8829,7 @@ ethers@5.5.4: "@ethersproject/web" "5.5.1" "@ethersproject/wordlists" "5.5.0" -ethers@5.7.2: +ethers@5.7.2, ethers@^5.7.1: version "5.7.2" resolved "https://registry.yarnpkg.com/ethers/-/ethers-5.7.2.tgz#3a7deeabbb8c030d4126b24f84e525466145872e" integrity sha512-wswUsmWo1aOK8rR7DIKiWSw9DbLWe6x98Jrn8wcTflTVvaXhAMaB5zGAXy0GYQEQp9iO1iSHWVyARQm11zUtyg== From b326d524c573d91afb55498702a9075745e628d9 Mon Sep 17 00:00:00 2001 From: iamacook Date: Tue, 14 Nov 2023 17:50:38 +0100 Subject: [PATCH 06/18] feat: recovery proposal flow --- src/components/settings/Recovery/index.tsx | 19 +- .../tx-flow/common/NewOwnerList/index.tsx | 30 + .../common/NewOwnerList/styles.module.css | 7 + .../tx-flow/flows/AddOwner/ReviewOwner.tsx | 10 +- .../RecoverAccountFlowReview.tsx | 158 +++++ .../RecoverAccountFlowSetup.tsx | 166 +++++ .../tx-flow/flows/RecoverAccount/index.tsx | 44 ++ .../tx/SignOrExecuteForm/TxChecks.tsx | 4 +- src/components/tx/security/tenderly/index.tsx | 10 +- src/services/exceptions/ErrorCodes.ts | 1 + .../recovery/__tests__/transaction.test.ts | 666 ++++++++++++++++++ src/services/recovery/transaction.ts | 133 ++++ src/services/tx/tx-sender/dispatch.ts | 33 +- src/store/recoverySlice.ts | 11 +- 14 files changed, 1275 insertions(+), 17 deletions(-) create mode 100644 src/components/tx-flow/common/NewOwnerList/index.tsx create mode 100644 src/components/tx-flow/common/NewOwnerList/styles.module.css create mode 100644 src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx create mode 100644 src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx create mode 100644 src/components/tx-flow/flows/RecoverAccount/index.tsx create mode 100644 src/services/recovery/__tests__/transaction.test.ts create mode 100644 src/services/recovery/transaction.ts diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx index 3783806a2c..ba7a6f4379 100644 --- a/src/components/settings/Recovery/index.tsx +++ b/src/components/settings/Recovery/index.tsx @@ -5,10 +5,16 @@ import type { ReactElement } from 'react' import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' import { TxModalContext } from '@/components/tx-flow' import { useDarkMode } from '@/hooks/useDarkMode' +import { RecoverAccountFlow } from '@/components/tx-flow/flows/RecoverAccount' +import useWallet from '@/hooks/wallets/useWallet' +import { useAppSelector } from '@/store' +import { selectRecoveryByGuardian } from '@/store/recoverySlice' export function Recovery(): ReactElement { const { setTxFlow } = useContext(TxModalContext) const isDarkMode = useDarkMode() + const wallet = useWallet() + const recovery = useAppSelector((state) => selectRecoveryByGuardian(state, wallet?.address ?? '')) return ( @@ -35,9 +41,16 @@ export function Recovery(): ReactElement { Enabling the Account recovery module will require a transactions. - + {recovery ? ( + // TODO: Move to correct location when widget is ready + + ) : ( + + )} diff --git a/src/components/tx-flow/common/NewOwnerList/index.tsx b/src/components/tx-flow/common/NewOwnerList/index.tsx new file mode 100644 index 0000000000..2164ba756a --- /dev/null +++ b/src/components/tx-flow/common/NewOwnerList/index.tsx @@ -0,0 +1,30 @@ +import { Paper, Typography, SvgIcon } from '@mui/material' +import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk' +import type { ReactElement } from 'react' + +import PlusIcon from '@/public/images/common/plus.svg' +import EthHashInfo from '@/components/common/EthHashInfo' + +import css from './styles.module.css' + +export function NewOwnerList({ newOwners }: { newOwners: Array }): ReactElement { + return ( + + + + New owner{newOwners.length > 1 ? 's' : ''} + + {newOwners.map((newOwner) => ( + + ))} + + ) +} diff --git a/src/components/tx-flow/common/NewOwnerList/styles.module.css b/src/components/tx-flow/common/NewOwnerList/styles.module.css new file mode 100644 index 0000000000..dcf6189778 --- /dev/null +++ b/src/components/tx-flow/common/NewOwnerList/styles.module.css @@ -0,0 +1,7 @@ +.container { + display: flex; + flex-direction: column; + gap: var(--space-1); + padding: var(--space-2); + background-color: var(--color-success-background); +} diff --git a/src/components/tx-flow/flows/AddOwner/ReviewOwner.tsx b/src/components/tx-flow/flows/AddOwner/ReviewOwner.tsx index d2c18301b3..f8ca7bbe70 100644 --- a/src/components/tx-flow/flows/AddOwner/ReviewOwner.tsx +++ b/src/components/tx-flow/flows/AddOwner/ReviewOwner.tsx @@ -10,7 +10,7 @@ import { upsertAddressBookEntry } from '@/store/addressBookSlice' import { SafeTxContext } from '../../SafeTxProvider' import type { AddOwnerFlowProps } from '.' import type { ReplaceOwnerFlowProps } from '../ReplaceOwner' -import PlusIcon from '@/public/images/common/plus.svg' +import { NewOwnerList } from '../../common/NewOwnerList' import MinusIcon from '@/public/images/common/minus.svg' import EthHashInfo from '@/components/common/EthHashInfo' import commonCss from '@/components/tx-flow/common/styles.module.css' @@ -68,13 +68,7 @@ export const ReviewOwner = ({ params }: { params: AddOwnerFlowProps | ReplaceOwn /> )} - palette.success.background, p: 2 }}> - - - New owner - - - + Any transaction requires the confirmation of: diff --git a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx new file mode 100644 index 0000000000..a7689a09eb --- /dev/null +++ b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx @@ -0,0 +1,158 @@ +import { CardActions, Button, Typography, Divider, Box } from '@mui/material' +import { useContext, useEffect, useState } from 'react' +import type { ReactElement } from 'react' + +import useSafeInfo from '@/hooks/useSafeInfo' +import { getRecoveryProposalTransactions } from '@/services/recovery/transaction' +import DecodedTx from '@/components/tx/DecodedTx' +import ErrorMessage from '@/components/tx/ErrorMessage' +import { RedefineBalanceChanges } from '@/components/tx/security/redefine/RedefineBalanceChange' +import ConfirmationTitle, { ConfirmationTitleTypes } from '@/components/tx/SignOrExecuteForm/ConfirmationTitle' +import TxChecks from '@/components/tx/SignOrExecuteForm/TxChecks' +import { WrongChainWarning } from '@/components/tx/WrongChainWarning' +import useDecodeTx from '@/hooks/useDecodeTx' +import TxCard from '../../common/TxCard' +import { SafeTxContext } from '../../SafeTxProvider' +import CheckWallet from '@/components/common/CheckWallet' +import { createMultiSendCallOnlyTx, createTx, dispatchRecoveryProposal } from '@/services/tx/tx-sender' +import { RecoverAccountFlowFields } from '.' +import { NewOwnerList } from '../../common/NewOwnerList' +import { useAppSelector } from '@/store' +import { selectRecoveryByGuardian } from '@/store/recoverySlice' +import useWallet from '@/hooks/wallets/useWallet' +import useOnboard from '@/hooks/wallets/useOnboard' +import { TxModalContext } from '../..' +import { asError } from '@/services/exceptions/utils' +import { trackError, Errors } from '@/services/exceptions' +import type { RecoverAccountFlowProps } from '.' + +import commonCss from '@/components/tx-flow/common/styles.module.css' + +export function RecoverAccountFlowReview({ params }: { params: RecoverAccountFlowProps }): ReactElement | null { + // Form state + const [isSubmittable, setIsSubmittable] = useState(true) + const [submitError, setSubmitError] = useState() + + // Hooks + const { setTxFlow } = useContext(TxModalContext) + const { safeTx, safeTxError, setSafeTx, setSafeTxError } = useContext(SafeTxContext) + const [decodedData, decodedDataError, decodedDataLoading] = useDecodeTx(safeTx) + const { safe } = useSafeInfo() + const wallet = useWallet() + const onboard = useOnboard() + const recovery = useAppSelector((state) => selectRecoveryByGuardian(state, wallet?.address ?? '')) + + // Proposal + const txCooldown = recovery?.txCooldown?.toNumber() + const newThreshold = Number(params[RecoverAccountFlowFields.threshold]) + const newOwners = params[RecoverAccountFlowFields.owners] + + useEffect(() => { + const transactions = getRecoveryProposalTransactions({ + safe, + newThreshold, + newOwners, + }) + + const promise = transactions.length > 1 ? createMultiSendCallOnlyTx(transactions) : createTx(transactions[0]) + + promise.then(setSafeTx).catch(setSafeTxError) + }, [newThreshold, newOwners, safe, setSafeTx, setSafeTxError]) + + // On modal submit + const onSubmit = async () => { + if (!recovery || !onboard) { + return + } + + setIsSubmittable(false) + setSubmitError(undefined) + + try { + await dispatchRecoveryProposal({ onboard, safe, newThreshold, newOwners, delayModifierAddress: recovery.address }) + } catch (_err) { + const err = asError(_err) + trackError(Errors._810, err) + setIsSubmittable(true) + setSubmitError(err) + return + } + + setTxFlow(undefined) + } + + const submitDisabled = !safeTx || !isSubmittable || !recovery + + return ( + <> + + + This transaction will reset the Account setup, changing the owners + {newThreshold !== safe.threshold ? ' and threshold' : ''}. + + + + + + + + + After recovery, Safe Account transactions will require: + + + {params.threshold} out of {params[RecoverAccountFlowFields.owners].length} owners. + + + + + + + + + + + + + + + + + + {safeTxError && ( + + This recovery will most likely fail. To save gas costs, avoid executing the transaction. + + )} + + {submitError && ( + Error submitting the transaction. Please try again. + )} + + + + + {/* // TODO: Convert txCooldown to days, minutes, seconds when https://github.com/safe-global/safe-wallet-web/pull/2772 is merged */} + Recovery will be {txCooldown === 0 ? 'immediately possible' : `possible ${txCooldown}`} after this transaction + is executed. + + + + + + + {(isOk) => ( + + )} + + + + + ) +} diff --git a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx new file mode 100644 index 0000000000..2d67ae8915 --- /dev/null +++ b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx @@ -0,0 +1,166 @@ +import { + Typography, + Divider, + CardActions, + Button, + SvgIcon, + Grid, + MenuItem, + TextField, + IconButton, + Tooltip, +} from '@mui/material' +import { useForm, FormProvider, useFieldArray, Controller } from 'react-hook-form' +import type { ReactElement } from 'react' + +import TxCard from '../../common/TxCard' +import AddIcon from '@/public/images/common/add.svg' +import DeleteIcon from '@/public/images/common/delete.svg' +import { RecoverAccountFlowFields } from '.' +import AddressBookInput from '@/components/common/AddressBookInput' +import { TOOLTIP_TITLES } from '../../common/constants' +import InfoIcon from '@/public/images/notifications/info.svg' +import useSafeInfo from '@/hooks/useSafeInfo' +import { sameAddress } from '@/utils/addresses' +import type { RecoverAccountFlowProps } from '.' + +import commonCss from '@/components/tx-flow/common/styles.module.css' + +export function RecoverAccountFlowSetup({ + params, + onSubmit, +}: { + params: RecoverAccountFlowProps + onSubmit: (formData: RecoverAccountFlowProps) => void +}): ReactElement { + const { safeAddress } = useSafeInfo() + + const formMethods = useForm({ + defaultValues: params, + mode: 'all', + }) + + const { fields, append, remove } = useFieldArray({ + control: formMethods.control, + name: RecoverAccountFlowFields.owners, + }) + + const owners = formMethods.watch(RecoverAccountFlowFields.owners) + + return ( + + + + + Add owner(s) + + + + Set the new owner wallet(s) of this Safe Account and how many need to confirm a transaction before it can be + executed. + + + + {fields.map((field, index) => ( + <> + + { + if (sameAddress(value, safeAddress)) { + return 'The Safe Account cannot own itself' + } + + const isDuplicate = owners.filter((owner) => owner.value === value).length > 1 + if (isDuplicate) { + return 'Already designated to be an owner' + } + }} + /> + + + + {index > 0 && ( + remove(index)}> + + + )} + + + ))} + + + + + + + + Threshold + + + + + + + + + After recovery, Safe Account transactions will require: + + + + + ( + + {fields.map((_, index) => { + const value = index + 1 + return ( + + {value} + + ) + })} + + )} + /> + + + + out of {fields.length} owner(s) + + + + + + + + + + + + ) +} diff --git a/src/components/tx-flow/flows/RecoverAccount/index.tsx b/src/components/tx-flow/flows/RecoverAccount/index.tsx new file mode 100644 index 0000000000..af075b6e77 --- /dev/null +++ b/src/components/tx-flow/flows/RecoverAccount/index.tsx @@ -0,0 +1,44 @@ +import type { ReactElement } from 'react' +import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk' + +import TxLayout from '@/components/tx-flow/common/TxLayout' +import SaveAddressIcon from '@/public/images/common/save-address.svg' +import useTxStepper from '../../useTxStepper' +import { RecoverAccountFlowReview } from './RecoverAccountFlowReview' +import { RecoverAccountFlowSetup } from './RecoverAccountFlowSetup' + +export enum RecoverAccountFlowFields { + owners = 'owners', + threshold = 'threshold', +} + +export type RecoverAccountFlowProps = { + // RHF accept primitive field arrays + [RecoverAccountFlowFields.owners]: Array + [RecoverAccountFlowFields.threshold]: string +} + +export function RecoverAccountFlow(): ReactElement { + const { data, step, nextStep, prevStep } = useTxStepper({ + [RecoverAccountFlowFields.owners]: [{ value: '' }], + [RecoverAccountFlowFields.threshold]: '1', + }) + + const steps = [ + nextStep({ ...data, ...formData })} />, + , + ] + + return ( + + {steps} + + ) +} diff --git a/src/components/tx/SignOrExecuteForm/TxChecks.tsx b/src/components/tx/SignOrExecuteForm/TxChecks.tsx index d1fb2444fa..9ad3f069f0 100644 --- a/src/components/tx/SignOrExecuteForm/TxChecks.tsx +++ b/src/components/tx/SignOrExecuteForm/TxChecks.tsx @@ -6,14 +6,14 @@ import { Redefine, RedefineMessage } from '@/components/tx/security/redefine' import css from './styles.module.css' -const TxChecks = (): ReactElement => { +const TxChecks = ({ isRecovery = false }: { isRecovery?: boolean }): ReactElement => { const { safeTx } = useContext(SafeTxContext) return ( <> Transaction checks - + diff --git a/src/components/tx/security/tenderly/index.tsx b/src/components/tx/security/tenderly/index.tsx index 4f1487205e..fa4ce7c6a3 100644 --- a/src/components/tx/security/tenderly/index.tsx +++ b/src/components/tx/security/tenderly/index.tsx @@ -23,12 +23,18 @@ export type TxSimulationProps = { transactions?: SimulationTxParams['transactions'] gasLimit?: number disabled: boolean + isRecovery?: boolean } // TODO: Investigate resetting on gasLimit change as we are not simulating with the gasLimit of the tx // otherwise remove all usage of gasLimit in simulation. Note: this was previously being done. // TODO: Test this component -const TxSimulationBlock = ({ transactions, disabled, gasLimit }: TxSimulationProps): ReactElement => { +const TxSimulationBlock = ({ + transactions, + disabled, + gasLimit, + isRecovery = false, +}: TxSimulationProps): ReactElement => { const { safe } = useSafeInfo() const wallet = useWallet() const isDarkMode = useDarkMode() @@ -45,7 +51,7 @@ const TxSimulationBlock = ({ transactions, disabled, gasLimit }: TxSimulationPro simulateTransaction({ safe, - executionOwner: wallet.address, + executionOwner: isRecovery ? safe.owners[0].value : wallet.address, transactions, gasLimit, } as SimulationTxParams) diff --git a/src/services/exceptions/ErrorCodes.ts b/src/services/exceptions/ErrorCodes.ts index 90993a46ef..8319df5f75 100644 --- a/src/services/exceptions/ErrorCodes.ts +++ b/src/services/exceptions/ErrorCodes.ts @@ -61,6 +61,7 @@ enum ErrorCodes { _807 = '807: Failed to remove guard', _808 = '808: Failed to get transaction origin', _809 = '809: Failed decoding transaction', + _810 = '810: Error executing a recovery proposal transaction', _900 = '900: Error loading Safe App', _901 = '901: Error processing Safe Apps SDK request', diff --git a/src/services/recovery/__tests__/transaction.test.ts b/src/services/recovery/__tests__/transaction.test.ts new file mode 100644 index 0000000000..faf447c8d8 --- /dev/null +++ b/src/services/recovery/__tests__/transaction.test.ts @@ -0,0 +1,666 @@ +import { faker } from '@faker-js/faker' +import { Interface } from 'ethers/lib/utils' +import { SENTINEL_ADDRESS } from '@safe-global/safe-core-sdk/dist/src/utils/constants' +import { OperationType } from '@safe-global/safe-core-sdk-types' +import * as deployments from '@safe-global/safe-deployments' +import type { SafeInfo } from '@safe-global/safe-gateway-typescript-sdk' + +import { getRecoveryProposalTransaction, getRecoveryProposalTransactions } from '../transaction' + +describe('transaction', () => { + describe('getRecoveryTransactions', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + const encodeFunctionDataSpy = jest.spyOn(Interface.prototype, 'encodeFunctionData') + + describe('when recovering with the same number of owner(s) as the current Safe owner(s)', () => { + describe('with unique owners', () => { + describe('should swap all owners when the threshold remains the same', () => { + it('for singular owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(1) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(1) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + }) + + it('for multiple owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + }) + }) + + it('should swap all owners and finally change the threshold if it changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(4) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(4) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'changeThreshold', [newThreshold]) + }) + }) + + describe('with duplicate owners', () => { + describe('should swap all differing owners when the threshold remains the same', () => { + it('for singular owners it should return nothing', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(0) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(0) + }) + + it('for multiple owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = oldOwner3 + + const oldThreshold = 2 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(2) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + }) + }) + + it('should swap all differing owners and finally change the threshold if it changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: oldOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'changeThreshold', [newThreshold]) + }) + }) + + it('should change the threshold with the same owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(1) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(1) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'changeThreshold', [newThreshold]) + }) + }) + + describe('when recovering with more owner(s) than the current Safe owner(s)', () => { + describe('with unique owners', () => { + describe('should swap as many owners as possible then add the rest when the threshold remains the same', () => { + it('for singular owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner2, 1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) + }) + + it('for multiple owners', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) + }) + }) + + it('should swap as many owners as possible then add the rest when with a final threshold change if the threshold changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, newThreshold]) + }) + }) + + describe('with duplicates owners', () => { + it('should swap as many differing owners as possible then add the rest when the threshold remains the same', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(2) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, 1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) + }) + + it('should swap as many differing owners as possible then add the rest when with a final threshold change if the threshold changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }, { value: newOwner2 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(2) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner1, 1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner2, newThreshold]) + }) + }) + }) + + describe('when recovering with less owner(s) than the current Safe owner(s)', () => { + describe('with unique owners', () => { + it('should swap as many owners as possible then remove the rest when the threshold remains the same', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }] + + const transactions = getRecoveryProposalTransactions({ + safe, + newThreshold: oldThreshold, + newOwners, + }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + }) + + it('should swap as many owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(3) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(3) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + }) + }) + + describe('with duplicates owners', () => { + it('should swap as many differing owners as possible then remove the rest when the threshold remains the same', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold: oldThreshold, newOwners }) + + expect(transactions).toHaveLength(2) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + }) + + it('should swap as many differing owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }] + + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + expect(transactions).toHaveLength(2) + + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + }) + }) + }) + }) + + describe('getRecoveryProposalTransaction', () => { + it('should throw an error when no recovery transactions are found', () => { + const safe = { + address: { value: faker.finance.ethereumAddress() }, + owners: [{ value: faker.finance.ethereumAddress() }], + threshold: 1, + } as SafeInfo + + expect(() => + getRecoveryProposalTransaction({ + safe, + newThreshold: safe.threshold, + newOwners: safe.owners, + }), + ).toThrow('No recovery transactions found') + }) + + it('should return the transaction when a single recovery transaction is found', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const newOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }] + + const transaction = getRecoveryProposalTransaction({ + safe, + newThreshold: oldThreshold, + newOwners, + }) + + expect(transaction).toEqual({ + to: safeAddresss, + value: '0', + data: expect.any(String), + operation: OperationType.Call, + }) + }) + + describe('when multiple recovery transactions are found', () => { + it('should return a MetaTransactionData object ', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + const oldOwner3 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + const newThreshold = oldThreshold + 1 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }], + threshold: oldThreshold, + } as SafeInfo + + const multiSendDeployment = deployments.getMultiSendCallOnlyDeployment()! + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + const transaction = getRecoveryProposalTransaction({ + safe, + newThreshold, + newOwners, + }) + + expect(transaction).toEqual({ + to: multiSendDeployment.defaultAddress, + value: '0', + data: expect.any(String), + operation: OperationType.Call, + }) + }) + + it('should throw an error when MultiSend deployment is not found', () => { + jest.spyOn(deployments, 'getMultiSendCallOnlyDeployment').mockReturnValue(undefined) + + const safeAddresss = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + const oldOwner2 = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + + const oldThreshold = 2 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }, { value: oldOwner2 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }] + + expect(() => + getRecoveryProposalTransaction({ + safe, + newThreshold: oldThreshold, + newOwners, + }), + ).toThrow('MultiSend deployment not found') + }) + }) + }) +}) diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts new file mode 100644 index 0000000000..83e6786904 --- /dev/null +++ b/src/services/recovery/transaction.ts @@ -0,0 +1,133 @@ +import { Interface } from 'ethers/lib/utils' +import { getMultiSendCallOnlyDeployment, getSafeSingletonDeployment } from '@safe-global/safe-deployments' +import { SENTINEL_ADDRESS } from '@safe-global/safe-core-sdk/dist/src/utils/constants' +import { encodeMultiSendData } from '@safe-global/safe-core-sdk/dist/src/utils/transactions/utils' +import { OperationType } from '@safe-global/safe-core-sdk-types' +import type { MetaTransactionData } from '@safe-global/safe-core-sdk-types' +import type { AddressEx, SafeInfo } from '@safe-global/safe-gateway-typescript-sdk' +import { sameAddress } from '@/utils/addresses' + +export function getRecoveryProposalTransactions({ + safe, + newThreshold, + newOwners, +}: { + safe: SafeInfo + newThreshold: number + newOwners: Array +}): Array { + const INTERMEDIARY_THRESHOLD = 1 + + const safeDeployment = getSafeSingletonDeployment({ network: safe.chainId, version: safe.version ?? undefined }) + + if (!safeDeployment) { + throw new Error('Safe deployment not found') + } + + const safeInterface = new Interface(safeDeployment.abi) + + // Cache owner changes to determine prevOwner + let _owners = [...safe.owners] + + // Don't add/remove same owners + const ownersToAdd = newOwners.filter( + (newOwner) => !_owners.some((oldOwner) => sameAddress(oldOwner.value, newOwner.value)), + ) + const ownersToRemove = safe.owners.filter( + (oldOwner) => !newOwners.some((newOwner) => sameAddress(newOwner.value, oldOwner.value)), + ) + + // Check whether threshold should be changed after owner management + let changeThreshold = newThreshold !== safe.threshold + + const txData: Array = [] + + const length = Math.max(ownersToAdd.length, ownersToRemove.length) + + for (let index = 0; index < length; index++) { + const ownerToAdd = ownersToAdd[index]?.value + const ownerToRemove = ownersToRemove[index]?.value + + const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) + const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value + + // Swap owner if possible + if (ownerToRemove && ownerToAdd) { + txData.push(safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd])) + + // Update cache to reflect swap + _owners = _owners.map((owner) => { + if (sameAddress(owner.value, ownerToRemove)) { + return ownersToAdd[index] + } + return owner + }) + continue + } + + // Add or remove owner, finally setting threshold (intermediary value prevents threshold > owner length) + const threshold = index === length - 1 ? newThreshold : INTERMEDIARY_THRESHOLD + + if (!ownerToRemove) { + txData.push(safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold])) + + // Update cache to reflect addition + _owners.push(ownersToAdd[index]) + } else { + txData.push(safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold])) + + // Update cache to reflect removal + _owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove)) + } + + changeThreshold = false + } + + // If only swapOwners exist but there is a threshold change, changeThreshold + if (changeThreshold) { + txData.push(safeInterface.encodeFunctionData('changeThreshold', [newThreshold])) + } + + return txData.map((data) => ({ + to: safe.address.value, + value: '0', + operation: OperationType.Call, + data, + })) +} + +export function getRecoveryProposalTransaction({ + safe, + newThreshold, + newOwners, +}: { + safe: SafeInfo + newThreshold: number + newOwners: Array +}): MetaTransactionData { + const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) + + if (transactions.length === 0) { + throw new Error('No recovery transactions found') + } + + if (transactions.length === 1) { + return transactions[0] + } + + const multiSendDeployment = getMultiSendCallOnlyDeployment({ + network: safe.chainId, + version: safe.version ?? undefined, + }) + + if (!multiSendDeployment) { + throw new Error('MultiSend deployment not found') + } + + return { + to: multiSendDeployment.networkAddresses[safe.chainId] ?? multiSendDeployment.defaultAddress, + value: '0', + operation: OperationType.Call, + data: encodeMultiSendData(transactions), + } +} diff --git a/src/services/tx/tx-sender/dispatch.ts b/src/services/tx/tx-sender/dispatch.ts index 570aa0e0ea..98b9ce244c 100644 --- a/src/services/tx/tx-sender/dispatch.ts +++ b/src/services/tx/tx-sender/dispatch.ts @@ -1,4 +1,5 @@ -import type { SafeInfo, TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' +import type { AddressEx, SafeInfo, TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' +import { OperationType } from '@safe-global/safe-core-sdk-types' import type { SafeTransaction, TransactionOptions, TransactionResult } from '@safe-global/safe-core-sdk-types' import type { EthersError } from '@/utils/ethers-utils' import { didReprice, didRevert } from '@/utils/ethers-utils' @@ -22,6 +23,8 @@ import { import { createWeb3 } from '@/hooks/wallets/web3' import { type OnboardAPI } from '@web3-onboard/core' import { asError } from '@/services/exceptions/utils' +import { getRecoveryProposalTransaction } from '@/services/recovery/transaction' +import { getModuleInstance, KnownContracts } from '@gnosis.pm/zodiac' /** * Propose a transaction @@ -400,3 +403,31 @@ export const dispatchBatchExecutionRelay = async ( groupKey, ) } + +export async function dispatchRecoveryProposal({ + onboard, + safe, + newThreshold, + newOwners, + delayModifierAddress, +}: { + onboard: OnboardAPI + safe: SafeInfo + newThreshold: number + newOwners: Array + delayModifierAddress: string +}) { + const wallet = await assertWalletChain(onboard, safe.chainId) + const provider = createWeb3(wallet.provider) + + const { to, value, data } = getRecoveryProposalTransaction({ + safe, + newThreshold, + newOwners, + }) + + const delayModifier = getModuleInstance(KnownContracts.DELAY, delayModifierAddress, provider) + + const signer = provider.getSigner() + await delayModifier.connect(signer).execTransactionFromModule(to, value, data, OperationType.Call) +} diff --git a/src/store/recoverySlice.ts b/src/store/recoverySlice.ts index 1755799cf3..855bc2da0b 100644 --- a/src/store/recoverySlice.ts +++ b/src/store/recoverySlice.ts @@ -3,6 +3,8 @@ import type { TransactionAddedEvent } from '@gnosis.pm/zodiac/dist/cjs/types/Del import type { BigNumber } from 'ethers' import { makeLoadableSlice } from './common' +import { sameAddress } from '@/utils/addresses' +import type { RootState } from '.' export type RecoveryQueueItem = TransactionAddedEvent & { timestamp: number @@ -26,4 +28,11 @@ const { slice, selector } = makeLoadableSlice('recovery', initialState) export const recoverySlice = slice -export const selectRecovery = createSelector(selector, (recovery) => recovery.data) +const selectRecovery = createSelector(selector, (recovery) => recovery.data) + +export const selectRecoveryByGuardian = createSelector( + [selectRecovery, (_: RootState, walletAddress: string) => walletAddress], + (recovery, walletAddress) => { + return recovery.find(({ modules }) => modules.some((module) => sameAddress(module, walletAddress))) + }, +) From 76ee0dfc3d5fabaaa271a997ccf3bd50566a3a52 Mon Sep 17 00:00:00 2001 From: iamacook Date: Tue, 14 Nov 2023 18:31:06 +0100 Subject: [PATCH 07/18] fix: only reference owners cache --- src/services/recovery/transaction.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index 83e6786904..9cbe01b4bc 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -33,7 +33,7 @@ export function getRecoveryProposalTransactions({ const ownersToAdd = newOwners.filter( (newOwner) => !_owners.some((oldOwner) => sameAddress(oldOwner.value, newOwner.value)), ) - const ownersToRemove = safe.owners.filter( + const ownersToRemove = _owners.filter( (oldOwner) => !newOwners.some((newOwner) => sameAddress(newOwner.value, oldOwner.value)), ) @@ -49,19 +49,13 @@ export function getRecoveryProposalTransactions({ const ownerToRemove = ownersToRemove[index]?.value const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) - const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value + const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1].value // Swap owner if possible if (ownerToRemove && ownerToAdd) { txData.push(safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd])) + _owners = _owners.map((owner) => (sameAddress(owner.value, ownerToRemove) ? ownersToAdd[index] : owner)) - // Update cache to reflect swap - _owners = _owners.map((owner) => { - if (sameAddress(owner.value, ownerToRemove)) { - return ownersToAdd[index] - } - return owner - }) continue } @@ -70,13 +64,9 @@ export function getRecoveryProposalTransactions({ if (!ownerToRemove) { txData.push(safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold])) - - // Update cache to reflect addition _owners.push(ownersToAdd[index]) } else { txData.push(safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold])) - - // Update cache to reflect removal _owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove)) } From 6b338f2867e40b1f3f95be967d718c80ecde850f Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 15 Nov 2023 08:07:40 +0100 Subject: [PATCH 08/18] fix: owner management transaction --- .../recovery/__tests__/transaction.test.ts | 94 ++++++++++++++----- src/services/recovery/transaction.ts | 56 ++++++++--- 2 files changed, 112 insertions(+), 38 deletions(-) diff --git a/src/services/recovery/__tests__/transaction.test.ts b/src/services/recovery/__tests__/transaction.test.ts index faf447c8d8..a3d5b56ed9 100644 --- a/src/services/recovery/__tests__/transaction.test.ts +++ b/src/services/recovery/__tests__/transaction.test.ts @@ -78,8 +78,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3]) }) }) @@ -115,8 +115,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'changeThreshold', [newThreshold]) }) }) @@ -176,7 +176,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) }) }) @@ -211,7 +211,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'changeThreshold', [newThreshold]) }) }) @@ -309,7 +309,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) }) }) @@ -345,7 +345,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, newThreshold]) }) }) @@ -374,20 +374,23 @@ describe('transaction', () => { expect(transactions).toHaveLength(2) expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, 1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, oldThreshold]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) }) it('should swap as many differing owners as possible then add the rest when with a final threshold change if the threshold changes', () => { const safeAddresss = faker.finance.ethereumAddress() - const oldOwner1 = faker.finance.ethereumAddress() - const newOwner1 = faker.finance.ethereumAddress() const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + const newOwner4 = faker.finance.ethereumAddress() + const newOwner5 = faker.finance.ethereumAddress() - const oldThreshold = 2 - const newThreshold = oldThreshold + 1 + const oldOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + const newThreshold = 4 const safe = { address: { value: safeAddresss }, @@ -395,15 +398,34 @@ describe('transaction', () => { threshold: oldThreshold, } as SafeInfo - const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }, { value: newOwner2 }] + const newOwners = [ + { value: newOwner1 }, + { value: newOwner2 }, + { value: newOwner3 }, + { value: newOwner4 }, + { value: newOwner5 }, + ] const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) - expect(transactions).toHaveLength(2) + expect(transactions).toHaveLength(5) - expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner1, 1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner2, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(5) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [ + newOwner2, + 2, // Intemediary threshold - length of current owners + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [ + newOwner3, + 3, // Intemediary threshold - length of current owners + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'addOwnerWithThreshold', [newOwner4, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(5, 'addOwnerWithThreshold', [newOwner5, newThreshold]) }) }) }) @@ -444,8 +466,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, oldThreshold]) }) it('should swap as many owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { @@ -479,8 +501,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, newThreshold]) }) }) @@ -510,7 +532,7 @@ describe('transaction', () => { expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, oldThreshold]) }) it('should swap as many differing owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { @@ -539,10 +561,34 @@ describe('transaction', () => { expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, newThreshold]) }) }) }) + + it('should throw if the new threshold is higher than the final owner output', () => { + const safeAddresss = faker.finance.ethereumAddress() + + const newOwner1 = faker.finance.ethereumAddress() + const newOwner2 = faker.finance.ethereumAddress() + + const oldOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + const newThreshold = 10 + + const safe = { + address: { value: safeAddresss }, + owners: [{ value: oldOwner1 }], + threshold: oldThreshold, + } as SafeInfo + + const newOwners = [{ value: newOwner1 }, { value: newOwner2 }] + + expect(() => getRecoveryProposalTransactions({ safe, newThreshold, newOwners })).toThrow( + 'New threshold is higher than desired owners', + ) + }) }) describe('getRecoveryProposalTransaction', () => { diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index 9cbe01b4bc..dd36048bad 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -16,8 +16,6 @@ export function getRecoveryProposalTransactions({ newThreshold: number newOwners: Array }): Array { - const INTERMEDIARY_THRESHOLD = 1 - const safeDeployment = getSafeSingletonDeployment({ network: safe.chainId, version: safe.version ?? undefined }) if (!safeDeployment) { @@ -40,40 +38,70 @@ export function getRecoveryProposalTransactions({ // Check whether threshold should be changed after owner management let changeThreshold = newThreshold !== safe.threshold + // Owner management transaction data const txData: Array = [] + // Minimum number of owner management transactions required const length = Math.max(ownersToAdd.length, ownersToRemove.length) + // Iterate of existing/new owners and swap, add, remove accordingly for (let index = 0; index < length; index++) { const ownerToAdd = ownersToAdd[index]?.value const ownerToRemove = ownersToRemove[index]?.value - const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) - const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1].value + const prevOwner = (() => { + const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) + return ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value + })() - // Swap owner if possible + // Swap existing owner with new one if (ownerToRemove && ownerToAdd) { - txData.push(safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd])) + const swapOwner = safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd]) + txData.push(swapOwner) + + // Swap owner in cache _owners = _owners.map((owner) => (sameAddress(owner.value, ownerToRemove) ? ownersToAdd[index] : owner)) continue } - // Add or remove owner, finally setting threshold (intermediary value prevents threshold > owner length) - const threshold = index === length - 1 ? newThreshold : INTERMEDIARY_THRESHOLD + const threshold = (() => { + const newOwnerLength = ownerToAdd ? _owners.length + 1 : _owners.length - 1 + + // Final transaction + if (index === length - 1) { + if (newThreshold > newOwnerLength) { + throw new Error('New threshold is higher than desired owners') + } + return newThreshold + } + + // Prevent intermediary threshold > number of owners + return newThreshold > newOwnerLength ? newOwnerLength : newThreshold + })() + + // Add new owner and set threshold + if (ownerToAdd) { + const addOwnerWithThreshold = safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold]) + txData.push(addOwnerWithThreshold) + + // Add owner to cache + _owners.push({ value: ownerToAdd }) + } + // Remove existing owner and set threshold + else { + const removeOwner = safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold]) + txData.push(removeOwner) - if (!ownerToRemove) { - txData.push(safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold])) - _owners.push(ownersToAdd[index]) - } else { - txData.push(safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold])) + // Remove owner from cache _owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove)) } + // addOwnerWithThreshold/removeOwner changed threshold changeThreshold = false } - // If only swapOwners exist but there is a threshold change, changeThreshold + // Only swapOwner will be called if (changeThreshold) { txData.push(safeInterface.encodeFunctionData('changeThreshold', [newThreshold])) } From 6f3d3c700925feb70bd8e0b9e89847773197447d Mon Sep 17 00:00:00 2001 From: iamacook Date: Thu, 16 Nov 2023 13:24:21 +0300 Subject: [PATCH 09/18] fix: move error --- src/services/recovery/transaction.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index dd36048bad..9d425efcf8 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -67,17 +67,8 @@ export function getRecoveryProposalTransactions({ const threshold = (() => { const newOwnerLength = ownerToAdd ? _owners.length + 1 : _owners.length - 1 - - // Final transaction - if (index === length - 1) { - if (newThreshold > newOwnerLength) { - throw new Error('New threshold is higher than desired owners') - } - return newThreshold - } - // Prevent intermediary threshold > number of owners - return newThreshold > newOwnerLength ? newOwnerLength : newThreshold + return index === length - 1 ? newThreshold : newThreshold > newOwnerLength ? newOwnerLength : newThreshold })() // Add new owner and set threshold @@ -106,6 +97,10 @@ export function getRecoveryProposalTransactions({ txData.push(safeInterface.encodeFunctionData('changeThreshold', [newThreshold])) } + if (newThreshold > _owners.length) { + throw new Error('New threshold is higher than desired owners') + } + return txData.map((data) => ({ to: safe.address.value, value: '0', From 127a934065d84470473031e034f642dda5fb598d Mon Sep 17 00:00:00 2001 From: iamacook Date: Sat, 18 Nov 2023 10:19:42 +0300 Subject: [PATCH 10/18] fix: encode `multiSend` `data` --- src/services/recovery/transaction.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index 9d425efcf8..b93b41b48b 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -137,10 +137,13 @@ export function getRecoveryProposalTransaction({ throw new Error('MultiSend deployment not found') } + const multiSendInterface = new Interface(multiSendDeployment.abi) + const multiSendData = encodeMultiSendData(transactions) + return { to: multiSendDeployment.networkAddresses[safe.chainId] ?? multiSendDeployment.defaultAddress, value: '0', operation: OperationType.Call, - data: encodeMultiSendData(transactions), + data: multiSendInterface.encodeFunctionData('multiSend', [multiSendData]), } } From 0fb37869516077a163a979e0269b1eecacddd993 Mon Sep 17 00:00:00 2001 From: iamacook Date: Sat, 18 Nov 2023 20:36:33 +0300 Subject: [PATCH 11/18] fix: cleanup code + rename test --- .../EnableRecoveryFlowReview.tsx | 29 +++++++++---------- .../EnableRecoveryFlowSettings.tsx | 7 +---- .../tx-flow/flows/EnableRecovery/index.tsx | 19 +++++------- src/services/recovery/__tests__/setup.test.ts | 2 +- 4 files changed, 22 insertions(+), 35 deletions(-) diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx index 97bb575dcb..245087b784 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowReview.tsx @@ -1,4 +1,4 @@ -import { useContext, useEffect, useMemo } from 'react' +import { useContext, useEffect } from 'react' import type { ReactElement } from 'react' import SignOrExecuteForm from '@/components/tx/SignOrExecuteForm' @@ -20,23 +20,27 @@ export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlo const { safe } = useSafeInfo() const { setSafeTx, safeTxError, setSafeTxError } = useContext(SafeTxContext) - const recoverySetup = useMemo(() => { + const guardian = params[EnableRecoveryFlowFields.guardians] + const delay = RecoveryDelayPeriods.find(({ value }) => value === params[EnableRecoveryFlowFields.txCooldown])!.label + const expiration = RecoveryExpirationPeriods.find( + ({ value }) => value === params[EnableRecoveryFlowFields.txExpiration], + )!.label + const emailAddress = params[EnableRecoveryFlowFields.emailAddress] + + useEffect(() => { if (!web3) { return } - return getRecoverySetup({ + const { transactions } = getRecoverySetup({ ...params, + guardians: [guardian], safe, provider: web3, }) - }, [params, safe, web3]) - useEffect(() => { - if (recoverySetup) { - createMultiSendCallOnlyTx(recoverySetup.transactions).then(setSafeTx).catch(setSafeTxError) - } - }, [recoverySetup, setSafeTx, setSafeTxError]) + createMultiSendCallOnlyTx(transactions).then(setSafeTx).catch(setSafeTxError) + }, [guardian, params, safe, setSafeTx, setSafeTxError, web3]) useEffect(() => { if (safeTxError) { @@ -44,13 +48,6 @@ export function EnableRecoveryFlowReview({ params }: { params: EnableRecoveryFlo } }, [safeTxError]) - const guardian = params[EnableRecoveryFlowFields.guardians][0] - const delay = RecoveryDelayPeriods.find(({ value }) => value === params[EnableRecoveryFlowFields.txCooldown])!.label - const expiration = RecoveryExpirationPeriods.find( - ({ value }) => value === params[EnableRecoveryFlowFields.txExpiration], - )!.label - const emailAddress = params[EnableRecoveryFlowFields.emailAddress] - return ( null}> This transaction will enable the Account recovery feature once executed. diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx index a404ed6e73..5cd84668c9 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx @@ -59,12 +59,7 @@ export function EnableRecoveryFlowSettings({ recovery process in the future. - + Recovery delay diff --git a/src/components/tx-flow/flows/EnableRecovery/index.tsx b/src/components/tx-flow/flows/EnableRecovery/index.tsx index 376d70bead..18a5ea88ef 100644 --- a/src/components/tx-flow/flows/EnableRecovery/index.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/index.tsx @@ -40,6 +40,8 @@ export const RecoveryExpirationPeriods = [ ...RecoveryDelayPeriods, ] as const +const Subtitles = ['How does recovery work?', 'Set up recovery settings', 'Set up account recovery'] + export enum EnableRecoveryFlowFields { guardians = 'guardians', txCooldown = 'txCooldown', @@ -48,7 +50,7 @@ export enum EnableRecoveryFlowFields { } export type EnableRecoveryFlowProps = { - [EnableRecoveryFlowFields.guardians]: Array + [EnableRecoveryFlowFields.guardians]: string [EnableRecoveryFlowFields.txCooldown]: string [EnableRecoveryFlowFields.txExpiration]: string [EnableRecoveryFlowFields.emailAddress]: string @@ -56,8 +58,8 @@ export type EnableRecoveryFlowProps = { export function EnableRecoveryFlow(): ReactElement { const { data, step, nextStep, prevStep } = useTxStepper({ - [EnableRecoveryFlowFields.guardians]: [''], - [EnableRecoveryFlowFields.txCooldown]: `${60 * 60 * 24 * 28}`, // 28 days in seconds + [EnableRecoveryFlowFields.guardians]: '', + [EnableRecoveryFlowFields.txCooldown]: `${DAY_SECONDS * 28}`, // 28 days in seconds [EnableRecoveryFlowFields.txExpiration]: '0', [EnableRecoveryFlowFields.emailAddress]: '', }) @@ -65,24 +67,17 @@ export function EnableRecoveryFlow(): ReactElement { const steps = [ nextStep(data)} />, nextStep({ ...data, ...formData })} />, - , + , ] const isIntro = step === 0 - const isSettings = step === 1 - - const subtitle = isIntro - ? 'How does recovery work?' - : isSettings - ? 'Set up account recovery settings' - : 'Set up account recovery' const icon = isIntro ? undefined : RecoveryPlus return ( { jest.clearAllMocks() }) - it('should deploy Delay Modifier, enable it on Safe and add a Guardian', () => { + it('should return deploy Delay Modifier, enable Safe guardian and add a Guardian transactions', () => { const txCooldown = faker.string.numeric() const txExpiration = faker.string.numeric() const guardians = [faker.finance.ethereumAddress()] From 54f2aeaaabcbce818b15efdabc4709c49059c8be Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 09:29:55 +0100 Subject: [PATCH 12/18] fix: test --- src/hooks/__tests__/useLoadRecovery.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/hooks/__tests__/useLoadRecovery.test.ts b/src/hooks/__tests__/useLoadRecovery.test.ts index fb77e655c7..f4b693b873 100644 --- a/src/hooks/__tests__/useLoadRecovery.test.ts +++ b/src/hooks/__tests__/useLoadRecovery.test.ts @@ -127,6 +127,12 @@ describe('useLoadRecovery', () => { txNonce, queueNonce, queue: [ + { + ...transactionsAdded[0], + timestamp: 69, + validFrom: BigNumber.from(69).add(txCooldown), + expiresAt: null, + }, { ...transactionsAdded[1], timestamp: 420, From 2a78391b6029fb5b7ddaa5c84656125e7f7f9333 Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 10:24:41 +0100 Subject: [PATCH 13/18] fix: spacing + add connector --- src/components/settings/Recovery/index.tsx | 14 +++++++--- .../EnableRecoveryFlowIntro.tsx | 19 ++++++++------ .../EnableRecoveryFlowSettings.tsx | 26 ++++++++++++------- .../flows/EnableRecovery/styles.module.css | 16 ++++++++++++ 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx index 3783806a2c..d5a1704e37 100644 --- a/src/components/settings/Recovery/index.tsx +++ b/src/components/settings/Recovery/index.tsx @@ -1,10 +1,11 @@ -import { Box, Button, Chip, Grid, Paper, Typography } from '@mui/material' +import { Alert, Box, Button, Chip, Grid, Paper, Typography } from '@mui/material' import { useContext } from 'react' import type { ReactElement } from 'react' import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' import { TxModalContext } from '@/components/tx-flow' import { useDarkMode } from '@/hooks/useDarkMode' +import ExternalLink from '@/components/common/ExternalLink' export function Recovery(): ReactElement { const { setTxFlow } = useContext(TxModalContext) @@ -30,12 +31,19 @@ export function Recovery(): ReactElement { - + Choose a trusted guardian to recover your Safe Account, in case you should ever lose access to your Account. Enabling the Account recovery module will require a transactions. - diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx index 6e75d7aa85..bd727faf11 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx @@ -7,22 +7,23 @@ import RecoveryGuardian from '@/public/images/transactions/recovery-guardian.svg import RecoveryDelay from '@/public/images/settings/spending-limit/time.svg' import RecoveryExecution from '@/public/images/transactions/recovery-execution.svg' +import css from './styles.module.css' import commonCss from '@/components/tx-flow/common/styles.module.css' -const RecoverySteps: Array<{ icon: ReactElement; title: string; subtitle: ReactNode }> = [ +const RecoverySteps: Array<{ Icon: ReactElement; title: string; subtitle: ReactNode }> = [ { - icon: , + Icon: RecoveryGuardians, title: 'Choose a guardian and set a delay', subtitle: 'Only your chosen guardian can initiate the recovery process. The process can be cancelled at any time during the delay period.', }, { - icon: , + Icon: RecoveryGuardian, title: 'Lost access? Let the guardian connect', subtitle: 'The recovery process can be initiated by a trusted guardian when connected to your Safe Account.', }, { - icon: , + Icon: RecoveryDelay, title: 'Start the recovery process', subtitle: ( <> @@ -32,7 +33,7 @@ const RecoverySteps: Array<{ icon: ReactElement; title: string; subtitle: ReactN ), }, { - icon: , + Icon: RecoveryExecution, title: 'All done! The Account is yours again', subtitle: 'Once the delay period has passed, you can execute the recovery transaction and regain access to your Safe Account.', @@ -42,11 +43,13 @@ const RecoverySteps: Array<{ icon: ReactElement; title: string; subtitle: ReactN export function EnableRecoveryFlowIntro({ onSubmit }: { onSubmit: () => void }): ReactElement { return ( - - {RecoverySteps.map(({ icon, title, subtitle }, index) => ( + + {RecoverySteps.map(({ Icon, title, subtitle }, index) => ( - {icon} + + + {title} diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx index 5cd84668c9..7bbac1363f 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowSettings.tsx @@ -52,20 +52,28 @@ export function EnableRecoveryFlowSettings({
- Trusted guardian +
+ + Trusted guardian + - - Choose a guardian, such as a hardware wallet or family member's wallet, that can initiate the - recovery process in the future. - + + Choose a guardian, such as a hardware wallet or family member's wallet, that can initiate the + recovery process in the future. + +
- Recovery delay +
+ + Recovery delay + - - You can cancel any recovery attempt when it is not needed or wanted within the delay period. - + + You can cancel any recovery attempt when it is not needed or wanted within the delay period. + +
Date: Mon, 20 Nov 2023 10:43:11 +0100 Subject: [PATCH 14/18] refactor: extract `Chip` component --- src/components/common/Chip/index.tsx | 18 ++++++++++++++++++ src/components/dashboard/Recovery/index.tsx | 13 ++++++++----- .../dashboard/Recovery/styles.module.css | 5 ----- src/components/settings/Recovery/index.tsx | 13 +++---------- 4 files changed, 29 insertions(+), 20 deletions(-) create mode 100644 src/components/common/Chip/index.tsx diff --git a/src/components/common/Chip/index.tsx b/src/components/common/Chip/index.tsx new file mode 100644 index 0000000000..56fb9c3137 --- /dev/null +++ b/src/components/common/Chip/index.tsx @@ -0,0 +1,18 @@ +import { Chip as MuiChip } from '@mui/material' +import type { ChipProps } from '@mui/material' +import type { ReactElement } from 'react' + +import { useDarkMode } from '@/hooks/useDarkMode' + +export function Chip(props: ChipProps): ReactElement { + const isDarkMode = useDarkMode() + return ( + + ) +} diff --git a/src/components/dashboard/Recovery/index.tsx b/src/components/dashboard/Recovery/index.tsx index ec5e5faa8d..0227367f34 100644 --- a/src/components/dashboard/Recovery/index.tsx +++ b/src/components/dashboard/Recovery/index.tsx @@ -1,17 +1,20 @@ -import { Box, Button, Card, Chip, Grid, Typography } from '@mui/material' +import { Box, Button, Card, Grid, Typography } from '@mui/material' +import { useContext } from 'react' import type { ReactElement } from 'react' import RecoveryLogo from '@/public/images/common/recovery.svg' import { WidgetBody, WidgetContainer } from '@/components/dashboard/styled' -import { useDarkMode } from '@/hooks/useDarkMode' +import { Chip } from '@/components/common/Chip' +import { TxModalContext } from '@/components/tx-flow' +import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' import css from './styles.module.css' export function Recovery(): ReactElement { - const isDarkMode = useDarkMode() + const { setTxFlow } = useContext(TxModalContext) const onClick = () => { - // TODO: Open enable recovery flow + setTxFlow() } return ( @@ -31,7 +34,7 @@ export function Recovery(): ReactElement { Introducing account recovery{' '} - + Ensure that you never lose access to your funds by choosing a guardian to recover your account. diff --git a/src/components/dashboard/Recovery/styles.module.css b/src/components/dashboard/Recovery/styles.module.css index df21b43216..352f6e4e6b 100644 --- a/src/components/dashboard/Recovery/styles.module.css +++ b/src/components/dashboard/Recovery/styles.module.css @@ -25,8 +25,3 @@ font-weight: 700; display: inline; } - -.chip { - border-radius: 4px; - font-size: 12px; -} diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx index d5a1704e37..fd355b49b7 100644 --- a/src/components/settings/Recovery/index.tsx +++ b/src/components/settings/Recovery/index.tsx @@ -1,15 +1,14 @@ -import { Alert, Box, Button, Chip, Grid, Paper, Typography } from '@mui/material' +import { Alert, Box, Button, Grid, Paper, Typography } from '@mui/material' import { useContext } from 'react' import type { ReactElement } from 'react' import { EnableRecoveryFlow } from '@/components/tx-flow/flows/EnableRecovery' import { TxModalContext } from '@/components/tx-flow' -import { useDarkMode } from '@/hooks/useDarkMode' +import { Chip } from '@/components/common/Chip' import ExternalLink from '@/components/common/ExternalLink' export function Recovery(): ReactElement { const { setTxFlow } = useContext(TxModalContext) - const isDarkMode = useDarkMode() return ( @@ -20,13 +19,7 @@ export function Recovery(): ReactElement { Account recovery - {/* TODO: Extract when widget is merged https://github.com/safe-global/safe-wallet-web/pull/2768 */} - + From 4a9bc6e3198a523f9551d2967806f9830af228cb Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 11:01:09 +0100 Subject: [PATCH 15/18] fix: spacing --- src/components/settings/Recovery/index.tsx | 26 ++++---- .../RecoverAccountFlowSetup.tsx | 60 ++++++++++--------- 2 files changed, 46 insertions(+), 40 deletions(-) diff --git a/src/components/settings/Recovery/index.tsx b/src/components/settings/Recovery/index.tsx index 6448ca0d38..b4e34399b6 100644 --- a/src/components/settings/Recovery/index.tsx +++ b/src/components/settings/Recovery/index.tsx @@ -29,8 +29,8 @@ export function Recovery(): ReactElement { - - + + Choose a trusted guardian to recover your Safe Account, in case you should ever lose access to your Account. Enabling the Account recovery module will require a transactions. @@ -42,16 +42,18 @@ export function Recovery(): ReactElement { - {recovery ? ( - // TODO: Move to correct location when widget is ready - - ) : ( - - )} + + {recovery ? ( + // TODO: Move to correct location when widget is ready + + ) : ( + + )} + diff --git a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx index 2d67ae8915..a11d7ba3d7 100644 --- a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx +++ b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx @@ -51,14 +51,16 @@ export function RecoverAccountFlowSetup({ - - Add owner(s) - +
+ + Add owner(s) + - - Set the new owner wallet(s) of this Safe Account and how many need to confirm a transaction before it can be - executed. - + + Set the new owner wallet(s) of this Safe Account and how many need to confirm a transaction before it can + be executed. + +
{fields.map((field, index) => ( @@ -105,27 +107,29 @@ export function RecoverAccountFlowSetup({ - - Threshold - - - - - - - - - After recovery, Safe Account transactions will require: - +
+ + Threshold + + + + + + + + + After recovery, Safe Account transactions will require: + +
From 56c79103aafb3888751240550bb2f91d30c27b89 Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 12:31:28 +0100 Subject: [PATCH 16/18] fix: lint + types --- .../tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx | 4 ++-- src/store/recoverySlice.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx index bd727faf11..ef28ee30b3 100644 --- a/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx +++ b/src/components/tx-flow/flows/EnableRecovery/EnableRecoveryFlowIntro.tsx @@ -1,5 +1,5 @@ import { Button, CardActions, Divider, Grid, Typography } from '@mui/material' -import type { ReactElement, ReactNode } from 'react' +import type { ReactElement } from 'react' import TxCard from '../../common/TxCard' import RecoveryGuardians from '@/public/images/settings/spending-limit/beneficiary.svg' @@ -10,7 +10,7 @@ import RecoveryExecution from '@/public/images/transactions/recovery-execution.s import css from './styles.module.css' import commonCss from '@/components/tx-flow/common/styles.module.css' -const RecoverySteps: Array<{ Icon: ReactElement; title: string; subtitle: ReactNode }> = [ +const RecoverySteps = [ { Icon: RecoveryGuardians, title: 'Choose a guardian and set a delay', diff --git a/src/store/recoverySlice.ts b/src/store/recoverySlice.ts index 855bc2da0b..4a30b615ac 100644 --- a/src/store/recoverySlice.ts +++ b/src/store/recoverySlice.ts @@ -28,7 +28,7 @@ const { slice, selector } = makeLoadableSlice('recovery', initialState) export const recoverySlice = slice -const selectRecovery = createSelector(selector, (recovery) => recovery.data) +export const selectRecovery = createSelector(selector, (recovery) => recovery.data) export const selectRecoveryByGuardian = createSelector( [selectRecovery, (_: RootState, walletAddress: string) => walletAddress], From cf66d57f382d61118e4bc033b41b476cceec9677 Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 13:02:06 +0100 Subject: [PATCH 17/18] fix: countdown --- .../RecoveryInProgress/index.test.tsx | 24 +------------------ .../dashboard/RecoveryInProgress/index.tsx | 17 ++----------- .../RecoverAccountFlowReview.tsx | 10 +++++--- src/utils/__tests__/date.test.ts | 23 ++++++++++++++++++ src/utils/date.ts | 14 +++++++++++ 5 files changed, 47 insertions(+), 41 deletions(-) create mode 100644 src/utils/__tests__/date.test.ts diff --git a/src/components/dashboard/RecoveryInProgress/index.test.tsx b/src/components/dashboard/RecoveryInProgress/index.test.tsx index bd48c980bd..dd7c7d2584 100644 --- a/src/components/dashboard/RecoveryInProgress/index.test.tsx +++ b/src/components/dashboard/RecoveryInProgress/index.test.tsx @@ -1,31 +1,9 @@ import { render } from '@testing-library/react' import { BigNumber } from 'ethers' -import { _getCountdown, _RecoveryInProgress } from '.' +import { _RecoveryInProgress } from '.' import type { RecoveryQueueItem, RecoveryState } from '@/store/recoverySlice' -describe('getCountdown', () => { - it('should convert 0 seconds to 0 days, 0 hours, and 0 minutes', () => { - const result = _getCountdown(0) - expect(result).toEqual({ days: 0, hours: 0, minutes: 0 }) - }) - - it('should convert 3600 seconds to 0 days, 1 hour, and 0 minutes', () => { - const result = _getCountdown(3600) - expect(result).toEqual({ days: 0, hours: 1, minutes: 0 }) - }) - - it('should convert 86400 seconds to 1 day, 0 hours, and 0 minutes', () => { - const result = _getCountdown(86400) - expect(result).toEqual({ days: 1, hours: 0, minutes: 0 }) - }) - - it('should convert 123456 seconds to 1 day, 10 hours, and 17 minutes', () => { - const result = _getCountdown(123456) - expect(result).toEqual({ days: 1, hours: 10, minutes: 17 }) - }) -}) - describe('RecoveryInProgress', () => { beforeEach(() => { jest.resetAllMocks() diff --git a/src/components/dashboard/RecoveryInProgress/index.tsx b/src/components/dashboard/RecoveryInProgress/index.tsx index 342503768a..9409a84f1d 100644 --- a/src/components/dashboard/RecoveryInProgress/index.tsx +++ b/src/components/dashboard/RecoveryInProgress/index.tsx @@ -12,6 +12,7 @@ import { FEATURES } from '@/utils/chains' import { selectRecovery } from '@/store/recoverySlice' import type { RecoveryState } from '@/store/recoverySlice' import madProps from '@/utils/mad-props' +import { getCountdown } from '@/utils/date' export function _RecoveryInProgress({ blockTimestamp, @@ -80,26 +81,12 @@ export function _RecoveryInProgress({ ) } -export function _getCountdown(seconds: number): { days: number; hours: number; minutes: number } { - const MINUTE_IN_SECONDS = 60 - const HOUR_IN_SECONDS = 60 * MINUTE_IN_SECONDS - const DAY_IN_SECONDS = 24 * HOUR_IN_SECONDS - - const days = Math.floor(seconds / DAY_IN_SECONDS) - - const remainingSeconds = seconds % DAY_IN_SECONDS - const hours = Math.floor(remainingSeconds / HOUR_IN_SECONDS) - const minutes = Math.floor((remainingSeconds % HOUR_IN_SECONDS) / MINUTE_IN_SECONDS) - - return { days, hours, minutes } -} - function Countdown({ seconds }: { seconds: number }): ReactElement | null { if (seconds <= 0) { return null } - const { days, hours, minutes } = _getCountdown(seconds) + const { days, hours, minutes } = getCountdown(seconds) return ( diff --git a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx index a7689a09eb..e1b7c96976 100644 --- a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx +++ b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowReview.tsx @@ -24,6 +24,7 @@ import useOnboard from '@/hooks/wallets/useOnboard' import { TxModalContext } from '../..' import { asError } from '@/services/exceptions/utils' import { trackError, Errors } from '@/services/exceptions' +import { getCountdown } from '@/utils/date' import type { RecoverAccountFlowProps } from '.' import commonCss from '@/components/tx-flow/common/styles.module.css' @@ -44,6 +45,7 @@ export function RecoverAccountFlowReview({ params }: { params: RecoverAccountFlo // Proposal const txCooldown = recovery?.txCooldown?.toNumber() + const txCooldownCountdown = txCooldown ? getCountdown(txCooldown) : undefined const newThreshold = Number(params[RecoverAccountFlowFields.threshold]) const newOwners = params[RecoverAccountFlowFields.owners] @@ -136,9 +138,11 @@ export function RecoverAccountFlowReview({ params }: { params: RecoverAccountFlo - {/* // TODO: Convert txCooldown to days, minutes, seconds when https://github.com/safe-global/safe-wallet-web/pull/2772 is merged */} - Recovery will be {txCooldown === 0 ? 'immediately possible' : `possible ${txCooldown}`} after this transaction - is executed. + Recovery will be{' '} + {txCooldown === 0 + ? 'immediately possible' + : `possible ${txCooldownCountdown?.days} day${txCooldownCountdown?.days === 1 ? '' : 's'}`}{' '} + after this transaction is executed. diff --git a/src/utils/__tests__/date.test.ts b/src/utils/__tests__/date.test.ts new file mode 100644 index 0000000000..71213cfee9 --- /dev/null +++ b/src/utils/__tests__/date.test.ts @@ -0,0 +1,23 @@ +import { getCountdown } from '../date' + +describe('getCountdown', () => { + it('should convert 0 seconds to 0 days, 0 hours, and 0 minutes', () => { + const result = getCountdown(0) + expect(result).toEqual({ days: 0, hours: 0, minutes: 0 }) + }) + + it('should convert 3600 seconds to 0 days, 1 hour, and 0 minutes', () => { + const result = getCountdown(3600) + expect(result).toEqual({ days: 0, hours: 1, minutes: 0 }) + }) + + it('should convert 86400 seconds to 1 day, 0 hours, and 0 minutes', () => { + const result = getCountdown(86400) + expect(result).toEqual({ days: 1, hours: 0, minutes: 0 }) + }) + + it('should convert 123456 seconds to 1 day, 10 hours, and 17 minutes', () => { + const result = getCountdown(123456) + expect(result).toEqual({ days: 1, hours: 10, minutes: 17 }) + }) +}) diff --git a/src/utils/date.ts b/src/utils/date.ts index d54cffbb85..0c66c1065d 100644 --- a/src/utils/date.ts +++ b/src/utils/date.ts @@ -21,3 +21,17 @@ export const formatTime = (timestamp: number): string => formatWithSchema(timest export const formatDateTime = (timestamp: number): string => formatWithSchema(timestamp, 'MMM d, yyyy - h:mm:ss a') export const formatTimeInWords = (timestamp: number): string => formatDistanceToNow(timestamp, { addSuffix: true }) + +export function getCountdown(seconds: number): { days: number; hours: number; minutes: number } { + const MINUTE_IN_SECONDS = 60 + const HOUR_IN_SECONDS = 60 * MINUTE_IN_SECONDS + const DAY_IN_SECONDS = 24 * HOUR_IN_SECONDS + + const days = Math.floor(seconds / DAY_IN_SECONDS) + + const remainingSeconds = seconds % DAY_IN_SECONDS + const hours = Math.floor(remainingSeconds / HOUR_IN_SECONDS) + const minutes = Math.floor((remainingSeconds % HOUR_IN_SECONDS) / MINUTE_IN_SECONDS) + + return { days, hours, minutes } +} From 365c3da4f970cb55379da7741564f4e748ebb034 Mon Sep 17 00:00:00 2001 From: iamacook Date: Mon, 20 Nov 2023 17:06:27 +0100 Subject: [PATCH 18/18] refactor: code clarity --- src/services/recovery/transaction.ts | 53 ++++++++++++---------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index b93b41b48b..a4dd3f227e 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -25,14 +25,14 @@ export function getRecoveryProposalTransactions({ const safeInterface = new Interface(safeDeployment.abi) // Cache owner changes to determine prevOwner - let _owners = [...safe.owners] + let _owners = safe.owners.map((owner) => owner.value) // Don't add/remove same owners - const ownersToAdd = newOwners.filter( - (newOwner) => !_owners.some((oldOwner) => sameAddress(oldOwner.value, newOwner.value)), - ) + const ownersToAdd = newOwners + .filter((newOwner) => !_owners.some((oldOwner) => sameAddress(oldOwner, newOwner.value))) + .map((owner) => owner.value) const ownersToRemove = _owners.filter( - (oldOwner) => !newOwners.some((newOwner) => sameAddress(newOwner.value, oldOwner.value)), + (oldOwner) => !newOwners.some((newOwner) => sameAddress(newOwner.value, oldOwner)), ) // Check whether threshold should be changed after owner management @@ -41,17 +41,14 @@ export function getRecoveryProposalTransactions({ // Owner management transaction data const txData: Array = [] - // Minimum number of owner management transactions required - const length = Math.max(ownersToAdd.length, ownersToRemove.length) - // Iterate of existing/new owners and swap, add, remove accordingly - for (let index = 0; index < length; index++) { - const ownerToAdd = ownersToAdd[index]?.value - const ownerToRemove = ownersToRemove[index]?.value + for (let index = 0; index < Math.max(ownersToAdd.length, ownersToRemove.length); index++) { + const ownerToAdd = ownersToAdd[index] + const ownerToRemove = ownersToRemove[index] const prevOwner = (() => { - const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) - return ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value + const ownerIndex = _owners.findIndex((owner) => sameAddress(owner, ownerToRemove)) + return ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1] })() // Swap existing owner with new one @@ -60,36 +57,32 @@ export function getRecoveryProposalTransactions({ txData.push(swapOwner) // Swap owner in cache - _owners = _owners.map((owner) => (sameAddress(owner.value, ownerToRemove) ? ownersToAdd[index] : owner)) - - continue + _owners = _owners.map((owner) => (sameAddress(owner, ownerToRemove) ? ownersToAdd[index] : owner)) } - - const threshold = (() => { - const newOwnerLength = ownerToAdd ? _owners.length + 1 : _owners.length - 1 - // Prevent intermediary threshold > number of owners - return index === length - 1 ? newThreshold : newThreshold > newOwnerLength ? newOwnerLength : newThreshold - })() - // Add new owner and set threshold - if (ownerToAdd) { + else if (ownerToAdd) { + const threshold = Math.min(newThreshold, _owners.length + 1) + const addOwnerWithThreshold = safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold]) txData.push(addOwnerWithThreshold) + changeThreshold = false + // Add owner to cache - _owners.push({ value: ownerToAdd }) + _owners.push(ownerToAdd) } // Remove existing owner and set threshold - else { + else if (ownerToRemove) { + const threshold = Math.min(newThreshold, _owners.length - 1) + const removeOwner = safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold]) txData.push(removeOwner) + changeThreshold = false + // Remove owner from cache - _owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove)) + _owners = _owners.filter((owner) => !sameAddress(owner, ownerToRemove)) } - - // addOwnerWithThreshold/removeOwner changed threshold - changeThreshold = false } // Only swapOwner will be called