From 774e97f4001ce4151d94dd4e4a646a1a7f35f3a3 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Thu, 6 Jun 2024 12:43:48 +0400 Subject: [PATCH 01/15] feat: sanity checker for network config Implement a sanity checker that checks that the information stored in the DB corresponds the chain for which the app is run. If the app is run for one chain, and the DB contains data for another chain, the sanity checker prevents the app from start and throws the error. --- src/app/app.service.ts | 22 +---- src/jobs/jobs.module.ts | 3 +- src/jobs/jobs.service.ts | 6 ++ src/migrations/Migration20240427195401.ts | 16 ++++ src/mikro-orm.config.ts | 2 + src/network-validation/index.ts | 2 + .../network-validation.constants.ts | 4 + .../network-validation.module.ts | 8 ++ .../network-validation.service.ts | 84 +++++++++++++++++++ src/storage/app-info.entity.ts | 22 +++++ src/storage/app-info.repository.ts | 4 + src/storage/app-info.storage.ts | 28 +++++++ src/storage/storage.module.ts | 8 +- 13 files changed, 184 insertions(+), 25 deletions(-) create mode 100644 src/migrations/Migration20240427195401.ts create mode 100644 src/network-validation/index.ts create mode 100644 src/network-validation/network-validation.constants.ts create mode 100644 src/network-validation/network-validation.module.ts create mode 100644 src/network-validation/network-validation.service.ts create mode 100644 src/storage/app-info.entity.ts create mode 100644 src/storage/app-info.repository.ts create mode 100644 src/storage/app-info.storage.ts diff --git a/src/app/app.service.ts b/src/app/app.service.ts index 04b541f6..77f93e49 100644 --- a/src/app/app.service.ts +++ b/src/app/app.service.ts @@ -1,7 +1,6 @@ import { Inject, Injectable, LoggerService, OnModuleInit } from '@nestjs/common'; import { LOGGER_PROVIDER } from '@lido-nestjs/logger'; import { ExecutionProviderService } from '../common/execution-provider'; -import { ConsensusProviderService } from '../common/consensus-provider'; import { ConfigService } from '../common/config'; import { PrometheusService } from '../common/prometheus'; import { APP_NAME, APP_VERSION } from './app.constants'; @@ -11,35 +10,16 @@ export class AppService implements OnModuleInit { constructor( @Inject(LOGGER_PROVIDER) protected readonly logger: LoggerService, protected readonly executionProviderService: ExecutionProviderService, - protected readonly consensusProviderService: ConsensusProviderService, protected readonly configService: ConfigService, protected readonly prometheusService: PrometheusService, ) {} public async onModuleInit(): Promise { - if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { - await this.validateNetwork(); - } - const env = this.configService.get('NODE_ENV'); const version = APP_VERSION; const name = APP_NAME; const network = await this.executionProviderService.getNetworkName(); this.prometheusService.buildInfo.labels({ env, name, version, network }).inc(); - this.logger.log('Init app', { env, name, version }); - } - - /** - * Validates the CL and EL chains match - */ - protected async validateNetwork(): Promise { - const chainId = this.configService.get('CHAIN_ID'); - const depositContract = await this.consensusProviderService.getDepositContract(); - const elChainId = await this.executionProviderService.getChainId(); - const clChainId = Number(depositContract.data?.chain_id); - - if (chainId !== elChainId || elChainId !== clChainId) { - throw new Error('Execution and consensus chain ids do not match'); - } + this.logger.log('Init app', { env, name, version, network }); } } diff --git a/src/jobs/jobs.module.ts b/src/jobs/jobs.module.ts index 2f78e054..d2ed3c99 100644 --- a/src/jobs/jobs.module.ts +++ b/src/jobs/jobs.module.ts @@ -1,10 +1,11 @@ import { Module } from '@nestjs/common'; import { JobsService } from './jobs.service'; +import { NetworkValidationModule } from '../network-validation'; import { KeysUpdateModule } from './keys-update/keys-update.module'; import { ValidatorsUpdateModule } from './validators-update/validators-update.module'; @Module({ - imports: [KeysUpdateModule, ValidatorsUpdateModule], + imports: [NetworkValidationModule, KeysUpdateModule, ValidatorsUpdateModule], providers: [JobsService], }) export class JobsModule {} diff --git a/src/jobs/jobs.service.ts b/src/jobs/jobs.service.ts index c2c60ad8..e7feb466 100644 --- a/src/jobs/jobs.service.ts +++ b/src/jobs/jobs.service.ts @@ -1,5 +1,6 @@ import { Inject, Injectable, OnModuleDestroy, OnModuleInit } from '@nestjs/common'; import { LOGGER_PROVIDER, LoggerService } from 'common/logger'; +import { NetworkValidationService } from '../network-validation'; import { ValidatorsUpdateService } from './validators-update/validators-update.service'; import { KeysUpdateService } from './keys-update'; import { SchedulerRegistry } from '@nestjs/schedule'; @@ -9,6 +10,7 @@ import { PrometheusService } from 'common/prometheus'; export class JobsService implements OnModuleInit, OnModuleDestroy { constructor( @Inject(LOGGER_PROVIDER) protected readonly logger: LoggerService, + protected readonly networkValidationService: NetworkValidationService, protected readonly keysUpdateService: KeysUpdateService, protected readonly validatorUpdateService: ValidatorsUpdateService, protected readonly schedulerRegistry: SchedulerRegistry, @@ -16,6 +18,10 @@ export class JobsService implements OnModuleInit, OnModuleDestroy { ) {} public async onModuleInit(): Promise { + this.logger.log('Started network config and DB validation'); + await this.networkValidationService.validate(); + this.logger.log('Finished network config and DB validation'); + // Do not wait for initialization to avoid blocking the main process this.initialize().catch((err) => this.logger.error(err)); } diff --git a/src/migrations/Migration20240427195401.ts b/src/migrations/Migration20240427195401.ts new file mode 100644 index 00000000..06d82095 --- /dev/null +++ b/src/migrations/Migration20240427195401.ts @@ -0,0 +1,16 @@ +import { Migration } from '@mikro-orm/migrations'; + +export class Migration20240427195401 extends Migration { + async up(): Promise { + this.addSql( + 'create table "app_info_entity" ("chain_id" serial primary key, "locator_address" varchar(42) not null);', + ); + this.addSql( + 'alter table "app_info_entity" add constraint "app_info_entity_locator_address_unique" unique ("locator_address");', + ); + } + + async down(): Promise { + this.addSql('drop table if exists "app_info_entity" cascade;'); + } +} diff --git a/src/mikro-orm.config.ts b/src/mikro-orm.config.ts index fcd70d5b..87e4151c 100644 --- a/src/mikro-orm.config.ts +++ b/src/mikro-orm.config.ts @@ -11,6 +11,7 @@ import { ConsensusValidatorEntity } from '@lido-nestjs/validators-registry'; import { readFileSync } from 'fs'; import { SrModuleEntity } from './storage/sr-module.entity'; import { ElMetaEntity } from './storage/el-meta.entity'; +import { AppInfoEntity } from './storage/app-info.entity'; import { Logger } from '@nestjs/common'; dotenv.config(); @@ -126,6 +127,7 @@ const config: Options = { ConsensusMetaEntity, SrModuleEntity, ElMetaEntity, + AppInfoEntity, ], migrations: getMigrationOptions(path.join(__dirname, 'migrations'), ['@lido-nestjs/validators-registry']), }; diff --git a/src/network-validation/index.ts b/src/network-validation/index.ts new file mode 100644 index 00000000..ce675d4d --- /dev/null +++ b/src/network-validation/index.ts @@ -0,0 +1,2 @@ +export * from './network-validation.module'; +export * from './network-validation.service'; diff --git a/src/network-validation/network-validation.constants.ts b/src/network-validation/network-validation.constants.ts new file mode 100644 index 00000000..9080942b --- /dev/null +++ b/src/network-validation/network-validation.constants.ts @@ -0,0 +1,4 @@ +export const MODULE_ADDRESSES_FOR_CHAINS = { + 1: '0x55032650b14df07b85bf18a3a3ec8e0af2e028d5', + 17000: '0x595f64ddc3856a3b5ff4f4cc1d1fb4b46cfd2bac', +}; diff --git a/src/network-validation/network-validation.module.ts b/src/network-validation/network-validation.module.ts new file mode 100644 index 00000000..cb5f7a26 --- /dev/null +++ b/src/network-validation/network-validation.module.ts @@ -0,0 +1,8 @@ +import { Module } from '@nestjs/common'; +import { NetworkValidationService } from './network-validation.service'; + +@Module({ + providers: [NetworkValidationService], + exports: [NetworkValidationService], +}) +export class NetworkValidationModule {} diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts new file mode 100644 index 00000000..4ae28090 --- /dev/null +++ b/src/network-validation/network-validation.service.ts @@ -0,0 +1,84 @@ +import { MikroORM, UseRequestContext } from '@mikro-orm/core'; +import { Inject, Injectable, LoggerService } from '@nestjs/common'; +import { LidoLocator, LIDO_LOCATOR_CONTRACT_TOKEN } from '@lido-nestjs/contracts'; +import { LOGGER_PROVIDER } from '@lido-nestjs/logger'; +import { ConfigService } from '../common/config'; +import { ConsensusProviderService } from '../common/consensus-provider'; +import { ExecutionProviderService } from '../common/execution-provider'; +import { RegistryKeyStorageService, RegistryOperatorStorageService } from '../common/registry'; +import { SRModuleStorageService } from '../storage/sr-module.storage'; +import { AppInfoStorageService } from '../storage/app-info.storage'; +import { MODULE_ADDRESSES_FOR_CHAINS } from './network-validation.constants'; + +@Injectable() +export class NetworkValidationService { + constructor( + protected readonly orm: MikroORM, + @Inject(LIDO_LOCATOR_CONTRACT_TOKEN) protected readonly contract: LidoLocator, + @Inject(LOGGER_PROVIDER) protected readonly logger: LoggerService, + protected readonly configService: ConfigService, + protected readonly consensusProviderService: ConsensusProviderService, + protected readonly executionProviderService: ExecutionProviderService, + protected readonly keyStorageService: RegistryKeyStorageService, + protected readonly moduleStorageService: SRModuleStorageService, + protected readonly operatorStorageService: RegistryOperatorStorageService, + protected readonly appInfoStorageService: AppInfoStorageService, + ) {} + + @UseRequestContext() + public async validate(): Promise { + const configChainId = this.configService.get('CHAIN_ID'); + const elChainId = await this.executionProviderService.getChainId(); + + if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { + const depositContract = await this.consensusProviderService.getDepositContract(); + const clChainId = Number(depositContract.data?.chain_id); + + if (configChainId !== elChainId || elChainId !== clChainId) { + throw new Error('Execution and consensus chain ids do not match'); + } + } + + const [dbKey, dbCuratedModule, dbOperator] = await Promise.all([ + await this.keyStorageService.find({}, { limit: 1 }), + await this.moduleStorageService.findOneById(1), + await this.operatorStorageService.find({}, { limit: 1 }), + ]); + + if (dbKey.length === 0 && dbCuratedModule == null && dbOperator.length === 0) { + this.logger.log('DB is empty, write chain info into DB'); + return await this.appInfoStorageService.update({ + chainId: configChainId, + locatorAddress: this.contract.address, + }); + } + + const appInfo = await this.appInfoStorageService.get(); + + if (appInfo != null) { + if (appInfo.chainId !== configChainId || appInfo.locatorAddress !== this.contract.address) { + throw new Error( + `Chain configuration mismatch. Database is not empty and contains information for chain ${appInfo.chainId} and locator address ${appInfo.locatorAddress}, but the service is trying to start for chain ${configChainId} and locator address ${this.contract.address}`, + ); + } + + return; + } + + if (dbCuratedModule == null) { + throw new Error('Inconsistent data in database. Some DB tables are empty, but some are not.'); + } + + if (dbCuratedModule.stakingModuleAddress !== MODULE_ADDRESSES_FOR_CHAINS[configChainId]) { + throw new Error( + `Chain configuration mismatch. Service is trying to start for chain ${configChainId}, but DB contains data for another chain.`, + ); + } + + this.logger.log('DB is not empty and chain info is not found in DB, write chain info into DB'); + await this.appInfoStorageService.update({ + chainId: configChainId, + locatorAddress: this.contract.address, + }); + } +} diff --git a/src/storage/app-info.entity.ts b/src/storage/app-info.entity.ts new file mode 100644 index 00000000..bf33973f --- /dev/null +++ b/src/storage/app-info.entity.ts @@ -0,0 +1,22 @@ +import { Entity, EntityRepositoryType, PrimaryKey, Property, Unique } from '@mikro-orm/core'; +import { ADDRESS_LEN } from '../common/registry/storage/constants'; +import { AppInfoRepository } from './app-info.repository'; + +@Entity({ customRepository: () => AppInfoRepository }) +export class AppInfoEntity { + [EntityRepositoryType]?: AppInfoRepository; + + constructor(info: AppInfoEntity) { + this.chainId = info.chainId; + this.locatorAddress = info.locatorAddress; + } + + @PrimaryKey() + chainId: number; + + @Unique() + @Property({ + length: ADDRESS_LEN, + }) + locatorAddress: string; +} diff --git a/src/storage/app-info.repository.ts b/src/storage/app-info.repository.ts new file mode 100644 index 00000000..9ccc26c5 --- /dev/null +++ b/src/storage/app-info.repository.ts @@ -0,0 +1,4 @@ +import { EntityRepository } from '@mikro-orm/knex'; +import { AppInfoEntity } from './app-info.entity'; + +export class AppInfoRepository extends EntityRepository {} diff --git a/src/storage/app-info.storage.ts b/src/storage/app-info.storage.ts new file mode 100644 index 00000000..e25fb77d --- /dev/null +++ b/src/storage/app-info.storage.ts @@ -0,0 +1,28 @@ +import { Injectable } from '@nestjs/common'; +import { AppInfoEntity } from './app-info.entity'; +import { AppInfoRepository } from './app-info.repository'; + +@Injectable() +export class AppInfoStorageService { + constructor(private readonly repository: AppInfoRepository) {} + + async get(): Promise { + const result = await this.repository.find({}, { limit: 1 }); + return result[0] ?? null; + } + + async update(appInfo: AppInfoEntity): Promise { + await this.repository.nativeDelete({}); + await this.repository.persist( + new AppInfoEntity({ + chainId: appInfo.chainId, + locatorAddress: appInfo.locatorAddress, + }), + ); + await this.repository.flush(); + } + + async removeAll() { + return await this.repository.nativeDelete({}); + } +} diff --git a/src/storage/storage.module.ts b/src/storage/storage.module.ts index e03baa86..13502d1e 100644 --- a/src/storage/storage.module.ts +++ b/src/storage/storage.module.ts @@ -4,15 +4,17 @@ import { ElMetaEntity } from './el-meta.entity'; import { ElMetaStorageService } from './el-meta.storage'; import { SrModuleEntity } from './sr-module.entity'; import { SRModuleStorageService } from './sr-module.storage'; +import { AppInfoEntity } from './app-info.entity'; +import { AppInfoStorageService } from './app-info.storage'; @Global() @Module({ imports: [ MikroOrmModule.forFeature({ - entities: [SrModuleEntity, ElMetaEntity], + entities: [SrModuleEntity, ElMetaEntity, AppInfoEntity], }), ], - providers: [SRModuleStorageService, ElMetaStorageService], - exports: [SRModuleStorageService, ElMetaStorageService], + providers: [SRModuleStorageService, ElMetaStorageService, AppInfoStorageService], + exports: [SRModuleStorageService, ElMetaStorageService, AppInfoStorageService], }) export class StorageModule {} From 69af2721f13fd4ccbddc310ee7aa2d32edce9791 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Sat, 8 Jun 2024 00:25:01 +0400 Subject: [PATCH 02/15] refactor: rename `MODULE_ADDRESSES_FOR_CHAINS` const --- src/network-validation/network-validation.constants.ts | 2 +- src/network-validation/network-validation.service.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network-validation/network-validation.constants.ts b/src/network-validation/network-validation.constants.ts index 9080942b..f46ea96b 100644 --- a/src/network-validation/network-validation.constants.ts +++ b/src/network-validation/network-validation.constants.ts @@ -1,4 +1,4 @@ -export const MODULE_ADDRESSES_FOR_CHAINS = { +export const CURATED_MODULE_ADDRESSES_FOR_CHAINS = { 1: '0x55032650b14df07b85bf18a3a3ec8e0af2e028d5', 17000: '0x595f64ddc3856a3b5ff4f4cc1d1fb4b46cfd2bac', }; diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 4ae28090..1d784539 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -8,7 +8,7 @@ import { ExecutionProviderService } from '../common/execution-provider'; import { RegistryKeyStorageService, RegistryOperatorStorageService } from '../common/registry'; import { SRModuleStorageService } from '../storage/sr-module.storage'; import { AppInfoStorageService } from '../storage/app-info.storage'; -import { MODULE_ADDRESSES_FOR_CHAINS } from './network-validation.constants'; +import { CURATED_MODULE_ADDRESSES_FOR_CHAINS } from './network-validation.constants'; @Injectable() export class NetworkValidationService { @@ -69,7 +69,7 @@ export class NetworkValidationService { throw new Error('Inconsistent data in database. Some DB tables are empty, but some are not.'); } - if (dbCuratedModule.stakingModuleAddress !== MODULE_ADDRESSES_FOR_CHAINS[configChainId]) { + if (dbCuratedModule.stakingModuleAddress !== CURATED_MODULE_ADDRESSES_FOR_CHAINS[configChainId]) { throw new Error( `Chain configuration mismatch. Service is trying to start for chain ${configChainId}, but DB contains data for another chain.`, ); From f6051f0dc402475a71cfc6a1f94dfe4cf4eb060f Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Sat, 8 Jun 2024 00:48:30 +0400 Subject: [PATCH 03/15] refactor: rename `findOneById` method Rename the `findOneById` method of the `SRModuleStorageService` to `findOneByModuleId`. --- .../keys-update/staking-module-updater.service.ts | 2 +- src/jobs/keys-update/test/detect-reorg.spec.ts | 2 +- src/jobs/keys-update/test/update-cases.spec.ts | 12 ++++++------ src/network-validation/network-validation.service.ts | 2 +- src/staking-router-modules/staking-router.service.ts | 2 +- src/storage/sr-module.storage.ts | 3 +-- 6 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/jobs/keys-update/staking-module-updater.service.ts b/src/jobs/keys-update/staking-module-updater.service.ts index a1232b02..83260947 100644 --- a/src/jobs/keys-update/staking-module-updater.service.ts +++ b/src/jobs/keys-update/staking-module-updater.service.ts @@ -39,7 +39,7 @@ export class StakingModuleUpdaterService { // Read current nonce from contract const currNonce = await moduleInstance.getCurrentNonce(stakingModuleAddress, currentBlockHash); // Read module in storage - const moduleInStorage = await this.srModulesStorage.findOneById(contractModule.moduleId); + const moduleInStorage = await this.srModulesStorage.findOneByModuleId(contractModule.moduleId); const prevNonce = moduleInStorage?.nonce; this.logger.log(`Nonce previous value: ${prevNonce}, nonce current value: ${currNonce}`); diff --git a/src/jobs/keys-update/test/detect-reorg.spec.ts b/src/jobs/keys-update/test/detect-reorg.spec.ts index 1260713c..c91572c6 100644 --- a/src/jobs/keys-update/test/detect-reorg.spec.ts +++ b/src/jobs/keys-update/test/detect-reorg.spec.ts @@ -45,7 +45,7 @@ describe('detect reorg', () => { { provide: SRModuleStorageService, useValue: { - findOneById: jest.fn(), + findOneByModuleId: jest.fn(), upsert: jest.fn(), }, }, diff --git a/src/jobs/keys-update/test/update-cases.spec.ts b/src/jobs/keys-update/test/update-cases.spec.ts index 81de727f..245cac29 100644 --- a/src/jobs/keys-update/test/update-cases.spec.ts +++ b/src/jobs/keys-update/test/update-cases.spec.ts @@ -11,7 +11,7 @@ import { UpdaterState } from '../keys-update.interfaces'; describe('update cases', () => { let updaterService: StakingModuleUpdaterService; let stakingRouterService: StakingRouterService; - let sRModuleStorageService: SRModuleStorageService; + let srModuleStorageService: SRModuleStorageService; let elMetaStorageService: ElMetaStorageService; let loggerService: { log: jest.Mock }; let executionProviderService: ExecutionProviderService; @@ -50,7 +50,7 @@ describe('update cases', () => { { provide: SRModuleStorageService, useValue: { - findOneById: jest.fn(), + findOneByModuleId: jest.fn(), upsert: jest.fn(), }, }, @@ -60,7 +60,7 @@ describe('update cases', () => { updaterService = module.get(StakingModuleUpdaterService); stakingRouterService = module.get(StakingRouterService); - sRModuleStorageService = module.get(SRModuleStorageService); + srModuleStorageService = module.get(SRModuleStorageService); executionProviderService = module.get(ExecutionProviderService); elMetaStorageService = module.get(ElMetaStorageService); loggerService = module.get(LOGGER_PROVIDER); @@ -126,7 +126,7 @@ describe('update cases', () => { } as any), ); - jest.spyOn(sRModuleStorageService, 'findOneById').mockImplementation( + jest.spyOn(srModuleStorageService, 'findOneByModuleId').mockImplementation( () => ({ nonce: 0, @@ -153,7 +153,7 @@ describe('update cases', () => { updaterState.lastChangedBlockHash = currBh; }); const mockElUpdate = jest.spyOn(elMetaStorageService, 'update').mockImplementation(); - jest.spyOn(sRModuleStorageService, 'findOneById').mockImplementation( + jest.spyOn(srModuleStorageService, 'findOneByModuleId').mockImplementation( () => ({ nonce: 1, @@ -179,7 +179,7 @@ describe('update cases', () => { updaterState.lastChangedBlockHash = currBh; }); const mockElUpdate = jest.spyOn(elMetaStorageService, 'update').mockImplementation(); - jest.spyOn(sRModuleStorageService, 'findOneById').mockImplementation( + jest.spyOn(srModuleStorageService, 'findOneByModuleId').mockImplementation( () => ({ nonce: 1, diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 1d784539..d4b7097f 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -41,7 +41,7 @@ export class NetworkValidationService { const [dbKey, dbCuratedModule, dbOperator] = await Promise.all([ await this.keyStorageService.find({}, { limit: 1 }), - await this.moduleStorageService.findOneById(1), + await this.moduleStorageService.findOneByModuleId(1), await this.operatorStorageService.find({}, { limit: 1 }), ]); diff --git a/src/staking-router-modules/staking-router.service.ts b/src/staking-router-modules/staking-router.service.ts index 02eb93f0..6a8066b9 100644 --- a/src/staking-router-modules/staking-router.service.ts +++ b/src/staking-router-modules/staking-router.service.ts @@ -43,7 +43,7 @@ export class StakingRouterService { return await this.srModulesStorage.findOneByContractAddress(moduleId); } - return await this.srModulesStorage.findOneById(moduleId); + return await this.srModulesStorage.findOneByModuleId(moduleId); } public getStakingRouterModuleImpl(moduleType: string): StakingModuleInterface { diff --git a/src/storage/sr-module.storage.ts b/src/storage/sr-module.storage.ts index 85bc1a87..f9602b81 100644 --- a/src/storage/sr-module.storage.ts +++ b/src/storage/sr-module.storage.ts @@ -7,8 +7,7 @@ import { SRModuleRepository } from './sr-module.repository'; export class SRModuleStorageService { constructor(private readonly repository: SRModuleRepository) {} - /** find key by index */ - async findOneById(moduleId: number): Promise { + async findOneByModuleId(moduleId: number): Promise { return await this.repository.findOne({ moduleId }); } From 23c807bd941dc4ca62224ce8ed278a5df04011a6 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 11 Jun 2024 02:23:27 +0400 Subject: [PATCH 04/15] chore: unit tests for sanity checker service --- .../network-validation.service.spec.ts | 480 ++++++++++++++++++ 1 file changed, 480 insertions(+) create mode 100644 src/network-validation/network-validation.service.spec.ts diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts new file mode 100644 index 00000000..293e6cee --- /dev/null +++ b/src/network-validation/network-validation.service.spec.ts @@ -0,0 +1,480 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { LOGGER_PROVIDER } from '@lido-nestjs/logger'; +import { REGISTRY_CONTRACT_ADDRESSES, LIDO_LOCATOR_CONTRACT_TOKEN } from '@lido-nestjs/contracts'; +import { ConfigModule, ConfigService } from 'common/config'; +import { ConsensusProviderService } from '../common/consensus-provider'; +import { ExecutionProviderService } from '../common/execution-provider'; +import { RegistryKeyStorageService, RegistryOperatorStorageService } from '../common/registry'; +import { SRModuleStorageService } from '../storage/sr-module.storage'; +import { AppInfoStorageService } from '../storage/app-info.storage'; +import { NetworkValidationService } from './network-validation.service'; +import { key } from 'common/registry/test/fixtures/key.fixture'; +import { curatedModule } from '../http/db.fixtures'; +import { operator } from 'common/registry/test/fixtures/operator.fixture'; +import { DatabaseE2ETestingModule } from '../app'; + +describe('network configuration correctness sanity checker', () => { + let configService: ConfigService; + let locator: { address: string }; + let consensusProviderService: ConsensusProviderService; + let executionProviderService: ExecutionProviderService; + let registryKeyStorageService: RegistryKeyStorageService; + let moduleStorageService: SRModuleStorageService; + let operatorStorageService: RegistryOperatorStorageService; + let networkValidationService: NetworkValidationService; + let appInfoStorageService: AppInfoStorageService; + + const keyFixture = { + ...key, + index: 1, + operatorIndex: 0, + moduleAddress: REGISTRY_CONTRACT_ADDRESSES[17000].toLowerCase(), + }; + + const holeskyCuratedModuleFixture = { + ...curatedModule, + id: 1, + stakingModuleAddress: REGISTRY_CONTRACT_ADDRESSES[17000].toLowerCase(), + nonce: 14100, + lastChangedBlockHash: '0x662e3e713207240b25d01324b6eccdc91493249a5048881544254994694530a5', + }; + + const operatorFixture = { + ...operator, + index: 0, + moduleAddress: REGISTRY_CONTRACT_ADDRESSES[17000].toLowerCase(), + }; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + imports: [ + ConfigModule, + DatabaseE2ETestingModule.forRoot(), + ], + providers: [ + { + provide: LIDO_LOCATOR_CONTRACT_TOKEN, + useValue: { + address: '0x17000', + } + }, + { + provide: LOGGER_PROVIDER, + useValue: { + log: jest.fn(), + }, + }, + { + provide: ConsensusProviderService, + useValue: { + getDepositContract: jest.fn(async () => ({ + data: { + chain_id: '1' + } + })), + } + }, + { + provide: ExecutionProviderService, + useValue: { + getChainId: jest.fn(async () => 17000), + } + }, + { + provide: RegistryKeyStorageService, + useValue: { + find: jest.fn(async () => []), + } + }, + { + provide: SRModuleStorageService, + useValue: { + findOneByModuleId: jest.fn(async () => null), + } + }, + { + provide: RegistryOperatorStorageService, + useValue: { + find: jest.fn(async () => []), + } + }, + { + provide: AppInfoStorageService, + useValue: { + update: jest.fn(), + get: jest.fn(async () => ({ + chainId: 17000, + locatorAddress: '0x17000', + })), + } + }, + NetworkValidationService, + ], + }).compile(); + + configService = module.get(ConfigService); + locator = module.get(LIDO_LOCATOR_CONTRACT_TOKEN); + consensusProviderService = module.get(ConsensusProviderService); + executionProviderService = module.get(ExecutionProviderService); + registryKeyStorageService = module.get(RegistryKeyStorageService); + moduleStorageService = module.get(SRModuleStorageService); + operatorStorageService = module.get(RegistryOperatorStorageService); + appInfoStorageService = module.get(AppInfoStorageService); + networkValidationService = module.get(NetworkValidationService); + + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } + + if (path === 'CHAIN_ID') { + return 17000; + } + + return undefined; + }); + }); + + it('should be defined', () => { + expect(networkValidationService).toBeDefined(); + }); + + it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by EL node if the validator registry is enabled in config", async () => { + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } + + if (path === 'CHAIN_ID') { + return 1; + } + + return undefined; + }); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if the chain ID returned by EL node doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } + + if (path === 'CHAIN_ID') { + return 17000; + } + + return undefined; + }); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } + + if (path === 'CHAIN_ID') { + return 17000; + } + + return undefined; + }); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should save information about the chain and locator to the DB if there are no keys, modules, and operators in the DB", async () => { + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).resolves.not.toThrow(); + expect(updateAppInfoMock).toBeCalledTimes(1); + expect(updateAppInfoMock).toBeCalledWith({ + chainId: 17000, + locatorAddress: '0x17000', + }); + }); + + it("should throw error if info about chain ID stored in the DB doesn't match chain ID configured in env variables and keys, modules or operators tables have some info in the DB", async () => { + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } + + if (path === 'CHAIN_ID') { + return 1; + } + + return undefined; + }); + + jest + .spyOn(executionProviderService, 'getChainId') + .mockImplementationOnce(() => Promise.resolve(1)); + + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if info about locator address stored in the DB doesn't match locator address returned by locator service and keys, modules or operators tables have some info in the DB", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + locator.address = '0x1'; + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should execute successfully if info about chain ID matches the chain ID configured in env variables and locator address stored in the DB matches the locator address returned by locator service, and keys, modules, or operators tables have some info in the DB", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).resolves.not.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if modules table is not empty in the DB, but the keys table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if operators table is not empty in the DB, but the keys table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if keys table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if operators table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if keys table is not empty in the DB, but the operators table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if modules table is not empty in the DB, but the operators table is empty, and DB doesn't have information about chain ID and locator", async () => { + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + const mainnetCuratedModuleFixture = { + ...holeskyCuratedModuleFixture, + stakingModuleAddress: REGISTRY_CONTRACT_ADDRESSES[1].toLowerCase(), + }; + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(mainnetCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { + jest + .spyOn(configService, 'get') + .mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } + + if (path === 'CHAIN_ID') { + return 1; + } + + return configService.get(path); + }); + + jest + .spyOn(executionProviderService, 'getChainId') + .mockImplementationOnce(() => Promise.resolve(1)); + + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).rejects.toThrow(); + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); + + it("should execute successfully if DB has information about keys, modules, and operators, doesn't have information about chain and locator, and address of the curated module stored in the DB matches the address of the curated module for the chain for which the app was started", async () => { + jest + .spyOn(registryKeyStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([keyFixture])); + + jest + .spyOn(moduleStorageService, 'findOneByModuleId') + .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); + + jest + .spyOn(operatorStorageService, 'find') + .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + + jest + .spyOn(appInfoStorageService, 'get') + .mockImplementationOnce(() => Promise.resolve(null)); + + const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); + + await expect(networkValidationService.validate()).resolves.not.toThrow(); + expect(updateAppInfoMock).toBeCalledTimes(1); + expect(updateAppInfoMock).toBeCalledWith({ + chainId: 17000, + locatorAddress: '0x17000', + }); + }); +}) From ef7d2d307ccbab20b54fb668ee25cd72d60d5add Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 11 Jun 2024 11:43:51 +0400 Subject: [PATCH 05/15] refactor: replace constants with module addresses Replace the internal `CURATED_MODULE_ADDRESSES_FOR_CHAINS` constant with curated module addresses with the appropriate `REGISTRY_CONTRACT_ADDRESSES` constant from `@lido-nestjs` repo. --- src/network-validation/network-validation.constants.ts | 4 ---- src/network-validation/network-validation.service.ts | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) delete mode 100644 src/network-validation/network-validation.constants.ts diff --git a/src/network-validation/network-validation.constants.ts b/src/network-validation/network-validation.constants.ts deleted file mode 100644 index f46ea96b..00000000 --- a/src/network-validation/network-validation.constants.ts +++ /dev/null @@ -1,4 +0,0 @@ -export const CURATED_MODULE_ADDRESSES_FOR_CHAINS = { - 1: '0x55032650b14df07b85bf18a3a3ec8e0af2e028d5', - 17000: '0x595f64ddc3856a3b5ff4f4cc1d1fb4b46cfd2bac', -}; diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index d4b7097f..69383bd1 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -2,13 +2,13 @@ import { MikroORM, UseRequestContext } from '@mikro-orm/core'; import { Inject, Injectable, LoggerService } from '@nestjs/common'; import { LidoLocator, LIDO_LOCATOR_CONTRACT_TOKEN } from '@lido-nestjs/contracts'; import { LOGGER_PROVIDER } from '@lido-nestjs/logger'; +import { REGISTRY_CONTRACT_ADDRESSES } from '@lido-nestjs/contracts'; import { ConfigService } from '../common/config'; import { ConsensusProviderService } from '../common/consensus-provider'; import { ExecutionProviderService } from '../common/execution-provider'; import { RegistryKeyStorageService, RegistryOperatorStorageService } from '../common/registry'; import { SRModuleStorageService } from '../storage/sr-module.storage'; import { AppInfoStorageService } from '../storage/app-info.storage'; -import { CURATED_MODULE_ADDRESSES_FOR_CHAINS } from './network-validation.constants'; @Injectable() export class NetworkValidationService { @@ -69,7 +69,7 @@ export class NetworkValidationService { throw new Error('Inconsistent data in database. Some DB tables are empty, but some are not.'); } - if (dbCuratedModule.stakingModuleAddress !== CURATED_MODULE_ADDRESSES_FOR_CHAINS[configChainId]) { + if (dbCuratedModule.stakingModuleAddress !== REGISTRY_CONTRACT_ADDRESSES[configChainId].toLowerCase()) { throw new Error( `Chain configuration mismatch. Service is trying to start for chain ${configChainId}, but DB contains data for another chain.`, ); From ed54f9031bbf907238029bf04939ff4ef059b7e3 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 11 Jun 2024 11:51:38 +0400 Subject: [PATCH 06/15] refactor: fix lint errors in tests Remove unnecessary `consensusProviderService` variable and fix lint errors in unit tests for `NetworkValidation` service. --- .../network-validation.service.spec.ts | 257 +++++++----------- 1 file changed, 93 insertions(+), 164 deletions(-) diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts index 293e6cee..eb925b66 100644 --- a/src/network-validation/network-validation.service.spec.ts +++ b/src/network-validation/network-validation.service.spec.ts @@ -16,7 +16,6 @@ import { DatabaseE2ETestingModule } from '../app'; describe('network configuration correctness sanity checker', () => { let configService: ConfigService; let locator: { address: string }; - let consensusProviderService: ConsensusProviderService; let executionProviderService: ExecutionProviderService; let registryKeyStorageService: RegistryKeyStorageService; let moduleStorageService: SRModuleStorageService; @@ -47,16 +46,13 @@ describe('network configuration correctness sanity checker', () => { beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ - imports: [ - ConfigModule, - DatabaseE2ETestingModule.forRoot(), - ], + imports: [ConfigModule, DatabaseE2ETestingModule.forRoot()], providers: [ { provide: LIDO_LOCATOR_CONTRACT_TOKEN, useValue: { address: '0x17000', - } + }, }, { provide: LOGGER_PROVIDER, @@ -69,34 +65,34 @@ describe('network configuration correctness sanity checker', () => { useValue: { getDepositContract: jest.fn(async () => ({ data: { - chain_id: '1' - } + chain_id: '1', + }, })), - } + }, }, { provide: ExecutionProviderService, useValue: { getChainId: jest.fn(async () => 17000), - } + }, }, { provide: RegistryKeyStorageService, useValue: { find: jest.fn(async () => []), - } + }, }, { provide: SRModuleStorageService, useValue: { findOneByModuleId: jest.fn(async () => null), - } + }, }, { provide: RegistryOperatorStorageService, useValue: { find: jest.fn(async () => []), - } + }, }, { provide: AppInfoStorageService, @@ -106,7 +102,7 @@ describe('network configuration correctness sanity checker', () => { chainId: 17000, locatorAddress: '0x17000', })), - } + }, }, NetworkValidationService, ], @@ -114,7 +110,6 @@ describe('network configuration correctness sanity checker', () => { configService = module.get(ConfigService); locator = module.get(LIDO_LOCATOR_CONTRACT_TOKEN); - consensusProviderService = module.get(ConsensusProviderService); executionProviderService = module.get(ExecutionProviderService); registryKeyStorageService = module.get(RegistryKeyStorageService); moduleStorageService = module.get(SRModuleStorageService); @@ -122,18 +117,16 @@ describe('network configuration correctness sanity checker', () => { appInfoStorageService = module.get(AppInfoStorageService); networkValidationService = module.get(NetworkValidationService); - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return false; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } - if (path === 'CHAIN_ID') { - return 17000; - } + if (path === 'CHAIN_ID') { + return 17000; + } - return undefined; + return undefined; }); }); @@ -142,19 +135,17 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by EL node if the validator registry is enabled in config", async () => { - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return true; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } - if (path === 'CHAIN_ID') { - return 1; - } + if (path === 'CHAIN_ID') { + return 1; + } - return undefined; - }); + return undefined; + }); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -163,19 +154,17 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if the chain ID returned by EL node doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return true; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } - if (path === 'CHAIN_ID') { - return 17000; - } + if (path === 'CHAIN_ID') { + return 17000; + } - return undefined; - }); + return undefined; + }); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -184,19 +173,17 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return true; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return true; + } - if (path === 'CHAIN_ID') { - return 17000; - } + if (path === 'CHAIN_ID') { + return 17000; + } - return undefined; - }); + return undefined; + }); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -204,7 +191,7 @@ describe('network configuration correctness sanity checker', () => { expect(updateAppInfoMock).not.toHaveBeenCalled(); }); - it("should save information about the chain and locator to the DB if there are no keys, modules, and operators in the DB", async () => { + it('should save information about the chain and locator to the DB if there are no keys, modules, and operators in the DB', async () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); await expect(networkValidationService.validate()).resolves.not.toThrow(); @@ -216,35 +203,27 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if info about chain ID stored in the DB doesn't match chain ID configured in env variables and keys, modules or operators tables have some info in the DB", async () => { - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return false; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } - if (path === 'CHAIN_ID') { - return 1; - } + if (path === 'CHAIN_ID') { + return 1; + } - return undefined; - }); + return undefined; + }); - jest - .spyOn(executionProviderService, 'getChainId') - .mockImplementationOnce(() => Promise.resolve(1)); + jest.spyOn(executionProviderService, 'getChainId').mockImplementationOnce(() => Promise.resolve(1)); - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); jest .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -253,17 +232,13 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if info about locator address stored in the DB doesn't match locator address returned by locator service and keys, modules or operators tables have some info in the DB", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); jest .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); locator.address = '0x1'; @@ -273,18 +248,14 @@ describe('network configuration correctness sanity checker', () => { expect(updateAppInfoMock).not.toHaveBeenCalled(); }); - it("should execute successfully if info about chain ID matches the chain ID configured in env variables and locator address stored in the DB matches the locator address returned by locator service, and keys, modules, or operators tables have some info in the DB", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + it('should execute successfully if info about chain ID matches the chain ID configured in env variables and locator address stored in the DB matches the locator address returned by locator service, and keys, modules, or operators tables have some info in the DB', async () => { + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); jest .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -297,9 +268,7 @@ describe('network configuration correctness sanity checker', () => { .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -308,13 +277,9 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if operators table is not empty in the DB, but the keys table is empty, and DB doesn't have information about chain ID and locator", async () => { - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -323,13 +288,9 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if keys table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -338,13 +299,9 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if operators table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -353,13 +310,9 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if keys table is not empty in the DB, but the operators table is empty, and DB doesn't have information about chain ID and locator", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -372,9 +325,7 @@ describe('network configuration correctness sanity checker', () => { .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -383,9 +334,7 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); const mainnetCuratedModuleFixture = { ...holeskyCuratedModuleFixture, @@ -396,13 +345,9 @@ describe('network configuration correctness sanity checker', () => { .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(mainnetCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -411,39 +356,29 @@ describe('network configuration correctness sanity checker', () => { }); it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { - jest - .spyOn(configService, 'get') - .mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return false; - } + jest.spyOn(configService, 'get').mockImplementation((path) => { + if (path === 'VALIDATOR_REGISTRY_ENABLE') { + return false; + } - if (path === 'CHAIN_ID') { - return 1; - } + if (path === 'CHAIN_ID') { + return 1; + } - return configService.get(path); - }); + return configService.get(path); + }); - jest - .spyOn(executionProviderService, 'getChainId') - .mockImplementationOnce(() => Promise.resolve(1)); + jest.spyOn(executionProviderService, 'getChainId').mockImplementationOnce(() => Promise.resolve(1)); - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); jest .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -452,21 +387,15 @@ describe('network configuration correctness sanity checker', () => { }); it("should execute successfully if DB has information about keys, modules, and operators, doesn't have information about chain and locator, and address of the curated module stored in the DB matches the address of the curated module for the chain for which the app was started", async () => { - jest - .spyOn(registryKeyStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([keyFixture])); + jest.spyOn(registryKeyStorageService, 'find').mockImplementationOnce(() => Promise.resolve([keyFixture])); jest .spyOn(moduleStorageService, 'findOneByModuleId') .mockImplementationOnce(() => Promise.resolve(holeskyCuratedModuleFixture)); - jest - .spyOn(operatorStorageService, 'find') - .mockImplementationOnce(() => Promise.resolve([operatorFixture])); + jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - jest - .spyOn(appInfoStorageService, 'get') - .mockImplementationOnce(() => Promise.resolve(null)); + jest.spyOn(appInfoStorageService, 'get').mockImplementationOnce(() => Promise.resolve(null)); const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); @@ -477,4 +406,4 @@ describe('network configuration correctness sanity checker', () => { locatorAddress: '0x17000', }); }); -}) +}); From 4a6c3ca61e3c56674e8aeebfcb7ec42eaf051bcf Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Mon, 17 Jun 2024 18:15:10 +0400 Subject: [PATCH 07/15] refactor: move network ID checking Move checking that the network ID specified in .env config matches the EL chain ID one level up in the `validate` method of the `NetworkValidation` service. --- src/network-validation/network-validation.service.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 69383bd1..ad329586 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -30,12 +30,16 @@ export class NetworkValidationService { const configChainId = this.configService.get('CHAIN_ID'); const elChainId = await this.executionProviderService.getChainId(); + if (configChainId !== elChainId) { + throw new Error("Chain ID in the config doesn't match EL chain ID"); + } + if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { const depositContract = await this.consensusProviderService.getDepositContract(); const clChainId = Number(depositContract.data?.chain_id); - if (configChainId !== elChainId || elChainId !== clChainId) { - throw new Error('Execution and consensus chain ids do not match'); + if (elChainId !== clChainId) { + throw new Error('Execution and consensus chain IDs do not match'); } } From 529f2f8c5c38030f15079689eb0179b8171e3c7b Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Mon, 17 Jun 2024 18:50:59 +0400 Subject: [PATCH 08/15] refactor: new `checkChainIdMismatch` method Move checking that chain ID from the config, EL chain ID and CL chain ID match each other to the separate `checkChainIdMismatch` private method of the `NetworkValidation` service. --- .../network-validation.service.ts | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index ad329586..90896933 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -30,18 +30,7 @@ export class NetworkValidationService { const configChainId = this.configService.get('CHAIN_ID'); const elChainId = await this.executionProviderService.getChainId(); - if (configChainId !== elChainId) { - throw new Error("Chain ID in the config doesn't match EL chain ID"); - } - - if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { - const depositContract = await this.consensusProviderService.getDepositContract(); - const clChainId = Number(depositContract.data?.chain_id); - - if (elChainId !== clChainId) { - throw new Error('Execution and consensus chain IDs do not match'); - } - } + await this.checkChainIdMismatch(configChainId, elChainId); const [dbKey, dbCuratedModule, dbOperator] = await Promise.all([ await this.keyStorageService.find({}, { limit: 1 }), @@ -85,4 +74,19 @@ export class NetworkValidationService { locatorAddress: this.contract.address, }); } + + private async checkChainIdMismatch(configChainId: number, elChainId: number): Promise { + if (configChainId !== elChainId) { + throw new Error("Chain ID in the config doesn't match EL chain ID"); + } + + if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { + const depositContract = await this.consensusProviderService.getDepositContract(); + const clChainId = Number(depositContract.data?.chain_id); + + if (elChainId !== clChainId) { + throw new Error('Execution and consensus chain IDs do not match'); + } + } + } } From 407dabeefc31663bd9333b2def88fd0f0678ee41 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Mon, 17 Jun 2024 22:07:44 +0400 Subject: [PATCH 09/15] refactor: new specific error types Add two new custom error types (`ChainMismatchError` and `InconsistentDataInDBError`) with properties that help better indicate specific of the error. Update test cases to make sure that correct error types with correct properties are returned in each test case. --- .../network-validation.service.spec.ts | 82 +++++++++++++++---- .../network-validation.service.ts | 48 +++++++++-- 2 files changed, 107 insertions(+), 23 deletions(-) diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts index eb925b66..91eafc2e 100644 --- a/src/network-validation/network-validation.service.spec.ts +++ b/src/network-validation/network-validation.service.spec.ts @@ -12,6 +12,11 @@ import { key } from 'common/registry/test/fixtures/key.fixture'; import { curatedModule } from '../http/db.fixtures'; import { operator } from 'common/registry/test/fixtures/operator.fixture'; import { DatabaseE2ETestingModule } from '../app'; +import { + InconsistentDataInDBErrorTypes, + ChainMismatchError, + InconsistentDataInDBError, +} from './network-validation.service'; describe('network configuration correctness sanity checker', () => { let configService: ConfigService; @@ -149,8 +154,13 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(ChainMismatchError); + expect(error).toHaveProperty('configChainId', 1); + expect(error).toHaveProperty('elChainId', 17000); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if the chain ID returned by EL node doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { @@ -168,8 +178,14 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(ChainMismatchError); + expect(error).toHaveProperty('configChainId', 17000); + expect(error).toHaveProperty('elChainId', 17000); + expect(error).toHaveProperty('clChainId', 1); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by CL node if the validator registry is enabled in config", async () => { @@ -187,8 +203,14 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(ChainMismatchError); + expect(error).toHaveProperty('configChainId', 17000); + expect(error).toHaveProperty('elChainId', 17000); + expect(error).toHaveProperty('clChainId', 1); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it('should save information about the chain and locator to the DB if there are no keys, modules, and operators in the DB', async () => { @@ -227,8 +249,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.appInfoMismatch); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if info about locator address stored in the DB doesn't match locator address returned by locator service and keys, modules or operators tables have some info in the DB", async () => { @@ -244,8 +270,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.appInfoMismatch); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it('should execute successfully if info about chain ID matches the chain ID configured in env variables and locator address stored in the DB matches the locator address returned by locator service, and keys, modules, or operators tables have some info in the DB', async () => { @@ -294,8 +324,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.emptyModule); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if operators table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { @@ -305,8 +339,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.emptyModule); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if keys table is not empty in the DB, but the operators table is empty, and DB doesn't have information about chain ID and locator", async () => { @@ -351,8 +389,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.curatedModuleAddressMismatch); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { @@ -382,8 +424,12 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.curatedModuleAddressMismatch); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should execute successfully if DB has information about keys, modules, and operators, doesn't have information about chain and locator, and address of the curated module stored in the DB matches the address of the curated module for the chain for which the app was started", async () => { diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 90896933..4ffd8755 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -10,6 +10,34 @@ import { RegistryKeyStorageService, RegistryOperatorStorageService } from '../co import { SRModuleStorageService } from '../storage/sr-module.storage'; import { AppInfoStorageService } from '../storage/app-info.storage'; +export enum InconsistentDataInDBErrorTypes { + appInfoMismatch = 'APP_INFO_TABLE_DATA_MISMATCH_ERROR', + emptyModule = 'EMPTY_MODULE_TABLE_ERROR', + curatedModuleAddressMismatch = 'CURATED_MODULE_ADDRESS_MISMATCH', +} + +export class ChainMismatchError extends Error { + configChainId: number; + elChainId: number; + clChainId: number | undefined; + + constructor(message: string, configChainId: number, elChainId: number, clChainId?: number | undefined) { + super(message); + this.configChainId = configChainId; + this.elChainId = elChainId; + this.clChainId = clChainId; + } +} + +export class InconsistentDataInDBError extends Error { + type: InconsistentDataInDBErrorTypes; + + constructor(message: string, type: InconsistentDataInDBErrorTypes) { + super(message); + this.type = type; + } +} + @Injectable() export class NetworkValidationService { constructor( @@ -50,8 +78,9 @@ export class NetworkValidationService { if (appInfo != null) { if (appInfo.chainId !== configChainId || appInfo.locatorAddress !== this.contract.address) { - throw new Error( + throw new InconsistentDataInDBError( `Chain configuration mismatch. Database is not empty and contains information for chain ${appInfo.chainId} and locator address ${appInfo.locatorAddress}, but the service is trying to start for chain ${configChainId} and locator address ${this.contract.address}`, + InconsistentDataInDBErrorTypes.appInfoMismatch, ); } @@ -59,12 +88,16 @@ export class NetworkValidationService { } if (dbCuratedModule == null) { - throw new Error('Inconsistent data in database. Some DB tables are empty, but some are not.'); + throw new InconsistentDataInDBError( + 'Inconsistent data in database. Some DB tables are empty, but some are not.', + InconsistentDataInDBErrorTypes.emptyModule, + ); } if (dbCuratedModule.stakingModuleAddress !== REGISTRY_CONTRACT_ADDRESSES[configChainId].toLowerCase()) { - throw new Error( + throw new InconsistentDataInDBError( `Chain configuration mismatch. Service is trying to start for chain ${configChainId}, but DB contains data for another chain.`, + InconsistentDataInDBErrorTypes.curatedModuleAddressMismatch, ); } @@ -77,7 +110,7 @@ export class NetworkValidationService { private async checkChainIdMismatch(configChainId: number, elChainId: number): Promise { if (configChainId !== elChainId) { - throw new Error("Chain ID in the config doesn't match EL chain ID"); + throw new ChainMismatchError("Chain ID in the config doesn't match EL chain ID", configChainId, elChainId); } if (this.configService.get('VALIDATOR_REGISTRY_ENABLE')) { @@ -85,7 +118,12 @@ export class NetworkValidationService { const clChainId = Number(depositContract.data?.chain_id); if (elChainId !== clChainId) { - throw new Error('Execution and consensus chain IDs do not match'); + throw new ChainMismatchError( + 'Execution and consensus chain IDs do not match', + configChainId, + elChainId, + clChainId, + ); } } } From 7016a00edeae65a61ea83a196c05f457f58e9d4e Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Mon, 17 Jun 2024 22:14:45 +0400 Subject: [PATCH 10/15] refactor: rename symbols Rename `dbKey` local constant to `dbKeys`; Rename `dbOperator` local constant to `dbOperators`. --- src/network-validation/network-validation.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 4ffd8755..b88a8ecb 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -60,13 +60,13 @@ export class NetworkValidationService { await this.checkChainIdMismatch(configChainId, elChainId); - const [dbKey, dbCuratedModule, dbOperator] = await Promise.all([ + const [dbKeys, dbCuratedModule, dbOperators] = await Promise.all([ await this.keyStorageService.find({}, { limit: 1 }), await this.moduleStorageService.findOneByModuleId(1), await this.operatorStorageService.find({}, { limit: 1 }), ]); - if (dbKey.length === 0 && dbCuratedModule == null && dbOperator.length === 0) { + if (dbKeys.length === 0 && dbCuratedModule == null && dbOperators.length === 0) { this.logger.log('DB is empty, write chain info into DB'); return await this.appInfoStorageService.update({ chainId: configChainId, From 9a08698e0905a619aaed708619823bc4cfcd8cfe Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Mon, 17 Jun 2024 22:25:35 +0400 Subject: [PATCH 11/15] refactor: rename `contract` to `locatorContract` --- .../network-validation.service.spec.ts | 6 +++--- src/network-validation/network-validation.service.ts | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts index 91eafc2e..7e745bac 100644 --- a/src/network-validation/network-validation.service.spec.ts +++ b/src/network-validation/network-validation.service.spec.ts @@ -20,7 +20,7 @@ import { describe('network configuration correctness sanity checker', () => { let configService: ConfigService; - let locator: { address: string }; + let locatorContract: { address: string }; let executionProviderService: ExecutionProviderService; let registryKeyStorageService: RegistryKeyStorageService; let moduleStorageService: SRModuleStorageService; @@ -114,7 +114,7 @@ describe('network configuration correctness sanity checker', () => { }).compile(); configService = module.get(ConfigService); - locator = module.get(LIDO_LOCATOR_CONTRACT_TOKEN); + locatorContract = module.get(LIDO_LOCATOR_CONTRACT_TOKEN); executionProviderService = module.get(ExecutionProviderService); registryKeyStorageService = module.get(RegistryKeyStorageService); moduleStorageService = module.get(SRModuleStorageService); @@ -266,7 +266,7 @@ describe('network configuration correctness sanity checker', () => { jest.spyOn(operatorStorageService, 'find').mockImplementationOnce(() => Promise.resolve([operatorFixture])); - locator.address = '0x1'; + locatorContract.address = '0x1'; const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index b88a8ecb..9f488f7d 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -42,7 +42,7 @@ export class InconsistentDataInDBError extends Error { export class NetworkValidationService { constructor( protected readonly orm: MikroORM, - @Inject(LIDO_LOCATOR_CONTRACT_TOKEN) protected readonly contract: LidoLocator, + @Inject(LIDO_LOCATOR_CONTRACT_TOKEN) protected readonly locatorContract: LidoLocator, @Inject(LOGGER_PROVIDER) protected readonly logger: LoggerService, protected readonly configService: ConfigService, protected readonly consensusProviderService: ConsensusProviderService, @@ -70,16 +70,16 @@ export class NetworkValidationService { this.logger.log('DB is empty, write chain info into DB'); return await this.appInfoStorageService.update({ chainId: configChainId, - locatorAddress: this.contract.address, + locatorAddress: this.locatorContract.address, }); } const appInfo = await this.appInfoStorageService.get(); if (appInfo != null) { - if (appInfo.chainId !== configChainId || appInfo.locatorAddress !== this.contract.address) { + if (appInfo.chainId !== configChainId || appInfo.locatorAddress !== this.locatorContract.address) { throw new InconsistentDataInDBError( - `Chain configuration mismatch. Database is not empty and contains information for chain ${appInfo.chainId} and locator address ${appInfo.locatorAddress}, but the service is trying to start for chain ${configChainId} and locator address ${this.contract.address}`, + `Chain configuration mismatch. Database is not empty and contains information for chain ${appInfo.chainId} and locator address ${appInfo.locatorAddress}, but the service is trying to start for chain ${configChainId} and locator address ${this.locatorContract.address}`, InconsistentDataInDBErrorTypes.appInfoMismatch, ); } @@ -104,7 +104,7 @@ export class NetworkValidationService { this.logger.log('DB is not empty and chain info is not found in DB, write chain info into DB'); await this.appInfoStorageService.update({ chainId: configChainId, - locatorAddress: this.contract.address, + locatorAddress: this.locatorContract.address, }); } From a194a83941d678bf7ed24e1ddb0245ff9f9ef100 Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 18 Jun 2024 11:38:16 +0400 Subject: [PATCH 12/15] chore: add new specific error types Add new specific error types for cases when keys or operators table is empty. Change error messages to better reflect the essence of the error. Add new tests for testing these new error types. --- .../network-validation.service.spec.ts | 72 ++++++++++++++++--- .../network-validation.service.ts | 22 +++++- 2 files changed, 81 insertions(+), 13 deletions(-) diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts index 7e745bac..61912d22 100644 --- a/src/network-validation/network-validation.service.spec.ts +++ b/src/network-validation/network-validation.service.spec.ts @@ -302,8 +302,18 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyKeys, InconsistentDataInDBErrorTypes.emptyOperators]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyModules); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if operators table is not empty in the DB, but the keys table is empty, and DB doesn't have information about chain ID and locator", async () => { @@ -313,8 +323,18 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyKeys, InconsistentDataInDBErrorTypes.emptyModules]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyOperators); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if keys table is not empty in the DB, but the module table is empty, and DB doesn't have information about chain ID and locator", async () => { @@ -326,7 +346,13 @@ describe('network configuration correctness sanity checker', () => { return networkValidationService.validate().catch((error: Error) => { expect(error).toBeInstanceOf(InconsistentDataInDBError); - expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.emptyModule); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyModules, InconsistentDataInDBErrorTypes.emptyOperators]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyKeys); expect(updateAppInfoMock).not.toHaveBeenCalled(); }); @@ -341,7 +367,13 @@ describe('network configuration correctness sanity checker', () => { return networkValidationService.validate().catch((error: Error) => { expect(error).toBeInstanceOf(InconsistentDataInDBError); - expect(error).toHaveProperty('type', InconsistentDataInDBErrorTypes.emptyModule); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyKeys, InconsistentDataInDBErrorTypes.emptyModules]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyOperators); expect(updateAppInfoMock).not.toHaveBeenCalled(); }); @@ -354,8 +386,18 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyModules, InconsistentDataInDBErrorTypes.emptyOperators]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyKeys); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if modules table is not empty in the DB, but the operators table is empty, and DB doesn't have information about chain ID and locator", async () => { @@ -367,8 +409,18 @@ describe('network configuration correctness sanity checker', () => { const updateAppInfoMock = jest.spyOn(appInfoStorageService, 'update'); - await expect(networkValidationService.validate()).rejects.toThrow(); - expect(updateAppInfoMock).not.toHaveBeenCalled(); + return networkValidationService.validate().catch((error: Error) => { + expect(error).toBeInstanceOf(InconsistentDataInDBError); + expect(error).toHaveProperty('type'); + + const dbDataError = error as InconsistentDataInDBError; + expect([InconsistentDataInDBErrorTypes.emptyKeys, InconsistentDataInDBErrorTypes.emptyOperators]).toContain( + dbDataError.type, + ); + expect(dbDataError.type).not.toEqual(InconsistentDataInDBErrorTypes.emptyModules); + + expect(updateAppInfoMock).not.toHaveBeenCalled(); + }); }); it("should throw error if DB has information about keys, modules and operators, but doesn't have information about chain and locator, and address of the curated module stored in the DB doesn't match the correct address of the curated module for the chain for which the app was started", async () => { diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 9f488f7d..2084b716 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -12,7 +12,9 @@ import { AppInfoStorageService } from '../storage/app-info.storage'; export enum InconsistentDataInDBErrorTypes { appInfoMismatch = 'APP_INFO_TABLE_DATA_MISMATCH_ERROR', - emptyModule = 'EMPTY_MODULE_TABLE_ERROR', + emptyKeys = 'EMPTY_KEYS_TABLE_ERROR', + emptyModules = 'EMPTY_MODULES_TABLE_ERROR', + emptyOperators = 'EMPTY_OPERATORS_TABLE_ERROR', curatedModuleAddressMismatch = 'CURATED_MODULE_ADDRESS_MISMATCH', } @@ -87,10 +89,24 @@ export class NetworkValidationService { return; } + if (dbKeys.length === 0) { + throw new InconsistentDataInDBError( + 'Inconsistent data in database. Keys table is empty, but other tables are not empty.', + InconsistentDataInDBErrorTypes.emptyKeys, + ); + } + if (dbCuratedModule == null) { throw new InconsistentDataInDBError( - 'Inconsistent data in database. Some DB tables are empty, but some are not.', - InconsistentDataInDBErrorTypes.emptyModule, + 'Inconsistent data in database. Modules table is empty, but other tables are not empty.', + InconsistentDataInDBErrorTypes.emptyModules, + ); + } + + if (dbOperators.length === 0) { + throw new InconsistentDataInDBError( + 'Inconsistent data in database. Operators table is empty, but other tables are not empty.', + InconsistentDataInDBErrorTypes.emptyOperators, ); } From 8c04d2f708510d45fc955cda27418f30bf13756b Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 18 Jun 2024 11:48:25 +0400 Subject: [PATCH 13/15] fix: test for chain and EL IDs mismatch Fix test that tests the case when chain ID and EL ID don't match each other. This test should not depend on the value of the `VALIDATOR_REGISTRY_ENABLE` env variable. --- src/network-validation/network-validation.service.spec.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/network-validation/network-validation.service.spec.ts b/src/network-validation/network-validation.service.spec.ts index 61912d22..0112742e 100644 --- a/src/network-validation/network-validation.service.spec.ts +++ b/src/network-validation/network-validation.service.spec.ts @@ -139,12 +139,8 @@ describe('network configuration correctness sanity checker', () => { expect(networkValidationService).toBeDefined(); }); - it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by EL node if the validator registry is enabled in config", async () => { + it("should throw error if the chain ID defined in env variables doesn't match the chain ID returned by EL node", async () => { jest.spyOn(configService, 'get').mockImplementation((path) => { - if (path === 'VALIDATOR_REGISTRY_ENABLE') { - return true; - } - if (path === 'CHAIN_ID') { return 1; } From 8bf17bf0cd0b4d563a2cf500a3b5ed44caf961af Mon Sep 17 00:00:00 2001 From: Alexander Lukin Date: Tue, 18 Jun 2024 12:28:44 +0400 Subject: [PATCH 14/15] docs: add notes with code logic explanations --- .../network-validation.service.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/network-validation/network-validation.service.ts b/src/network-validation/network-validation.service.ts index 2084b716..adb916e1 100644 --- a/src/network-validation/network-validation.service.ts +++ b/src/network-validation/network-validation.service.ts @@ -68,6 +68,11 @@ export class NetworkValidationService { await this.operatorStorageService.find({}, { limit: 1 }), ]); + /** + * @note If all these 3 tables are empty, it is assumed that the service is run on the clean DB and it is safe to + * store info about the chain ID for which the service is run to tie information about the chain ID and locator to + * keys, modules, and operators that will be downloaded and stored into the DB. + */ if (dbKeys.length === 0 && dbCuratedModule == null && dbOperators.length === 0) { this.logger.log('DB is empty, write chain info into DB'); return await this.appInfoStorageService.update({ @@ -79,6 +84,12 @@ export class NetworkValidationService { const appInfo = await this.appInfoStorageService.get(); if (appInfo != null) { + /** + * @note If the app info table has information about the chain and locator, and this information doesn't match the + * chain specified in the env variables, it indicates that the service was already run and saved to the DB info + * for one chain, and now it is going to run for another chain. This case is detected here to prevent corruption + * of data in the DB. + */ if (appInfo.chainId !== configChainId || appInfo.locatorAddress !== this.locatorContract.address) { throw new InconsistentDataInDBError( `Chain configuration mismatch. Database is not empty and contains information for chain ${appInfo.chainId} and locator address ${appInfo.locatorAddress}, but the service is trying to start for chain ${configChainId} and locator address ${this.locatorContract.address}`, @@ -110,6 +121,13 @@ export class NetworkValidationService { ); } + /** + * @note If the service is upgraded to the new version (so that in the previous version there was no "app_info" + * table and this sanity checker service, but in the new version it appears), it has information in the DB. If the + * service starts after the version upgrade with an incorrect chain ID specified in the env variables, it will lead + * to data corruption. To prevent this case, this code checks that the curated module stored in the DB has the + * correct address for the chain specified in the env variables. + */ if (dbCuratedModule.stakingModuleAddress !== REGISTRY_CONTRACT_ADDRESSES[configChainId].toLowerCase()) { throw new InconsistentDataInDBError( `Chain configuration mismatch. Service is trying to start for chain ${configChainId}, but DB contains data for another chain.`, @@ -117,6 +135,12 @@ export class NetworkValidationService { ); } + /** + * @note If the service is upgraded to the new version, it doesn't have the "app_info" table yet, but the curated + * module stored in the DB has the correct address for the chain specified in env variables, it's pretty safe to + * assume that the service is run with the correct config. In this case, the service just stores the information + * about chain ID and locator for which it is run into the DB. + */ this.logger.log('DB is not empty and chain info is not found in DB, write chain info into DB'); await this.appInfoStorageService.update({ chainId: configChainId, From be0b2d67b847109f5eb6b3b4fe99fb8d5d087138 Mon Sep 17 00:00:00 2001 From: ziars Date: Tue, 18 Jun 2024 15:23:40 +0300 Subject: [PATCH 15/15] feat: added envs to test workflow's step --- .github/workflows/test.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2415cf18..9fe004f4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -35,6 +35,12 @@ jobs: run: yarn lint - name: Run tests run: yarn test + env: + DB_NAME: node_operator_keys_service_db + DB_PORT: 5432 + DB_HOST: localhost + DB_USER: postgres + DB_PASSWORD: postgres - name: Run E2E tests run: yarn test:e2e env: