Closed Phanco closed 10 months ago
Merging #9085 (8e3b944) into release/6.1.0 (930492c) will increase coverage by
0.00%
. The diff coverage isn/a
.
The list below is copied from @AndreasKendziorra's #8201
Legend: 🟢: Updated 🔵: Resolved by other PRs/No longer needed 🔴: Ongoing
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
appendToOutboxTree(chainID, ccm)
should hold.
🟢 Updated
🔵 Tests have been performed here here
sendInternal
and createTerminatedStateAccount
are expected to be called, it should also be tested that they are called with the correct arguments.
🟢 Updated,
sendInternal
andcreateTerminatedStateAccount
are checked withtoHaveBeenCalledWith
In the first test, it was missed to check that
chainAccount(chainID).status
was set to CHAIN_STATUS_TERMINATED
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
chainID
was removed from the outbox root substoreEVENT_NAME_CHAIN_ACCOUNT_UPDATED
event was createdEVENT_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.
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
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 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
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
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 https://github.com/LiskHQ/lips/issues/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 https://github.com/LiskHQ/lips/issues/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
true
.
🟢: Test added
🟢: Test added
P.S. The TODO
tag will be removed after review
All comments have been replied and/or resolved, test also passed, feel free to take a look 🙇
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