Unit test review: internal methods of interoperability module

[AndreasKendziorra]

The internal methods in the following three files were reviewed:

• internal_method.spec.ts
• mainchain/internal_method.spec.ts
• sidechain/internal_method.spec.ts

internal_method.spec.ts

appendToInboxTree and appendToOutboxTree

• Since the Merkle tree computation (regularMerkleTree.calculateMerkleRoot) was mocked, it would be good to test that (1) the tree corresponding to chainID was updated and (2) that regularMerkleTree.calculateMerkleRoot was called with the expected arguments (sha256(appendData)).

addToOutbox

• It was missed to test that the channel substore was updated. That means, the same expectations as for testing appendToOutboxTree(chainID, ccm) should hold.

sendInternal

• Tests are missing.

terminateChainInternal

• For the test where sendInternal and createTerminatedStateAccount are expected to be called, it should also be tested that they are called with the correct arguments.

createTerminatedStateAccount

• In the first test, it was missed to check that
  • chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED
  • the entry for the key chainID was removed from the outbox root substore.
• For the second test, the following expectations are missing:
  • chainAccount(chainID).status was set to CHAIN_STATUS_TERMINATED
  • the entry for the key chainID was removed from the outbox root substore
  • an EVENT_NAME_CHAIN_ACCOUNT_UPDATED event was created
  • an EVENT_NAME_TERMINATED_STATE_CREATED event was created.
• For the third test, it would good to test that the corresponding terminated state account was NOT created.

verifyCertificate

• For the properties blockID, stateRoot, validatorsHash and signature of a certificate, the correct length should be used in the setup. Otherwise, verifyCertificate should already fail due to schema validation. This must be fixed for every test for verifyCertificate.
  • 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.
• The setup for the test for "valid certificate" is a bit odd. Because the validators hash/update check seems to pass because chainAccount(ccu.params.sendingChainID) does not exist. And not because (1) validatorsHash in certificate and state store are equal or (2) there is a proper validators update in the CCU. Instead, we should replace this one by two other tests. In both tests, chainAccount(ccu.params.sendingChainID) should exist. And then
  • in the first one, (1) is fulfilled. Expectation: verifyCertificate passes.
  • in the secon one, (1) is not fulfilled but (2) is fulfilled. Expectation: verifyCertificate passes.

verifyCertificateSignature

• BUG: In this test, verifyWeightedAggSig should be expected to be called with the certificate threshold stored in the validators store for sendingChainID and not with txParams.certificateThreshold.
  • It should also be tested, that this event was emitted.
• It would be good to have an additional test where the validator list in the validators store is NOT sorted. The expectation should be that verifyWeightedAggSig is called with validatorKeys and weights from the SORTED validators list.

verifyValidatorsUpdate

• Test is missing where the length of bftWeightsUpdateBitmap is too large. For example, let the setup be as in this test, but let bftWeightsUpdateBitmap be equal to Buffer.from([0], [7]). Expectation: it should fail as stated in the LIP.
• Test is missing where the validator list returned by calculateNewActiveValidators is empty. Expectation: verifyValidatorsUpdate fails.
• Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries. Expectation: verifyValidatorsUpdate fails.
• In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments.

verifyPartnerChainOutboxRoot

• It doesn't hurt, but this test is a dubplication of this one.
• In this test or this one (or another one where calculateRootFromRightWitness gets mocked), it should be checked that calculateRootFromRightWitness is called with the correct arguments.
• It is very hard to see that outboxKey has the right value in this test. Could we write the value more explicitly like outboxKey = Buffer.concat([Buffer.from('83ed0d25', 'hex'), Buffer.from('0000', 'hex'), cryptoUtils.hash(OWN_CHAIN_ID)])? Note that LIP 0053 was until recently (or still is) falsely stating that the first part must be MODULE_NAME_INTEROPERABILITY instead of STORE_PREFIX_INTEROPERABILITY, and that the third part must use partnerchainID instead of OWN_CHAIN_ID. See also LIP0053: fix inclusion proof for outbox root lips#291. Note that there may be another issue in this repository for this particular issue.
• This value must be sha256(codec.encode(outboxRootSchema, { root: nextRoot })). Note that this was wrong in LIP 0053 as well. See LIP0053: fix inclusion proof for outbox root lips#291. Note that there may be another issue in this repository for this particular thing.
• There should be some test(s) where inboxUpdate.crossChainMessages is non empty, and where it is checked that regularMerkleTree.calculateMerkleRoot is called for every ccm and that it is called with the correct arguments. Maybe a test with two ccms. So, calculateMerkleRoot must be called two times. The first time with appendPath and size from the inbox in channel data store. The second time, appendPath and size must be derived from the return value of the first call of calculateMerkleRoot.

updateCertificate

• Couldn't this be improved to the following?

  {
      name: 'chain1',
      status: 1,
      lastCertificate: {
          height: certificate.height,
          stateRoot: certificate.stateRoot,
          timestamp: certificate.timestamp,
          validatorsHash: certificate.validatorsHash,
      },
  }

  The same here?

mainchain/internal_method.spec.ts

• A test is missing for the case where the chain account exists, status is ACTIVE and liveness requirement IS fulfilled. Expectation: returns true.

sidechain/internal_method.spec.ts

• Tests are missing for the case where the chain account exists and
  • status is ACTIVE. Expectation: returns true.
  • status is REGISTERED. Expectation: returns true.
• For clarity, it would be good to change this description to "should return true if chain account and terminated chain account do not exist".