Enhance test coverage for upgrade contracts #1240

saxenism · 2025-02-06T11:28:46Z

Replaces PR #496 to counter the failing CI check for coverage (in the post-coverage report step)

…t pass

…gramist/era-contracts into test/upgrade-contracts

StanislavBreadless · 2025-02-06T12:29:43Z

l1-contracts/test/foundry/unit/concrete/upgrades/BaseZkSyncUpgradeGenesis.t.sol

+    }
+
+    // Major version is not zero
+    function test_revertWhen_MajorVersionIsNotZero() public {


this file is very similar to the BaseUpgrade, is it possible to reuse the code for tests? The less code to maintain, the better

Yeah, it's similar in a sense but I still think it would be better to keep them as separate files for better maintainability and understanding.

If you disagree, we can open a PR later on to merge all such similar code/files

StanislavBreadless · 2025-02-06T12:32:10Z

l1-contracts/test/foundry/unit/concrete/upgrades/BaseZkSyncUpgradeGenesis.t.sol

+
+contract DummyBaseZkSyncUpgradeGenesis is BaseZkSyncUpgradeGenesis, BaseUpgradeUtils {}
+
+contract BaseZkSyncUpgradeGenesisTest is BaseUpgrade {


when I take a look inside the CI's test-foundry section, I can not see that these tests are ever run

Pushed them under the l1-contracts/tests/foundry/l1/*. Now they'll be run in the CI.

StanislavBreadless · 2025-02-06T12:34:21Z

l1-contracts/test/foundry/unit/concrete/upgrades/BaseZkSyncUpgrade.t.sol

+        vm.expectRevert(
+            abi.encodeWithSelector(
+                PreviousUpgradeNotFinalized.selector,
+                bytes32(proposedUpgrade.l2ProtocolUpgradeTx.factoryDeps[0])


the test logic seem to be wrong, it says "wrong factory deps", but in fact the upgrade expects to receive the factory dep in the error

Yeah, this test was targeted towards an earlier version (v-25). Changed it.

StanislavBreadless · 2025-02-10T09:17:43Z

l1-contracts/test/foundry/l1/upgrades/BaseZkSyncUpgrade.t.sol

+import {VerifierParams} from "contracts/state-transition/chain-interfaces/IVerifier.sol";
+import {MAX_NEW_FACTORY_DEPS, SYSTEM_UPGRADE_L2_TX_TYPE, MAX_ALLOWED_MINOR_VERSION_DELTA} from "contracts/common/Config.sol";
+import {SemVer} from "contracts/common/libraries/SemVer.sol";
+import {ProtocolVersionMinorDeltaTooBig, TimeNotReached, InvalidTxType, L2UpgradeNonceNotEqualToNewProtocolVersion, TooManyFactoryDeps, ProtocolVersionTooSmall, PreviousUpgradeNotFinalized, PreviousUpgradeNotCleaned, PreviousUpgradeNotFinalized, PatchCantSetUpgradeTxn, PreviousProtocolMajorVersionNotZero, NewProtocolMajorVersionNotZero, PatchUpgradeCantSetDefaultAccount, PatchUpgradeCantSetBootloader} from "contracts/upgrades/ZkSyncUpgradeErrors.sol";


nit: PreviousUpgradeNotFinalized listed twice

Removed one import.

StanislavBreadless · 2025-02-10T09:18:24Z

l1-contracts/test/foundry/l1/upgrades/BaseZkSyncUpgrade.t.sol

+    }
+
+    // Upgrade with mock factoryDepHash
+    function test_revertWhen_WrongFactoryDepHash() public {


this function does not test any revert

True. Changed the name.

StanislavBreadless · 2025-02-10T09:18:57Z

l1-contracts/test/foundry/l1/upgrades/BaseZkSyncUpgrade.t.sol

+    }
+
+    // Factory deps can be at most 64 (MAX_NEW_FACTORY_DEPS)
+    function test_revertWhen_FactoryDepsCanBeAtMost32(uint8 maxNewFactoryDeps) public {


the name is incorrect

github-actions · 2025-02-10T10:32:12Z

Coverage after merging saxenism-test-upgrade-contracts into dev will be

84.15%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
contracts/bridge
BridgeHelper.sol	76%	40%	100%	84.21%	29, 31, 34, 36, 39, 41
BridgedStandardERC20.sol	74.68%	25%	92.31%	77.59%	120–121, 126–127, 139–140, 163–164, 205, 205, 212, 212, 219, 219, 230, 62–63, 90–91
L1ERC20Bridge.sol	89.58%	66.67%	100%	91.43%	188–189, 207–208, 269
L1Nullifier.sol	77.97%	54.76%	86.21%	82.69%	115–116, 131, 131–132, 161–162, 222–223, 225–226, 235–236, 238–239, 248–249, 251–252, 418, 420–421, 421–422, 425–426, 426–427, 453–454, 519–520, 611–612, 649–652, 708, 711, 713, 726, 740, 745, 764–765
contracts/bridge/asset-router
AssetRouterBase.sol	90.24%	60%	100%	92.86%	58–59, 86–87
L1AssetRouter.sol	84.66%	62.50%	88.89%	89.23%	208–209, 245–247, 258, 260, 263, 361, 391–392, 435–437, 450–451, 553–554, 59–60, 654, 673, 75–76, 83–84
contracts/bridge/interfaces
AssetHandlerModifiers.sol	50%	0%	100%	50%	12–13
contracts/bridge/ntv
L1NativeTokenVault.sol	79.34%	62.07%	87.50%	84.21%	141, 144–145, 145, 145–147, 147, 147–149, 149, 149–150, 152, 207, 218, 220, 220, 220–221, 223, 236
NativeTokenVault.sol	83.87%	59.38%	92%	88.37%	101–102, 230–231, 235–236, 252–253, 270–271, 275–276, 290, 292, 310–311, 318–319, 483, 485, 499–500, 527–528, 562, 567, 73–74
contracts/bridgehub
Bridgehub.sol	77.65%	39.29%	93.18%	83.75%	115–116, 123–124, 130–131, 137, 137–138, 167, 182–183, 227–228, 230–231, 239–240, 249–250, 262–263, 277–278, 306–307, 330–331, 333–334, 399–400, 415–416, 446–447, 526–527, 608–609, 717–718, 722–723, 725–726, 730, 730–731, 735–736, 738–739, 778–779, 781–782, 796–797, 842–843, 845–846, 848–849, 883–884, 887–888, 890–891, 926, 931
CTMDeploymentTracker.sol	62.79%	0%	90%	69.23%	122–123, 128, 31–32, 39–40, 62–63, 89–90, 93–94, 97–98
MessageRoot.sol	88.89%	40%	100%	91.89%	121–122, 67–68, 88–89
contracts/chain-registrar
ChainRegistrar.sol	0%	0%	0%	0%	104, 113–115, 141, 155, 155–156, 159, 162, 162–163, 166, 169, 169–170, 172, 172–173, 177, 183–184, 191–192, 192–193, 196–200, 200–201, 204, 211
contracts/common
ReentrancyGuard.sol	90%	66.67%	100%	92.86%	78–79
contracts/common/libraries
DataEncoding.sol	78.26%	50%	100%	81.48%	110, 118, 143, 156, 163, 172, 174, 177, 43, 45
DynamicIncrementalMerkle.sol	74.42%	100%	80%	72.22%	67–70, 72–74, 76–78
FullMerkle.sol	100%	100%	100%	100%
L2ContractHelper.sol	69.81%	0%	100%	78.38%	100–101, 106–107, 110–111, 127–128, 132–133, 71–72, 77–78, 81–82
Merkle.sol	96.43%	85.71%	100%	97.73%	81–82
MessageHashing.sol	100%	100%	100%	100%
SemVer.sol	100%	100%	100%	100%
SystemContractsCaller.sol	0%	0%	0%	0%	114, 122–125, 135–138, 138–139, 141, 141–142, 33, 33–34, 37, 45, 47, 49, 51, 53, 66, 66, 66, 69, 72, 75, 78, 89, 91, 93, 96, 98
UncheckedMath.sol	100%	100%	100%	100%
UnsafeBytes.sol	100%	100%	100%	100%
contracts/governance
AccessControlRestriction.sol	100%	100%	100%	100%
ChainAdmin.sol	95.24%	80%	100%	96.30%	38–39
ChainAdminOwnable.sol	39.29%	0%	40%	47.37%	27–28, 39–40, 47–48, 56–57, 63, 66, 78, 78–79, 81
Governance.sol	98.15%	94.74%	100%	98.55%	45–46
L2ProxyAdminDeployer.sol	0%	100%	0%	0%	17–18, 20
PermanentRestriction.sol	83.45%	67.86%	100%	85.57%	103–104, 111, 111–112, 200, 200–201, 204, 204–205, 208, 210, 210–211, 240, 242, 289–290, 311–312, 342–343
TransitionaryOwner.sol	0%	100%	0%	0%	17, 22–23
contracts/governance/restriction
Restriction.sol	100%	100%	100%	100%
RestrictionValidator.sol	100%	100%	100%	100%
contracts/state-transition
ChainTypeManager.sol	72.12%	22.22%	71.43%	80.36%	147–148, 150–151, 153–154, 156–157, 212–213, 243–244, 268, 292, 311, 318, 325, 333, 340, 348, 355, 371, 373, 434–435, 462–463, 469–470, 496–497, 544–545, 79, 94–95
TestnetVerifier.sol	77.78%	66.67%	100%	75%	16, 28
ValidatorTimelock.sol	92.06%	71.43%	100%	93.02%	186–187, 202, 78–79
Verifier.sol	89.90%	40%	96.30%	90.93%	1674–1675, 287–302, 305–308, 311–318, 321–328, 331–332, 335–336, 339, 383–384, 394–395, 405–406, 416–417, 427–428, 443–444, 453, 453–454, 905–906
contracts/state-transition/chain-deps
DiamondInit.sol	80.43%	50%	100%	88.24%	39–40, 42–43, 45–46, 48–49, 73
DiamondProxy.sol	92.31%	75%	100%	100%	19, 30
GatewayCTMDeployer.sol	0%	0%	0%	0%	156, 161–163, 165, 167, 169, 177, 179–180, 182–183, 185, 205, 208–209, 211, 217, 221–222, 235, 235, 235–236,

StanislavBreadless · 2025-02-10T17:00:28Z

system-contracts/last_compilation_bootloader.timestamp

@@ -1 +1 @@
-1718185700858


just in case, could you please remove changes for timestamp? We don't use hardhat anymore so this thing is not needed. What's more, we probably shouldn't commit it even as it was intended for local compiler caching

That makes sense. Will make this change in the next PR.

tejks and others added 30 commits September 30, 2024 18:39

feat: Create BaseUpgrade setup and setter

cb406ea

feat: BaseZkSyncUpgrade tests

09dd298

feat: BaseZkSyncUpgradeGenesis tests

ad72ed6

feat: UpgradeHyperchains tests

ee013c9

feat: DefaultUpgrade + Upgrade_v1_4_1 tests

4394ff3

fix: Test factoryDeps

513c30a

feat: Fuzz testing for selected functions

e0366f1

feat: Check storage after upgrade

aa3e564

fix: Move tests to the upgrade folder

3ed5beb

fix: NewProtocolVersion fuzzing too many inputs

1b4286b

test: Update tests for semver protocol version

fe78057

feat: Update upgrade contracts to handle selector errors

635022a

fix: Correct typos

80ab825

fix: Update incorrect contract name

23be6f6

Merge branch 'dev' into test/upgrade-contracts

421e044

Merge branch 'dev' into test/upgrade-contracts

4e502fa

test commit

f32bddc

resolved issue of missing import files

42ebfe0

removed last refs of ZkSyncHyperchainBase

582ab22

removed/replaced deprecated vars

bb8a0da

commented out L1GatewayTests -- Integration

927a896

reran hashes

1c443ad

prettier

c444061

fixing comparison hash

2ebd663

fix lint/prettier, uncommented tests, Hashes match, locally every tes…

81b9645

…t pass

test revert removed. please work this time :(

daddfe1

added explicit permission to read from l1-contracts/script-out

603c4f0

updated hash

863d05e

resolved pr comments from stas

9d79667

ran lint check

92e43dc

saxenism added 16 commits February 6, 2025 15:07

removed/replaced deprecated vars

b695124

commented out L1GatewayTests -- Integration

1bf34ff

merged dev

0ce33ef

prettier

bcf0181

fixing comparison hash

4876a4f

fix lint/prettier, uncommented tests, Hashes match, locally every tes…

d2cb5a0

…t pass

test revert removed. please work this time :(

213434a

added explicit permission to read from l1-contracts/script-out

62bc12f

updated hash

f3f13fa

resolved pr comments from stas

1cb4151

ran lint check

9e26c49

skipping hash calculation for bootloader test

c2f12f1

lint fix

ae3c2ad

copied AllContractsHashes from dev

f9b7cf0

Merge branch 'test/upgrade-contracts' of https://github.com/neothepro…

4d2058b

…gramist/era-contracts into test/upgrade-contracts

ran lint:fix

425e01c

saxenism requested review from vladbochok and StanislavBreadless as code owners February 6, 2025 11:28

pushing updated compilation timestamp

1ad9c19

saxenism mentioned this pull request Feb 6, 2025

Enhance test coverage for upgrade contracts #496

Closed

3 tasks

StanislavBreadless reviewed Feb 6, 2025

View reviewed changes

modified wrong tests and moved them under the foundry directory

b27483f

StanislavBreadless reviewed Feb 10, 2025

View reviewed changes

saxenism added 2 commits February 10, 2025 15:18

Fixed PR review comments

de2dbfa

rebuilt

015f6d3

StanislavBreadless approved these changes Feb 10, 2025

View reviewed changes

saxenism merged commit 197d00f into dev Feb 11, 2025
22 checks passed

saxenism deleted the saxenism-test-upgrade-contracts branch February 11, 2025 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance test coverage for upgrade contracts #1240

Enhance test coverage for upgrade contracts #1240

saxenism commented Feb 6, 2025

StanislavBreadless Feb 6, 2025

saxenism Feb 7, 2025

StanislavBreadless Feb 6, 2025

saxenism Feb 7, 2025

StanislavBreadless Feb 6, 2025

saxenism Feb 7, 2025

StanislavBreadless Feb 10, 2025

saxenism Feb 10, 2025

StanislavBreadless Feb 10, 2025

saxenism Feb 10, 2025 •

edited

Loading

StanislavBreadless Feb 10, 2025

saxenism Feb 10, 2025

github-actions bot commented Feb 10, 2025

StanislavBreadless Feb 10, 2025

saxenism Feb 11, 2025


		contract DummyBaseZkSyncUpgradeGenesis is BaseZkSyncUpgradeGenesis, BaseUpgradeUtils {}

		contract BaseZkSyncUpgradeGenesisTest is BaseUpgrade {

		@@ -1 +1 @@
		1718185700858

Enhance test coverage for upgrade contracts #1240

Enhance test coverage for upgrade contracts #1240

Conversation

saxenism commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxenism Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saxenism Feb 10, 2025 •

edited

Loading