Unit test review: PoS module (commission, reward)

[gkoumout]

NOTE: The review was done on Jan 16, therefore for each file it considers the version at that time (mainly while specifying line numbers). Since several modifications happened since then, a link to the version considered at the time of review is added.

commands/change_commission.spec.ts

https://github.com/LiskHQ/lisk-sdk/blob/99ee1c21b9503ebfd0a357f29fa1ebf5987227e5/framework/test/unit/modules/pos/commands/change_commission.spec.ts#L185

• Line 185: This check of newCommission > MAX_COMMISSION ("should not allow the commission to be set higher than 100%") could be better done for scenario where there is no other reason for error; concretely, for a validator with commission > 9500. This way newCommission - validatorDetails.Commission < MAX_COMMISSION_INCREASE_RATE (in the current case, newCommission - validatorDetails.Commission > MAX_COMMISSION_INCREASE_RATE, so there is another reason for error anyways).

commands/claim_rewards.spec.ts

• line 29: 'change commission command' should be changed to ''claim rewards command"

commands/stake.spec.ts

Version at time of review

• line 178-188: There is some repetition in code, not sure if that's intentional or not.
• line 239-242: The check should be checking that number of elements is no more than 2 * MAX_NUMBER_STAKING_SLOTS (called MAX_NUMBER_SENT_STAKES in /src/modules/pos/constants -- it would be good to align the name between the LIP and the code) than just 20, which is just the mainchain value. Therefore, in line 242, use Array(2 * MAX_NUMBER_STAKING_SLOTS + 1) instead of 21. Also in line 246 it would be nice to use a non-zero amount (e.g., liskToBeddows(10)) to make sure that the only error is due to the size of the stakes array.
• Similarly for the tests of lines 366 and 391, 10 should be replaced by MAX_NUMBER_STAKING_SLOTS. In particular, in lines 370 and 395 replace Array(11) by Array(MAX_NUMBER_STAKING_SLOTS + 1)
• After tests of lines 419 and 445, it could be checked also that trs.params.stakes does not include duplicate validators within negative amounts
• line 496: Again the check should be against the configurable constant BASE_STAKING_AMOUNT (called BASE_STAKE_AMOUNT in constants.ts --please align) instead of just 10^8. Same in line 520.
• test of lines 917-939 seems equivalent to the one of 941 - 952
• In the scenario described in line 788 (negative amounts leading to 0 entries) it would be good to check also that assignStakingRewards function is correctly called.
• line 1311: This check is the same as the one performed in line 366, therefore such a transaction (12 upstakes) would fail in the verification. It would be better to assume 7 ( = MAX_NUMBER_STAKING_SLOTS -3) sentStakes and then include 4 more stakes in the new transaction and test whether the correct exception will be raised.
• line 1372: The description is not aligned with the actual test: Initially we set 8 validators. The 2 negative stakes do not decrease the size of the stakerData.sentStakes array, since the negative stakes are -10 and the staked amounts are 20. Therefore the stakerData.sentStakes array has still size 8. Then with the 3 more stakes, it reaches 11 and an error is thrown.
  i) The negative votes could be completely deleted since they don't affect the size of the array.
  ii) Again, all constants should be around the config constant MAX_NUMBER_STAKING_SLOTS and not 10. So 8 should be replaced by MAX_NUMBER_STAKING_SLOTS-2.
• After the test case of lines 1372-1487, more tests of the same flavor could be performed. Examples: Same test as of line 1372 where indeed the 2 negative stakes are of amount -20 and therefore the length of stakerData.sentStakes array decreases from 8 to 6. Then, even after adding 3 positive stakes, the size is 9 and should be valid again. It could be nice to have a single transaction with those 5 stakes (3 positive and 2 negative -- in that order, first the positive and then the negative) to check if the command runs correctly.
• line 1492: Replace 19 by MAX_NUMBER_PENDING_UNLOCKS-1.
• line 1783: I guess amount: totalStakeReceived + senderStakeAmountPositive - senderStakeAmountNegative

commands/validator_registration.spec.ts

Version at time of review

• line 51: validatorRegistrationFee does not exist anymore on validatorRegistrationCommandParamsSchema. Test of line 301 should be modified accordingly.
• line 75: not sure is much important here, but to have the transaction not failing need the fee to be larger.
• line 143: This test checks invalid name in case of dis-allowed char set. Could also add tests for invalid name such as i) containing capital letters, (ii) having length > MAX_LENGTH_NAME or iii) name = " ".

commands/unlock.spec.ts

Version at time of review

• lines 212 - 218: This is a non-valid state since there are multiple entries in the sentStakes array with the same validatorAddress (not-sure this is an issue here).
• test starting in line 305: Scenarios 1 and 2 (validator1/unlockableObject, validator2/unlockableObject2) are ok and cover valid cases. Scenario 3 (validator3/unlockableObject3) is completely equivalent to scenario 2 and does not make sense to check it. I guess the intention was to set in line 327 pomHeights: [blockHeight - 1001] (therefore the text in line 323 becomes "pomHeight is less than unstakeHeight + ... ". This way, the sender waited the locking period but the PoM happened before it expired, therefore they can not unlock (and therefore unlockableObject3 will become non-unlockable and expected result should expect it to not be removed from pendingUnlocks). Scenario 4 (validator4/unlockableObject4) seems ok as well.
• line 414: initRounds should be at least 3, not sure if this will cause a problem for unit tests, but in general setting initRounds = 1 we get an invalid genesis state.
• test of line 471: One case is missing. I suggest adding an unlockable object with unstakeHeight: blockHeight - LOCKING_PERIOD_SELF_STAKES -2 and check also this case: the object is unlockable because pomHeights - unstakeHeight > LOCKING_PERIOD_SELF_STAKES.
• line 515: I am not sure this test properly covers the case that certificate is not generated, since the unstake seems unlockable due to the unstakeHeight. More generally, I suggest adding several extra tests regarding certificate generation (for example find 3-4 cases from the tests above where some objects are unlockable in case certificate is generated and check that they don't get unlocked in case certificate is not generated).

commands/report_misbehavior.spec.ts

Version at time of review

• line 289: I am not sure this test is correct. One needs to compare the height of the block including the transaction to header1.height and header2.height and make sure both differences are less than LOCKING_PERIOD_SELF_STAKING.
• I think it is not checked whether the updateValidatorEligibility function is called during execution

update_generator_key.spec.ts

• I think this command does not exist anymore in PoS module

endpoint.spec.ts

Version at time of review

• some endpoints do not exist in PoS LIP, it would be good to get more info on them
• line 347: getPendingUnlocks as specified in LIP does not specify whether it is unlockable or not and expected unlockable height, would be nice to align on this (and especially the computation of expected unlockable height in case of PoM).

module.spec.ts

Version at time of review

• line 60: Most of the constants of defaultConfig are configurable and would be better to import them from PoS constants (similar to COMMISSION_INCREASE_PERIOD and MAX_COMMISSION_INCREASE_RATE rather than fixing mainchain value.
• line 283: I guess there should be a separation between validators and non-validators here (see genesis state finalization part of LIP57); in particular the token method getLockedAmount can be used to count total stakes of non-validators; for validators, it should be replaced by the PoS method getLockedStakedAmount.
• line 451: Additional unit tests needed for updateValidators: Assume empty snapshot and a pre-defined set of initValidators. Then check the result output for height = ROUND_LENGTH * i for i such that initRounds < i < initRounds + numberActiveValidators.

genesis_block_test_data.ts

Version at time of review

• after test of lines 457-510 (inconsistent value of sharing coefficients) we could add a check of invalid token ID for some sharing coefficient of a staker (i.e., tokenID that is not included in validator's sharing coefficients). Conversely, if sharing coefficients array of staker contain only a subset of tokenIDs of the validators sharing coefficients array, that is totally valid.
• Missing test: All validators have commission < 10000.
• Missing test: All validators have lastCommissionIncreaseHeight < genesisHeight
• Missing test: for each element of stakers array, either sentStakes (called stakes in LIP) or pendingUnlocks is non-empty.
• Missing test: for all elements in sentStakes, pendingUnlocks, amount should be non-zero.
• Missing test: sentStakes (called stakes in LIP) has size at most MAX_NUMBER_STAKING_SLOTS.
• Missing test: all elements in pendingUnlocks array of a staker should have unstakeHeight < genesisHeight.
• Missing test: initRounds should always be at least MIN_INIT_ROUNDS.