[WIP] Chain registrar #1064

Deniallugo · 2024-11-14T11:02:59Z

What ❔

Add Chain registrar contracts, that allow partners to register their chains and then configure their chain from L1

Why ❔

For making our registration process a bit more transparent and later on make it even easier

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.

Signed-off-by: Danil <[email protected]>

github-actions · 2024-11-14T12:41:23Z

Coverage after merging deniallugo-chain-registrator into release-v25-protocol-defense will be

85.95%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
contracts/bridge
L1ERC20Bridge.sol	81.82%	80%	75%	83.87%	62, 70, 70–71, 73–74
L1SharedBridge.sol	79.59%	66.23%	84.21%	83.41%	101–102, 109–110, 117–118, 125, 125–126, 133, 177, 190–191, 199–200, 212–213, 215–216, 227, 227, 227–231, 231–232, 234, 239–241, 241–242, 244–246, 246–247, 249, 261, 270, 270–271, 279–280, 290, 444–445, 447–448, 579–580, 596–597, 607–608, 623–624, 720–721, 962, 967
contracts/bridgehub
Bridgehub.sol	89.29%	74.07%	100%	91.49%	100–101, 112–113, 132–133, 155–156, 158–159, 332–333, 49, 63–64
contracts/chain-registrar
ChainRegistrar.sol	48.08%	11.11%	50%	56.76%	103–104, 106, 106–107, 114–115, 119–120, 126–127, 130, 130–131, 134–135, 139, 142–143, 68–70, 98–99
contracts/common
ReentrancyGuard.sol	90%	66.67%	100%	92.86%	78–79
contracts/common/libraries
L2ContractHelper.sol	42.86%	0%	50%	52.63%	25–26, 31–32, 35–36, 50, 52, 52–53, 57, 57–58, 66
SemVer.sol	100%	100%	100%	100%
UncheckedMath.sol	100%	100%	100%	100%
UnsafeBytes.sol	100%	100%	100%	100%
contracts/governance
ChainAdmin.sol	0%	0%	0%	0%	27, 27–28, 30, 32–33, 39–40, 47–48, 56, 56–57, 60, 62–63, 63, 66, 69, 78, 78–79, 81
Governance.sol	91.67%	94.74%	95%	89.86%	44, 44–45, 48, 50–51, 53–54
contracts/state-transition
StateTransitionManager.sol	59.48%	35.71%	50%	65.42%	101, 106–110, 116, 149–150, 152–153, 155–156, 158–159, 201, 203–204, 209, 211, 211–212, 215–217, 219–220, 255, 275, 289, 294, 299, 304, 309, 314, 319, 386, 386–387, 390, 455–456, 79, 92, 92–93
TestnetVerifier.sol	44.44%	33.33%	50%	50%	16, 16, 16, 32
ValidatorTimelock.sol	95.89%	83.33%	100%	95.83%	241, 82–83
Verifier.sol	89.88%	35.71%	96.30%	90.93%	1673–1674, 287–302, 305–308, 311–318, 321–328, 331–332, 335–336, 339, 384–385, 395–396, 406–407, 417–418, 428–429, 444–445, 454, 454–455, 904–905
contracts/state-transition/chain-deps
DiamondInit.sol	77.55%	45.45%	100%	86.11%	34–35, 37–38, 40–41, 43–44, 46–47, 71
DiamondProxy.sol	92.31%	75%	100%	100%	16, 27
contracts/state-transition/chain-deps/facets
Admin.sol	86.21%	72.73%	92.31%	87.30%	109, 109–110, 112–113, 178, 180, 83–84, 94–95
Executor.sol	82.04%	63.41%	84.38%	87.90%	137–138, 192, 197, 202, 207, 212, 217, 222, 227, 230–231, 235–236, 240–242, 244–245, 260–261, 280, 294–295, 361–362, 425, 447–449, 469, 48, 48–49, 519–520, 528–529, 549, 556–557, 57, 59, 59–60, 619, 62, 620, 63, 646–647, 696–697, 70, 700–701, 71, 74–75, 775
Getters.sol	92.08%	100%	90.24%	93.10%	153, 206, 72, 77
Mailbox.sol	100%	100%	100%	100%
ZkSyncHyperchainBase.sol	92.86%	85.71%	100%	92.86%	55–56
contracts/state-transition/libraries
Diamond.sol	93.33%	80.77%	100%	95.96%	109–110, 113, 115, 117, 120, 198–199, 316
LibMap.sol	100%	100%	100%	100%
Merkle.sol	100%	100%	100%	100%
PriorityQueue.sol	100%	100%	100%	100%
TransactionValidator.sol	94.44%	88.24%	100%	96%	66–67, 69–70
contracts/upgrades
BaseZkSyncUpgrade.sol	58.20%	27.27%	100%	60.23%	104, 104–105, 108, 111, 114–115, 126, 126–127, 130, 133, 136–137, 151–153, 171–173, 212–213, 215, 215–216, 232–233, 249–250, 252–253, 258–259, 259–260, 271–272, 278–279, 285–286, 293–294, 298–299, 308–309, 311–312, 75–76
BaseZkSyncUpgradeGenesis.sol	56.67%	14.29%	100%	68.18%	25, 25–26, 33–34, 40–41, 52–53, 62–63, 65–66
DefaultUpgrade.sol	100%	100%	100%	100%
GenesisUpgrade.sol	100%	100%	100%	100%
contracts/vendor
AddressAliasHelper.sol	100%	100%	100%	100%

Signed-off-by: Danil <[email protected]>

sanekmelnikov

have a feeling that will improve UX by x2 :)

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

sanekmelnikov · 2024-11-14T13:53:14Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        //        require(success);
+        //        address chainAdmin = bytesToAddress(returnData);
+        address chainAdmin = IGetters(diamondProxy).getAdmin();
+        address l2BridgeAddress = bridgehub.sharedBridge().l2BridgeAddress(chainId);


Hm, you can do that without specifying class?
IL1SharedBridge(bridgehub.sharedBridge()).l2BridgeAddress(chainId);

Yes, we can. Because it's already specified.

sanekmelnikov · 2024-11-14T13:54:04Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        emit NewChainDeployed(chainId, diamondProxy, author, chainAdmin);
+        emit SharedBridgeRegistered(chainId, l2BridgeAddress);


If these are always emitted together - any point of not putting l2BridgeAddress into NewChainDeployed?

I was thinking about it, but for the future those events, will be most likely independent. because it's two different actions and for not breaking the future interface I think it's better to make it like this from the very beginning

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

sanekmelnikov · 2024-11-14T13:54:52Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        //        (bool success, bytes memory returnData) = diamondProxy.call(abi.encodeWithSelector(IGetters.getAdmin.selector));
+        //        require(success);
+        //        address chainAdmin = bytesToAddress(returnData);
+        address chainAdmin = IGetters(diamondProxy).getAdmin();


just as alternative:
address chainAdmin = IStateTransitionManager(stm).getChainAdmin(chainId);

we need pendingAdmin... and IGetter is the only way

l1-contracts/deploy-scripts/DeployL2Contracts.sol

l1-contracts/deploy-scripts/RegisterHyperchain.s.sol

sanekmelnikov · 2024-11-14T14:11:44Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+            baseToken: BaseToken({
+                tokenAddress: tokenAddress,
+                tokenMultiplierSetter: tokenMultiplierSetter,
+                gasPriceMultiplierNominator: gasPriceMultiplierNominator,


maybe adding more validations on gasPriceMultiplierNominator, I'd say at min check both values are 1, 1 if base token is ETH

sanekmelnikov · 2024-11-14T14:17:40Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        uint128 gasPriceMultiplierNominator,
+        uint128 gasPriceMultiplierDenominator
+    ) public {
+        ChainConfig memory config = ChainConfig({


should we also think about returning base token once the registration is done ? :D
idk, if it makes sense to keep author there...

No, we need some token to operate on chain in the future

Signed-off-by: Danil <[email protected]>

Deniallugo · 2024-11-14T12:28:08Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+            revert ChainIsNotYetDeployed();
+        }
+        address diamondProxy = IStateTransitionManager(stm).getHyperchain(chainId);
+        (bool success, bytes memory returnData) = diamondProxy.call(abi.encodeWithSelector(IGetters.getAdmin.selector));


for some reasons only this way correctly work with tests, meanwhile all selectors are identical

Deniallugo · 2024-11-14T14:21:41Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        emit NewChainDeployed(chainId, diamondProxy, author, chainAdmin);
+        emit SharedBridgeRegistered(chainId, l2BridgeAddress);


I was thinking about it, but for the future those events, will be most likely independent. because it's two different actions and for not breaking the future interface I think it's better to make it like this from the very beginning

Deniallugo · 2024-11-14T15:16:34Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        uint128 gasPriceMultiplierNominator,
+        uint128 gasPriceMultiplierDenominator
+    ) public {
+        ChainConfig memory config = ChainConfig({


No, we need some token to operate on chain in the future

Deniallugo · 2024-11-14T15:17:37Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        //        (bool success, bytes memory returnData) = diamondProxy.call(abi.encodeWithSelector(IGetters.getAdmin.selector));
+        //        require(success);
+        //        address chainAdmin = bytesToAddress(returnData);
+        address chainAdmin = IGetters(diamondProxy).getAdmin();


we need pendingAdmin... and IGetter is the only way

Deniallugo · 2024-11-14T15:18:00Z

l1-contracts/contracts/chain-registrar/ChainRegistrar.sol

+        //        require(success);
+        //        address chainAdmin = bytesToAddress(returnData);
+        address chainAdmin = IGetters(diamondProxy).getAdmin();
+        address l2BridgeAddress = bridgehub.sharedBridge().l2BridgeAddress(chainId);


Yes, we can. Because it's already specified.

Signed-off-by: Danil <[email protected]>

Deniallugo added 8 commits November 11, 2024 22:00

add new chain registrator contract

9c3118c

Signed-off-by: Danil <[email protected]>

Add test

9fd56a3

Signed-off-by: Danil <[email protected]>

Make test passes

0589a9b

Signed-off-by: Danil <[email protected]>

Add deployer

ca8edff

Signed-off-by: Danil <[email protected]>

Fix registrator

0e9e30d

Signed-off-by: Danil <[email protected]>

Simplify propose chain signature

e4b0c85

Signed-off-by: Danil <[email protected]>

Simplify propose chain signature

1804692

Signed-off-by: Danil <[email protected]>

Finalize registration

9294bac

Signed-off-by: Danil <[email protected]>

Deniallugo requested review from vladbochok and StanislavBreadless as code owners November 14, 2024 11:03

Update initialization

5579d5a

Signed-off-by: Danil <[email protected]>

Deniallugo force-pushed the deniallugo-chain-registrator branch from 9cfe4de to 5579d5a Compare November 14, 2024 12:38

Deniallugo added 2 commits November 14, 2024 19:43

Fix reentranc

43b788e

Signed-off-by: Danil <[email protected]>

Change to call of get admin

9c148cc

Signed-off-by: Danil <[email protected]>

Deniallugo force-pushed the deniallugo-chain-registrator branch 2 times, most recently from 8b32b48 to 65c9acb Compare November 14, 2024 13:24

Fix lints

7610082

Signed-off-by: Danil <[email protected]>

Deniallugo force-pushed the deniallugo-chain-registrator branch from 65c9acb to 7610082 Compare November 14, 2024 13:45

sanekmelnikov reviewed Nov 14, 2024

View reviewed changes

Return correct admin

567abc5

Signed-off-by: Danil <[email protected]>

Deniallugo commented Nov 14, 2024

View reviewed changes

Deniallugo force-pushed the deniallugo-chain-registrator branch 3 times, most recently from 3883f46 to e10fc7a Compare November 14, 2024 16:18

Small refactoring

863a189

Signed-off-by: Danil <[email protected]>

Deniallugo force-pushed the deniallugo-chain-registrator branch from e10fc7a to 863a189 Compare November 14, 2024 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Chain registrar #1064

[WIP] Chain registrar #1064

Deniallugo commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024

sanekmelnikov left a comment

sanekmelnikov Nov 14, 2024

Deniallugo Nov 14, 2024

sanekmelnikov Nov 14, 2024

Deniallugo Nov 14, 2024

sanekmelnikov Nov 14, 2024

Deniallugo Nov 14, 2024

sanekmelnikov Nov 14, 2024

sanekmelnikov Nov 14, 2024

Deniallugo Nov 14, 2024

Deniallugo Nov 14, 2024

Deniallugo Nov 14, 2024

Deniallugo Nov 14, 2024

Deniallugo Nov 14, 2024

Deniallugo Nov 14, 2024

		emit NewChainDeployed(chainId, diamondProxy, author, chainAdmin);
		emit SharedBridgeRegistered(chainId, l2BridgeAddress);

[WIP] Chain registrar #1064

Are you sure you want to change the base?

[WIP] Chain registrar #1064

Conversation

Deniallugo commented Nov 14, 2024 • edited Loading

What ❔

Why ❔

Checklist

github-actions bot commented Nov 14, 2024

sanekmelnikov left a 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

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

Deniallugo commented Nov 14, 2024 •

edited

Loading