Unit tests review for validators module (LIP 0044)

[AndreasKendziorra]

The tests in this folder were reviewed.

Endpoints

validateBLSKey

• for the test for existing BLS key, a valid proof of possession should be used for the given BLS key. Otherwise, the function may return false due to invalid proof of possession and not because the key exists already

Methods

registerValidatorKeys

• For some tests for which the function is expected to fail, it is tested that the substores does not contain the corresponding entries afterwards. For example here. For some other tests, (for example here) it is not done. It would be good to do it for every test where it is expected to fail.

setValidatorBLSKey

• This test should be improved by using a different BLS key in the validator account. Otherwise, it is not clear if the test should fail because the BLS key exists in the corresponding validator account in the validators substore or in the registered bls keys substore.
• We should have a test where a validator account with a valid BLS key is already stored in the validators substore, and setValidatorBLSKey is called for this account. In contrast to an old version of the LIP, this should NOT fail.
• For tests where the BLS keys have incorrect length, it would be good to set up a validator account with the given address in the validators substore. Otherwise, it may just fail because the account does not exist.

setValidatorGeneratorKey

• It would be good to have a test like this one, but where the new generator key is different from the existing one. And then it should be tested that the generator key in the substore was indeed updated. The existing test, where the new and the old generator key are equal, should be kept though.
• For the tests where the generator key has an incorrect length, it would be good to set up the validator account with corresponding address before. Otherwise, it may just fail because the account does not exist.

getGeneratorsBetweenTimestamps

• For this test:
  • It must also work for even stricter inputs, e.g. lowering the endTimestamp to timePerRound + 2 * blockTime.
  • The desciption "if ... difference between input timestamps is greater than one round" is not correct. It must be "if difference between timestamps is larger than or equal to the length of one round plus two block slots."
• For this test:
  • It must also work for even stricter inputs, e.g. lowering the endTimestamp to timePerRound * 2 + 2 * blockTime.
  • The desciption "if ... difference between input timestamps is greater than 2 rounds" is not correct. It must be "if difference between timestamps is larger than or equal to the length of two rounds plus two block slots."
• The description of this test is wrong because the timestamps are equal but not startSlotNumber and endSlotNumber
• it would be nice to have an additional test that checks which subset of validators is counted and which one is not. For example:
  • startTimestamp = 99 * blockTime
  • endTimestamp = 103 * blockTime
  • expected result: the result contains the 3 entries generatorList[99], generatorList[100] and generatorList[0] with count 1 for each entry

getValidatorKeys

• Maybe not relevant for the tests, but the generator key should have 32 bytes and the BLS key 48 bytes, and not the other way around.

registerValidatorWithoutBLSKey

• in this test, it should also be checked that setValidatorAccount.blsKey equals INVALID_BLS_KEY

setValidatorParams

There are no tests yet for this function. It should at least be tested that

• the validators params substore is updated correctly, and that
• the params are forwarded to the consensus domain

[has5aan]

setValidatorBLSKey

This test should be improved by using a different BLS key in the validator account. Otherwise, it is not clear if the test should fail because the BLS key exists in the corresponding validator account in the validators substore or in the registered bls keys substore.

The test description should not be able to set bls key for validator if address exists but bls key is already registered intends to target duplicate BLS key insertion scenario and usage of a different BLS key would not be correct setup for the scenario.

For tests where the BLS keys have incorrect length, it would be good to set up a validator account with the given address in the validators substore. Otherwise, it may just fail because the account does not exist.

The test cases explicitly test for a validation error to be thrown to be thorough.

setValidatorGeneratorKey

For the tests where the generator key has an incorrect length, it would be good to set up the validator account with corresponding address before. Otherwise, it may just fail because the account does not exist.

The test cases explicitly test for a validation error to be thrown to be thorough.

getValidatorKeys

Maybe not relevant for the tests, but the generator key should have 32 bytes and the BLS key 48 bytes, and not the other way around.

This has been updated in a previous commit.

[AndreasKendziorra]

The test description should not be able to set bls key for validator if address exists but bls key is already registered intends to target duplicate BLS key insertion scenario and usage of a different BLS key would not be correct setup for the scenario.

My interpretation of the scenario was that the BLS key is used already by another account, and therefore it should fail. But if this test is about the scenario that this account uses the provided BLS key, then it is correct. But then I would say another test is needed for the scenario I described.

[has5aan]

The test description should not be able to set bls key for validator if address exists but bls key is already registered intends to target duplicate BLS key insertion scenario and usage of a different BLS key would not be correct setup for the scenario.

My interpretation of the scenario was that the BLS key is used already by another account, and therefore it should fail. But if this test is about the scenario that this account uses the provided BLS key, then it is correct. But then I would say another test is needed for the scenario I described.

The code logic throws and logs duplicate event, if an entry already exists in the BLSKeyStore regardless of the address stored against the BLS key, and the test case covers the general scenario of existing BLS key, specifically testing the clause.