Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support zkSyncEra and zkSyndTestnet chains #1259

Merged
merged 17 commits into from
Nov 22, 2023
Merged

Conversation

KolevDarko
Copy link
Contributor

Description of the changes

Deployed ERC20FeeProxy, EthereumFeeProxy and BatchConversionPayments contracts to zkSyncEra.

Added the new chains and contracts to our configs.

Wrote a separate deploy script, based on their docs, that uses their own native Deployer contract.

@KolevDarko KolevDarko changed the title feat: support zksync chain feat: support zkSyncEra and zkSyndTestnet chains Nov 21, 2023
@KolevDarko KolevDarko requested review from MantisClone and leoslr and removed request for kevindavee and yomarion November 21, 2023 11:54
packages/smart-contracts/deploy/utils.ts Outdated Show resolved Hide resolved
packages/smart-contracts/deploy/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@benjlevesque benjlevesque left a comment

Choose a reason for hiding this comment

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

the new deploy/ folder could use some details: Why do we need it? What does it do compare to hardhat? How to run the it?

packages/smart-contracts/hardhat.config.ts Show resolved Hide resolved
packages/currency/src/native.ts Outdated Show resolved Hide resolved
@KolevDarko
Copy link
Contributor Author

the new deploy/ folder could use some details: Why do we need it? What does it do compare to hardhat? How to run the it?

@benjlevesque I was also hesitant to create a completely new folder just for this specific chain, but their hardhat deploy plugin has the /deploy name hardcoded 😢 it's here

I'll add a zkSync section to the README.

I don't know where else to explain this?

@coveralls
Copy link

coveralls commented Nov 22, 2023

Coverage Status

Changes unknown
when pulling cc85517 on feat/support-zksync
into ** on master**.

@@ -96,7 +96,7 @@ describe('erc777-stream', () => {
it.each([
{ network: 'goerli' },
{ network: 'matic' },
{ network: 'xdai' },
// { network: 'xdai' },
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Comment on lines 6 to 8
// An example of a basic deploy script
// It will deploy a Greeter contract to selected network
// as well as verify it on Block Explorer if possible for the network
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// An example of a basic deploy script
// It will deploy a Greeter contract to selected network
// as well as verify it on Block Explorer if possible for the network

@@ -0,0 +1,14 @@
import { deployContract } from './utils-zk';

// An example of a basic deploy script
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment


config();

const accounts = process.env.DEPLOYMENT_PRIVATE_KEY
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird, no? Don't we just have 1 PK for deployment/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the logic from hardhat.config.ts file, we get the accounts the same way

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be simplified with the only key you need for deployment.

For example the DEPLOYER_MASTER_KEY is never used in this context.
I think you can also remove DEPLOYMENT_PRIVATE_KEY, and only use ADMIN_PRIVATE_KEY. Both of them exists in the original hardhat config, as we needed a distinction when we deployed the contract with the scripts not using CREATE2.

symbol: 'ETH-zksync-testnet',
decimals: 18,
name: 'Ether',
network: 'zkSyncEraTestnet',
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we don't keep the usual format (zk-sync-era-testnet) ?


config();

const accounts = process.env.DEPLOYMENT_PRIVATE_KEY
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be simplified with the only key you need for deployment.

For example the DEPLOYER_MASTER_KEY is never used in this context.
I think you can also remove DEPLOYMENT_PRIVATE_KEY, and only use ADMIN_PRIVATE_KEY. Both of them exists in the original hardhat config, as we needed a distinction when we deployed the contract with the scripts not using CREATE2.

@KolevDarko KolevDarko merged commit 624a636 into master Nov 22, 2023
25 of 26 checks passed
@KolevDarko KolevDarko deleted the feat/support-zksync branch November 22, 2023 11:04
@MantisClone MantisClone linked an issue Dec 11, 2023 that may be closed by this pull request
6 tasks
@MantisClone MantisClone mentioned this pull request Dec 11, 2023
6 tasks
@MantisClone
Copy link
Member

For posterity, here's the zkSync Era Explorer links:

zkSync Era:
ERC20FeeProxy
EthereumFeeProxy
BatchConversionPayments

zkSync Goerli Testnet
ERC20FeeProxy
EthereumFeeProxy
BatchConversionPayments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy to zkSync Era
8 participants