From 914cd7070451d3107926ce4acbc7144b6bf7a370 Mon Sep 17 00:00:00 2001 From: Franco NG Date: Wed, 4 Oct 2023 23:38:29 +0200 Subject: [PATCH 1/5] WIP - Added TODOs to unresolved tests --- .../interoperability/internal_method.spec.ts | 61 +++++++++++++++++++ .../mainchain/internal_method.spec.ts | 2 + .../sidechain/internal_method.spec.ts | 4 +- 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/framework/test/unit/modules/interoperability/internal_method.spec.ts b/framework/test/unit/modules/interoperability/internal_method.spec.ts index a7acb99820c..1c6809404f9 100644 --- a/framework/test/unit/modules/interoperability/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/internal_method.spec.ts @@ -211,6 +211,9 @@ describe('Base interoperability internal method', () => { ...channelData, inbox: updatedInboxTree, }); + + // TODO: tree corresponding to chainID was updated + // TODO: regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). }); }); @@ -228,6 +231,9 @@ describe('Base interoperability internal method', () => { ...channelData, outbox: updatedOutboxTree, }); + + // TODO: tree corresponding to chainID was updated + // TODO: regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). }); }); @@ -240,9 +246,15 @@ describe('Base interoperability internal method', () => { expect(outboxRootSubstore.set).toHaveBeenCalledWith(expect.anything(), chainID, { root: updatedOutboxTree.root, }); + + // TODO: to test that the channel substore was updated }); }); + describe('sendInternal', () => { + // TODO: Add missing tests + }); + describe('createTerminatedOutboxAccount', () => { const terminatedOutboxCreatedEventMock = { log: jest.fn(), @@ -335,6 +347,9 @@ describe('Base interoperability internal method', () => { initialized: true, }, ); + + // TODO: Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED + // TODO: Check the entry for the key chainID was removed from the outbox root substore. }); it('should set appropriate terminated state for chain id in the terminatedState sub store if chain account exists for the id but state root is not provided', async () => { @@ -355,6 +370,11 @@ describe('Base interoperability internal method', () => { mainchainStateRoot: EMPTY_HASH, initialized: true, }); + + // TODO: Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED + // TODO: Check the entry for the key chainID was removed from the outbox root substore + // TODO: Check an EVENT_NAME_CHAIN_ACCOUNT_UPDATED event was created + // TODO: Check an EVENT_NAME_TERMINATED_STATE_CREATED event was created. }); it('should throw error if chain account does not exist for the id and ownchain account id is mainchain id', async () => { @@ -370,6 +390,8 @@ describe('Base interoperability internal method', () => { chainIdNew, ), ).rejects.toThrow('Chain to be terminated is not valid'); + + // TODO: Check test that the corresponding terminated state account was NOT created. }); it('should set appropriate terminated state for chain id if chain account does not exist for the id and stateRoot is EMPTY_HASH', async () => { @@ -485,6 +507,7 @@ describe('Base interoperability internal method', () => { ), ).toBeUndefined(); + // TODO: called with correct arguments expect(mainchainInteroperabilityInternalMethod.sendInternal).toHaveBeenCalled(); expect( mainchainInteroperabilityInternalMethod.createTerminatedStateAccount, @@ -662,6 +685,17 @@ describe('Base interoperability internal method', () => { certificate: codec.encode(certificateSchema, defaultCertificate), }; + // TODO: Use this + // { + // name: 'chain1', + // status: 1, + // lastCertificate: { + // height: certificate.height, + // stateRoot: certificate.stateRoot, + // timestamp: certificate.timestamp, + // validatorsHash: certificate.validatorsHash, + // }, + // } await interopMod.stores.get(ChainAccountStore).set(storeContext, ccuParams.sendingChainID, { lastCertificate: { height: 20, @@ -768,6 +802,11 @@ describe('Base interoperability internal method', () => { }); }); + // TODO: Test is missing where the length of bftWeightsUpdateBitmap is too large, e.g. bftWeightsUpdateBitmap be equal to Buffer.from([0], [7]) + // TODO: Test the validator list returned by calculateNewActiveValidators is empty + // TODO: Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries + // TODO: In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments. + describe('verifyValidatorsUpdate', () => { it('should reject if the certificate is empty', async () => { const ccu = { @@ -1010,6 +1049,7 @@ describe('Base interoperability internal method', () => { }); describe('verifyCertificate', () => { + // TODO: Use correct length const txParams: CrossChainUpdateTransactionParams = { certificate: Buffer.alloc(0), activeValidatorsUpdate: { @@ -1048,6 +1088,7 @@ describe('Base interoperability internal method', () => { }); }); + // TODO: However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this. it('should reject when certificate height is lower than last certificate height', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1117,6 +1158,11 @@ describe('Base interoperability internal method', () => { ); }); + // (1): validatorsHash in certificate and state store are equal + // (2): there is a proper validators update in the CCU + // TODO: Replace by: chainAccount(ccu.params.sendingChainID) should exist + // TODO: 1. (1) is fulfilled,Expectation: verifyCertificate passes. + // TODO: 2. (1) not fulfilled, (2) fulfilled, Expectation: verifyCertificate passes it('should resolve when certificate is valid', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1140,6 +1186,7 @@ describe('Base interoperability internal method', () => { }); }); + // TODO: test where the validator list in the validators store is NOT sorted describe('verifyCertificateSignature', () => { const activeValidators = [ { blsKey: cryptoUtils.getRandomBytes(48), bftWeight: BigInt(1) }, @@ -1208,6 +1255,11 @@ describe('Base interoperability internal method', () => { ); expect(interopMod.events.get(InvalidCertificateSignatureEvent).add).toHaveBeenCalledTimes(1); + + // TODO: verifyWeightedAggSig should be expected to be called with the certificate threshold stored in the validators store for sendingChainID + // TODO: not with txParams.certificateThreshold. + + // TODO: Should also test EVENT_NAME_INVALID_CERTIFICATE_SIGNATURE }); it('should resolve when verifyWeightedAggSig return true', async () => { @@ -1397,6 +1449,8 @@ describe('Base interoperability internal method', () => { certificate: Buffer.alloc(0), }), ).resolves.toBeUndefined(); + + // TODO: checked that calculateRootFromRightWitness is called with the correct arguments. }); it('should resolve when certificate provides valid inclusion proof', async () => { @@ -1410,6 +1464,7 @@ describe('Base interoperability internal method', () => { }), ).resolves.toBeUndefined(); + // TODO: Explicitly state: outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)]) const outboxKey = Buffer.concat([ interopMod.stores.get(OutboxRootStore).key, cryptoUtils.hash(ownChainAccount.chainID), @@ -1432,6 +1487,12 @@ describe('Base interoperability internal method', () => { certificateSchema, expect.toBeObject() as Certificate, ); + + // TODO: checked that calculateRootFromRightWitness is called with the correct arguments. }); + + // TODO: There should be some test(s) where inboxUpdate.crossChainMessages is non empty + // TODO: checked that regularMerkleTree.calculateMerkleRoot is called for every ccm and that it is called with the correct arguments + // TODO: i.e. calculateMerkleRoot must be called two times }); }); diff --git a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts index 0ac861e9749..fc4d13c5b2c 100644 --- a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts @@ -145,4 +145,6 @@ describe('Mainchain interoperability internal method', () => { expect(isLive).toBe(false); }); }); + + // TODO: A test is missing for the case where the chain account exists, status is ACTIVE and liveness requirement IS fulfilled. }); diff --git a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts index 91fc93ff2ec..4fc11dd868b 100644 --- a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts @@ -71,6 +71,8 @@ describe('Sidechain interoperability store', () => { ); }); + // TODO: Tests are missing for status is ACTIVE, expectation: true + // TODO: Tests are missing for status is REGISTERED, expectation: true describe('isLive', () => { beforeEach(() => { ownChainAccountStoreMock.get.mockResolvedValue({ chainID: EMPTY_BYTES }); @@ -100,7 +102,7 @@ describe('Sidechain interoperability store', () => { expect(isLive).toBe(false); }); - it('should return true if chain is not terminated', async () => { + it('should return true if chain account and terminated chain account do not exist', async () => { const isLive = await sidechainInteroperabilityInternalMethod.isLive(context, chainID); expect(isLive).toBe(true); From 81acf8b64cca27652f7b2dbbc519696b604c8103 Mon Sep 17 00:00:00 2001 From: Franco NG Date: Wed, 11 Oct 2023 19:45:20 +0200 Subject: [PATCH 2/5] WIP - Update Interoperabiliy Internal Method --- .../interoperability/internal_method.spec.ts | 266 +++++++++++++++--- 1 file changed, 226 insertions(+), 40 deletions(-) diff --git a/framework/test/unit/modules/interoperability/internal_method.spec.ts b/framework/test/unit/modules/interoperability/internal_method.spec.ts index 1c6809404f9..a243ebb0cbe 100644 --- a/framework/test/unit/modules/interoperability/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/internal_method.spec.ts @@ -12,8 +12,8 @@ * Removal or modification of this copyright notice is prohibited. */ -import { utils as cryptoUtils } from '@liskhq/lisk-cryptography'; import * as cryptography from '@liskhq/lisk-cryptography'; +import { utils as cryptoUtils } from '@liskhq/lisk-cryptography'; import { regularMerkleTree } from '@liskhq/lisk-tree'; import { codec } from '@liskhq/lisk-codec'; import { SparseMerkleTree } from '@liskhq/lisk-db'; @@ -21,11 +21,14 @@ import { validator } from '@liskhq/lisk-validator'; import { BLS_PUBLIC_KEY_LENGTH, BLS_SIGNATURE_LENGTH, + CROSS_CHAIN_COMMAND_CHANNEL_TERMINATED, EMPTY_BYTES, EMPTY_HASH, HASH_LENGTH, + MAX_NUM_VALIDATORS, MESSAGE_TAG_CERTIFICATE, MIN_RETURN_FEE_PER_BYTE_BEDDOWS, + MODULE_NAME_INTEROPERABILITY, } from '../../../../src/modules/interoperability/constants'; import { MainchainInteroperabilityInternalMethod } from '../../../../src/modules/interoperability/mainchain/internal_method'; import * as utils from '../../../../src/modules/interoperability/utils'; @@ -62,6 +65,9 @@ import { Certificate } from '../../../../src/engine/consensus/certificate_genera import { TerminatedOutboxCreatedEvent } from '../../../../src/modules/interoperability/events/terminated_outbox_created'; import { createStoreGetter } from '../../../../src/testing/utils'; import { InvalidCertificateSignatureEvent } from '../../../../src/modules/interoperability/events/invalid_certificate_signature'; +import { EMPTY_FEE_ADDRESS } from '../../../../dist-node/modules/interoperability/constants'; +import { CCM_STATUS_OK } from '../../../../src/modules/token/constants'; +import { ChainStatus } from '../../../../dist-node/modules/interoperability/stores/chain_account'; describe('Base interoperability internal method', () => { const interopMod = new MainchainInteroperabilityModule(); @@ -178,10 +184,12 @@ describe('Base interoperability internal method', () => { jest.spyOn(channelDataSubstore, 'set'); outboxRootSubstore = interopMod.stores.get(OutboxRootStore); jest.spyOn(outboxRootSubstore, 'set'); + jest.spyOn(outboxRootSubstore, 'del'); terminatedOutboxSubstore = interopMod.stores.get(TerminatedOutboxStore); chainValidatorsSubstore = interopMod.stores.get(ChainValidatorsStore); // jest.spyOn(terminatedOutboxSubstore, 'set'); chainDataSubstore = interopMod.stores.get(ChainAccountStore); + jest.spyOn(chainDataSubstore, 'set'); terminatedStateSubstore = interopMod.stores.get(TerminatedStateStore); mainchainInteroperabilityInternalMethod = new MainchainInteroperabilityInternalMethod( @@ -199,6 +207,8 @@ describe('Base interoperability internal method', () => { describe('appendToInboxTree', () => { it('should update the channel store with the new inbox tree info', async () => { + const { inbox: originalInbox } = await channelDataSubstore.get(methodContext, chainID); + // Act await mainchainInteroperabilityInternalMethod.appendToInboxTree( methodContext, @@ -212,13 +222,23 @@ describe('Base interoperability internal method', () => { inbox: updatedInboxTree, }); - // TODO: tree corresponding to chainID was updated - // TODO: regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). + // TODO: [DONE] tree corresponding to chainID was updated + // TODO: [DONE] regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). + const { inbox } = await channelDataSubstore.get(methodContext, chainID); + expect(inbox.size).toBe(originalInbox.size + 1); + + expect(regularMerkleTree.calculateMerkleRoot).toHaveBeenCalledWith({ + value: cryptoUtils.hash(appendData), + appendPath: originalInbox.appendPath, + size: originalInbox.size, + }); }); }); describe('appendToOutboxTree', () => { it('should update the channel store with the new outbox tree info', async () => { + const { outbox: originalOutbox } = await channelDataSubstore.get(methodContext, chainID); + // Act await mainchainInteroperabilityInternalMethod.appendToOutboxTree( methodContext, @@ -232,8 +252,17 @@ describe('Base interoperability internal method', () => { outbox: updatedOutboxTree, }); - // TODO: tree corresponding to chainID was updated - // TODO: regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). + // TODO: [DONE] tree corresponding to chainID was updated + // TODO: [DONE] regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). + + const { outbox } = await channelDataSubstore.get(methodContext, chainID); + expect(outbox.size).toBe(originalOutbox.size + 1); + + expect(regularMerkleTree.calculateMerkleRoot).toHaveBeenCalledWith({ + value: cryptoUtils.hash(appendData), + appendPath: originalOutbox.appendPath, + size: originalOutbox.size, + }); }); }); @@ -247,7 +276,11 @@ describe('Base interoperability internal method', () => { root: updatedOutboxTree.root, }); - // TODO: to test that the channel substore was updated + // TODO: [DONE] to test that the channel substore was updated + + await expect(outboxRootSubstore.get(methodContext, chainID)).resolves.toEqual({ + root: updatedOutboxTree.root, + }); }); }); @@ -348,8 +381,14 @@ describe('Base interoperability internal method', () => { }, ); - // TODO: Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED - // TODO: Check the entry for the key chainID was removed from the outbox root substore. + // TODO: [DONE] Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED + // TODO: [DONE] Check the entry for the key chainID was removed from the outbox root substore. + + expect(chainDataSubstore.set).toHaveBeenCalledWith(crossChainMessageContext, chainId, { + ...chainAccount, + status: ChainStatus.TERMINATED, + }); + expect(outboxRootSubstore.del).toHaveBeenCalledWith(crossChainMessageContext, chainId); }); it('should set appropriate terminated state for chain id in the terminatedState sub store if chain account exists for the id but state root is not provided', async () => { @@ -371,10 +410,19 @@ describe('Base interoperability internal method', () => { initialized: true, }); - // TODO: Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED - // TODO: Check the entry for the key chainID was removed from the outbox root substore - // TODO: Check an EVENT_NAME_CHAIN_ACCOUNT_UPDATED event was created - // TODO: Check an EVENT_NAME_TERMINATED_STATE_CREATED event was created. + // TODO: [DONE] Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED + expect(chainDataSubstore.set).toHaveBeenCalledWith(crossChainMessageContext, chainId, { + ...chainAccount, + status: ChainStatus.TERMINATED, + }); + + // TODO: [DONE] Check the entry for the key chainID was removed from the outbox root substore + expect(outboxRootSubstore.del).toHaveBeenCalledWith(crossChainMessageContext, chainId); + // TODO: [DONE] Check an EVENT_NAME_CHAIN_ACCOUNT_UPDATED event was created + expect(chainAccountUpdatedEvent.log).toHaveBeenCalled(); + + // TODO: [DONE] Check an EVENT_NAME_TERMINATED_STATE_CREATED event was created. + expect(terminatedStateCreatedEvent.log).toHaveBeenCalled(); }); it('should throw error if chain account does not exist for the id and ownchain account id is mainchain id', async () => { @@ -391,7 +439,10 @@ describe('Base interoperability internal method', () => { ), ).rejects.toThrow('Chain to be terminated is not valid'); - // TODO: Check test that the corresponding terminated state account was NOT created. + // TODO: [DONE] Check test that the corresponding terminated state account was NOT created. + await expect(terminatedStateSubstore.has(crossChainMessageContext, chainIdNew)).resolves.toBe( + false, + ); }); it('should set appropriate terminated state for chain id if chain account does not exist for the id and stateRoot is EMPTY_HASH', async () => { @@ -507,11 +558,20 @@ describe('Base interoperability internal method', () => { ), ).toBeUndefined(); - // TODO: called with correct arguments - expect(mainchainInteroperabilityInternalMethod.sendInternal).toHaveBeenCalled(); + // TODO: [DONE]called with correct arguments + expect(mainchainInteroperabilityInternalMethod.sendInternal).toHaveBeenCalledWith( + crossChainMessageContext, + EMPTY_FEE_ADDRESS, + MODULE_NAME_INTEROPERABILITY, + CROSS_CHAIN_COMMAND_CHANNEL_TERMINATED, + SIDECHAIN_ID, + BigInt(0), + CCM_STATUS_OK, + EMPTY_BYTES, + ); expect( mainchainInteroperabilityInternalMethod.createTerminatedStateAccount, - ).toHaveBeenCalled(); + ).toHaveBeenCalledWith(crossChainMessageContext, SIDECHAIN_ID); }); }); @@ -685,23 +745,13 @@ describe('Base interoperability internal method', () => { certificate: codec.encode(certificateSchema, defaultCertificate), }; - // TODO: Use this - // { - // name: 'chain1', - // status: 1, - // lastCertificate: { - // height: certificate.height, - // stateRoot: certificate.stateRoot, - // timestamp: certificate.timestamp, - // validatorsHash: certificate.validatorsHash, - // }, - // } + // TODO: [DONE] Use defaultCertificate await interopMod.stores.get(ChainAccountStore).set(storeContext, ccuParams.sendingChainID, { lastCertificate: { - height: 20, - stateRoot: cryptoUtils.getRandomBytes(HASH_LENGTH), - timestamp: 99, - validatorsHash: cryptoUtils.getRandomBytes(HASH_LENGTH), + height: defaultCertificate.height, + stateRoot: defaultCertificate.stateRoot, + timestamp: defaultCertificate.timestamp, + validatorsHash: defaultCertificate.validatorsHash, }, name: 'chain1', status: 1, @@ -802,12 +852,103 @@ describe('Base interoperability internal method', () => { }); }); - // TODO: Test is missing where the length of bftWeightsUpdateBitmap is too large, e.g. bftWeightsUpdateBitmap be equal to Buffer.from([0], [7]) - // TODO: Test the validator list returned by calculateNewActiveValidators is empty - // TODO: Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries - // TODO: In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments. - describe('verifyValidatorsUpdate', () => { + // TODO: [DONE] Test is missing where the length of bftWeightsUpdateBitmap is too large, e.g. bftWeightsUpdateBitmap be equal to Buffer.from([0], [7]) + it('should reject if length of bftWeightsUpdateBitmap is too large', async () => { + const ccu = { + ...ccuParams, + certificate: codec.encode(certificateSchema, defaultCertificate), + activeValidatorsUpdate: { + blsKeysUpdate: [ + Buffer.from([0, 0, 0, 0]), + Buffer.from([0, 0, 0, 1]), + Buffer.from([0, 0, 3, 0]), + ], + bftWeightsUpdate: [BigInt(1), BigInt(3), BigInt(4), BigInt(3)], + bftWeightsUpdateBitmap: Buffer.from([1, 0, 0, 0, 0, 0, 0, 0]), + }, + }; + + const activeValidators = [{ blsKey: Buffer.from([0, 0, 2, 0]), bftWeight: BigInt(2) }]; + await chainValidatorsSubstore.set(methodContext, ccu.sendingChainID, { + activeValidators, + certificateThreshold: BigInt(1), + }); + + const expectedBitmapLength = + BigInt(ccu.activeValidatorsUpdate.blsKeysUpdate.length + activeValidators.length + 7) / + BigInt(8); + await expect( + mainchainInteroperabilityInternalMethod.verifyValidatorsUpdate(methodContext, ccu), + ).rejects.toThrow(`Invalid bftWeightsUpdateBitmap. Expected length ${expectedBitmapLength}.`); + }); + + // TODO: [DONE] Test the validator list returned by calculateNewActiveValidators is empty + it('should reject if the validator list returned by calculateNewActiveValidators is empty', async () => { + const ccu = { + ...ccuParams, + certificate: codec.encode(certificateSchema, defaultCertificate), + activeValidatorsUpdate: { + blsKeysUpdate: [ + Buffer.from([0, 0, 0, 0]), + Buffer.from([0, 0, 0, 1]), + Buffer.from([0, 0, 3, 0]), + ], + bftWeightsUpdate: [BigInt(1), BigInt(3), BigInt(4)], + // 7 corresponds to 0111 + bftWeightsUpdateBitmap: Buffer.from([7]), + }, + }; + + const existingKey = Buffer.from([0, 2, 3, 0]); + await chainValidatorsSubstore.set(methodContext, ccu.sendingChainID, { + activeValidators: [{ blsKey: existingKey, bftWeight: BigInt(2) }], + certificateThreshold: BigInt(1), + }); + jest.spyOn(utils, 'calculateNewActiveValidators').mockReturnValue([]); + + await expect( + mainchainInteroperabilityInternalMethod.verifyValidatorsUpdate(methodContext, ccu), + ).rejects.toThrow( + `Invalid validators array. It must have at least 1 element and at most ${MAX_NUM_VALIDATORS} elements.`, + ); + }); + + // TODO: [DONE] Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries + it('should reject if the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries', async () => { + const ccu = { + ...ccuParams, + certificate: codec.encode(certificateSchema, defaultCertificate), + activeValidatorsUpdate: { + blsKeysUpdate: [ + Buffer.from([0, 0, 0, 0]), + Buffer.from([0, 0, 0, 1]), + Buffer.from([0, 0, 3, 0]), + ], + bftWeightsUpdate: [BigInt(1), BigInt(3), BigInt(4)], + // 7 corresponds to 0111 + bftWeightsUpdateBitmap: Buffer.from([7]), + }, + }; + + const existingKey = Buffer.from([0, 2, 3, 0]); + await chainValidatorsSubstore.set(methodContext, ccu.sendingChainID, { + activeValidators: [{ blsKey: existingKey, bftWeight: BigInt(2) }], + certificateThreshold: BigInt(1), + }); + const newValidators = Array.from(new Array(MAX_NUM_VALIDATORS + 1)).map((_, index) => ({ + blsKey: Buffer.from([0, 0, 0, index]), + bftWeight: BigInt(1), + })); + jest.spyOn(utils, 'calculateNewActiveValidators').mockReturnValue(newValidators); + + await expect( + mainchainInteroperabilityInternalMethod.verifyValidatorsUpdate(methodContext, ccu), + ).rejects.toThrow( + `Invalid validators array. It must have at least 1 element and at most ${MAX_NUM_VALIDATORS} elements.`, + ); + }); + it('should reject if the certificate is empty', async () => { const ccu = { ...ccuParams, @@ -1008,6 +1149,7 @@ describe('Base interoperability internal method', () => { ); }); + // TODO: [DONE] In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments. it('should resolve if updates are valid', async () => { const ccu = { ...ccuParams, @@ -1025,10 +1167,11 @@ describe('Base interoperability internal method', () => { }; const existingKey = Buffer.from([0, 2, 3, 0]); - await chainValidatorsSubstore.set(methodContext, ccu.sendingChainID, { + const chainValidator = { activeValidators: [{ blsKey: existingKey, bftWeight: BigInt(2) }], certificateThreshold: BigInt(1), - }); + }; + await chainValidatorsSubstore.set(methodContext, ccu.sendingChainID, chainValidator); const newValidators = [ { blsKey: Buffer.from([0, 0, 0, 0]), bftWeight: BigInt(1) }, { blsKey: Buffer.from([0, 0, 0, 1]), bftWeight: BigInt(3) }, @@ -1041,6 +1184,12 @@ describe('Base interoperability internal method', () => { await expect( mainchainInteroperabilityInternalMethod.verifyValidatorsUpdate(methodContext, ccu), ).resolves.toBeUndefined(); + expect(utils.calculateNewActiveValidators).toHaveBeenCalledWith( + chainValidator.activeValidators, + ccu.activeValidatorsUpdate.blsKeysUpdate, + ccu.activeValidatorsUpdate.bftWeightsUpdate, + ccu.activeValidatorsUpdate.bftWeightsUpdateBitmap, + ); expect(validator.validate).toHaveBeenCalledWith( certificateSchema, expect.toBeObject() as Certificate, @@ -1088,7 +1237,7 @@ describe('Base interoperability internal method', () => { }); }); - // TODO: However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this. + // TODO: [DONE] However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this. it('should reject when certificate height is lower than last certificate height', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1161,8 +1310,9 @@ describe('Base interoperability internal method', () => { // (1): validatorsHash in certificate and state store are equal // (2): there is a proper validators update in the CCU // TODO: Replace by: chainAccount(ccu.params.sendingChainID) should exist - // TODO: 1. (1) is fulfilled,Expectation: verifyCertificate passes. + // TODO: [DONE] 1. (1) is fulfilled, Expectation: verifyCertificate passes. // TODO: 2. (1) not fulfilled, (2) fulfilled, Expectation: verifyCertificate passes + // e.g. chainAccount does exist, otherwise it will throw error when calling .get it('should resolve when certificate is valid', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1184,6 +1334,42 @@ describe('Base interoperability internal method', () => { expect.toBeObject() as Certificate, ); }); + + it('should resolve when validatorsHash in certificate and state store are equal', async () => { + const certificate: Certificate = { + ...defaultCertificate, + timestamp: 1000, + }; + + await interopMod.stores.get(ChainAccountStore).set(methodContext, txParams.sendingChainID, { + lastCertificate: { + height: 100, + timestamp: 10, + stateRoot: cryptoUtils.getRandomBytes(HASH_LENGTH), + validatorsHash: defaultCertificate.validatorsHash, + }, + name: 'rand', + status: 0, + }); + + const encodedCertificate = codec.encode(certificateSchema, certificate); + await expect( + mainchainInteroperabilityInternalMethod.verifyCertificate( + methodContext, + { + ...txParams, + certificate: encodedCertificate, + }, + 1001, + ), + ).resolves.toBeUndefined(); + expect(validator.validate).toHaveBeenCalledWith( + certificateSchema, + expect.toBeObject() as Certificate, + ); + }); + + it('should resolve when validatorsHash are NOT equal, but validators are updated', async () => {}); }); // TODO: test where the validator list in the validators store is NOT sorted From 7595f5899f05785171c9d27b548bb145a4a25e80 Mon Sep 17 00:00:00 2001 From: Franco NG Date: Thu, 9 Nov 2023 11:06:06 +0100 Subject: [PATCH 3/5] Update unit tests on mainchain and sidechain internal methods --- .../interoperability/internal_method.spec.ts | 81 +++++++++++++------ .../mainchain/internal_method.spec.ts | 18 ++++- .../sidechain/internal_method.spec.ts | 17 +++- 3 files changed, 87 insertions(+), 29 deletions(-) diff --git a/framework/test/unit/modules/interoperability/internal_method.spec.ts b/framework/test/unit/modules/interoperability/internal_method.spec.ts index 6d97d66e89c..92d5f663b34 100644 --- a/framework/test/unit/modules/interoperability/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/internal_method.spec.ts @@ -13,7 +13,7 @@ */ import * as cryptography from '@liskhq/lisk-cryptography'; -import { utils as cryptoUtils } from '@liskhq/lisk-cryptography'; +import { bls, utils as cryptoUtils } from '@liskhq/lisk-cryptography'; import { regularMerkleTree } from '@liskhq/lisk-tree'; import { codec } from '@liskhq/lisk-codec'; import { SparseMerkleTree } from '@liskhq/lisk-db'; @@ -305,10 +305,6 @@ describe('Base interoperability internal method', () => { }); }); - describe('sendInternal', () => { - // TODO: Add missing tests - }); - describe('createTerminatedOutboxAccount', () => { const terminatedOutboxCreatedEventMock = { log: jest.fn(), @@ -1590,11 +1586,6 @@ describe('Base interoperability internal method', () => { ); expect(interopMod.events.get(InvalidCertificateSignatureEvent).add).toHaveBeenCalledTimes(1); - - // TODO: verifyWeightedAggSig should be expected to be called with the certificate threshold stored in the validators store for sendingChainID - // TODO: not with txParams.certificateThreshold. - - // TODO: Should also test EVENT_NAME_INVALID_CERTIFICATE_SIGNATURE }); it('should resolve when verifyWeightedAggSig return true', async () => { @@ -1610,6 +1601,34 @@ describe('Base interoperability internal method', () => { expect.toBeObject() as Certificate, ); }); + + it('should resolve correctly when validators store is NOT sorted', async () => { + jest.spyOn(cryptography.bls, 'verifyWeightedAggSig').mockReturnValue(true); + + await interopMod.stores + .get(ChainValidatorsStore) + .set(methodContext, txParams.sendingChainID, { + ...chainValidators, + activeValidators: [...activeValidators].sort(() => Math.random() - 0.5), // Shuffle activeValidators + }); + + await expect( + mainchainInteroperabilityInternalMethod.verifyCertificateSignature(methodContext, txParams), + ).resolves.toBeUndefined(); + + expect(cryptography.bls.verifyWeightedAggSig).toHaveBeenCalledTimes(1); + + expect(bls.verifyWeightedAggSig).toHaveBeenCalledWith( + activeValidators.map(activeValidator => activeValidator.blsKey), + expect.anything(), + expect.anything(), + expect.anything(), + expect.anything(), + expect.anything(), + activeValidators.map(activeValidator => activeValidator.bftWeight), + expect.anything(), + ); + }); }); describe('verifyOutboxRootWitness', () => { @@ -1859,25 +1878,31 @@ describe('Base interoperability internal method', () => { .spyOn(regularMerkleTree, 'calculateRootFromRightWitness') .mockReturnValue(channelData.partnerChainOutboxRoot); + const params = { + ...crossChainUpdateParams, + inboxUpdate: { + crossChainMessages: [], + messageWitnessHashes: [], + outboxRootWitness: { + bitmap: Buffer.alloc(0), + siblingHashes: [], + }, + }, + certificate: Buffer.alloc(0), + }; await expect( mainchainInteroperabilityInternalMethod.verifyPartnerChainOutboxRoot( commandExecuteContext as any, - { - ...crossChainUpdateParams, - inboxUpdate: { - crossChainMessages: [], - messageWitnessHashes: [], - outboxRootWitness: { - bitmap: Buffer.alloc(0), - siblingHashes: [], - }, - }, - certificate: Buffer.alloc(0), - }, + params, ), ).resolves.toBeUndefined(); - // TODO: checked that calculateRootFromRightWitness is called with the correct arguments. + // TODO [DONE]: checked that calculateRootFromRightWitness is called with the correct arguments. + expect(regularMerkleTree.calculateRootFromRightWitness).toHaveBeenCalledWith( + channelData.inbox.size, + channelData.inbox.appendPath, + params.inboxUpdate.messageWitnessHashes, + ); }); it('should resolve when certificate provides valid inclusion proof', async () => { @@ -1894,7 +1919,8 @@ describe('Base interoperability internal method', () => { ), ).resolves.toBeUndefined(); - // TODO: Explicitly state: outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)]) + // TODO [DONE]: Explicitly state: outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)]) + // outboxKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_OUTBOX_ROOT + sha256(OWN_CHAIN_ID) const outboxKey = Buffer.concat([ interopMod.stores.get(OutboxRootStore).key, cryptoUtils.hash(ownChainAccount.chainID), @@ -1918,7 +1944,12 @@ describe('Base interoperability internal method', () => { expect.toBeObject() as Certificate, ); - // TODO: checked that calculateRootFromRightWitness is called with the correct arguments. + // TODO [DONE]: checked that calculateRootFromRightWitness is called with the correct arguments. + expect(regularMerkleTree.calculateRootFromRightWitness).toHaveBeenCalledWith( + updatedInboxTree.size, + updatedInboxTree.appendPath, + crossChainUpdateParams.inboxUpdate.messageWitnessHashes, + ); }); // TODO: There should be some test(s) where inboxUpdate.crossChainMessages is non empty diff --git a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts index fc4d13c5b2c..98c8bbf91eb 100644 --- a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts @@ -144,7 +144,21 @@ describe('Mainchain interoperability internal method', () => { expect(isLive).toBe(false); }); - }); - // TODO: A test is missing for the case where the chain account exists, status is ACTIVE and liveness requirement IS fulfilled. + // TODO [DONE]: A test is missing for the case where the chain account exists, status is ACTIVE and liveness requirement IS fulfilled. + it(`should return true when chain account exists, status is ${ChainStatus.ACTIVE} and liveness requirement IS fulfilled`, async () => { + await chainDataSubstore.set(context, chainID, { + ...chainAccount, + status: ChainStatus.ACTIVE, + }); + + const isLive = await mainchainInteroperabilityInternalMethod.isLive( + context, + chainID, + timestamp, + ); + + expect(isLive).toBe(true); + }); + }); }); diff --git a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts index 4fc11dd868b..1d3f3428ca9 100644 --- a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts @@ -71,8 +71,6 @@ describe('Sidechain interoperability store', () => { ); }); - // TODO: Tests are missing for status is ACTIVE, expectation: true - // TODO: Tests are missing for status is REGISTERED, expectation: true describe('isLive', () => { beforeEach(() => { ownChainAccountStoreMock.get.mockResolvedValue({ chainID: EMPTY_BYTES }); @@ -102,6 +100,21 @@ describe('Sidechain interoperability store', () => { expect(isLive).toBe(false); }); + // TODO [DONE]: Tests are missing for status is ACTIVE, expectation: true + // TODO [DONE]: Tests are missing for status is REGISTERED, expectation: true + it('should return true if status is ACTIVE or REGISTERED', async () => { + for (const status of [ChainStatus.ACTIVE, ChainStatus.REGISTERED]) { + await chainDataSubstore.set(context, chainID, { + ...chainAccount, + status, + }); + const isLive = await sidechainInteroperabilityInternalMethod.isLive(context, chainID); + + expect(isLive).toBe(true); + } + }); + + // TODO [DONE]: For clarity, it would be good to change this description to "should return true if chain account and terminated chain account do not exist". it('should return true if chain account and terminated chain account do not exist', async () => { const isLive = await sidechainInteroperabilityInternalMethod.isLive(context, chainID); From 04623ece0ccba878fe5dafcd9f59cf7bb61f9e4e Mon Sep 17 00:00:00 2001 From: Franco NG Date: Tue, 28 Nov 2023 11:02:02 +0100 Subject: [PATCH 4/5] Update internal_method.spec.ts according to comments --- .../interoperability/internal_method.spec.ts | 87 +++++++++++-------- 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/framework/test/unit/modules/interoperability/internal_method.spec.ts b/framework/test/unit/modules/interoperability/internal_method.spec.ts index 92d5f663b34..f118910897a 100644 --- a/framework/test/unit/modules/interoperability/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/internal_method.spec.ts @@ -299,9 +299,10 @@ describe('Base interoperability internal method', () => { // TODO: [DONE] to test that the channel substore was updated - await expect(outboxRootSubstore.get(methodContext, chainID)).resolves.toEqual({ - root: updatedOutboxTree.root, - }); + const { outbox } = await channelDataSubstore.get(methodContext, chainID); + expect(outbox.size).toBe(outboxTree.size + 1); + expect(outbox.root).toStrictEqual(updatedOutboxTree.root); + expect(outbox.appendPath).toStrictEqual(updatedOutboxTree.appendPath); }); }); @@ -762,8 +763,7 @@ describe('Base interoperability internal method', () => { certificate: codec.encode(certificateSchema, defaultCertificate), }; - // TODO: [DONE] Use defaultCertificate - await interopMod.stores.get(ChainAccountStore).set(storeContext, ccuParams.sendingChainID, { + const certificate = { lastCertificate: { height: defaultCertificate.height, stateRoot: defaultCertificate.stateRoot, @@ -772,27 +772,23 @@ describe('Base interoperability internal method', () => { }, name: 'chain1', status: 1, - }); + }; + // TODO: [DONE] Use defaultCertificate + await interopMod.stores + .get(ChainAccountStore) + .set(storeContext, ccuParams.sendingChainID, certificate); await mainchainInteroperabilityInternalMethod.updateCertificate(methodContext, ccu); - const updatedChainAccount = { - lastCertificate: { - height: defaultCertificate.height, - stateRoot: defaultCertificate.stateRoot, - timestamp: defaultCertificate.timestamp, - validatorsHash: defaultCertificate.validatorsHash, - }, - }; expect(interopMod.stores.get(ChainAccountStore).set).toHaveBeenCalledWith( expect.anything(), ccu.sendingChainID, - expect.objectContaining(updatedChainAccount), + certificate, ); expect(interopMod.events.get(ChainAccountUpdatedEvent).log).toHaveBeenCalledWith( expect.anything(), ccu.sendingChainID, - expect.objectContaining(updatedChainAccount), + certificate, ); expect(validator.validate).toHaveBeenCalledWith( certificateSchema, @@ -882,7 +878,7 @@ describe('Base interoperability internal method', () => { Buffer.from([0, 0, 3, 0]), ], bftWeightsUpdate: [BigInt(1), BigInt(3), BigInt(4), BigInt(3)], - bftWeightsUpdateBitmap: Buffer.from([1, 0, 0, 0, 0, 0, 0, 0]), + bftWeightsUpdateBitmap: Buffer.from([0, 7]), }, }; @@ -892,9 +888,9 @@ describe('Base interoperability internal method', () => { certificateThreshold: BigInt(1), }); - const expectedBitmapLength = - BigInt(ccu.activeValidatorsUpdate.blsKeysUpdate.length + activeValidators.length + 7) / - BigInt(8); + // the bitmap for 1 active validator and 3 new validators fits into one byte + const expectedBitmapLength = 1; + await expect( mainchainInteroperabilityInternalMethod.verifyValidatorsUpdate(methodContext, ccu), ).rejects.toThrow(`Invalid bftWeightsUpdateBitmap. Expected length ${expectedBitmapLength}.`); @@ -1343,7 +1339,7 @@ describe('Base interoperability internal method', () => { }); describe('verifyCertificate', () => { - // TODO: Use correct length + // TODO: [DONE] Use correct length const txParams: CrossChainUpdateTransactionParams = { certificate: Buffer.alloc(0), activeValidatorsUpdate: { @@ -1377,12 +1373,12 @@ describe('Base interoperability internal method', () => { await interopMod.stores .get(ChainValidatorsStore) .set(methodContext, txParams.sendingChainID, { - certificateThreshold: BigInt(99), + certificateThreshold: BigInt(txParams.certificateThreshold), activeValidators: [], }); }); - // TODO: [DONE] However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this. + // TODO: [DONE] However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this it('should reject when certificate height is lower than last certificate height', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1456,13 +1452,24 @@ describe('Base interoperability internal method', () => { // (2): there is a proper validators update in the CCU // TODO: Replace by: chainAccount(ccu.params.sendingChainID) should exist // TODO: [DONE] 1. (1) is fulfilled, Expectation: verifyCertificate passes. - // TODO: 2. (1) not fulfilled, (2) fulfilled, Expectation: verifyCertificate passes - // e.g. chainAccount does exist, otherwise it will throw error when calling .get - it('should resolve when certificate is valid', async () => { + // TODO: [DONE] 2. (1) not fulfilled, (2) fulfilled, Expectation: verifyCertificate passes + it('should resolve when validatorsHash in certificate and state store are equal', async () => { const certificate: Certificate = { ...defaultCertificate, timestamp: 1000, }; + + await interopMod.stores.get(ChainAccountStore).set(methodContext, txParams.sendingChainID, { + lastCertificate: { + height: 100, + timestamp: 10, + stateRoot: cryptoUtils.getRandomBytes(HASH_LENGTH), + validatorsHash: defaultCertificate.validatorsHash, + }, + name: 'rand', + status: 0, + }); + const encodedCertificate = codec.encode(certificateSchema, certificate); await expect( mainchainInteroperabilityInternalMethod.verifyCertificate( @@ -1480,7 +1487,7 @@ describe('Base interoperability internal method', () => { ); }); - it('should resolve when validatorsHash in certificate and state store are equal', async () => { + it('should resolve when validatorsHash are NOT equal, but validators are updated', async () => { const certificate: Certificate = { ...defaultCertificate, timestamp: 1000, @@ -1491,7 +1498,7 @@ describe('Base interoperability internal method', () => { height: 100, timestamp: 10, stateRoot: cryptoUtils.getRandomBytes(HASH_LENGTH), - validatorsHash: defaultCertificate.validatorsHash, + validatorsHash: cryptoUtils.getRandomBytes(HASH_LENGTH), }, name: 'rand', status: 0, @@ -1502,7 +1509,13 @@ describe('Base interoperability internal method', () => { mainchainInteroperabilityInternalMethod.verifyCertificate( methodContext, { - ...txParams, + ...{ + ...txParams, + activeValidatorsUpdate: { + ...txParams.activeValidatorsUpdate, + bftWeightsUpdateBitmap: Buffer.alloc(1), + }, + }, certificate: encodedCertificate, }, 1001, @@ -1513,11 +1526,9 @@ describe('Base interoperability internal method', () => { expect.toBeObject() as Certificate, ); }); - - it('should resolve when validatorsHash are NOT equal, but validators are updated', async () => {}); }); - // TODO: test where the validator list in the validators store is NOT sorted + // TODO: [DONE] test where the validator list in the validators store is NOT sorted describe('verifyCertificateSignature', () => { const activeValidators = [ { blsKey: cryptoUtils.getRandomBytes(48), bftWeight: BigInt(1) }, @@ -1609,7 +1620,11 @@ describe('Base interoperability internal method', () => { .get(ChainValidatorsStore) .set(methodContext, txParams.sendingChainID, { ...chainValidators, - activeValidators: [...activeValidators].sort(() => Math.random() - 0.5), // Shuffle activeValidators + activeValidators: [ + activeValidators[activeValidators.length - 1], + ...activeValidators.slice(1, activeValidators.length - 1), + activeValidators[0], + ], }); await expect( @@ -1882,7 +1897,7 @@ describe('Base interoperability internal method', () => { ...crossChainUpdateParams, inboxUpdate: { crossChainMessages: [], - messageWitnessHashes: [], + messageWitnessHashes: [cryptoUtils.getRandomBytes(32)], outboxRootWitness: { bitmap: Buffer.alloc(0), siblingHashes: [], @@ -1921,8 +1936,10 @@ describe('Base interoperability internal method', () => { // TODO [DONE]: Explicitly state: outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)]) // outboxKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_OUTBOX_ROOT + sha256(OWN_CHAIN_ID) + // https://github.com/LiskHQ/lips/blob/main/proposals/lip-0053.md#verifypartnerchainoutboxroot const outboxKey = Buffer.concat([ - interopMod.stores.get(OutboxRootStore).key, + Buffer.from('83ed0d25', 'hex'), + Buffer.from('0000', 'hex'), cryptoUtils.hash(ownChainAccount.chainID), ]); expect(SparseMerkleTree.prototype.verify).toHaveBeenCalledWith( From 8e3b944715bf450e46b6629bb3af88c6a5f7d7ea Mon Sep 17 00:00:00 2001 From: Franco NG Date: Wed, 29 Nov 2023 13:25:00 +0100 Subject: [PATCH 5/5] Remove TODO Lines, added one more test case --- .../interoperability/internal_method.spec.ts | 66 +++++++++---------- .../mainchain/internal_method.spec.ts | 1 - .../sidechain/internal_method.spec.ts | 3 - 3 files changed, 31 insertions(+), 39 deletions(-) diff --git a/framework/test/unit/modules/interoperability/internal_method.spec.ts b/framework/test/unit/modules/interoperability/internal_method.spec.ts index f118910897a..7719564558f 100644 --- a/framework/test/unit/modules/interoperability/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/internal_method.spec.ts @@ -243,8 +243,6 @@ describe('Base interoperability internal method', () => { inbox: updatedInboxTree, }); - // TODO: [DONE] tree corresponding to chainID was updated - // TODO: [DONE] regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). const { inbox } = await channelDataSubstore.get(methodContext, chainID); expect(inbox.size).toBe(originalInbox.size + 1); @@ -273,9 +271,6 @@ describe('Base interoperability internal method', () => { outbox: updatedOutboxTree, }); - // TODO: [DONE] tree corresponding to chainID was updated - // TODO: [DONE] regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)). - const { outbox } = await channelDataSubstore.get(methodContext, chainID); expect(outbox.size).toBe(originalOutbox.size + 1); @@ -297,8 +292,6 @@ describe('Base interoperability internal method', () => { root: updatedOutboxTree.root, }); - // TODO: [DONE] to test that the channel substore was updated - const { outbox } = await channelDataSubstore.get(methodContext, chainID); expect(outbox.size).toBe(outboxTree.size + 1); expect(outbox.root).toStrictEqual(updatedOutboxTree.root); @@ -399,9 +392,6 @@ describe('Base interoperability internal method', () => { }, ); - // TODO: [DONE] Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED - // TODO: [DONE] Check the entry for the key chainID was removed from the outbox root substore. - expect(chainDataSubstore.set).toHaveBeenCalledWith(crossChainMessageContext, chainId, { ...chainAccount, status: ChainStatus.TERMINATED, @@ -428,18 +418,13 @@ describe('Base interoperability internal method', () => { initialized: true, }); - // TODO: [DONE] Check chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED expect(chainDataSubstore.set).toHaveBeenCalledWith(crossChainMessageContext, chainId, { ...chainAccount, status: ChainStatus.TERMINATED, }); - // TODO: [DONE] Check the entry for the key chainID was removed from the outbox root substore expect(outboxRootSubstore.del).toHaveBeenCalledWith(crossChainMessageContext, chainId); - // TODO: [DONE] Check an EVENT_NAME_CHAIN_ACCOUNT_UPDATED event was created expect(chainAccountUpdatedEvent.log).toHaveBeenCalled(); - - // TODO: [DONE] Check an EVENT_NAME_TERMINATED_STATE_CREATED event was created. expect(terminatedStateCreatedEvent.log).toHaveBeenCalled(); }); @@ -457,7 +442,6 @@ describe('Base interoperability internal method', () => { ), ).rejects.toThrow('Chain to be terminated is not valid'); - // TODO: [DONE] Check test that the corresponding terminated state account was NOT created. await expect(terminatedStateSubstore.has(crossChainMessageContext, chainIdNew)).resolves.toBe( false, ); @@ -576,7 +560,6 @@ describe('Base interoperability internal method', () => { ), ).toBeUndefined(); - // TODO: [DONE]called with correct arguments expect(mainchainInteroperabilityInternalMethod.sendInternal).toHaveBeenCalledWith( crossChainMessageContext, EMPTY_FEE_ADDRESS, @@ -773,7 +756,7 @@ describe('Base interoperability internal method', () => { name: 'chain1', status: 1, }; - // TODO: [DONE] Use defaultCertificate + await interopMod.stores .get(ChainAccountStore) .set(storeContext, ccuParams.sendingChainID, certificate); @@ -866,7 +849,6 @@ describe('Base interoperability internal method', () => { }); describe('verifyValidatorsUpdate', () => { - // TODO: [DONE] Test is missing where the length of bftWeightsUpdateBitmap is too large, e.g. bftWeightsUpdateBitmap be equal to Buffer.from([0], [7]) it('should reject if length of bftWeightsUpdateBitmap is too large', async () => { const ccu = { ...ccuParams, @@ -896,7 +878,6 @@ describe('Base interoperability internal method', () => { ).rejects.toThrow(`Invalid bftWeightsUpdateBitmap. Expected length ${expectedBitmapLength}.`); }); - // TODO: [DONE] Test the validator list returned by calculateNewActiveValidators is empty it('should reject if the validator list returned by calculateNewActiveValidators is empty', async () => { const ccu = { ...ccuParams, @@ -927,7 +908,6 @@ describe('Base interoperability internal method', () => { ); }); - // TODO: [DONE] Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries it('should reject if the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries', async () => { const ccu = { ...ccuParams, @@ -1290,7 +1270,6 @@ describe('Base interoperability internal method', () => { ); }); - // TODO: [DONE] In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments. it('should resolve if updates are valid', async () => { const ccu = { ...ccuParams, @@ -1339,7 +1318,6 @@ describe('Base interoperability internal method', () => { }); describe('verifyCertificate', () => { - // TODO: [DONE] Use correct length const txParams: CrossChainUpdateTransactionParams = { certificate: Buffer.alloc(0), activeValidatorsUpdate: { @@ -1378,7 +1356,6 @@ describe('Base interoperability internal method', () => { }); }); - // TODO: [DONE] However, there should be an additional test where the schema is not followed, e.g. by an incorrect length of a property, and verifyCertificate should fail due to this it('should reject when certificate height is lower than last certificate height', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1450,9 +1427,6 @@ describe('Base interoperability internal method', () => { // (1): validatorsHash in certificate and state store are equal // (2): there is a proper validators update in the CCU - // TODO: Replace by: chainAccount(ccu.params.sendingChainID) should exist - // TODO: [DONE] 1. (1) is fulfilled, Expectation: verifyCertificate passes. - // TODO: [DONE] 2. (1) not fulfilled, (2) fulfilled, Expectation: verifyCertificate passes it('should resolve when validatorsHash in certificate and state store are equal', async () => { const certificate: Certificate = { ...defaultCertificate, @@ -1487,7 +1461,7 @@ describe('Base interoperability internal method', () => { ); }); - it('should resolve when validatorsHash are NOT equal, but validators are updated', async () => { + it('should resolve when validatorsHash is NOT equal, but validators are updated', async () => { const certificate: Certificate = { ...defaultCertificate, timestamp: 1000, @@ -1528,7 +1502,6 @@ describe('Base interoperability internal method', () => { }); }); - // TODO: [DONE] test where the validator list in the validators store is NOT sorted describe('verifyCertificateSignature', () => { const activeValidators = [ { blsKey: cryptoUtils.getRandomBytes(48), bftWeight: BigInt(1) }, @@ -1912,7 +1885,6 @@ describe('Base interoperability internal method', () => { ), ).resolves.toBeUndefined(); - // TODO [DONE]: checked that calculateRootFromRightWitness is called with the correct arguments. expect(regularMerkleTree.calculateRootFromRightWitness).toHaveBeenCalledWith( channelData.inbox.size, channelData.inbox.appendPath, @@ -1934,7 +1906,6 @@ describe('Base interoperability internal method', () => { ), ).resolves.toBeUndefined(); - // TODO [DONE]: Explicitly state: outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)]) // outboxKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_OUTBOX_ROOT + sha256(OWN_CHAIN_ID) // https://github.com/LiskHQ/lips/blob/main/proposals/lip-0053.md#verifypartnerchainoutboxroot const outboxKey = Buffer.concat([ @@ -1961,7 +1932,6 @@ describe('Base interoperability internal method', () => { expect.toBeObject() as Certificate, ); - // TODO [DONE]: checked that calculateRootFromRightWitness is called with the correct arguments. expect(regularMerkleTree.calculateRootFromRightWitness).toHaveBeenCalledWith( updatedInboxTree.size, updatedInboxTree.appendPath, @@ -1969,8 +1939,34 @@ describe('Base interoperability internal method', () => { ); }); - // TODO: There should be some test(s) where inboxUpdate.crossChainMessages is non empty - // TODO: checked that regularMerkleTree.calculateMerkleRoot is called for every ccm and that it is called with the correct arguments - // TODO: i.e. calculateMerkleRoot must be called two times + it('should resolve correctly when crossChainMessages is non-empty', async () => { + jest.spyOn(SparseMerkleTree.prototype, 'verify').mockResolvedValue(false); + jest + .spyOn(regularMerkleTree, 'calculateRootFromRightWitness') + .mockReturnValue(channelData.partnerChainOutboxRoot); + + const params = { + ...crossChainUpdateParams, + inboxUpdate: { + crossChainMessages: [cryptoUtils.getRandomBytes(32), cryptoUtils.getRandomBytes(32)], + messageWitnessHashes: [cryptoUtils.getRandomBytes(32)], + outboxRootWitness: { + bitmap: Buffer.alloc(0), + siblingHashes: [], + }, + }, + certificate: Buffer.alloc(0), + }; + await expect( + mainchainInteroperabilityInternalMethod.verifyPartnerChainOutboxRoot( + commandExecuteContext as any, + params, + ), + ).resolves.toBeUndefined(); + + expect(regularMerkleTree.calculateMerkleRoot).toHaveBeenCalledTimes( + params.inboxUpdate.crossChainMessages.length, + ); + }); }); }); diff --git a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts index 98c8bbf91eb..e6530f7a545 100644 --- a/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/mainchain/internal_method.spec.ts @@ -145,7 +145,6 @@ describe('Mainchain interoperability internal method', () => { expect(isLive).toBe(false); }); - // TODO [DONE]: A test is missing for the case where the chain account exists, status is ACTIVE and liveness requirement IS fulfilled. it(`should return true when chain account exists, status is ${ChainStatus.ACTIVE} and liveness requirement IS fulfilled`, async () => { await chainDataSubstore.set(context, chainID, { ...chainAccount, diff --git a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts index 1d3f3428ca9..1b40e992e7f 100644 --- a/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts +++ b/framework/test/unit/modules/interoperability/sidechain/internal_method.spec.ts @@ -100,8 +100,6 @@ describe('Sidechain interoperability store', () => { expect(isLive).toBe(false); }); - // TODO [DONE]: Tests are missing for status is ACTIVE, expectation: true - // TODO [DONE]: Tests are missing for status is REGISTERED, expectation: true it('should return true if status is ACTIVE or REGISTERED', async () => { for (const status of [ChainStatus.ACTIVE, ChainStatus.REGISTERED]) { await chainDataSubstore.set(context, chainID, { @@ -114,7 +112,6 @@ describe('Sidechain interoperability store', () => { } }); - // TODO [DONE]: For clarity, it would be good to change this description to "should return true if chain account and terminated chain account do not exist". it('should return true if chain account and terminated chain account do not exist', async () => { const isLive = await sidechainInteroperabilityInternalMethod.isLive(context, chainID);