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

fix: zksync networks names to lowercase #1261

Merged
merged 2 commits into from Nov 23, 2023
Merged

Conversation

KolevDarko
Copy link
Contributor

@KolevDarko KolevDarko commented Nov 23, 2023

Description of the changes

Following the convention of specifying chain names as lowercase.

Also added the batch contract address from zksync testnet.

@KolevDarko KolevDarko enabled auto-merge (squash) November 23, 2023 09:37
Copy link
Contributor

@leoslr leoslr left a comment

Choose a reason for hiding this comment

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

@KolevDarko Did you also update the contracts (Batch I think) to modify the nativeHash that was previously used ?

@KolevDarko KolevDarko merged commit 9c9a2c1 into master Nov 23, 2023
26 checks passed
@KolevDarko KolevDarko deleted the fix/zksync-era-lowercase branch November 23, 2023 09:43
@coveralls
Copy link

Coverage Status

coverage: 87.098%. remained the same
when pulling 43c4f45 on fix/zksync-era-lowercase
into 624a636 on master.

@MantisClone MantisClone linked an issue Dec 11, 2023 that may be closed by this pull request
6 tasks
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

@KolevDarko @benjlevesque @leoslr I have a couple questions. I just want to make sure the correct addresses are documented in the lib/artifacts/../index.ts files.

@@ -61,10 +61,14 @@ export const batchPaymentsArtifact = new ContractArtifact<BatchPayments>(
address: '0x0DD57FFe83a53bCbd657e234B16A3e74fEDb8fBA',
creationBlockNumber: 35498688,
},
zkSyncEra: {
zksyncera: {
address: '0x0C41700ee1B363DB2ebC1a985f65cAf6eC4b1023',
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why is this address identical to the one listed for zkSync Era BatchConversionPayments

  2. Why does the zkSync Era explorer show the contract name "BatchConversionPayments" instead of "BatchPayments"?
    image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BatchConversionPayments is the contract that is used in production, so that's why that one is deployed.

At the time I thought BatchPayments was the one we were using but if we see the block explorer on mainnet: https://etherscan.io/address/0x0dd57ffe83a53bcbd657e234b16a3e74fedb8fba it hasn't been used for more than a year.

So if you want to clean things up, we can just remove the entries from BatchPayments/index.ts for both prod and test zksync.

Copy link
Member

Choose a reason for hiding this comment

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

Added issue to fix 👍 #1303

address: '0x0C41700ee1B363DB2ebC1a985f65cAf6eC4b1023',
creationBlockNumber: 19545614,
},
zksynceratestnet: {
address: '0x276A92F78aA527b8e157B0E47cEB63b4239dfa0b',
Copy link
Member

Choose a reason for hiding this comment

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

Why does the zkSync Era Goerli explorer show the contract name "BatchConversionPayments" instead of "BatchPayments"?
image

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
6 participants