diff --git a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx index b51ab5f7a8..b9af9b8c66 100644 --- a/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx +++ b/src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx @@ -9,9 +9,10 @@ import { TextField, IconButton, Tooltip, + FormHelperText, } from '@mui/material' import { useForm, FormProvider, useFieldArray, Controller } from 'react-hook-form' -import { Fragment } from 'react' +import { Fragment, useCallback } from 'react' import type { ReactElement } from 'react' import TxCard from '../../common/TxCard' @@ -24,9 +25,34 @@ import InfoIcon from '@/public/images/notifications/info.svg' import useSafeInfo from '@/hooks/useSafeInfo' import { sameAddress } from '@/utils/addresses' import type { RecoverAccountFlowProps } from '.' +import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk' import commonCss from '@/components/tx-flow/common/styles.module.css' +export function _isSameSetup({ + oldOwners, + oldThreshold, + newOwners, + newThreshold, +}: { + oldOwners: Array + oldThreshold: number + newOwners: Array + newThreshold: number +}): boolean { + if (oldThreshold !== newThreshold) { + return false + } + + if (oldOwners.length !== newOwners.length) { + return false + } + + return oldOwners.every((oldOwner) => { + return newOwners.some((newOwner) => sameAddress(oldOwner.value, newOwner.value)) + }) +} + export function RecoverAccountFlowSetup({ params, onSubmit, @@ -34,7 +60,7 @@ export function RecoverAccountFlowSetup({ params: RecoverAccountFlowProps onSubmit: (formData: RecoverAccountFlowProps) => void }): ReactElement { - const { safeAddress } = useSafeInfo() + const { safeAddress, safe } = useSafeInfo() const formMethods = useForm({ defaultValues: params, @@ -46,7 +72,12 @@ export function RecoverAccountFlowSetup({ name: RecoverAccountFlowFields.owners, }) - const owners = formMethods.watch(RecoverAccountFlowFields.owners) + const isSameSetup = useCallback( + (newOwners: Array, newThreshold: number): boolean => { + return _isSameSetup({ oldOwners: safe.owners, oldThreshold: safe.threshold, newOwners, newThreshold }) + }, + [safe.owners, safe.threshold], + ) return ( @@ -78,11 +109,18 @@ export function RecoverAccountFlowSetup({ return 'The Safe Account cannot own itself' } - const isDuplicate = owners.filter((owner) => owner.value === value).length > 1 + const newOwners = formMethods.getValues(RecoverAccountFlowFields.owners) + const isDuplicate = newOwners.filter((owner) => owner.value === value).length > 1 if (isDuplicate) { return 'Already designated to be an owner' } + + const newThreshold = formMethods.getValues(RecoverAccountFlowFields.threshold) + if (isSameSetup(newOwners, Number(newThreshold))) { + return 'Proposed Account setup is the same' + } }} + deps={[RecoverAccountFlowFields.threshold]} /> @@ -132,13 +170,13 @@ export function RecoverAccountFlowSetup({ - - - ( - + ( + + + {fields.map((_, index) => { const value = index + 1 return ( @@ -148,14 +186,29 @@ export function RecoverAccountFlowSetup({ ) })} - )} - /> - + - - out of {fields.length} owner(s) - - + + out of {fields.length} owner(s) + + + {fieldState.error && ( + + {fieldState.error?.message} + + )} + + )} + rules={{ + validate: (newThreshold) => { + const newOwners = formMethods.getValues(RecoverAccountFlowFields.owners) + if (isSameSetup(newOwners, Number(newThreshold))) { + return 'Proposed Account setup is the same' + } + }, + deps: [RecoverAccountFlowFields.owners], + }} + /> diff --git a/src/components/tx-flow/flows/RecoverAccount/__tests__/RecoverAccountFlowSetup.test.ts b/src/components/tx-flow/flows/RecoverAccount/__tests__/RecoverAccountFlowSetup.test.ts new file mode 100644 index 0000000000..9a076f4e58 --- /dev/null +++ b/src/components/tx-flow/flows/RecoverAccount/__tests__/RecoverAccountFlowSetup.test.ts @@ -0,0 +1,78 @@ +import { faker } from '@faker-js/faker' +import { shuffle } from 'lodash' +import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk' + +import { _isSameSetup } from '../RecoverAccountFlowSetup' + +describe('RecoverAccountFlowSetup', () => { + describe('isSameSetup', () => { + it('should return true if the owners and threshold are the same', () => { + const oldOwners: Array = [ + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + ] + const oldThreshold = faker.number.int({ min: 1, max: oldOwners.length }) + + const newOwners = shuffle(oldOwners) + + expect( + _isSameSetup({ + oldOwners, + oldThreshold, + newOwners, + newThreshold: oldThreshold, + }), + ).toBe(true) + }) + + it('should return false if the owners are the same but the threshold is different', () => { + const oldOwners: Array = [ + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + ] + const oldThreshold = 1 + + const newOwners = shuffle(oldOwners) + const newThreshold = 2 + + expect( + _isSameSetup({ + oldOwners, + oldThreshold, + newOwners, + newThreshold, + }), + ).toBe(false) + }) + + it('should return false if the threshold is the same but the owners are different', () => { + const oldOwners: Array = [ + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + ] + const oldThreshold = 2 + + const newOwners = [ + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + { value: faker.finance.ethereumAddress() }, + ] + + expect( + _isSameSetup({ + oldOwners, + oldThreshold, + newOwners, + newThreshold: oldThreshold, + }), + ).toBe(false) + }) + }) +}) diff --git a/src/services/recovery/__tests__/transaction.test.ts b/src/services/recovery/__tests__/transaction.test.ts index a3d5b56ed9..216554a2e6 100644 --- a/src/services/recovery/__tests__/transaction.test.ts +++ b/src/services/recovery/__tests__/transaction.test.ts @@ -1,11 +1,9 @@ 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' +import { getRecoveryProposalTransactions } from '../transaction' describe('transaction', () => { describe('getRecoveryTransactions', () => { @@ -565,148 +563,5 @@ describe('transaction', () => { }) }) }) - - 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', () => { - 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 index ab772059f1..f3cfc602b8 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -1,7 +1,6 @@ import { Interface } from 'ethers/lib/utils' -import { getMultiSendCallOnlyDeployment, getSafeSingletonDeployment } from '@safe-global/safe-deployments' +import { 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 { sameAddress } from '@/utils/addresses' import { getModuleInstance, KnownContracts } from '@gnosis.pm/zodiac' @@ -93,10 +92,6 @@ 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', @@ -105,45 +100,6 @@ export function getRecoveryProposalTransactions({ })) } -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') - } - - const multiSendInterface = new Interface(multiSendDeployment.abi) - const multiSendData = encodeMultiSendData(transactions) - - return { - to: multiSendDeployment.networkAddresses[safe.chainId] ?? multiSendDeployment.defaultAddress, - value: '0', - operation: OperationType.Call, - data: multiSendInterface.encodeFunctionData('multiSend', [multiSendData]), - } -} - export function getRecoverySkipTransaction( recovery: RecoveryQueueItem, provider: JsonRpcProvider,