From faaedad373c21fd2d3abc12ba04abc582b78b2c4 Mon Sep 17 00:00:00 2001 From: Albert Llimos <53186777+albert-llimos@users.noreply.github.com> Date: Mon, 12 Feb 2024 21:30:26 +0100 Subject: [PATCH] chore: remove _lastValidateTime initialization in constructor (#493) * chore: remove _lastValidateTime initialization in constructor * chore: update stateful tests * chore: small update * chore: reintroduce nzKey * chore: fix flakyness --- contracts/KeyManager.sol | 1 - .../keyManager/test_setKey_setKey.py | 18 ------------------ tests/stateful/test_all.py | 4 ++-- tests/stateful/test_keyManager.py | 2 +- tests/stateful/test_upgradability.py | 4 ++-- .../keyManager/test_keyManager_constructor.py | 2 +- .../keyManager/test_setAggKeyWithGovKey.py | 9 +++------ tests/unit/vault/test_executeActions.py | 10 +++++----- 8 files changed, 14 insertions(+), 36 deletions(-) diff --git a/contracts/KeyManager.sol b/contracts/KeyManager.sol index 1579326a..2f6c8ba6 100644 --- a/contracts/KeyManager.sol +++ b/contracts/KeyManager.sol @@ -32,7 +32,6 @@ contract KeyManager is SchnorrSECP256K1, Shared, IKeyManager { _aggKey = initialAggKey; _govKey = initialGovKey; _commKey = initialCommKey; - _lastValidateTime = block.timestamp; } ////////////////////////////////////////////////////////////// diff --git a/tests/integration/keyManager/test_setKey_setKey.py b/tests/integration/keyManager/test_setKey_setKey.py index 98f9ee42..2ff93ff3 100644 --- a/tests/integration/keyManager/test_setKey_setKey.py +++ b/tests/integration/keyManager/test_setKey_setKey.py @@ -30,15 +30,6 @@ def test_setAggKeyWithAggKey_setAggKeyWithAggKey(cf): def test_setGovKeyWithGovKey_setAggKeyWithGovKey(cf): - # Changing the agg key with the gov key should fail if the delay hasn't been long enough yet - # No time has passed since the constructor has set the first value of _lastValidateTime - with reverts(REV_MSG_DELAY): - cf.keyManager.setAggKeyWithGovKey( - AGG_SIGNER_2.getPubData(), {"from": cf.GOVERNOR_2} - ) - - chain.sleep(AGG_KEY_TIMEOUT) - # Change the gov key setGovKeyWithGovKey_test(cf) @@ -63,15 +54,6 @@ def test_setGovKeyWithGovKey_setAggKeyWithGovKey(cf): # Check that _withAggKey calls update the _lastValidateTime def test_setAggKeyWithGovKey_setKeyWithAggKey(cf): - # Changing the agg key with the gov key should fail if the delay hasn't been long enough yet - # No time has passed since the constructor has set the first value of _lastValidateTime - with reverts(REV_MSG_DELAY): - cf.keyManager.setAggKeyWithGovKey( - AGG_SIGNER_2.getPubData(), {"from": cf.GOVERNOR} - ) - - chain.sleep(AGG_KEY_TIMEOUT) - setAggKeyWithAggKey_test(cf) # Reverts due to setAggKeyWithAggKey updating the _lastValidateTime diff --git a/tests/stateful/test_all.py b/tests/stateful/test_all.py index 6414faa5..c22b3281 100644 --- a/tests/stateful/test_all.py +++ b/tests/stateful/test_all.py @@ -235,7 +235,7 @@ def setup(self): self.v_suspended = self.v.getSuspendedState() # KeyManager - self.lastValidateTime = self.deployerContract.tx.timestamp + self.lastValidateTime = 0 self.keyIDToCurKeys = {AGG: AGG_SIGNER_1} self.allKeys = [*self.keyIDToCurKeys.values()] + ( [Signer.gen_signer(None, {})] @@ -2123,7 +2123,7 @@ def rule_upgrade_keyManager(self, st_sender): self._updateBalancesOnUpgrade(self.km, newKeyManager) self.km = newKeyManager - self.lastValidateTime = self.km.tx.timestamp + self.lastValidateTime = 0 # Deploys a new Vault and transfers the funds from the old Vault to the new one def rule_upgrade_Vault(self, st_sender, st_sleep_time): diff --git a/tests/stateful/test_keyManager.py b/tests/stateful/test_keyManager.py index e068d1bd..50798332 100644 --- a/tests/stateful/test_keyManager.py +++ b/tests/stateful/test_keyManager.py @@ -31,7 +31,7 @@ def __init__(cls, a, cfDeploy): # Reset the local versions of state to compare the contract to after every run def setup(self): - self.lastValidateTime = self.deployerContract.tx.timestamp + self.lastValidateTime = 0 self.keyIDToCurKeys = {AGG: AGG_SIGNER_1} self.allKeys = [*self.keyIDToCurKeys.values()] + ( [Signer.gen_signer(None, {})] diff --git a/tests/stateful/test_upgradability.py b/tests/stateful/test_upgradability.py index 91a38c15..c2293bf5 100644 --- a/tests/stateful/test_upgradability.py +++ b/tests/stateful/test_upgradability.py @@ -50,7 +50,7 @@ def setup(self): self.v = self.orig_v self.km = self.orig_km - self.lastValidateTime = self.deployerContract.tx.timestamp + self.lastValidateTime = 0 self.numTxsTested = 0 # StateChainGateway @@ -104,7 +104,7 @@ def rule_upgrade_keyManager(self, st_sender): assert aggKeyNonceConsumer.getKeyManager() == newKeyManager self.km = newKeyManager - self.lastValidateTime = self.km.tx.timestamp + self.lastValidateTime = 0 # Deploys a new Vault and transfers the funds from the old Vault to the new one def rule_upgrade_Vault( diff --git a/tests/unit/keyManager/test_keyManager_constructor.py b/tests/unit/keyManager/test_keyManager_constructor.py index 9805d4ba..6e8061fc 100644 --- a/tests/unit/keyManager/test_keyManager_constructor.py +++ b/tests/unit/keyManager/test_keyManager_constructor.py @@ -6,4 +6,4 @@ def test_constructor(a, cf): assert cf.keyManager.getAggregateKey() == AGG_SIGNER_1.getPubDataWith0x() assert cf.keyManager.getGovernanceKey() == cf.GOVERNOR - assert cf.keyManager.getLastValidateTime() == cf.deployerContract.tx.timestamp + assert cf.keyManager.getLastValidateTime() == 0 diff --git a/tests/unit/keyManager/test_setAggKeyWithGovKey.py b/tests/unit/keyManager/test_setAggKeyWithGovKey.py index e5cf8c50..7cb24ab5 100644 --- a/tests/unit/keyManager/test_setAggKeyWithGovKey.py +++ b/tests/unit/keyManager/test_setAggKeyWithGovKey.py @@ -6,15 +6,12 @@ def test_setAggKeyWithGovKey(cf): - chain.sleep(AGG_KEY_TIMEOUT) setAggKeyWithGovKey_test(cf) -def test_setAggKeyWithGovKey_rev_time(cf): - with reverts(REV_MSG_DELAY): - cf.keyManager.setAggKeyWithGovKey( - cf.keyManager.getAggregateKey(), {"from": cf.GOVERNOR} - ) +def test_setAggKeyWithGovKey_delay_success(cf): + chain.sleep(AGG_KEY_TIMEOUT) + setAggKeyWithGovKey_test(cf) def test_setAggKeyWithGovKey_rev_governor(cf): diff --git a/tests/unit/vault/test_executeActions.py b/tests/unit/vault/test_executeActions.py index 3e34b6e4..b172d010 100644 --- a/tests/unit/vault/test_executeActions.py +++ b/tests/unit/vault/test_executeActions.py @@ -424,8 +424,8 @@ def test_executeActions_revgas(cf, multicall): # Gas limit that doesn't allow the Multicall to execute the actions but leaves # enough gas to trigger "Vault: gasMulticall too low". Succesfull tx according # to gas test is ~140k but it doesn't succeed until gas_limit is not at least - # 180k (without the gas check). Then from 190k to 210k, when adding the gas check, - # it reverts with "Vault: gasMulticall too low". After 210k it will succeed as normal + # 180k (without the gas check). Then from 190k to 220k, when adding the gas check, + # it reverts with "Vault: gasMulticall too low". After 220k it will succeed as normal gas_limit = 200000 # Reverted with empty revert string is to catch the invalid opcode @@ -437,7 +437,7 @@ def test_executeActions_revgas(cf, multicall): {"from": cf.DENICE, "gas_limit": gas_limit}, ) - gas_limit = 210000 + gas_limit = 220000 tx = cf.vault.executeActions( sigData, @@ -448,7 +448,7 @@ def test_executeActions_revgas(cf, multicall): @given( - st_gas_limit=strategy("uint256", min_value=80000, max_value=250000), + st_gas_limit=strategy("uint256", min_value=100000, max_value=250000), ) def test_executeActions_gas(cf, multicall, st_gas_limit): print("gas limit", st_gas_limit) @@ -473,7 +473,7 @@ def test_executeActions_gas(cf, multicall, st_gas_limit): # Exact gas limit that makes the transaction have enough gas to pass the # gas check, execute the actions and succeeed. - cutoff_gas_limit = 202288 + cutoff_gas_limit = 219452 # On low gas_limit values it will revert with not enough gas error and other # error such as no reason string. Arbitrary 80k under the cutoff gas limit