From f675a5699386c9fe28c92a0e06cc688acd87ac27 Mon Sep 17 00:00:00 2001 From: schmanu Date: Tue, 14 Nov 2023 09:59:22 +0100 Subject: [PATCH 1/3] fix: fix and optimise fetching the recovery state --- src/services/recovery/recovery-state.ts | 43 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/services/recovery/recovery-state.ts b/src/services/recovery/recovery-state.ts index 6de6791e2a..07108ef81b 100644 --- a/src/services/recovery/recovery-state.ts +++ b/src/services/recovery/recovery-state.ts @@ -7,17 +7,10 @@ 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' 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 +41,7 @@ export const _getSafeCreationReceipt = memoize( safeAddress: string provider: JsonRpcProvider }): Promise => { - const url = `${transactionService}/v1/${safeAddress}/creation/` + const url = `${transactionService}api/v1/safes/${safeAddress}/creation/` const { transactionHash } = await fetch(url).then((res) => { if (res.ok && res.status === 200) { @@ -63,6 +56,28 @@ export const _getSafeCreationReceipt = memoize( ({ transactionService, safeAddress }) => transactionService + safeAddress, ) +const queryAddedTransactions = async ( + delayModifier: Delay, + queueNonce: BigNumber, + txNonce: BigNumber, + startBlockNumber: number, +) => { + if (queueNonce.eq(txNonce)) { + // There are no queued txs + return [] + } + + const transactionAddedFilter = delayModifier.filters.TransactionAdded() + + const diff = queueNonce.sub(txNonce).toNumber() + if (transactionAddedFilter.topics) { + const queueNonceFilter = Array.from({ length: diff }, (_, idx) => hexZeroPad(txNonce.add(idx).toHexString(), 32)) + transactionAddedFilter.topics[1] = queueNonceFilter + } + + return await delayModifier.queryFilter(transactionAddedFilter, startBlockNumber, 'latest') +} + export const getRecoveryState = async ({ delayModifier, ...rest @@ -72,21 +87,17 @@ export const getRecoveryState = async ({ safeAddress: string provider: JsonRpcProvider }): Promise => { - const { blockHash } = await _getSafeCreationReceipt(rest) - - const transactionAddedFilter = delayModifier.filters.TransactionAdded() + const { blockNumber } = await _getSafeCreationReceipt(rest) - 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, blockNumber) const queue = await Promise.all( queuedTransactionsAdded.map((transactionAdded) => From 3d5e88f02bfce8aeec279a05597eb6315a56c3fd Mon Sep 17 00:00:00 2001 From: schmanu Date: Tue, 14 Nov 2023 18:05:36 +0100 Subject: [PATCH 2/3] fix: review issues --- .../recovery/__tests__/recovery-state.test.ts | 136 +++++++++++------- src/services/recovery/recovery-state.ts | 31 ++-- 2 files changed, 107 insertions(+), 60 deletions(-) diff --git a/src/services/recovery/__tests__/recovery-state.test.ts b/src/services/recovery/__tests__/recovery-state.test.ts index 13af65ce12..f7d4bccd14 100644 --- a/src/services/recovery/__tests__/recovery-state.test.ts +++ b/src/services/recovery/__tests__/recovery-state.test.ts @@ -1,16 +1,17 @@ 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, + normalizeTxServiceUrl, _getRecoveryQueueItem, _getSafeCreationReceipt, } from '../recovery-state' import { useWeb3ReadOnly } from '@/hooks/wallets/web3' +import { cloneDeep } from 'lodash' jest.mock('@/hooks/wallets/web3') @@ -22,43 +23,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 +155,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 +172,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 +189,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 +222,90 @@ 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() + }) + }) + + describe('normalizeTxServiceUrl', () => { + it('should append slash if missing', () => { + const urlWithoutSlash = faker.internet.url({ appendSlash: false }) + expect(normalizeTxServiceUrl(urlWithoutSlash)).toEqual(urlWithoutSlash + '/') + }) + + it('should not change urls with ending slash ', () => { + const urlWithSlash = faker.internet.url({ appendSlash: true }) + expect(normalizeTxServiceUrl(urlWithSlash)).toEqual(urlWithSlash) }) }) }) diff --git a/src/services/recovery/recovery-state.ts b/src/services/recovery/recovery-state.ts index 07108ef81b..9d77744408 100644 --- a/src/services/recovery/recovery-state.ts +++ b/src/services/recovery/recovery-state.ts @@ -11,6 +11,8 @@ import { hexZeroPad } from 'ethers/lib/utils' const MAX_PAGE_SIZE = 100 +export const normalizeTxServiceUrl = (url: string): string => (url.endsWith('/') ? url : `${url}/`) + export const _getRecoveryQueueItem = async ( transactionAdded: TransactionAddedEvent, txCooldown: BigNumber, @@ -41,7 +43,7 @@ export const _getSafeCreationReceipt = memoize( safeAddress: string provider: JsonRpcProvider }): Promise => { - const url = `${transactionService}api/v1/safes/${safeAddress}/creation/` + const url = `${normalizeTxServiceUrl(transactionService)}api/v1/safes/${safeAddress}/creation/` const { transactionHash } = await fetch(url).then((res) => { if (res.ok && res.status === 200) { @@ -60,7 +62,9 @@ const queryAddedTransactions = async ( delayModifier: Delay, queueNonce: BigNumber, txNonce: BigNumber, - startBlockNumber: number, + transactionService: string, + provider: JsonRpcProvider, + safeAddress: string, ) => { if (queueNonce.eq(txNonce)) { // There are no queued txs @@ -69,26 +73,30 @@ const queryAddedTransactions = async ( const transactionAddedFilter = delayModifier.filters.TransactionAdded() - const diff = queueNonce.sub(txNonce).toNumber() 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 } - return await delayModifier.queryFilter(transactionAddedFilter, startBlockNumber, 'latest') + 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 { blockNumber } = await _getSafeCreationReceipt(rest) - const [[modules], txExpiration, txCooldown, txNonce, queueNonce] = await Promise.all([ delayModifier.getModulesPaginated(SENTINEL_ADDRESS, MAX_PAGE_SIZE), delayModifier.txExpiration(), @@ -97,7 +105,14 @@ export const getRecoveryState = async ({ delayModifier.queueNonce(), ]) - const queuedTransactionsAdded = await queryAddedTransactions(delayModifier, queueNonce, txNonce, blockNumber) + const queuedTransactionsAdded = await queryAddedTransactions( + delayModifier, + queueNonce, + txNonce, + transactionService, + provider, + safeAddress, + ) const queue = await Promise.all( queuedTransactionsAdded.map((transactionAdded) => From e5bda7c6325edd151e27e6c256a6516978a02c78 Mon Sep 17 00:00:00 2001 From: schmanu Date: Tue, 14 Nov 2023 18:24:54 +0100 Subject: [PATCH 3/3] fix: use existing utils function --- .../recovery/__tests__/recovery-state.test.ts | 19 +------------------ src/services/recovery/recovery-state.ts | 5 ++--- 2 files changed, 3 insertions(+), 21 deletions(-) diff --git a/src/services/recovery/__tests__/recovery-state.test.ts b/src/services/recovery/__tests__/recovery-state.test.ts index f7d4bccd14..cd7226146c 100644 --- a/src/services/recovery/__tests__/recovery-state.test.ts +++ b/src/services/recovery/__tests__/recovery-state.test.ts @@ -4,12 +4,7 @@ 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, - normalizeTxServiceUrl, - _getRecoveryQueueItem, - _getSafeCreationReceipt, -} from '../recovery-state' +import { getRecoveryState, _getRecoveryQueueItem, _getSafeCreationReceipt } from '../recovery-state' import { useWeb3ReadOnly } from '@/hooks/wallets/web3' import { cloneDeep } from 'lodash' @@ -296,16 +291,4 @@ describe('recovery-state', () => { expect(queryFilterMock).not.toHaveBeenCalled() }) }) - - describe('normalizeTxServiceUrl', () => { - it('should append slash if missing', () => { - const urlWithoutSlash = faker.internet.url({ appendSlash: false }) - expect(normalizeTxServiceUrl(urlWithoutSlash)).toEqual(urlWithoutSlash + '/') - }) - - it('should not change urls with ending slash ', () => { - const urlWithSlash = faker.internet.url({ appendSlash: true }) - expect(normalizeTxServiceUrl(urlWithSlash)).toEqual(urlWithSlash) - }) - }) }) diff --git a/src/services/recovery/recovery-state.ts b/src/services/recovery/recovery-state.ts index 9d77744408..ac0f343497 100644 --- a/src/services/recovery/recovery-state.ts +++ b/src/services/recovery/recovery-state.ts @@ -8,11 +8,10 @@ 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 normalizeTxServiceUrl = (url: string): string => (url.endsWith('/') ? url : `${url}/`) - export const _getRecoveryQueueItem = async ( transactionAdded: TransactionAddedEvent, txCooldown: BigNumber, @@ -43,7 +42,7 @@ export const _getSafeCreationReceipt = memoize( safeAddress: string provider: JsonRpcProvider }): Promise => { - const url = `${normalizeTxServiceUrl(transactionService)}api/v1/safes/${safeAddress}/creation/` + const url = `${trimTrailingSlash(transactionService)}/api/v1/safes/${safeAddress}/creation/` const { transactionHash } = await fetch(url).then((res) => { if (res.ok && res.status === 200) {