From e0aeefd411fa68ed4b19536bba07e1f6398a5dd0 Mon Sep 17 00:00:00 2001 From: leonardotc Date: Wed, 8 May 2024 13:30:21 +0200 Subject: [PATCH] fix(protocol kit): Get modules paginated incorrect interface (#787) --- .../hardhat/deploy/deploy-contracts.ts | 14 ++ packages/protocol-kit/src/Safe.ts | 5 +- .../Safe/v1.0.0/SafeContract_v1_0_0.ts | 20 ++- .../src/managers/moduleManager.ts | 21 ++- .../protocol-kit/src/types/safeProvider.ts | 5 + packages/protocol-kit/src/utils/address.ts | 2 +- .../tests/e2e/moduleManager.test.ts | 142 ++++++++++++++---- .../tests/e2e/utils/setupContracts.ts | 12 ++ 8 files changed, 170 insertions(+), 51 deletions(-) diff --git a/packages/protocol-kit/hardhat/deploy/deploy-contracts.ts b/packages/protocol-kit/hardhat/deploy/deploy-contracts.ts index 7fc9e21d0..28789b6e7 100644 --- a/packages/protocol-kit/hardhat/deploy/deploy-contracts.ts +++ b/packages/protocol-kit/hardhat/deploy/deploy-contracts.ts @@ -188,6 +188,20 @@ const deploy: DeployFunction = async (hre: HardhatRuntimeEnvironment): Promise { + async getModulesPaginated(start: string, pageSize: number = 10): Promise { return this.#moduleManager.getModulesPaginated(start, pageSize) } diff --git a/packages/protocol-kit/src/contracts/Safe/v1.0.0/SafeContract_v1_0_0.ts b/packages/protocol-kit/src/contracts/Safe/v1.0.0/SafeContract_v1_0_0.ts index 8f6ff51ad..c969d0446 100644 --- a/packages/protocol-kit/src/contracts/Safe/v1.0.0/SafeContract_v1_0_0.ts +++ b/packages/protocol-kit/src/contracts/Safe/v1.0.0/SafeContract_v1_0_0.ts @@ -1,7 +1,7 @@ import SafeBaseContract from '@safe-global/protocol-kit/contracts/Safe/SafeBaseContract' import SafeProvider from '@safe-global/protocol-kit/SafeProvider' import { toTxResult } from '@safe-global/protocol-kit/contracts/utils' -import { sameString } from '@safe-global/protocol-kit/utils' +import { sameString, isSentinelAddress } from '@safe-global/protocol-kit/utils' import { SafeVersion, SafeContract_v1_0_0_Abi, @@ -247,15 +247,25 @@ class SafeContract_v1_0_0 return toTxResult(txResponse, options) } - async getModulesPaginated(start: string, pageSize: bigint): Promise { + async getModulesPaginated([start, pageSize]: [string, bigint]): Promise<[string[], string]> { if (pageSize <= 0) throw new Error('Invalid page size for fetching paginated modules') + const size = Number(pageSize) const [array] = await this.getModules() - if (start === SENTINEL_ADDRESS) { - return array.slice(0, Number(pageSize)) + + if (isSentinelAddress(start)) { + const next = pageSize < array.length ? array[size] : SENTINEL_ADDRESS + return [array.slice(0, size), next] } else { const moduleIndex = array.findIndex((module: string) => sameString(module, start)) - return moduleIndex === -1 ? [] : array.slice(moduleIndex + 1, Number(pageSize)) + if (moduleIndex === -1) { + return [[], SENTINEL_ADDRESS] + } + + const nextElementIndex = moduleIndex + 1 + const nextPageAddress = + nextElementIndex + size < array.length ? array[nextElementIndex + size] : SENTINEL_ADDRESS + return [array.slice(moduleIndex + 1, nextElementIndex + size), nextPageAddress] } } diff --git a/packages/protocol-kit/src/managers/moduleManager.ts b/packages/protocol-kit/src/managers/moduleManager.ts index ef036492c..0e91160d7 100644 --- a/packages/protocol-kit/src/managers/moduleManager.ts +++ b/packages/protocol-kit/src/managers/moduleManager.ts @@ -1,6 +1,9 @@ import { isRestrictedAddress, sameString } from '@safe-global/protocol-kit/utils/address' import { SENTINEL_ADDRESS } from '@safe-global/protocol-kit/utils/constants' -import { SafeContractImplementationType } from '@safe-global/protocol-kit/types' +import { + SafeContractImplementationType, + SafeModulesPaginated +} from '@safe-global/protocol-kit/types' import SafeProvider from '../SafeProvider' class ModuleManager { @@ -46,17 +49,13 @@ class ModuleManager { return [...modules] } - //TODO: Implement getModulesPaginated in the new code - async getModulesPaginated(start: string, pageSize: number): Promise { - console.log('getModulesPaginated', start, pageSize) - return [] - // if (!this.#safeContract) { - // throw new Error('Safe is not deployed') - // } - - // const [modules] = await this.#safeContract.getModulesPaginated(start, pageSize) + async getModulesPaginated(start: string, pageSize: number): Promise { + if (!this.#safeContract) { + throw new Error('Safe is not deployed') + } - // return [...modules] + const [modules, next] = await this.#safeContract.getModulesPaginated([start, BigInt(pageSize)]) + return { modules: modules as string[], next } } async isModuleEnabled(moduleAddress: string): Promise { diff --git a/packages/protocol-kit/src/types/safeProvider.ts b/packages/protocol-kit/src/types/safeProvider.ts index 3e99c279c..af47b84bb 100644 --- a/packages/protocol-kit/src/types/safeProvider.ts +++ b/packages/protocol-kit/src/types/safeProvider.ts @@ -29,3 +29,8 @@ export type SafeProviderTransaction = { maxFeePerGas?: number | string maxPriorityFeePerGas?: number | string } + +export type SafeModulesPaginated = { + modules: string[] + next: string +} diff --git a/packages/protocol-kit/src/utils/address.ts b/packages/protocol-kit/src/utils/address.ts index 5906f3c8b..472011460 100644 --- a/packages/protocol-kit/src/utils/address.ts +++ b/packages/protocol-kit/src/utils/address.ts @@ -8,7 +8,7 @@ export function isZeroAddress(address: string): boolean { return sameString(address, ZERO_ADDRESS) } -function isSentinelAddress(address: string): boolean { +export function isSentinelAddress(address: string): boolean { return sameString(address, SENTINEL_ADDRESS) } diff --git a/packages/protocol-kit/tests/e2e/moduleManager.test.ts b/packages/protocol-kit/tests/e2e/moduleManager.test.ts index bb0e53b82..62513be7c 100644 --- a/packages/protocol-kit/tests/e2e/moduleManager.test.ts +++ b/packages/protocol-kit/tests/e2e/moduleManager.test.ts @@ -11,11 +11,14 @@ import { getContractNetworks } from './utils/setupContractNetworks' import { getDailyLimitModule, getSafeWithOwners, - getSocialRecoveryModule + getSocialRecoveryModule, + getStateChannelModule, + getWhiteListModule } from './utils/setupContracts' import { getEip1193Provider } from './utils/setupProvider' import { getAccounts } from './utils/setupTestNetwork' import { waitSafeTxReceipt } from './utils/transactions' +import semverSatisfies from 'semver/functions/satisfies' chai.use(chaiAsPromised) @@ -39,6 +42,8 @@ describe('Safe modules manager', () => { return { dailyLimitModule: await getDailyLimitModule(), socialRecoveryModule: await getSocialRecoveryModule(), + stateChannelModule: await getStateChannelModule(), + whiteListModule: await getWhiteListModule(), safe: await getSafeWithOwners([accounts[0].address]), accounts, contractNetworks, @@ -86,8 +91,7 @@ describe('Safe modules manager', () => { }) }) - //TODO: Fix getModulesPaginated tests - describe.skip('getModulesPaginated', async () => { + describe('getModulesPaginated', async () => { it('should fail if the Safe is not deployed', async () => { const { predictedSafe, contractNetworks, provider } = await setupTests() const safeSdk = await Safe.create({ @@ -108,62 +112,136 @@ describe('Safe modules manager', () => { safeAddress, contractNetworks }) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).length).to.be.eq(0) + + const emptyModuleList = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10) const tx = await safeSdk.createEnableModuleTx(await dailyLimitModule.getAddress()) const txResponse = await safeSdk.executeTransaction(tx) await waitSafeTxReceipt(txResponse) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).length).to.be.eq(1) + const moduleList = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10) + + chai.expect(emptyModuleList.modules.length).to.be.eq(0) + chai.expect(emptyModuleList.next).to.be.eq(SENTINEL_ADDRESS) + chai.expect(moduleList.modules.length).to.be.eq(1) + chai.expect(emptyModuleList.next).to.be.eq(SENTINEL_ADDRESS) }) it('should constraint returned modules by pageSize', async () => { - const { safe, dailyLimitModule, contractNetworks, socialRecoveryModule, provider } = - await setupTests() + const { + safe, + dailyLimitModule, + contractNetworks, + socialRecoveryModule, + stateChannelModule, + whiteListModule, + provider + } = await setupTests() const safeAddress = await safe.getAddress() const dailyLimitsAddress = await dailyLimitModule.getAddress() const socialRecoveryAddress = await socialRecoveryModule.getAddress() + const stateChannelAddress = await stateChannelModule.getAddress() + const whiteListAddress = await whiteListModule.getAddress() const safeSdk = await Safe.create({ provider, safeAddress, contractNetworks }) + const currentPageNext = semverSatisfies(await safeSdk.getContractVersion(), '>=1.4.1') + + chai + .expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).modules.length) + .to.be.eq(0) + + const moduleDeployment = [ + dailyLimitsAddress, + socialRecoveryAddress, + stateChannelAddress, + whiteListAddress + ].map(async (moduleAddress) => { + const txModule = await safeSdk.createEnableModuleTx(moduleAddress) + const moduleResponse = await safeSdk.executeTransaction(txModule) + await waitSafeTxReceipt(moduleResponse) + }) + + await Promise.all(moduleDeployment) + + const modules1 = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10) + const modules2 = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 1) + const modules3 = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 2) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).length).to.be.eq(0) - const txDailyLimits = await safeSdk.createEnableModuleTx(dailyLimitsAddress) - const dailyLimitsResponse = await safeSdk.executeTransaction(txDailyLimits) - await waitSafeTxReceipt(dailyLimitsResponse) - const txSocialRecovery = await safeSdk.createEnableModuleTx(socialRecoveryAddress) - const soecialRecoveryResponse = await safeSdk.executeTransaction(txSocialRecovery) - await waitSafeTxReceipt(soecialRecoveryResponse) - - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).length).to.be.eq(2) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 1)).length).to.be.eq(1) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 1)).length).to.be.eq(1) + chai.expect(modules1.modules.length).to.be.eq(4) + chai + .expect(modules1.modules) + .to.deep.eq([ + whiteListAddress, + stateChannelAddress, + socialRecoveryAddress, + dailyLimitsAddress + ]) + chai.expect(modules1.next).to.be.eq(SENTINEL_ADDRESS) + + chai.expect(modules2.modules.length).to.be.eq(1) + chai.expect(modules2.modules).to.deep.eq([whiteListAddress]) + chai.expect(modules2.next).to.be.eq(currentPageNext ? whiteListAddress : stateChannelAddress) + + chai.expect(modules3.modules.length).to.be.eq(2) + chai.expect(modules3.modules).to.deep.eq([whiteListAddress, stateChannelAddress]) + chai + .expect(modules3.next) + .to.be.eq(currentPageNext ? stateChannelAddress : socialRecoveryAddress) }) it('should offset the returned modules', async () => { - const { safe, dailyLimitModule, contractNetworks, socialRecoveryModule, provider } = - await setupTests() + const { + safe, + dailyLimitModule, + contractNetworks, + socialRecoveryModule, + stateChannelModule, + whiteListModule, + provider + } = await setupTests() const safeAddress = await safe.getAddress() - const dailyLimitsAddress = await await dailyLimitModule.getAddress() - const socialRecoveryAddress = await await socialRecoveryModule.getAddress() + const dailyLimitsAddress = await dailyLimitModule.getAddress() + const socialRecoveryAddress = await socialRecoveryModule.getAddress() + const stateChannelAddress = await stateChannelModule.getAddress() + const whiteListAddress = await whiteListModule.getAddress() const safeSdk = await Safe.create({ provider, safeAddress, contractNetworks }) + const currentPageNext = semverSatisfies(await safeSdk.getContractVersion(), '>=1.4.1') + + const moduleDeployment = [ + dailyLimitsAddress, + socialRecoveryAddress, + stateChannelAddress, + whiteListAddress + ].map(async (moduleAddress) => { + const txModule = await safeSdk.createEnableModuleTx(moduleAddress) + const moduleResponse = await safeSdk.executeTransaction(txModule) + await waitSafeTxReceipt(moduleResponse) + }) - const txDailyLimits = await safeSdk.createEnableModuleTx(dailyLimitsAddress) - const dailyLimitsResponse = await safeSdk.executeTransaction(txDailyLimits) - await waitSafeTxReceipt(dailyLimitsResponse) - const txSocialRecovery = await safeSdk.createEnableModuleTx(socialRecoveryAddress) - const soecialRecoveryResponse = await safeSdk.executeTransaction(txSocialRecovery) - await waitSafeTxReceipt(soecialRecoveryResponse) + await Promise.all(moduleDeployment) - const [firstModule, secondModule] = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10) + const { + modules: [firstModule, secondModule, thirdModule, fourthModule] + } = await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10) - chai.expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).length).to.be.eq(2) - chai.expect((await safeSdk.getModulesPaginated(firstModule, 10)).length).to.be.eq(1) - chai.expect((await safeSdk.getModulesPaginated(secondModule, 10)).length).to.be.eq(0) + const modules1 = await safeSdk.getModulesPaginated(firstModule, 10) + const modules2 = await safeSdk.getModulesPaginated(firstModule, 2) + const modules3 = await safeSdk.getModulesPaginated(firstModule, 3) + + chai + .expect((await safeSdk.getModulesPaginated(SENTINEL_ADDRESS, 10)).modules.length) + .to.be.eq(4) + chai.expect(modules1.modules).to.deep.eq([secondModule, thirdModule, fourthModule]) + chai.expect(modules1.next).to.be.eq(SENTINEL_ADDRESS) + chai.expect(modules2.modules).to.deep.eq([secondModule, thirdModule]) + chai.expect(modules2.next).to.be.eq(currentPageNext ? thirdModule : fourthModule) + chai.expect(modules3.modules).to.deep.eq([secondModule, thirdModule, fourthModule]) + chai.expect(modules3.next).to.be.eq(SENTINEL_ADDRESS) }) it('should fail if pageSize is invalid', async () => { diff --git a/packages/protocol-kit/tests/e2e/utils/setupContracts.ts b/packages/protocol-kit/tests/e2e/utils/setupContracts.ts index 986d657a3..af82e3ba7 100644 --- a/packages/protocol-kit/tests/e2e/utils/setupContracts.ts +++ b/packages/protocol-kit/tests/e2e/utils/setupContracts.ts @@ -175,6 +175,18 @@ export const getSocialRecoveryModule = async (): Promise => { return SocialRecoveryModule.attach(SocialRecoveryModuleDeployment.address) } +export const getStateChannelModule = async (): Promise => { + const StateChannelModuleDeployment = await deployments.get('StateChannelModule') + const StateChannelModule = await ethers.getContractFactory('StateChannelModule') + return StateChannelModule.attach(StateChannelModuleDeployment.address) +} + +export const getWhiteListModule = async (): Promise => { + const WhiteListModuleDeployment = await deployments.get('WhitelistModule') + const WhiteListModule = await ethers.getContractFactory('WhitelistModule') + return WhiteListModule.attach(WhiteListModuleDeployment.address) +} + export const getERC20Mintable = async (): Promise => { const ERC20MintableDeployment = await deployments.get('ERC20Mintable') const ERC20Mintable = await ethers.getContractFactory('ERC20Mintable')