From 35c095a2a7e72ff2540b42c02444fe4efc3e104b Mon Sep 17 00:00:00 2001 From: katspaugh Date: Fri, 26 May 2023 17:14:14 +0200 Subject: [PATCH 1/3] Refactor: reactive recommended nonce Reduce re-renders Nonce -1 --- .../tx/AdvancedParams/AdvancedParamsForm.tsx | 2 +- .../tx/AdvancedParams/useAdvancedParams.ts | 10 +- src/components/tx/GasParams/index.tsx | 2 +- src/components/tx/SignOrExecuteForm/hooks.ts | 38 ++++++- src/components/tx/SignOrExecuteForm/index.tsx | 51 +++++----- src/hooks/useGasLimit.ts | 10 +- src/hooks/useWalletCanRelay.ts | 6 +- .../ts-sender.test.ts} | 37 ++----- src/services/tx/tx-sender/create.ts | 99 ++++--------------- src/services/tx/tx-sender/recommendedNonce.ts | 52 ++++++++++ src/utils/transactions.ts | 5 +- 11 files changed, 157 insertions(+), 155 deletions(-) rename src/services/tx/tx-sender/{index.test.ts => __tests__/ts-sender.test.ts} (94%) create mode 100644 src/services/tx/tx-sender/recommendedNonce.ts diff --git a/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx b/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx index 27de329299..d83e355348 100644 --- a/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx +++ b/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx @@ -93,7 +93,7 @@ const AdvancedParamsForm = ({ params, ...props }: AdvancedParamsFormProps) => { )} {/* Safe nonce */} - {params.nonce !== undefined && ( + {params.nonce !== undefined && params.nonce !== -1 && ( void] => { +export const useAdvancedParams = ( + gasLimit?: AdvancedParameters['gasLimit'], + nonce?: AdvancedParameters['nonce'], + safeTxGas?: AdvancedParameters['safeTxGas'], +): [AdvancedParameters, (params: AdvancedParameters) => void] => { const [manualParams, setManualParams] = useState() const { maxFeePerGas, maxPriorityFeePerGas } = useGasPrice() const userNonce = useUserNonce() diff --git a/src/components/tx/GasParams/index.tsx b/src/components/tx/GasParams/index.tsx index 265bc72449..49579adcb9 100644 --- a/src/components/tx/GasParams/index.tsx +++ b/src/components/tx/GasParams/index.tsx @@ -83,7 +83,7 @@ const GasParams = ({ ) : ( Signing the transaction with nonce  - {nonce !== undefined ? ( + {nonce !== undefined && nonce !== -1 ? ( nonce ) : ( diff --git a/src/components/tx/SignOrExecuteForm/hooks.ts b/src/components/tx/SignOrExecuteForm/hooks.ts index 211c575a79..fc80414dec 100644 --- a/src/components/tx/SignOrExecuteForm/hooks.ts +++ b/src/components/tx/SignOrExecuteForm/hooks.ts @@ -15,7 +15,8 @@ import { useHasPendingTxs } from '@/hooks/usePendingTxs' import { sameString } from '@safe-global/safe-core-sdk/dist/src/utils' import type { ConnectedWallet } from '@/services/onboard' import type { OnboardAPI } from '@web3-onboard/core' -import { hasEnoughSignatures } from '@/utils/transactions' +import { getRecommendedTxParams } from '@/services/tx/tx-sender/recommendedNonce' +import useAsync from '@/hooks/useAsync' type TxActions = { signTx: (safeTx?: SafeTransaction, txId?: string, origin?: string) => Promise @@ -97,7 +98,7 @@ export const useTxActions = (): TxActions => { assertOnboard(onboard) // Relayed transactions must be fully signed, so request a final signature if needed - if (isRelayed && !hasEnoughSignatures(safeTx, safe)) { + if (isRelayed && safeTx.signatures.size < safe.threshold) { safeTx = await signRelayedTx(safeTx) txId = await proposeTx(wallet.address, safeTx, txId, origin) } @@ -138,3 +139,36 @@ export const useIsExecutionLoop = (): boolean => { const { safeAddress } = useSafeInfo() return wallet ? sameString(wallet.address, safeAddress) : false } + +export const useRecommendedNonce = ( + safeTx?: SafeTransaction, +): + | { + nonce: number + safeTxGas: number + } + | undefined => { + const { safe } = useSafeInfo() + const safeTxData = safeTx?.data + + // Memoize only the necessary params, so that it doesn't change every time safeTx is changed + const safeTxParams = useMemo( + () => + safeTxData?.to + ? { + to: safeTxData.to, + value: safeTxData.value, + data: safeTxData.data, + operation: safeTxData.operation, + } + : undefined, + [safeTxData?.to, safeTxData?.value, safeTxData?.data, safeTxData?.operation], + ) + + const [recommendedParams] = useAsync(() => { + if (!safeTxParams) return + return getRecommendedTxParams(safeTxParams) + }, [safeTxParams, safe?.txQueuedTag]) + + return recommendedParams +} diff --git a/src/components/tx/SignOrExecuteForm/index.tsx b/src/components/tx/SignOrExecuteForm/index.tsx index 0e13af5d62..df64ba1d8f 100644 --- a/src/components/tx/SignOrExecuteForm/index.tsx +++ b/src/components/tx/SignOrExecuteForm/index.tsx @@ -4,7 +4,7 @@ import type { SafeTransaction } from '@safe-global/safe-core-sdk-types' import useGasLimit from '@/hooks/useGasLimit' import ErrorMessage from '@/components/tx/ErrorMessage' -import AdvancedParams, { type AdvancedParameters, useAdvancedParams } from '@/components/tx/AdvancedParams' +import AdvancedParams, { useAdvancedParams } from '@/components/tx/AdvancedParams' import DecodedTx from '../DecodedTx' import ExecuteCheckbox from '../ExecuteCheckbox' import { logError, Errors } from '@/services/exceptions' @@ -16,7 +16,13 @@ import useIsValidExecution from '@/hooks/useIsValidExecution' import { createTx } from '@/services/tx/tx-sender' import CheckWallet from '@/components/common/CheckWallet' import { WrongChainWarning } from '../WrongChainWarning' -import { useImmediatelyExecutable, useIsExecutionLoop, useTxActions, useValidateNonce } from './hooks' +import { + useImmediatelyExecutable, + useIsExecutionLoop, + useRecommendedNonce, + useTxActions, + useValidateNonce, +} from './hooks' import UnknownContractError from './UnknownContractError' import { useRelaysBySafe } from '@/hooks/useRemainingRelays' import useWalletCanRelay from '@/hooks/useWalletCanRelay' @@ -55,6 +61,7 @@ const SignOrExecuteForm = ({ const [isSubmittable, setIsSubmittable] = useState(true) const [tx, setTx] = useState(safeTx) const [submitError, setSubmitError] = useState() + const recommendedParams = useRecommendedNonce(tx) // Hooks const isOwner = useIsSafeOwner() @@ -88,11 +95,14 @@ const SignOrExecuteForm = ({ // Estimate gas limit const { gasLimit, gasLimitError, gasLimitLoading } = useGasLimit(willExecute ? tx : undefined) - const [advancedParams, setAdvancedParams] = useAdvancedParams({ - nonce: tx?.data.nonce, + // Advanced params + const [advancedParams, setAdvancedParams] = useAdvancedParams( gasLimit, - safeTxGas: tx?.data.safeTxGas, - }) + // Initial nonce or a recommended one + (safeTx && safeTx.data.nonce !== -1) || !recommendedParams ? safeTx?.data.nonce : recommendedParams?.nonce, + // Initial safeTxGas or a recommended one + safeTx?.data.safeTxGas ?? recommendedParams?.safeTxGas, + ) // Check if transaction will fail const { executionValidationError, isValidExecutionLoading } = useIsValidExecution( @@ -134,21 +144,6 @@ const SignOrExecuteForm = ({ onSubmit() } - // On advanced params submit (nonce, gas limit, price, etc), recreate the transaction - const onAdvancedSubmit = async (data: AdvancedParameters) => { - // If nonce was edited, create a new tx with that nonce - if (tx && (data.nonce !== tx.data.nonce || data.safeTxGas !== tx.data.safeTxGas)) { - try { - setTx(await createTx({ ...tx.data, safeTxGas: data.safeTxGas }, data.nonce)) - } catch (err) { - logError(Errors._103, (err as Error).message) - return - } - } - - setAdvancedParams(data) - } - const cannotPropose = !isOwner && !onlyExecute // Can't sign or create a tx if not an owner const submitDisabled = !isSubmittable || @@ -161,6 +156,16 @@ const SignOrExecuteForm = ({ const error = props.error || (willExecute ? gasLimitError || executionValidationError : undefined) + // Update the tx when the advancedParams change + useEffect(() => { + if (!isCreation || !tx?.data || advancedParams.nonce === undefined) return + if (tx.data.nonce === advancedParams.nonce && tx.data.safeTxGas === advancedParams.safeTxGas) return + + createTx({ ...tx.data, safeTxGas: advancedParams.safeTxGas }, advancedParams.nonce) + .then(setTx) + .catch((err) => logError(Errors._103, (err as Error).message)) + }, [isCreation, tx?.data, advancedParams.nonce, advancedParams.safeTxGas]) + return (
@@ -173,10 +178,10 @@ const SignOrExecuteForm = ({ diff --git a/src/hooks/useGasLimit.ts b/src/hooks/useGasLimit.ts index a5ede16440..0cd0130921 100644 --- a/src/hooks/useGasLimit.ts +++ b/src/hooks/useGasLimit.ts @@ -51,6 +51,7 @@ const useGasLimit = ( const walletAddress = wallet?.address const isOwner = useIsSafeOwner() const currentChainId = useChainId() + const hasSafeTxGas = !!safeTx?.data?.safeTxGas const encodedSafeTx = useMemo(() => { if (!safeTx || !safeSDK || !walletAddress) { @@ -77,17 +78,14 @@ const useGasLimit = ( .then((gasLimit) => { // Due to a bug in Nethermind estimation, we need to increment the gasLimit by 30% // when the safeTxGas is defined and not 0. Currently Nethermind is used only for Gnosis Chain. - if (currentChainId === chains.gno) { + if (currentChainId === chains.gno && hasSafeTxGas) { const incrementPercentage = 30 // value defined in %, ex. 30% - const isSafeTxGasSetAndNotZero = !!safeTx?.data?.safeTxGas - if (isSafeTxGasSetAndNotZero) { - return incrementByPercentage(gasLimit, incrementPercentage) - } + return incrementByPercentage(gasLimit, incrementPercentage) } return gasLimit }) - }, [currentChainId, safeAddress, safeTx, walletAddress, encodedSafeTx, web3ReadOnly, operationType]) + }, [currentChainId, safeAddress, hasSafeTxGas, walletAddress, encodedSafeTx, web3ReadOnly, operationType]) useEffect(() => { if (gasLimitError) { diff --git a/src/hooks/useWalletCanRelay.ts b/src/hooks/useWalletCanRelay.ts index 960ed8f808..605c29b1ae 100644 --- a/src/hooks/useWalletCanRelay.ts +++ b/src/hooks/useWalletCanRelay.ts @@ -3,12 +3,12 @@ import useSafeInfo from '@/hooks/useSafeInfo' import useWallet from '@/hooks/wallets/useWallet' import { isSmartContractWallet } from '@/hooks/wallets/wallets' import { Errors, logError } from '@/services/exceptions' -import { hasEnoughSignatures } from '@/utils/transactions' import { type SafeTransaction } from '@safe-global/safe-core-sdk-types' const useWalletCanRelay = (tx: SafeTransaction | undefined) => { const { safe } = useSafeInfo() const wallet = useWallet() + const hasEnoughSignatures = tx && tx.signatures.size >= safe.threshold return useAsync(() => { if (!tx || !wallet) return @@ -17,13 +17,13 @@ const useWalletCanRelay = (tx: SafeTransaction | undefined) => { .then((isSCWallet) => { if (!isSCWallet) return true - return hasEnoughSignatures(tx, safe) + return hasEnoughSignatures }) .catch((err) => { logError(Errors._106, err.message) return false }) - }, [tx, wallet, safe.threshold]) + }, [hasEnoughSignatures, wallet, safe.threshold]) } export default useWalletCanRelay diff --git a/src/services/tx/tx-sender/index.test.ts b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts similarity index 94% rename from src/services/tx/tx-sender/index.test.ts rename to src/services/tx/tx-sender/__tests__/ts-sender.test.ts index a82de18537..db95505322 100644 --- a/src/services/tx/tx-sender/index.test.ts +++ b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts @@ -2,10 +2,10 @@ import { setSafeSDK } from '@/hooks/coreSDK/safeCoreSDK' import type Safe from '@safe-global/safe-core-sdk' import { type TransactionResult } from '@safe-global/safe-core-sdk-types' import { type TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' -import { getTransactionDetails, postSafeGasEstimation } from '@safe-global/safe-gateway-typescript-sdk' -import extractTxInfo from '../extractTxInfo' -import proposeTx from '../proposeTransaction' -import * as txEvents from '../txEvents' +import { getTransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' +import extractTxInfo from '../../extractTxInfo' +import proposeTx from '../../proposeTransaction' +import * as txEvents from '../../txEvents' import { createExistingTx, createRejectTx, @@ -14,7 +14,7 @@ import { dispatchTxProposal, dispatchTxSigning, dispatchBatchExecutionRelay, -} from '.' +} from '..' import { ErrorCode } from '@ethersproject/logger' import { waitFor } from '@/tests/test-utils' import { ethers } from 'ethers' @@ -41,7 +41,7 @@ jest.mock('@safe-global/safe-gateway-typescript-sdk', () => ({ })) // Mock extractTxInfo -jest.mock('../extractTxInfo', () => ({ +jest.mock('../../extractTxInfo', () => ({ __esModule: true, default: jest.fn(() => ({ txParams: {}, @@ -50,7 +50,7 @@ jest.mock('../extractTxInfo', () => ({ })) // Mock proposeTx -jest.mock('../proposeTransaction', () => ({ +jest.mock('../../proposeTransaction', () => ({ __esModule: true, default: jest.fn(() => Promise.resolve({ txId: '123' })), })) @@ -141,6 +141,7 @@ describe('txSender', () => { to: '0x123', value: '1', data: '0x0', + safeTxGas: 60000, } await createTx(txParams) @@ -148,7 +149,7 @@ describe('txSender', () => { to: '0x123', value: '1', data: '0x0', - nonce: 17, + nonce: -1, safeTxGas: 60000, } expect(mockSafeSDK.createTransaction).toHaveBeenCalledWith({ safeTransactionData }) @@ -171,26 +172,6 @@ describe('txSender', () => { } expect(mockSafeSDK.createTransaction).toHaveBeenCalledWith({ safeTransactionData }) }) - - it('should create a tx with initial txParams if gas estimation fails', async () => { - // override postSafeGasEstimation default implementation - ;(postSafeGasEstimation as jest.Mock) - .mockImplementationOnce(() => Promise.reject(new Error('Failed to retrieve recommended nonce #1'))) - .mockImplementationOnce(() => Promise.reject(new Error('Failed to retrieve recommended nonce #2'))) - - const txParams = { - to: '0x123', - value: '1', - data: '0x0', - } - await createTx(txParams) - - // Shallow copy of txParams - const safeTransactionData = Object.assign({}, txParams) - - // calls SDK createTransaction withouth recommended nonce - expect(mockSafeSDK.createTransaction).toHaveBeenCalledWith({ safeTransactionData }) - }) }) describe('createExistingTx', () => { diff --git a/src/services/tx/tx-sender/create.ts b/src/services/tx/tx-sender/create.ts index 79031b41ed..5afa089a75 100644 --- a/src/services/tx/tx-sender/create.ts +++ b/src/services/tx/tx-sender/create.ts @@ -1,121 +1,56 @@ -import { isLegacyVersion } from '@/hooks/coreSDK/safeCoreSDK' -import { Errors, logError } from '@/services/exceptions' -import type { SafeTransactionEstimation, TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' -import { getTransactionDetails, Operation, postSafeGasEstimation } from '@safe-global/safe-gateway-typescript-sdk' +import type { TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' +import { getTransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' import type { AddOwnerTxParams, RemoveOwnerTxParams, SwapOwnerTxParams } from '@safe-global/safe-core-sdk' import type { MetaTransactionData, SafeTransaction, SafeTransactionDataPartial } from '@safe-global/safe-core-sdk-types' -import { EMPTY_DATA } from '@safe-global/safe-core-sdk/dist/src/utils/constants' import extractTxInfo from '../extractTxInfo' import { getAndValidateSafeSDK } from './sdk' -import type Safe from '@safe-global/safe-core-sdk' - -const estimateSafeTxGas = async ( - chainId: string, - safeAddress: string, - txParams: MetaTransactionData, -): Promise => { - return postSafeGasEstimation(chainId, safeAddress, { - to: txParams.to, - value: txParams.value, - data: txParams.data, - operation: (txParams.operation as unknown as Operation) || Operation.CALL, - }) -} - -const getRecommendedTxParams = async ( - txParams: SafeTransactionDataPartial, -): Promise<{ nonce: number; safeTxGas: number } | undefined> => { - const safeSDK = getAndValidateSafeSDK() - const chainId = await safeSDK.getChainId() - const safeAddress = safeSDK.getAddress() - const contractVersion = await safeSDK.getContractVersion() - const isSafeTxGasRequired = isLegacyVersion(contractVersion) - - let estimation: SafeTransactionEstimation | undefined - - try { - estimation = await estimateSafeTxGas(String(chainId), safeAddress, txParams) - } catch (e) { - try { - // If the initial transaction data causes the estimation to fail, - // we retry the request with a cancellation transaction to get the - // recommendedNonce, even if the original transaction will likely fail - const cancellationTxParams = { ...txParams, data: EMPTY_DATA, to: safeAddress, value: '0' } - estimation = await estimateSafeTxGas(String(chainId), safeAddress, cancellationTxParams) - } catch (e) { - logError(Errors._616, (e as Error).message) - return - } - } - - return { - nonce: estimation.recommendedNonce, - safeTxGas: isSafeTxGasRequired ? Number(estimation.safeTxGas) : 0, - } -} /** * Create a transaction from raw params */ export const createTx = async (txParams: SafeTransactionDataPartial, nonce?: number): Promise => { + txParams = { ...txParams, nonce: nonce ?? -1 } const safeSDK = getAndValidateSafeSDK() - - // If the nonce is not provided, we get the recommended one - if (nonce === undefined) { - const recParams = (await getRecommendedTxParams(txParams)) || {} - txParams = { ...txParams, ...recParams } - } else { - // Otherwise, we use the provided one - txParams = { ...txParams, nonce } - } - return safeSDK.createTransaction({ safeTransactionData: txParams }) } -const withRecommendedNonce = async ( - createFn: (safeSDK: Safe) => Promise, -): Promise => { - const safeSDK = getAndValidateSafeSDK() - const tx = await createFn(safeSDK) - return createTx(tx.data) -} - /** * Create a multiSendCallOnly transaction from an array of MetaTransactionData and options - * * If only one tx is passed it will be created without multiSend and without onlyCalls. */ export const createMultiSendCallOnlyTx = async (txParams: MetaTransactionData[]): Promise => { - return withRecommendedNonce((safeSDK) => - safeSDK.createTransaction({ - safeTransactionData: txParams, - onlyCalls: true, - }), - ) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createTransaction({ safeTransactionData: txParams, onlyCalls: true }) } export const createRemoveOwnerTx = async (txParams: RemoveOwnerTxParams): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createRemoveOwnerTx(txParams)) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createRemoveOwnerTx(txParams) } export const createAddOwnerTx = async (txParams: AddOwnerTxParams): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createAddOwnerTx(txParams)) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createAddOwnerTx(txParams) } export const createSwapOwnerTx = async (txParams: SwapOwnerTxParams): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createSwapOwnerTx(txParams)) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createSwapOwnerTx(txParams) } export const createUpdateThresholdTx = async (threshold: number): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createChangeThresholdTx(threshold)) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createChangeThresholdTx(threshold) } export const createRemoveModuleTx = async (moduleAddress: string): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createDisableModuleTx(moduleAddress)) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createDisableModuleTx(moduleAddress) } export const createRemoveGuardTx = async (): Promise => { - return withRecommendedNonce((safeSDK) => safeSDK.createDisableGuardTx()) + const safeSDK = getAndValidateSafeSDK() + return safeSDK.createDisableGuardTx() } /** diff --git a/src/services/tx/tx-sender/recommendedNonce.ts b/src/services/tx/tx-sender/recommendedNonce.ts new file mode 100644 index 0000000000..eb2ca55b50 --- /dev/null +++ b/src/services/tx/tx-sender/recommendedNonce.ts @@ -0,0 +1,52 @@ +import type { SafeTransactionEstimation } from '@safe-global/safe-gateway-typescript-sdk' +import { Operation, postSafeGasEstimation } from '@safe-global/safe-gateway-typescript-sdk' +import type { MetaTransactionData, SafeTransactionDataPartial } from '@safe-global/safe-core-sdk-types' +import { isLegacyVersion } from '@/hooks/coreSDK/safeCoreSDK' +import { EMPTY_DATA } from '@safe-global/safe-core-sdk/dist/src/utils/constants' +import { Errors, logError } from '@/services/exceptions' +import { getAndValidateSafeSDK } from './sdk' + +const estimateSafeTxGas = async ( + chainId: string, + safeAddress: string, + txParams: MetaTransactionData, +): Promise => { + return postSafeGasEstimation(chainId, safeAddress, { + to: txParams.to, + value: txParams.value, + data: txParams.data, + operation: (txParams.operation as unknown as Operation) || Operation.CALL, + }) +} + +export const getRecommendedTxParams = async ( + safeTxData: SafeTransactionDataPartial, +): Promise<{ nonce: number; safeTxGas: number } | undefined> => { + const safeSDK = getAndValidateSafeSDK() + const chainId = await safeSDK.getChainId() + const safeAddress = safeSDK.getAddress() + const contractVersion = await safeSDK.getContractVersion() + const isSafeTxGasRequired = isLegacyVersion(contractVersion) + + let estimation: SafeTransactionEstimation | undefined + + try { + estimation = await estimateSafeTxGas(String(chainId), safeAddress, safeTxData) + } catch (e) { + try { + // If the initial transaction data causes the estimation to fail, + // we retry the request with a cancellation transaction to get the + // recommendedNonce, even if the original transaction will likely fail + const cancellationTxParams = { ...safeTxData, data: EMPTY_DATA, to: safeAddress, value: '0' } + estimation = await estimateSafeTxGas(String(chainId), safeAddress, cancellationTxParams) + } catch (e) { + logError(Errors._616, (e as Error).message) + return + } + } + + return { + nonce: estimation.recommendedNonce, + safeTxGas: isSafeTxGasRequired ? Number(estimation.safeTxGas) : 0, + } +} diff --git a/src/utils/transactions.ts b/src/utils/transactions.ts index 4477269a99..f15e163de8 100644 --- a/src/utils/transactions.ts +++ b/src/utils/transactions.ts @@ -4,7 +4,6 @@ import type { MultisigExecutionDetails, MultisigExecutionInfo, SafeAppData, - SafeInfo, Transaction, TransactionDetails, TransactionListPage, @@ -22,7 +21,7 @@ import { OperationType } from '@safe-global/safe-core-sdk-types/dist/src/types' import { getReadOnlyGnosisSafeContract } from '@/services/contracts/safeContracts' import extractTxInfo from '@/services/tx/extractTxInfo' import type { AdvancedParameters } from '@/components/tx/AdvancedParams' -import type { TransactionOptions, SafeTransaction } from '@safe-global/safe-core-sdk-types' +import type { TransactionOptions } from '@safe-global/safe-core-sdk-types' import { FEATURES, hasFeature } from '@/utils/chains' import uniqBy from 'lodash/uniqBy' import { Errors, logError } from '@/services/exceptions' @@ -189,5 +188,3 @@ export const getTxOrigin = (app?: SafeAppData): string | undefined => { return origin } - -export const hasEnoughSignatures = (tx: SafeTransaction, safe: SafeInfo) => tx.signatures.size >= safe.threshold From 94cc648e2c57cbc0fd02578c7788701238abd57d Mon Sep 17 00:00:00 2001 From: katspaugh Date: Tue, 30 May 2023 15:17:36 +0200 Subject: [PATCH 2/3] Use "nonceReadyOnly" instead of -1 --- .../tx/AdvancedParams/AdvancedParamsForm.tsx | 2 +- src/components/tx/GasParams/index.tsx | 2 +- src/components/tx/SignOrExecuteForm/index.tsx | 11 ++++++----- src/services/tx/tx-sender/__tests__/ts-sender.test.ts | 1 - src/services/tx/tx-sender/create.ts | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx b/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx index d83e355348..27de329299 100644 --- a/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx +++ b/src/components/tx/AdvancedParams/AdvancedParamsForm.tsx @@ -93,7 +93,7 @@ const AdvancedParamsForm = ({ params, ...props }: AdvancedParamsFormProps) => { )} {/* Safe nonce */} - {params.nonce !== undefined && params.nonce !== -1 && ( + {params.nonce !== undefined && ( Signing the transaction with nonce  - {nonce !== undefined && nonce !== -1 ? ( + {nonce !== undefined ? ( nonce ) : ( diff --git a/src/components/tx/SignOrExecuteForm/index.tsx b/src/components/tx/SignOrExecuteForm/index.tsx index df64ba1d8f..6e76c942c6 100644 --- a/src/components/tx/SignOrExecuteForm/index.tsx +++ b/src/components/tx/SignOrExecuteForm/index.tsx @@ -76,6 +76,9 @@ const SignOrExecuteForm = ({ const isExecutionLoop = useIsExecutionLoop() const canExecute = isCorrectNonce && (isExecutable || isNewExecutableTx) + // Nonce cannot be edited if the tx is already proposed, or signed, or it's a rejection + const nonceReadonly = !isCreation || !!tx?.signatures.size || isRejection + // If checkbox is checked and the transaction is executable, execute it, otherwise sign it const willExecute = (onlyExecute || shouldExecute) && canExecute @@ -99,7 +102,7 @@ const SignOrExecuteForm = ({ const [advancedParams, setAdvancedParams] = useAdvancedParams( gasLimit, // Initial nonce or a recommended one - (safeTx && safeTx.data.nonce !== -1) || !recommendedParams ? safeTx?.data.nonce : recommendedParams?.nonce, + nonceReadonly ? safeTx?.data.nonce : recommendedParams?.nonce, // Initial safeTxGas or a recommended one safeTx?.data.safeTxGas ?? recommendedParams?.safeTxGas, ) @@ -112,8 +115,6 @@ const SignOrExecuteForm = ({ // Estimating gas const isEstimating = willExecute && gasLimitLoading - // Nonce cannot be edited if the tx is already proposed, or signed, or it's a rejection - const nonceReadonly = !isCreation || !!tx?.signatures.size || isRejection // Sign transaction const onSign = async (): Promise => { @@ -158,13 +159,13 @@ const SignOrExecuteForm = ({ // Update the tx when the advancedParams change useEffect(() => { - if (!isCreation || !tx?.data || advancedParams.nonce === undefined) return + if (nonceReadonly || !tx?.data || advancedParams.nonce === undefined) return if (tx.data.nonce === advancedParams.nonce && tx.data.safeTxGas === advancedParams.safeTxGas) return createTx({ ...tx.data, safeTxGas: advancedParams.safeTxGas }, advancedParams.nonce) .then(setTx) .catch((err) => logError(Errors._103, (err as Error).message)) - }, [isCreation, tx?.data, advancedParams.nonce, advancedParams.safeTxGas]) + }, [nonceReadonly, tx?.data, advancedParams.nonce, advancedParams.safeTxGas]) return ( diff --git a/src/services/tx/tx-sender/__tests__/ts-sender.test.ts b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts index db95505322..698a7f5608 100644 --- a/src/services/tx/tx-sender/__tests__/ts-sender.test.ts +++ b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts @@ -149,7 +149,6 @@ describe('txSender', () => { to: '0x123', value: '1', data: '0x0', - nonce: -1, safeTxGas: 60000, } expect(mockSafeSDK.createTransaction).toHaveBeenCalledWith({ safeTransactionData }) diff --git a/src/services/tx/tx-sender/create.ts b/src/services/tx/tx-sender/create.ts index 5afa089a75..bf4073c209 100644 --- a/src/services/tx/tx-sender/create.ts +++ b/src/services/tx/tx-sender/create.ts @@ -9,7 +9,7 @@ import { getAndValidateSafeSDK } from './sdk' * Create a transaction from raw params */ export const createTx = async (txParams: SafeTransactionDataPartial, nonce?: number): Promise => { - txParams = { ...txParams, nonce: nonce ?? -1 } + if (nonce !== undefined) txParams = { ...txParams, nonce } const safeSDK = getAndValidateSafeSDK() return safeSDK.createTransaction({ safeTransactionData: txParams }) } From c4598b24bc4cad1eb02eb0d09c9d4d1020689627 Mon Sep 17 00:00:00 2001 From: katspaugh Date: Tue, 30 May 2023 16:27:04 +0200 Subject: [PATCH 3/3] Split sign and execute into two components --- .../tx/SignOrExecuteForm/ExecuteForm.tsx | 126 ++++++++ .../tx/SignOrExecuteForm/SignForm.tsx | 92 ++++++ src/components/tx/SignOrExecuteForm/index.tsx | 274 ++++-------------- src/hooks/useWalletCanRelay.ts | 2 +- 4 files changed, 282 insertions(+), 212 deletions(-) create mode 100644 src/components/tx/SignOrExecuteForm/ExecuteForm.tsx create mode 100644 src/components/tx/SignOrExecuteForm/SignForm.tsx diff --git a/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx b/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx new file mode 100644 index 0000000000..826416552d --- /dev/null +++ b/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx @@ -0,0 +1,126 @@ +import { type ReactElement, type SyntheticEvent, useState } from 'react' +import { Box, Button } from '@mui/material' + +import ErrorMessage from '@/components/tx/ErrorMessage' +import { logError, Errors } from '@/services/exceptions' +import { useCurrentChain } from '@/hooks/useChains' +import { getTxOptions } from '@/utils/transactions' +import useIsValidExecution from '@/hooks/useIsValidExecution' +import CheckWallet from '@/components/common/CheckWallet' +import { useImmediatelyExecutable, useIsExecutionLoop, useTxActions } from './hooks' +import UnknownContractError from './UnknownContractError' +import { useRelaysBySafe } from '@/hooks/useRemainingRelays' +import useWalletCanRelay from '@/hooks/useWalletCanRelay' +import { ExecutionMethod, ExecutionMethodSelector } from '../ExecutionMethodSelector' +import { hasRemainingRelays } from '@/utils/relaying' +import type { SignOrExecuteProps } from '.' +import type { AdvancedParameters } from '../AdvancedParams' + +const ExecuteForm = ({ + safeTx, + txId, + onSubmit, + children, + isExecutable = false, + isRejection = false, + disableSubmit = false, + origin, + advancedParams, + ...props +}: SignOrExecuteProps & { advancedParams: AdvancedParameters }): ReactElement => { + // + // Hooks & variables + // + const [isSubmittable, setIsSubmittable] = useState(true) + const [submitError, setSubmitError] = useState() + + // Hooks + const currentChain = useCurrentChain() + const { executeTx } = useTxActions() + const [relays] = useRelaysBySafe() + + // Check that the transaction is executable + const isCreation = !txId + const isNewExecutableTx = useImmediatelyExecutable() && isCreation + const isExecutionLoop = useIsExecutionLoop() + + // We default to relay, but the option is only shown if we canRelay + const [executionMethod, setExecutionMethod] = useState(ExecutionMethod.RELAY) + + // SC wallets can relay fully signed transactions + const [walletCanRelay] = useWalletCanRelay(safeTx) + + // The transaction can/will be relayed + const canRelay = walletCanRelay && hasRemainingRelays(relays) + const willRelay = canRelay && executionMethod === ExecutionMethod.RELAY + + // Check if transaction will fail + const { executionValidationError, isValidExecutionLoading } = useIsValidExecution(safeTx, advancedParams.gasLimit) + + // On modal submit + const handleSubmit = async (e: SyntheticEvent) => { + e.preventDefault() + setIsSubmittable(false) + setSubmitError(undefined) + + const txOptions = getTxOptions(advancedParams, currentChain) + + try { + await executeTx(txOptions, safeTx, txId, origin, willRelay) + } catch (err) { + logError(Errors._804, (err as Error).message) + setIsSubmittable(true) + setSubmitError(err as Error) + return + } + + onSubmit() + } + + const submitDisabled = !safeTx || !isSubmittable || disableSubmit || isValidExecutionLoading || isExecutionLoop + + const error = props.error || executionValidationError + + return ( + + {canRelay && ( + + + + )} + + {/* Error messages */} + {isExecutionLoop ? ( + + Cannot execute a transaction from the Safe Account itself, please connect a different account. + + ) : error ? ( + + This transaction will most likely fail.{' '} + {isNewExecutableTx + ? 'To save gas costs, avoid creating the transaction.' + : 'To save gas costs, reject this transaction.'} + + ) : submitError ? ( + Error submitting the transaction. Please try again. + ) : ( + + )} + + {/* Submit button */} + + {(isOk) => ( + + )} + + + ) +} + +export default ExecuteForm diff --git a/src/components/tx/SignOrExecuteForm/SignForm.tsx b/src/components/tx/SignOrExecuteForm/SignForm.tsx new file mode 100644 index 0000000000..68f5f052fc --- /dev/null +++ b/src/components/tx/SignOrExecuteForm/SignForm.tsx @@ -0,0 +1,92 @@ +import { type ReactElement, type SyntheticEvent, useState } from 'react' +import { Button, Typography } from '@mui/material' + +import ErrorMessage from '@/components/tx/ErrorMessage' +import { logError, Errors } from '@/services/exceptions' +import useIsSafeOwner from '@/hooks/useIsSafeOwner' +import CheckWallet from '@/components/common/CheckWallet' +import { useTxActions } from './hooks' +import type { SignOrExecuteProps } from '.' + +const SignForm = ({ + safeTx, + txId, + onSubmit, + children, + isRejection = false, + disableSubmit = false, + origin, + ...props +}: SignOrExecuteProps): ReactElement => { + // + // Hooks & variables + // + const [isSubmittable, setIsSubmittable] = useState(true) + const [submitError, setSubmitError] = useState() + + // Hooks + const isOwner = useIsSafeOwner() + const { signTx } = useTxActions() + + // Check that the transaction is executable + const isCreation = !txId + + // On modal submit + const handleSubmit = async (e: SyntheticEvent) => { + e.preventDefault() + setIsSubmittable(false) + setSubmitError(undefined) + + try { + await signTx(safeTx, txId, origin) + } catch (err) { + logError(Errors._804, (err as Error).message) + setIsSubmittable(true) + setSubmitError(err as Error) + return + } + + onSubmit() + } + + const cannotPropose = !isOwner + const submitDisabled = !safeTx || !isSubmittable || disableSubmit || cannotPropose + + return ( +
+ {/* Error messages */} + {isSubmittable && cannotPropose ? ( + + You are currently not an owner of this Safe Account and won't be able to submit this transaction. + + ) : props.error ? ( + + This transaction will most likely fail.{' '} + {isCreation + ? 'To save gas costs, avoid creating the transaction.' + : 'To save gas costs, reject this transaction.'} + + ) : ( + submitError && ( + Error submitting the transaction. Please try again. + ) + )} + + + You're about to {isCreation ? 'create and ' : ''} + sign a transaction and will need to confirm it with your currently connected wallet. + + + {/* Submit button */} + + {(isOk) => ( + + )} + +
+ ) +} + +export default SignForm diff --git a/src/components/tx/SignOrExecuteForm/index.tsx b/src/components/tx/SignOrExecuteForm/index.tsx index 6e76c942c6..bd356459b7 100644 --- a/src/components/tx/SignOrExecuteForm/index.tsx +++ b/src/components/tx/SignOrExecuteForm/index.tsx @@ -1,35 +1,20 @@ -import { type ReactElement, type ReactNode, type SyntheticEvent, useEffect, useState } from 'react' -import { Box, Button, DialogContent, Typography } from '@mui/material' +import { type ReactElement, type ReactNode, useState, useEffect } from 'react' +import { DialogContent } from '@mui/material' import type { SafeTransaction } from '@safe-global/safe-core-sdk-types' -import useGasLimit from '@/hooks/useGasLimit' -import ErrorMessage from '@/components/tx/ErrorMessage' -import AdvancedParams, { useAdvancedParams } from '@/components/tx/AdvancedParams' import DecodedTx from '../DecodedTx' import ExecuteCheckbox from '../ExecuteCheckbox' -import { logError, Errors } from '@/services/exceptions' -import { useCurrentChain } from '@/hooks/useChains' -import { getTxOptions } from '@/utils/transactions' -import { TxSimulation } from '@/components/tx/TxSimulation' -import useIsSafeOwner from '@/hooks/useIsSafeOwner' -import useIsValidExecution from '@/hooks/useIsValidExecution' -import { createTx } from '@/services/tx/tx-sender' -import CheckWallet from '@/components/common/CheckWallet' import { WrongChainWarning } from '../WrongChainWarning' -import { - useImmediatelyExecutable, - useIsExecutionLoop, - useRecommendedNonce, - useTxActions, - useValidateNonce, -} from './hooks' -import UnknownContractError from './UnknownContractError' -import { useRelaysBySafe } from '@/hooks/useRemainingRelays' -import useWalletCanRelay from '@/hooks/useWalletCanRelay' -import { ExecutionMethod, ExecutionMethodSelector } from '../ExecutionMethodSelector' -import { hasRemainingRelays } from '@/utils/relaying' +import { useImmediatelyExecutable, useRecommendedNonce, useValidateNonce } from './hooks' +import ExecuteForm from './ExecuteForm' +import SignForm from './SignForm' +import AdvancedParams, { useAdvancedParams } from '../AdvancedParams' +import { TxSimulation } from '../TxSimulation' +import { createTx } from '@/services/tx/tx-sender' +import { Errors, logError } from '@/services/exceptions' +import useGasLimit from '@/hooks/useGasLimit' -type SignOrExecuteProps = { +export type SignOrExecuteProps = { safeTx?: SafeTransaction txId?: string onSubmit: () => void @@ -42,124 +27,41 @@ type SignOrExecuteProps = { origin?: string } -const SignOrExecuteForm = ({ - safeTx, - txId, - onSubmit, - children, - onlyExecute = false, - isExecutable = false, - isRejection = false, - disableSubmit = false, - origin, - ...props -}: SignOrExecuteProps): ReactElement => { - // - // Hooks & variables - // +const SignOrExecuteForm = (props: SignOrExecuteProps): ReactElement => { const [shouldExecute, setShouldExecute] = useState(true) - const [isSubmittable, setIsSubmittable] = useState(true) - const [tx, setTx] = useState(safeTx) - const [submitError, setSubmitError] = useState() - const recommendedParams = useRecommendedNonce(tx) - - // Hooks - const isOwner = useIsSafeOwner() - const currentChain = useCurrentChain() - const { signTx, executeTx } = useTxActions() - const [relays] = useRelaysBySafe() - - // Check that the transaction is executable - const isCreation = !txId + const isCreation = !props.txId const isNewExecutableTx = useImmediatelyExecutable() && isCreation + const [tx, setTx] = useState(props.safeTx) const isCorrectNonce = useValidateNonce(tx) - const isExecutionLoop = useIsExecutionLoop() - const canExecute = isCorrectNonce && (isExecutable || isNewExecutableTx) // Nonce cannot be edited if the tx is already proposed, or signed, or it's a rejection - const nonceReadonly = !isCreation || !!tx?.signatures.size || isRejection + const nonceReadonly = !isCreation || !!tx?.signatures.size || !!props.isRejection // If checkbox is checked and the transaction is executable, execute it, otherwise sign it - const willExecute = (onlyExecute || shouldExecute) && canExecute - - // We default to relay, but the option is only shown if we canRelay - const [executionMethod, setExecutionMethod] = useState(ExecutionMethod.RELAY) - - // SC wallets can relay fully signed transactions - const [walletCanRelay] = useWalletCanRelay(tx) - - // The transaction can/will be relayed - const canRelay = hasRemainingRelays(relays) && !!walletCanRelay && willExecute - const willRelay = canRelay && executionMethod === ExecutionMethod.RELAY - - // Synchronize the tx with the safeTx - useEffect(() => setTx(safeTx), [safeTx]) + const canExecute = isCorrectNonce && (props.isExecutable || isNewExecutableTx) + const willExecute = (props.onlyExecute || shouldExecute) && canExecute + // Recommended nonce and safeTxGas + const recommendedParams = useRecommendedNonce(tx) // Estimate gas limit - const { gasLimit, gasLimitError, gasLimitLoading } = useGasLimit(willExecute ? tx : undefined) + const { gasLimit, gasLimitError } = useGasLimit(tx) + + const error = props.error || gasLimitError - // Advanced params const [advancedParams, setAdvancedParams] = useAdvancedParams( gasLimit, // Initial nonce or a recommended one - nonceReadonly ? safeTx?.data.nonce : recommendedParams?.nonce, + nonceReadonly ? props.safeTx?.data.nonce : recommendedParams?.nonce, // Initial safeTxGas or a recommended one - safeTx?.data.safeTxGas ?? recommendedParams?.safeTxGas, - ) - - // Check if transaction will fail - const { executionValidationError, isValidExecutionLoading } = useIsValidExecution( - willExecute ? tx : undefined, - advancedParams.gasLimit, + nonceReadonly ? props.safeTx?.data.safeTxGas : recommendedParams?.safeTxGas, ) - // Estimating gas - const isEstimating = willExecute && gasLimitLoading - - // Sign transaction - const onSign = async (): Promise => { - return await signTx(tx, txId, origin) - } - - // Execute transaction - const onExecute = async (): Promise => { - const txOptions = getTxOptions(advancedParams, currentChain) - return await executeTx(txOptions, tx, txId, origin, willRelay) - } - - // On modal submit - const handleSubmit = async (e: SyntheticEvent) => { - e.preventDefault() - setIsSubmittable(false) - setSubmitError(undefined) - - try { - await (willExecute ? onExecute() : onSign()) - } catch (err) { - logError(Errors._804, (err as Error).message) - setIsSubmittable(true) - setSubmitError(err as Error) - return - } - - onSubmit() - } - - const cannotPropose = !isOwner && !onlyExecute // Can't sign or create a tx if not an owner - const submitDisabled = - !isSubmittable || - isEstimating || - !tx || - disableSubmit || - cannotPropose || - isValidExecutionLoading || - (willExecute && isExecutionLoop) - - const error = props.error || (willExecute ? gasLimitError || executionValidationError : undefined) + // Synchronize the tx with the safeTx + useEffect(() => setTx(props.safeTx), [props.safeTx]) // Update the tx when the advancedParams change useEffect(() => { - if (nonceReadonly || !tx?.data || advancedParams.nonce === undefined) return + if (nonceReadonly || !tx?.data) return if (tx.data.nonce === advancedParams.nonce && tx.data.safeTxGas === advancedParams.safeTxGas) return createTx({ ...tx.data, safeTxGas: advancedParams.safeTxGas }, advancedParams.nonce) @@ -168,92 +70,42 @@ const SignOrExecuteForm = ({ }, [nonceReadonly, tx?.data, advancedParams.nonce, advancedParams.safeTxGas]) return ( -
- - {children} - - - - {canExecute && } - - - - {canRelay && ( - div': { - marginTop: '-1px', - borderTopLeftRadius: 0, - borderTopRightRadius: 0, - }, - }} - > - - - )} - - - - {/* Warning message and switch button */} - - - {/* Error messages */} - {isSubmittable && cannotPropose ? ( - - You are currently not an owner of this Safe Account and won't be able to submit this transaction. - - ) : willExecute && isExecutionLoop ? ( - - Cannot execute a transaction from the Safe Account itself, please connect a different account. - - ) : error ? ( - - This transaction will most likely fail.{' '} - {isNewExecutableTx - ? 'To save gas costs, avoid creating the transaction.' - : 'To save gas costs, reject this transaction.'} - - ) : submitError ? ( - Error submitting the transaction. Please try again. - ) : ( - willExecute && - )} - - {/* Info text */} - - You're about to {txId ? '' : 'create and '} - {willExecute ? 'execute' : 'sign'} a transaction and will need to confirm it with your currently connected - wallet. - - - {/* Submit button */} - - {(isOk) => ( - - )} - - -
+ + {props.children} + + + + + + {canExecute && ( + + )} + + + + {/* Warning message and switch button */} + + + {willExecute ? ( + + ) : ( + + )} + ) } diff --git a/src/hooks/useWalletCanRelay.ts b/src/hooks/useWalletCanRelay.ts index 605c29b1ae..9d8b83d8cc 100644 --- a/src/hooks/useWalletCanRelay.ts +++ b/src/hooks/useWalletCanRelay.ts @@ -23,7 +23,7 @@ const useWalletCanRelay = (tx: SafeTransaction | undefined) => { logError(Errors._106, err.message) return false }) - }, [hasEnoughSignatures, wallet, safe.threshold]) + }, [hasEnoughSignatures, wallet]) } export default useWalletCanRelay