feat: Implement ZkSync Adapter and SpokePool #180

nicholaspai · 2022-08-09T00:57:57Z

Implements https://linear.app/uma/issue/ACX-169/write-test-zk-sync-contracts

and adds helper scripts

mrice32

LGTM! Just a few minor comments and nits. Nice work!!

README.md

mrice32 · 2022-08-15T19:25:41Z

contracts/ZkSync_SpokePool.sol

+    // On Ethereum, avoiding constructor parameters and putting them into constants reduces some of the gas cost
+    // upon contract deployment. On zkSync the opposite is true: deploying the same bytecode for contracts,
+    // while changing only constructor parameters can lead to substantial fee savings. So, the following params
+    // are all set by passing in constructor params where possible.


We don't expect to deploy many spoke pool contracts, right?

I think this mostly comes down to reads, since these values will be read for more than they will be deployed. Is reading directly from immutable bytecode cheaper or is reading from mutable storage cheaper? On ethereum mutable storage is far more expensive to read than immutable bytecode.

However, don't we typically make keep these bridge variables mutable to allow upgradability?

I guess I'm just a little confused about which variables this comment applies to.

Yeah we typically keep these variables mutable but perhaps the comment is misleading. I only included it for now as a consideration but I agree this contract should be rarely re-redeployed so i'll add that note

contracts/ZkSync_SpokePool.sol

mrice32 · 2022-08-15T19:31:28Z

contracts/ZkSync_SpokePool.sol

+            .withdraw(
+                hubPool,
+                // Note: If ETH, must use 0x0: https://github.com/matter-labs/v2-testnet-contracts/blob/3a0651357bb685751c2163e4cc65a240b0f602ef/l2/contracts/bridge/L2ETHBridge.sol#L57
+                (


nit: are these parens necessary?

mrice32 · 2022-08-15T19:32:26Z

contracts/ZkSync_SpokePool.sol

+    address public zkErc20Bridge;
+
+    // Bridge used to send ETH to L1: https://github.com/matter-labs/v2-testnet-contracts/blob/3a0651357bb685751c2163e4cc65a240b0f602ef/l2/contracts/bridge/L2ETHBridge.sol
+    address public zkEthBridge;


nit: do both of these conform to ZkBridgeLike? Might make the code below a little simpler if we stored them as ZkBridgeLike rather than addresses so we don't have to cast when using them.

Great point!

mrice32 · 2022-08-15T19:51:56Z

deploy/016_deploy_zksync_spokepool.ts

@@ -0,0 +1,40 @@
+import { HardhatRuntimeEnvironment } from "hardhat/types/runtime";
+import { Wallet } from "zksync-web3";
+import { Deployer } from "@matterlabs/hardhat-zksync-deploy";


Just to understand, hre does not contain getNamedAccounts like it does with normal hardhat-deploy?

Yeah this isn't the hardhat-deploy we're ued to, its a special zksync version

@matterlabs/hardhat-zksync-deploy pretty much only adds the deploy-zksync task

mrice32 · 2022-08-15T19:55:01Z

deploy/016_deploy_zksync_spokepool.ts

+  const artifact = await deployer.loadArtifact("ZkSync_SpokePool");
+  const args = [
+    L2_ADDRESS_MAP[chainId].zkErc20Bridge,
+    L2_ADDRESS_MAP[chainId].zkEthBridge,
+    hubPool.address, // Set hub pool as cross domain admin since it delegatecalls the ZkSync_Adapter logic.
+    hubPool.address,
+    L2_ADDRESS_MAP[chainId].l2Weth, // l2Weth
+    "0x0000000000000000000000000000000000000000", // timer
+  ];
+  const contract = await deployer.deploy(artifact, args);


Same as above, just want to double check -- we cannot just pass the name (ZkSync_SpokePool) directly into the deploy command like we do in other deployments?

Yeah i don't think this script has access to hardhat-deploy at all but I haven't tried importing hardhat-deploy so i'll try that now

Ah, so one complication is that the usual deploy command doesn't know to look in the artifacts-zk folder, instead it looks at the artifacts folder. So all of the tutorials use the deploy-zksync task

mrice32 · 2022-08-15T20:10:46Z

hardhat.config.ts

+  },
  networks: {
-    hardhat: { accounts: { accountsBalance: "1000000000000000000000000" } },
+    hardhat: { accounts: { accountsBalance: "1000000000000000000000000" }, zksync: compileZk },


Does this overwrite the normal artifacts or does it output to a different directory?

Outputs to artifacts-zk and cache-zk because the compiler is diff

mrice32 · 2022-08-15T20:11:38Z

scripts/zkSync.ts

+// zksync block explorer does not have contract verification yet so calling contracts via GUI is impossible. This file
+// contains useful scripts to interact with zksync goerli contracts. To run, run:
+// yarn hardhat run ./scripts/zksync.ts --network zksync-goerli


nit: could you put this at the top of the file so it's more visible?

mrice32 · 2022-08-15T20:12:50Z

scripts/zkSync.ts

+  const execute = await spokePool.executeRelayerRefundLeaf(
+    config.rootBundleIdToExecute,
+    {
+      // Recreating leaf constructed in  './buildSampleTree.ts'
+      amountToReturn: toBN("100000000000000000"), // 0.1
+      chainId: originChainId,
+      refundAmounts: [toBN("100000000000000000")], // -0.1
+      leafId: 0,
+      l2TokenAddress: erc20.address,
+      refundAddresses: ["0x9a8f92a830a5cb89a3816e3d267cb7791c16b04d"],
+    },
+    []
+  );
+  const executeReceipt = await execute.wait();


nit: should this be commented out with the other write transactions?

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai · 2022-08-16T14:21:47Z

@mrice32 there are a lot of inconsistencies with the deployment scripts in this PR because we use the zksync tutorial's suggested hardhat-deploy-zksync npm package instead of hardhat-deploy, but I just found out that the latter might actually support zksync now following this PR. ~~I'll look into this but its not well documented so i'm not super hopeful.~~

I bumped hardhat-deploy to a version that allegedly handles zksync and hardhat deploy task worked!

mrice32 · 2022-08-17T14:25:08Z

@mrice32 there are a lot of inconsistencies with the deployment scripts in this PR because we use the zksync tutorial's suggested hardhat-deploy-zksync npm package instead of hardhat-deploy, but I just found out that the latter might actually support zksync now following this PR. ~~I'll look into this but its not well documented so i'm not super hopeful.~~

I bumped hardhat-deploy to a version that allegedly handles zksync and hardhat deploy task worked!

Very cool!

mrice32

LGTM! This looks great. Thanks for looking into the hardhat-deploy issue

mrice32 · 2022-08-17T14:27:03Z

deploy/016_deploy_zksync_spokepool.ts

+
+  const chainId = parseInt(await getChainId());
+
+  await deploy("ZkSync_SpokePool", {


nicholaspai added 5 commits August 8, 2022 20:57

WIP: feat: Implement ZkSync Adapter and SpokePool

3c32e71

update contracts

d6a6e7a

fix

91feed3

fix version

0dac30f

Add scripts

233d4d6

nicholaspai marked this pull request as ready for review August 12, 2022 18:41

nicholaspai changed the title ~~WIP: feat: Implement ZkSync Adapter and SpokePool~~ feat: Implement ZkSync Adapter and SpokePool Aug 12, 2022

nicholaspai requested review from kevinuma and mrice32 August 12, 2022 21:02

mrice32 approved these changes Aug 15, 2022

View reviewed changes

nicholaspai and others added 2 commits August 15, 2022 21:11

Update README.md

cb00b19

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

respond to matt comments

625582f

Upgrade hardhat-deploy to use zk-sync

6473e2b

nicholaspai requested a review from mrice32 August 16, 2022 18:20

mrice32 approved these changes Aug 17, 2022

View reviewed changes

deploy/016_deploy_zksync_spokepool.ts

const chainId = parseInt(await getChainId());

await deploy("ZkSync_SpokePool", {

Copy link

Contributor

mrice32 Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

nicholaspai merged commit 0fa1436 into master Aug 18, 2022

nicholaspai deleted the npai/zksync branch August 18, 2022 13:15

nicholaspai restored the npai/zksync branch August 18, 2022 18:12


		const chainId = parseInt(await getChainId());

		await deploy("ZkSync_SpokePool", {

feat: Implement ZkSync Adapter and SpokePool #180

feat: Implement ZkSync Adapter and SpokePool #180

Uh oh!

Conversation

nicholaspai commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrice32 commented Aug 17, 2022

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicholaspai commented Aug 9, 2022 •

edited

Loading

nicholaspai commented Aug 16, 2022 •

edited

Loading