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

chore: Add ZKSync into the Chain type #2302

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

syahrul12345
Copy link
Contributor

Motivation

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

ethers-core/src/types/chain.rs Outdated Show resolved Hide resolved
@@ -140,6 +140,8 @@ pub enum Chain {
CantoTestnet = 740,

Boba = 288,

ZKSync = 324,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be renamed to lowercase for serde and strum, see the comments at the top of the file on how to do it

EmeraldTestnet | Boba => return None,
Morden | Ropsten | Rinkeby | Goerli | Kovan | XDai | Chiado | Sepolia | Moonbase
| MoonbeamDev | Optimism | OptimismGoerli | OptimismKovan | Poa | Sokol | Rsk
| EmeraldTestnet | Boba | ZKSync => return None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use the nightly toolchain to format your code
(cargo +nightly fmt)

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've run cargo +nigtly fmt in the project

@@ -460,6 +463,10 @@ impl Chain {

Boba => ("https://api.bobascan.com/api", "https://bobascan.com"),

ZKSync => {
Copy link
Collaborator

@DaniPopes DaniPopes Mar 27, 2023

Choose a reason for hiding this comment

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

one last thing, this wasn't renamed (you can see the warnings by running cargo clippy -p ethers-core)

Suggested change
ZKSync => {
Zksync => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in another commit :D thx

@prestwich
Copy link
Collaborator

prestwich commented Apr 9, 2023

2 requests:

  1. the S should be capitalized for rusty PascalCase - ZkSync
  2. Please rebase onto latest master, as we've fixed several of the CI issues hitting your checks 👍

@gakonst gakonst merged commit 2514163 into gakonst:master Apr 13, 2023
10 of 15 checks passed
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.

None yet

5 participants