Soloseng/streamline-L2-validator-test #11263
Conversation
### Description Using test contract inheritance to minimize duplicated code during L2 testing ### Other changes removed unused var ### Tested unit tested ### Related issues - Fixes #[issue number here] ### Backwards compatibility _Brief explanation of why these changes are/are not backwards compatible._ ### Documentation _The set of community facing docs that have been added/modified because of this change_
This reverts commit b0058c5.
EpochManager is already set in the Registry in setupEpochManager.
Utils already inherits from TestConstants
TransitionToL2AfterL1 already inherits from ValidatorsTest.
packages/protocol/test-sol/unit/governance/validators/mocks/ValidatorsMockTunnel.sol
Show resolved
Hide resolved
packages/protocol/test-sol/unit/governance/validators/mocks/ValidatorsMockTunnel.sol
Show resolved
Hide resolved
| function test_ShouldSendValidatorPayment_WhenL2() public { | ||
| _whenL2WithEpoch(); | ||
| function test_ShouldNotTryToSendValidatorPayment() public { | ||
| assertDoesNotEmit(_performAffiliation, "SendValidatorPaymentCalled(address)"); |
There was a problem hiding this comment.
NIT: Should this reflect the actual event emitted in epochManager? The mock has a different event.
There was a problem hiding this comment.
So this event is used as a mocking mechanism: it signals that EpochManager.sendValidatorPayment was called during the transaction, without implementing any payment logic in the mock. So we test for its presence in the L2 test (to ensure the function was called) and for its absence here.
There was a problem hiding this comment.
Yes, I was suggesting changing it to ValidatorEpochPaymentDistributed() to minimize difference between mock and actual contract.
Either way works.
| _whenL2(); | ||
|
|
||
| address[] memory _elected = new address[](2); | ||
| _elected[0] = validator; | ||
| _elected[1] = otherValidator; | ||
| epochManager.initializeSystem(l1EpochNumber, block.number, _elected); | ||
| } | ||
|
|
||
| function _getEpochNumberBasedOnLayer() internal returns (uint256) { | ||
| if (isL2()) { | ||
| return epochManager.getCurrentEpochNumber(); | ||
| } else { | ||
| return IPrecompiles(address(validators)).getEpochNumber(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Shouldn't this be moved to some parent class?
There was a problem hiding this comment.
Actually, this logic is already implemented in PrecompilesOverride. Created a V2 version (with hardcoded registry, to avoid owner variable clash) and use that in this test.
| return result.unwrap(); | ||
| } | ||
| } | ||
|
|
||
| contract TransitionToL2AfterL1 is ValidatorsTest { |
There was a problem hiding this comment.
Shall we remove the duplicated code from Election and push to some util file?
Description
Updates the Validators tests to remove duplicate code and streamline L2 testing
Other changes (Martin)
recoverErrorString). This can be added to the ReleaseGold tunnel as well.test_ShouldNotTryToSendValidatorPayment_WhenL1was not correctly checking for the absence of an event emitted -vm.recordLogs()was missing. AddedassertDoesNotEmithelper to Utils that checks for the absence of a log. Also applies to:Tested
unit tested
Related issues