feat: clean zksync_error integration #583

sayon · 2025-02-06T20:16:17Z

What 💻

Add zksync_error to this project and wire CLI launcher to it.

Why ✋

Unified failure system integration.

Evidence 📷

Intended to be transparent.

Cargo.toml

dutterbutter · 2025-02-07T17:51:41Z

Cargo.toml

+#########################
+# Build dependencies    #
+#########################
+zksync-error-codegen = { git = "https://github.com/matter-labs/zksync-error", branch = "main" }


We should reference a commit hash here instead unless we want to make an initial release of zksync-error?

Maybe rather a tag?
There are two locations in where it is important: here and in build.rs.
Something like stable.
This is a build dependency to rebuild the zksync-error crate, which is included. It is not triggered on build unless anvil.json is changed, therefore even if repository zksync-error breaks compatibility, anvil-zksync is still built without issues.

We need to start publishing it on crates.io (thankfully we have an automation for that now), but until then I'd prefer to depend on commit hash. Everything else can be a PITA to manage due to Cargo.lock (you can update the dep in Cargo.toml, but it won't be updated in Cargo.lock until you do cargo update).

+1 to commit hash

sayon · 2025-02-08T12:53:11Z

There is a point to discuss here -- how do we depend on zksync-error repository?
There are two Rust dependencies of interest here:

The code generator: crate zksync-error-codegen. This is a build-time dependency, and it is only used to rebuild the crate zksync-error. In other words, normally it is not triggered at all.
The documentation layout: crate zksync-error-description. It is a runtime dependency, but currently it is listed only in the generated file crates/zksync-error/Cargo.toml. This crate is an interface-only crate, meaning that it is supposed to be solid and it is detached from the codegen internals. If codegen changes, it is still obliged to conform to the interface in zksync-error-description.

I propose to depend on main or on some designated tag (stable?) for the codegen (can break but older builds will remain buildable) and on main for (2) (we promise that the interface remains stable).

@popzxc @itegulov @dutterbutter

popzxc · 2025-02-10T07:32:48Z

Cargo.toml

+#########################
+# Build dependencies    #
+#########################
+zksync-error-codegen = { git = "https://github.com/matter-labs/zksync-error", branch = "main" }


We need to start publishing it on crates.io (thankfully we have an automation for that now), but until then I'd prefer to depend on commit hash. Everything else can be a PITA to manage due to Cargo.lock (you can update the dep in Cargo.toml, but it won't be updated in Cargo.lock until you do cargo update).

popzxc · 2025-02-10T07:35:44Z

docs/zksync-error-howto.md

@@ -0,0 +1,377 @@
+# What is zksync-error?


Formatting in this file is odd. Could you please make sure that it's properly formatted, e.g.:

aligned to 120 line length

no blocks of many empty lines

only one H1 header

popzxc · 2025-02-10T07:38:06Z

docs/zksync-error-howto.md

@@ -0,0 +1,377 @@
+# What is zksync-error?
+
+A generated crate describing all errors that are:


It feels like this document tries to cover two things:

What is zksync-error

How to use it in anvil-zksync specifically.

IMHO, the first part should be a part of zksync-error repository, and here we would do something like "Please read this document first (link), and below we will cover how to use it for this codebase".

Otherwise, it will be a copypasta to be added to every repo and it will be a PITA to manage.

We can do it later if you prefer, however, I don't intend to block merging over this.

Ideally, zksync-error must have a book published similarly to core/prover/compiler docs covering the architecture/usage/etc, but this can wait until we integrate it here.

@dutterbutter if I recall correctly you suggested including project-agnostic documentation for zksync-error in this repository. If that was the case, could you remind the rationale?

I think for the initial integration having it in this repo is fine given most of the early usage / work with it will be within anvil-zksync. I know for myself I will be referencing it and having it within the repo is certainly convenient.

That being said, definitely once we've defined some standards / best practices it should be hosted and published separately. That will be done outside of this PR.

popzxc · 2025-02-10T07:38:53Z

etc/resources/anvil.json

+  "types": [],
+  "domains": [
+    {
+      "domain_name": "AnvilZKS",


Shouldn't it be AnvilZksync / anvil_zksync? ZKS feels super odd.

popzxc · 2025-02-10T07:40:58Z

.github/workflows/release.yml

@@ -61,8 +61,9 @@ jobs:

      - name: Pack anvil-zksync
        run: |
-          tar -czf anvil-zksync-${{ inputs.tag || inputs.prerelease_name }}-${{ matrix.arch }}.tar.gz \
-            ./target/${{ matrix.arch }}/release/anvil-zksync
+          tar -C ./target/${{ matrix.arch }}/release -czf \


Merge artifacts?

Yea this comes from a merge commit. Would you prefer me to rebase instead? I remember weird behavior on Github when I rebased branches and then conversations tied to the old commits are impossible to resolve.

Seems like you made #584 a part of your PR by accident

popzxc · 2025-02-10T07:42:36Z

etc/resources/anvil.json

I think it should be etc/errors/anvil-zksync.json.

itegulov

Yet to read through the howto doc but reviewed everything else. The new approach looks a lot more idiomatic!

itegulov · 2025-02-11T04:33:34Z

.github/workflows/release.yml

@@ -61,8 +61,9 @@ jobs:

      - name: Pack anvil-zksync
        run: |
-          tar -czf anvil-zksync-${{ inputs.tag || inputs.prerelease_name }}-${{ matrix.arch }}.tar.gz \
-            ./target/${{ matrix.arch }}/release/anvil-zksync
+          tar -C ./target/${{ matrix.arch }}/release -czf \


Seems like you made #584 a part of your PR by accident

itegulov · 2025-02-11T04:34:59Z

Cargo.lock

@@ -70,9 +70,9 @@ dependencies = [

 [[package]]
 name = "alloy-core"
-version = "0.8.15"
+version = "0.8.20"


Did you mean to run cargo update? I'd avoid bumping deps in this PR if there is no strong reason for it

itegulov · 2025-02-11T04:35:16Z

Cargo.toml

+#########################
+# Build dependencies    #
+#########################
+zksync-error-codegen = { git = "https://github.com/matter-labs/zksync-error", branch = "main" }


+1 to commit hash

itegulov · 2025-02-11T04:44:04Z

crates/core/build.rs

+fn main() {
+    // If we have modified anvil errors, forces rerunning the build script and
+    // regenerating the crate `zksync-error`.
+    println!("cargo:rerun-if-changed=../../etc/resources/anvil.json");


Nit: AFAIR cargo::KEY=VALUE is the preferred syntax now (starting from Rust 1.77):

Suggested change

println!("cargo:rerun-if-changed=../../etc/resources/anvil.json");

println!("cargo::rerun-if-changed=../../etc/resources/anvil.json");

crates/core/build.rs

itegulov · 2025-02-11T04:47:43Z

crates/zksync_error/Cargo.toml

@@ -0,0 +1,15 @@
+


Nit: random empty line

itegulov · 2025-02-11T04:48:53Z

crates/zksync_error/Cargo.toml

+name = "zksync_error"
+version = "0.1.0"
+edition = "2021"
+[lib]


Nit: unnecessary

itegulov · 2025-02-11T04:51:14Z

crates/zksync_error/Cargo.toml

+strum = "0.26.3"
+strum_macros = "0.26.4"
+zksync-error-description = { git = "https://github.com/matter-labs/zksync-error", branch = "main"}
+anyhow = "1.0"


Hmm, is this file auto-generated too? If so at the very least we need a header making this clear but I also think I would prefer if it was left for the user to write

dutterbutter · 2025-02-12T18:21:17Z

crates/zksync_error/src/lib.rs

+
+#![allow(unused)]
+pub mod documentation;
+pub(crate) mod error;


Suggested change

pub(crate) mod error;

pub mod error;

Access to types CustomErrorMessage and NamedError.

sayon requested a review from a team as a code owner February 6, 2025 20:16

sayon added 4 commits February 6, 2025 22:31

introduce zksync-error crate

f014353

migrate CLI to zksync_error

823d1fe

add basic zksync-error docs

402b9e6

change to domain errors

fb6f92f

sayon force-pushed the integrate-zksync-error branch from e5a6dcd to fb6f92f Compare February 6, 2025 21:31

sayon added 7 commits February 6, 2025 22:34

cargo fmt

5fc7b4f

update lock

fb13582

rename Anvil domain to AnvilZKS

1994ec9

update Cargo.lock

62a4876

fix linter errors

2f45445

fix dependency resolution

f78d701

fix formatting

c6ea61c

dutterbutter reviewed Feb 7, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

sayon and others added 2 commits February 7, 2025 15:44

Update Cargo.toml

17a2414

ci: fix packaging issue for anvil-zksync tar archives (#584)

8002b42

sayon requested review from itegulov and popzxc February 7, 2025 16:50

sayon added 3 commits February 7, 2025 17:58

fetch the root of errors from the main branch

a681da9

update cargo.lock

182c3be

update the zksync-error interface

b28db9e

dutterbutter reviewed Feb 7, 2025

View reviewed changes

dutterbutter added the needs review 👓 PR requires a review label Feb 7, 2025

popzxc reviewed Feb 10, 2025

View reviewed changes

itegulov reviewed Feb 11, 2025

View reviewed changes

dutterbutter reviewed Feb 12, 2025

View reviewed changes

dutterbutter added needs changes 🔧 Review requested changes on a PR and removed needs review 👓 PR requires a review labels Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: clean zksync_error integration #583

feat: clean zksync_error integration #583

sayon commented Feb 6, 2025

dutterbutter Feb 7, 2025

sayon Feb 8, 2025

popzxc Feb 10, 2025

itegulov Feb 11, 2025

sayon commented Feb 8, 2025 •

edited

Loading

popzxc Feb 10, 2025

popzxc Feb 10, 2025

popzxc Feb 10, 2025

popzxc Feb 10, 2025

sayon Feb 10, 2025

dutterbutter Feb 11, 2025

popzxc Feb 10, 2025

sayon Feb 10, 2025

popzxc Feb 10, 2025

sayon Feb 10, 2025

itegulov Feb 11, 2025

popzxc Feb 10, 2025

sayon Feb 10, 2025

itegulov left a comment

itegulov Feb 11, 2025

itegulov Feb 11, 2025

itegulov Feb 11, 2025

itegulov Feb 11, 2025

itegulov Feb 11, 2025

itegulov Feb 11, 2025

itegulov Feb 11, 2025

dutterbutter Feb 12, 2025

		@@ -0,0 +1,377 @@
		# What is zksync-error?

		A generated crate describing all errors that are:

	println!("cargo:rerun-if-changed=../../etc/resources/anvil.json");
	println!("cargo::rerun-if-changed=../../etc/resources/anvil.json");

feat: clean zksync_error integration #583

Are you sure you want to change the base?

feat: clean zksync_error integration #583

Conversation

sayon commented Feb 6, 2025

What 💻

Why ✋

Evidence 📷

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayon commented Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itegulov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayon commented Feb 8, 2025 •

edited

Loading