[Multi ZNS] Multi zNS Prototype [WIP] #118

Whytecrowe · 2024-09-19T21:51:03Z

rc/multi-zns-main needs to be merged into this branch to update the code with all the previous changes !!!

openzeppelin-code · 2024-09-19T21:54:35Z

[Multi ZNS] Multi zNS Prototype [WIP]

Generated at commit: b2701971cdbfa8f132fc0a3f8364b29051674286

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 2 0 2 20 27
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

… code

This reverts commit 6dce79b.

This reverts commit 33976c4.

JamesEarle · 2024-12-18T19:53:47Z

contracts/cross-chain/IZNSZChainPortal.sol

+
+    function destZnsPortal() external view returns (address);
+
+    function rootRegistrar() external view returns (IZNSRootRegistrarTrunk);


Consider moving this and a few of the other items that are shared between this contract and IZNSEthereumPortal to a higher level IZNSPortal, to avoid code duplication

JamesEarle · 2024-12-18T19:56:30Z

contracts/cross-chain/ZNSZChainPortal.sol

+import { IZNSRootRegistrarTrunk } from "../registrar/IZNSRootRegistrarTrunk.sol";
+import { IZNSSubRegistrarTrunk } from "../registrar/IZNSSubRegistrarTrunk.sol";
+import { IZNSTreasury } from "../treasury/IZNSTreasury.sol";
+import { PaymentConfig } from "../treasury/IZNSTreasury.sol";


same import file as what's used above, just do import { X, Y } from ....

contracts/cross-chain/ZNSZChainPortal.sol

JamesEarle · 2024-12-18T20:07:40Z

contracts/cross-chain/ZNSZChainPortal.sol

+        polygonZkEVMBridge.bridgeMessage(
+            destNetworkId,
+            destZnsPortal,
+            // TODO multi: figure out what this is and how to better pass it !!!


is this comment in reference to a similar comment for forceGlobalExitRoot above on L74?

JamesEarle · 2024-12-18T20:10:46Z

contracts/cross-chain/ZNSZChainPortal.sol

A bit confusing having the opposite chain in the name of the contract to me. This is the portal to ZChain, but it is not the portal on ZChain, and calling it ZChainPortal gives the opposite implication

JamesEarle · 2024-12-18T20:18:38Z

contracts/cross-chain/IZNSZChainPortal.sol

+
+interface IZNSZChainPortal is IDistributionConfig {
+
+    struct ZNSContractInput {


You put this in a struct but the other contract has the same arguments, except for IZNSAccessController, might make more sense to use this struct as an initializer argument for both contracts and just have the one additional argument of the access controller contract be given in the ZNSZChainPortal contract

JamesEarle · 2024-12-18T20:26:37Z

contracts/cross-chain/ZNSZChainPortal.sol

+            );
+        }
+
+        // TODO multi: analyze how to make these strings better !!!


Enums?

enum ResolverTypes { CHAIN, ADDRESS, STRING }

the downside here is that every time this enum is extended, we need to upgrade all contracts that use it

JamesEarle · 2024-12-18T20:31:29Z

contracts/cross-chain/ZNSZChainPortal.sol

+            domainHash,
+            destChainId,
+            destChainName,
+            address(0),


Do we not care about setting znsRegistryOnChain?

JamesEarle · 2024-12-18T20:36:41Z

contracts/cross-chain/ZNSZChainPortal.sol

+        bytes32 parentHash
+    ) internal {
+        // TODO multi: do we actually want to stake on BOTH sides? What other payment option can we do ???!!!
+        // TODO multi: if we leave this option (stake as Portal address) this needs to be optimized!!!


Can we not include the registrant address and have the portal use stake payment on the registrant's behalf?

JamesEarle · 2024-12-18T21:33:41Z

contracts/cross-chain/mock/PolygonZkEVMBridgeV2Mock.sol

+import { IBridgeMessageReceiver } from "@zero-tech/zkevm-contracts/contracts/interfaces/IBridgeMessageReceiver.sol";
+import { ERC20Mock } from "../../token/mocks/ERC20Mock.sol";
+import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
+// import { PolygonZkEVMBridgeV2 } from "@zero-tech/zkevm-contracts/contracts/v2/PolygonZkEVMBridgeV2.sol";


delete unneeded comments

JamesEarle · 2024-12-18T21:37:24Z

contracts/cross-chain/mock/PolygonZkEVMBridgeV2Mock.sol

+            revert MessageFailed();
+        }
+
+        emit ClaimEvent(


Think of a better name for the events here and above. MessageClaimed and MessageBridged or just Claimed and Bridged would be better

This is a mock of a Polygon Bridge contract. the names here are Polygon names.

JamesEarle · 2024-12-18T22:23:22Z

src/environment/set-env.ts

+  });
+
+  // override from local .env file if anything is present
+  require("dotenv").config({ override: true });


This part might be doing nothing, because env vars are loaded at run time, when a script that uses this function is called, all the process.env vars are mapped, then we override those with the defaults above in the Object.entries loop

not sure what you mean. we use the defaults from the file on the line above this, then this one overrides all the default from the env file if present.
env vars from .env file are loaded whenever this line is called. or am I misunderstanding?

JamesEarle · 2024-12-18T22:28:13Z

src/environment/validate.ts

+export const findMissingEnvVars = () => {
+  const missing = required.filter(
+    key =>
+      process.env[key] === undefined || process.env[key] === ""


small nitpick, you often add line breaks at arbitrary points that don't really make sense to me, like here, where it clearly isn't going past the character limit specified by the linter, whereas above in the required array or below on L18 you aren't using the same rule

Ideally we just abide by the rules specified in the linter so that we're both always on the same page, and if you prefer to shorten the max line length enforced we can do that instead

this is just basic readability to me rather then thinking about line length. here I am separating function param and def from the function code on the next line. we can split here or not split, it's not a big deal, it was just more readable to me this way, so I decided to do it.
Also, shorter lines make it easier to read code on smaller screens.

JamesEarle · 2024-12-18T22:40:04Z

hardhat.config.ts

+        chainId: 2012605151,
+        urls: {
+          apiURL: "https://wilderworld-dev-erigon1-blockscout.eu-north-2.gateway.fm/api/",
+          browserURL: "https://wilderworld-dev-erigon1-blockscout.eu-north-2.gateway.fm/",


put these values in env vars

These are public links that users would use. is there a reason for make them env vars?

JamesEarle · 2024-12-19T16:49:35Z

contracts/registrar/ZNSSubRegistrarTrunk.sol

@@ -91,57 +92,48 @@ contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable,
        DistributionConfig calldata distrConfig,
        PaymentConfig calldata paymentConfig
    ) external override returns (bytes32) {
+        return _coreSubdomainRegister(


Here and in other places it would be nicer if the internal _core functions were consistent with the naming of the public callers as much as they can be, so _coreRegisterSubdomain, this is clearer as well as the name describes the action the function is taking

JamesEarle · 2024-12-19T19:19:09Z

contracts/registrar/ZNSRootRegistrarBase.sol

@@ -182,7 +166,7 @@ contract ZNSRootRegistrar is
            registry.createDomainRecord(args.domainHash, args.registrant, "address");

            IZNSAddressResolver(registry.getDomainResolver(args.domainHash))
-                .setAddress(args.domainHash, args.domainAddress);
+            .setAddress(args.domainHash, args.domainAddress);


For formatting like this where lines from a single function call have to broken into multiple for the sake of line length, it makes more sense to have the extra indentation here. It shows visually that this line is not independent, I would undo this change and any other similar changes

Agreed. no idea how this changed tbh :)

JamesEarle · 2024-12-19T19:30:56Z

contracts/registrar/ZNSSubRegistrarTrunk.sol

+                !mintlist[args.parentHash]
+            .list
+            [mintlist[args.parentHash].ownerIndex]
+            [msg.sender]


indent these for clarity

init-env.ts

Whytecrowe · 2025-04-07T22:28:36Z

contracts/registrar/ZNSRootRegistrarBase.sol

    using StringUtils for string;

    IZNSPricer public rootPricer;
    IZNSTreasury public treasury;
    IZNSDomainToken public domainToken;
-    IZNSSubRegistrar public subRegistrar;
+    IZNSSubRegistrarTrunk public subRegistrar;


why is this a Trunk contract? is this a mistake?..

Whytecrowe added 12 commits September 17, 2024 14:20

install zkevm contracts from local package

be93527

create test contracts

cc745d6

create deploy script base

295edbf

create base scripts for bridge and claim

3715535

fix networkId

7001729

create extension of the Bridge interface to be able to call networkID()

c2c4fec

fix test contracts

103276e

add zchaintest to HH config

c974173

install error decoder package from ethers and add script for easy run

7160586

modify test flow scripts

3351405

add Bridge json for easy contract access and use it's abi

85649b9

save deployed contract addresses from testnets

0397d93

Whytecrowe changed the base branch from rc/zns-meowchain to zns-zchain-final September 19, 2024 21:51

Whytecrowe added 9 commits September 19, 2024 16:45

install zkevm-contracts from the newly made package

df6bb6d

remove zk-evm contracts for now and comment out test bridge contracts…

1694feb

… code

comment out BridgeSender code temporarily

6ac127a

add ChainResolver prototype

bb42ff1

small type fix for test helpers

14ae190

add chainRootHash concept to zns contract code

6dce79b

add chainRootHash to ts code and tests

33976c4

comment out networks in hh config

6db2c2f

update eslint config

72223ad

Whytecrowe changed the title ~~Multi zNS Prototype~~ [WIP] Multi zNS Prototype Sep 26, 2024

Whytecrowe added 6 commits October 7, 2024 13:13

Revert "add chainRootHash concept to zns contract code"

011e7ac

This reverts commit 6dce79b.

Revert "add chainRootHash to ts code and tests"

237cf32

This reverts commit 33976c4.

move and unblock the extended bridge interface

5927914

fix chain resolver and add todo

4467940

create initial PolygonZkEvmPortal contract and struct for bridging

fd02224

add zkevm-contracts package

8797db9

Whytecrowe changed the base branch from rc/multi-zns-main to zns-zchain-final December 18, 2024 00:59

Whytecrowe requested a review from JamesEarle December 18, 2024 01:01

JamesEarle reviewed Dec 18, 2024

View reviewed changes

contracts/cross-chain/ZNSZChainPortal.sol Show resolved Hide resolved

JamesEarle reviewed Dec 18, 2024

View reviewed changes

Whytecrowe changed the base branch from zns-zchain-final to rc/multi-zns-main December 18, 2024 22:55

Whytecrowe added 6 commits December 18, 2024 16:00

Merge branch 'rc/multi-zns-main' into multi-zns-proto

f8b97a9

regenerate yarn.lock

976f74a

remove meow token ts files and move z-token folder to zns-base for DMs

3381b4c

fix eslintrc

fa5d0f8

fix get-config file

1c2d4bf

small initial fixes after merge

109b5c5

JamesEarle reviewed Dec 19, 2024

View reviewed changes

init-env.ts Show resolved Hide resolved

Whytecrowe added 2 commits December 20, 2024 12:32

more test fixes

4767b84

fix test helpers to include zToken properly and transfer instead of mint

b270197

Whytecrowe commented Apr 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Multi ZNS] Multi zNS Prototype [WIP] #118

[Multi ZNS] Multi zNS Prototype [WIP] #118

Whytecrowe commented Sep 19, 2024 •

edited

Loading

openzeppelin-code bot commented Sep 19, 2024 •

edited

Loading

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

Whytecrowe Jan 3, 2025

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

JamesEarle Dec 18, 2024

Whytecrowe Jan 3, 2025

JamesEarle Dec 18, 2024

Whytecrowe Jan 7, 2025

JamesEarle Dec 18, 2024 •

edited

Loading

Whytecrowe Feb 24, 2025 •

edited

Loading

JamesEarle Dec 18, 2024

Whytecrowe Jan 3, 2025

JamesEarle Dec 19, 2024

JamesEarle Dec 19, 2024

Whytecrowe Jan 3, 2025

JamesEarle Dec 19, 2024

Whytecrowe Apr 7, 2025


		function destZnsPortal() external view returns (address);

		function rootRegistrar() external view returns (IZNSRootRegistrarTrunk);


		interface IZNSZChainPortal is IDistributionConfig {

		struct ZNSContractInput {

[Multi ZNS] Multi zNS Prototype [WIP] #118

Are you sure you want to change the base?

[Multi ZNS] Multi zNS Prototype [WIP] #118

Conversation

Whytecrowe commented Sep 19, 2024 • edited Loading

openzeppelin-code bot commented Sep 19, 2024 • edited Loading

[Multi ZNS] Multi zNS Prototype [WIP]

🚨 Report Summary

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

JamesEarle Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Whytecrowe Feb 24, 2025 • edited Loading

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

Whytecrowe commented Sep 19, 2024 •

edited

Loading

openzeppelin-code bot commented Sep 19, 2024 •

edited

Loading

JamesEarle Dec 18, 2024 •

edited

Loading

Whytecrowe Feb 24, 2025 •

edited

Loading