-
Notifications
You must be signed in to change notification settings - Fork 412
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: zksync support #4725
base: main
Are you sure you want to change the base?
feat: zksync support #4725
Conversation
…KSyncConstructorArgs().
… in favor of MultiChainResolver
feat: zksync post verifier
refactor: consolidate ISM compatibility logic and static deployment c…
…r' into feat/zksync-support
It seems like most issues were resolved and/or outdated
|
||
import { Annotated, ProtocolType } from '@hyperlane-xyz/utils'; | ||
|
||
export enum ProviderType { | ||
ZkSync = 'zksync', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a big deal but would you agree that when adding an entry to an enum with several entries already, it makes sense to add it at the end? Especially when it starts with 'Z'. I wonder why so many developer tend to shove new things at the top as though they're more important than the existing parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm interesting thought In my experience, new code is more visible when you put it at the top, especially when its code you write yourself, has nothing to do with importance. Will address this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved. All zksync references on this file are not below the existing ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that new code is more visible when it's at the top, but why do you feel that new code needs to me more visible than the existing code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being devil's advocate. I agree, It doesn't belong at the top, have no good explanation why this particular file is ordered the way it was, other files are fine. @mshojaei-txfusion lets make a note of this
networks: { | ||
zkSyncNetwork: { | ||
url: 'http://127.0.0.1:8011', | ||
ethNetwork: '', | ||
zksync: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed for building the zk-sync artifacts or can it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would keep it there, removing the zksync network reference has been know to cause issues for some reason
Description
This PR introduces a series of changes aimed at enhancing zkSync support within the codebase. Key updates include the addition of the zksolc compiler for zkSync, integration of contract artifacts, CLI automation for core deployment, and compatibility adjustments in tests for the zkSync environment. It also includes improvements in contract verification on zkSync explorer and handling gas limits for zkSync deployments.
Drive-by changes
Related issues
No related issue
Backward compatibility
Yes
Testing
Manual testing and some automated tests were performed, including end-to-end tests on zkSync for warp read and apply functionalities.