From 3ee9b08925e8ee196f3b970d50185e08d9161360 Mon Sep 17 00:00:00 2001 From: Manuel Gellfart Date: Tue, 14 Nov 2023 18:28:17 +0100 Subject: [PATCH] fix: fix and optimise fetching the recovery state (#2807) * fix: fix and optimise fetching the recovery state * fix: review issues * fix: use existing utils function --- .../recovery/__tests__/recovery-state.test.ts | 129 ++++++++++-------- src/services/recovery/recovery-state.ts | 61 ++++++--- 2 files changed, 115 insertions(+), 75 deletions(-) diff --git a/src/services/recovery/__tests__/recovery-state.test.ts b/src/services/recovery/__tests__/recovery-state.test.ts index 13af65ce12..cd7226146c 100644 --- a/src/services/recovery/__tests__/recovery-state.test.ts +++ b/src/services/recovery/__tests__/recovery-state.test.ts @@ -1,16 +1,12 @@ import { faker } from '@faker-js/faker' -import { BigNumber } from 'ethers' +import { BigNumber, ethers } from 'ethers' import { JsonRpcProvider } from '@ethersproject/providers' import type { Delay, TransactionAddedEvent } from '@gnosis.pm/zodiac/dist/cjs/types/Delay' import type { TransactionReceipt } from '@ethersproject/abstract-provider' -import { - getRecoveryState, - _getQueuedTransactionsAdded, - _getRecoveryQueueItem, - _getSafeCreationReceipt, -} from '../recovery-state' +import { getRecoveryState, _getRecoveryQueueItem, _getSafeCreationReceipt } from '../recovery-state' import { useWeb3ReadOnly } from '@/hooks/wallets/web3' +import { cloneDeep } from 'lodash' jest.mock('@/hooks/wallets/web3') @@ -22,43 +18,6 @@ describe('recovery-state', () => { _getSafeCreationReceipt.cache.clear?.() }) - describe('getQueuedTransactionsAdded', () => { - it('should filter queued transactions with queueNonce >= current txNonce', () => { - const transactionsAdded = [ - { - args: { - queueNonce: BigNumber.from(1), - }, - } as unknown, - { - args: { - queueNonce: BigNumber.from(2), - }, - } as unknown, - { - args: { - queueNonce: BigNumber.from(3), - }, - } as unknown, - ] as Array - - const txNonce = BigNumber.from(2) - - expect(_getQueuedTransactionsAdded(transactionsAdded, txNonce)).toStrictEqual([ - { - args: { - queueNonce: BigNumber.from(2), - }, - } as unknown, - { - args: { - queueNonce: BigNumber.from(3), - }, - }, - ]) - }) - }) - describe('getRecoveryQueueItem', () => { it('should return a recovery queue item', async () => { const transactionAdded = { @@ -191,9 +150,9 @@ describe('recovery-state', () => { const safeAddress = faker.finance.ethereumAddress() const transactionService = faker.internet.url({ appendSlash: false }) const transactionHash = `0x${faker.string.hexadecimal()}` - const blockHash = faker.string.alphanumeric() + const blockNumber = faker.number.int() const provider = { - getTransactionReceipt: () => Promise.resolve({ blockHash } as TransactionReceipt), + getTransactionReceipt: () => Promise.resolve({ blockNumber } as TransactionReceipt), } as unknown as JsonRpcProvider global.fetch = jest.fn().mockImplementation((_url: string) => { @@ -208,14 +167,8 @@ describe('recovery-state', () => { const txExpiration = BigNumber.from(0) const txCooldown = BigNumber.from(69420) const txNonce = BigNumber.from(2) - const queueNonce = BigNumber.from(3) + const queueNonce = BigNumber.from(4) const transactionsAdded = [ - { - getBlock: () => Promise.resolve({ timestamp: 69 }), - args: { - queueNonce: BigNumber.from(1), - }, - } as unknown, { getBlock: () => Promise.resolve({ timestamp: 420 }), args: { @@ -231,9 +184,13 @@ describe('recovery-state', () => { ] as Array const queryFilterMock = jest.fn() + const defaultTransactionAddedFilter = { + address: faker.finance.ethereumAddress(), + topics: [ethers.utils.id('TransactionAdded(uint256,bytes32,address,uint256,bytes,uint8)')], + } const delayModifier = { filters: { - TransactionAdded: () => ({}), + TransactionAdded: () => cloneDeep(defaultTransactionAddedFilter), }, address: faker.finance.ethereumAddress(), getModulesPaginated: () => Promise.resolve([modules]), @@ -260,20 +217,78 @@ describe('recovery-state', () => { queueNonce, queue: [ { - ...transactionsAdded[1], + ...transactionsAdded[0], timestamp: 420, validFrom: BigNumber.from(420).add(txCooldown), expiresAt: null, }, { - ...transactionsAdded[2], + ...transactionsAdded[1], timestamp: 69420, validFrom: BigNumber.from(69420).add(txCooldown), expiresAt: null, }, ], }) - expect(queryFilterMock).toHaveBeenCalledWith(delayModifier.filters.TransactionAdded(), blockHash) + expect(queryFilterMock).toHaveBeenCalledWith( + { + ...defaultTransactionAddedFilter, + topics: [ + ...defaultTransactionAddedFilter.topics, + [ethers.utils.hexZeroPad('0x2', 32), ethers.utils.hexZeroPad('0x3', 32)], + ], + }, + blockNumber, + 'latest', + ) + }) + + it('should not query data if the queueNonce equals the txNonce', async () => { + const safeAddress = faker.finance.ethereumAddress() + const transactionService = faker.internet.url({ appendSlash: true }) + const provider = {} as unknown as JsonRpcProvider + + const modules = [faker.finance.ethereumAddress()] + const txExpiration = BigNumber.from(0) + const txCooldown = BigNumber.from(69420) + const txNonce = BigNumber.from(2) + const queueNonce = BigNumber.from(2) + + const queryFilterMock = jest.fn() + const defaultTransactionAddedFilter = { + address: faker.finance.ethereumAddress(), + topics: [ethers.utils.id('TransactionAdded(uint256,bytes32,address,uint256,bytes,uint8)')], + } + const delayModifier = { + filters: { + TransactionAdded: () => cloneDeep(defaultTransactionAddedFilter), + }, + address: faker.finance.ethereumAddress(), + getModulesPaginated: () => Promise.resolve([modules]), + txExpiration: () => Promise.resolve(txExpiration), + txCooldown: () => Promise.resolve(txCooldown), + txNonce: () => Promise.resolve(txNonce), + queueNonce: () => Promise.resolve(queueNonce), + queryFilter: queryFilterMock.mockRejectedValue('Not required'), + } + + const recoveryState = await getRecoveryState({ + delayModifier: delayModifier as unknown as Delay, + safeAddress, + transactionService, + provider, + }) + + expect(recoveryState).toStrictEqual({ + address: delayModifier.address, + modules, + txExpiration, + txCooldown, + txNonce, + queueNonce, + queue: [], + }) + expect(queryFilterMock).not.toHaveBeenCalled() }) }) }) diff --git a/src/services/recovery/recovery-state.ts b/src/services/recovery/recovery-state.ts index 6de6791e2a..ac0f343497 100644 --- a/src/services/recovery/recovery-state.ts +++ b/src/services/recovery/recovery-state.ts @@ -7,17 +7,11 @@ import type { JsonRpcProvider } from '@ethersproject/providers' import type { TransactionReceipt } from '@ethersproject/abstract-provider' import type { RecoveryQueueItem, RecoveryState } from '@/store/recoverySlice' +import { hexZeroPad } from 'ethers/lib/utils' +import { trimTrailingSlash } from '@/utils/url' const MAX_PAGE_SIZE = 100 -export const _getQueuedTransactionsAdded = ( - transactionsAdded: Array, - txNonce: BigNumber, -): Array => { - // Only queued transactions with queueNonce >= current txNonce - return transactionsAdded.filter(({ args }) => args.queueNonce.gte(txNonce)) -} - export const _getRecoveryQueueItem = async ( transactionAdded: TransactionAddedEvent, txCooldown: BigNumber, @@ -48,7 +42,7 @@ export const _getSafeCreationReceipt = memoize( safeAddress: string provider: JsonRpcProvider }): Promise => { - const url = `${transactionService}/v1/${safeAddress}/creation/` + const url = `${trimTrailingSlash(transactionService)}/api/v1/safes/${safeAddress}/creation/` const { transactionHash } = await fetch(url).then((res) => { if (res.ok && res.status === 200) { @@ -63,30 +57,61 @@ export const _getSafeCreationReceipt = memoize( ({ transactionService, safeAddress }) => transactionService + safeAddress, ) +const queryAddedTransactions = async ( + delayModifier: Delay, + queueNonce: BigNumber, + txNonce: BigNumber, + transactionService: string, + provider: JsonRpcProvider, + safeAddress: string, +) => { + if (queueNonce.eq(txNonce)) { + // There are no queued txs + return [] + } + + const transactionAddedFilter = delayModifier.filters.TransactionAdded() + + if (transactionAddedFilter.topics) { + // We filter for the valid nonces while fetching the event logs. + // The nonce has to be one between the current queueNonce and the txNonce. + const diff = queueNonce.sub(txNonce).toNumber() + const queueNonceFilter = Array.from({ length: diff }, (_, idx) => hexZeroPad(txNonce.add(idx).toHexString(), 32)) + transactionAddedFilter.topics[1] = queueNonceFilter + } + + const { blockNumber } = await _getSafeCreationReceipt({ transactionService, provider, safeAddress }) + + return await delayModifier.queryFilter(transactionAddedFilter, blockNumber, 'latest') +} + export const getRecoveryState = async ({ delayModifier, - ...rest + transactionService, + safeAddress, + provider, }: { delayModifier: Delay transactionService: string safeAddress: string provider: JsonRpcProvider }): Promise => { - const { blockHash } = await _getSafeCreationReceipt(rest) - - const transactionAddedFilter = delayModifier.filters.TransactionAdded() - - const [[modules], txExpiration, txCooldown, txNonce, queueNonce, transactionsAdded] = await Promise.all([ + const [[modules], txExpiration, txCooldown, txNonce, queueNonce] = await Promise.all([ delayModifier.getModulesPaginated(SENTINEL_ADDRESS, MAX_PAGE_SIZE), delayModifier.txExpiration(), delayModifier.txCooldown(), delayModifier.txNonce(), delayModifier.queueNonce(), - // TODO: Improve log retrieval - delayModifier.queryFilter(transactionAddedFilter, blockHash), ]) - const queuedTransactionsAdded = _getQueuedTransactionsAdded(transactionsAdded, txNonce) + const queuedTransactionsAdded = await queryAddedTransactions( + delayModifier, + queueNonce, + txNonce, + transactionService, + provider, + safeAddress, + ) const queue = await Promise.all( queuedTransactionsAdded.map((transactionAdded) =>