Update interop internal methods Unit Test review

[Phanco]

What was the problem?

This PR resolves #8201

How was it solved?

Tests added/updated according to Unit Test Reviews

How was it tested?

CI Passed and coverage increased

[codecov]

Codecov Report

Merging #9085 (8e3b944) into release/6.1.0 (930492c) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

@@              Coverage Diff               @@
##           release/6.1.0    #9085   +/-   ##
==============================================
  Coverage          84.35%   84.36%           
==============================================
  Files                655      655           
  Lines              24113    24113           
  Branches            3497     3497           
==============================================
+ Hits               20341    20343    +2     
+ Misses              3772     3770    -2     

see 1 file with indirect coverage changes

[Phanco]

The list below is copied from @AndreasKendziorra's #8201

Legend:
🟢: Updated
🔵: Resolved by other PRs/No longer needed
🔴: Ongoing

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)).

🟢 Updated appendToInboxTree and appendToOutboxTree

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.

🟢 Updated

sendInternal

• Tests are missing.

🔵 Tests have been performed here here

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.

🟢 Updated, sendInternal and createTerminatedStateAccount are checked with toHaveBeenCalledWith

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.

🟢 Added additional Tests

• 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.

🟢 Added additional Tests

• For the third test, it would good to test that the corresponding terminated state account was NOT created.

🟢 Added additional Test

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.

🔵 Fixed by the global schema validation and calling check validator.validate here

• 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.

🔵 Fixed by the global schema validation

• 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.

🟢 Updated

• in the second one, (1) is not fulfilled but (2) is fulfilled. Expectation: verifyCertificate passes.

🟢 Updated

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 has been resolved previously, and the test for the event has also been added

• 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.

🟢 Test added

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.

🟢 Added

• Test is missing where the validator list returned by calculateNewActiveValidators is empty. Expectation: verifyValidatorsUpdate fails.

🟢 Added

• Test is missing where the validator list returned by calculateNewActiveValidators has more than MAX_NUM_VALIDATORS entries. Expectation: verifyValidatorsUpdate fails.

🟢 Added

• In at least one of the tests, it should be checked that calculateNewActiveValidators was called with the correct arguments.

🟢 Added

verifyPartnerChainOutboxRoot

• It doesn't hurt, but this test is a dubplication of this one.

🔵 The first test has already been removed

• 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.

🟢 Updated with correct checks

• 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.

🟢 Updated with comment

• 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.

🔵 It has been resolved, the value has been wrapped with sha256

• 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.

🟢: Updated here

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?

🟢: Updated to use defaultCertificate

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.

🟢: Test added

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".

🟢: Test added

[Phanco]

P.S. The TODO tag will be removed after review

[AndreasKendziorra]

All comments of the form "// TODO: [DONE]" should be removed

[AndreasKendziorra]

There is only this one comment with the question open and the comments of the form "// TODO" to remove.

[ishantiw]

Apart from a couple of minor comments and the comment from Andreas to remove TODO comments, LGTM.

[Phanco]

All comments have been replied and/or resolved, test also passed, feel free to take a look 🙇

[ishantiw]

Well done @Phanco 👏