refactor: migrate to `jsonrpsee` + `zksync_web3_decl` #497

itegulov · 2024-12-17T06:22:34Z

What 💻

Closes #495

Apologies for the big PR but I tried to keep functional changes to the minimum. Brief overview of the new crates:

api_decl - a combination of re-exported namespaces from zksync_web3_decl and jsonrpsee server bindings for other namespaces not supported there (anvil, evm, config, one method from eth)
api_server - implementations for the aforementioned namespaces; all of the methods are either "return a constant" or a single invocation of the corresponding method from InMemoryNode
types - just a collection of basic types we use, some stuff from config was moved here

The few functional changes made on purpose:

Removed zks_getTokenPrice as it was deprecated for a bit and removed from core in feat(api): Remove unused and obsolete token info zksync-era#1071
hardhat_impersonateAccount and hardhat_stopImpersonatingAccount no longer return bool - I didn't find any reference on why they should and the anvil version does not
A lot of API endpoints became async when they weren't in the past which uncovered a few concurrency bugs that I think I managed to fix. There is one ethers bug however that started causing flakiness in one of our e2e tests, so I had to tweak it a little bit for now.
CORS support was revamped due to completely different interface on the jsonrpsee side and is filtering based on Host now
Custom logging_middleware was replaced by the jsonrpsee's built-in RpcLogger. Format is different but arguably not a part of external API so does not matter as long as the semantics are the same.

Work that purposefully has not been done in this PR but should be done in the future:

Proper error hierarchy. For now I just crammed everything into RpcError which is either anyhow, Web3Error from core or LoadStateError which is the only custom error type we have
Rename InMemoryNode to ApiServer (name WIP) and move it to api_server. Essentially we don't want core to care too much about what kind of requests it will need to be able to answer.

Why ✋

jsonrpc crate has been deprecated for a while and we should align ourselves with core as much as we can

popzxc

Mostly looks good

popzxc · 2024-12-17T07:25:34Z

crates/cli/src/cli.rs

@@ -249,7 +249,7 @@ pub struct Cli {
    pub allow_origin: String,

    /// Disable CORS.
-    #[arg(long, default_missing_value = "true", num_args(0..=1), conflicts_with = "allow_origin", help_heading = "Server options")]


$ anvil --no-cors --allow-origin http://google.com error: the argument '--no-cors' cannot be used with '--allow-origin <ALLOW_ORIGIN>'

I guess this was taken from the upstream, not sure if we want to keep it just because it's there (OTOH I guess logically those options are in conflict)

Ah, I think I see where all of the confusion is coming from: --no-cors is false by default for anvil but it is true for us to keep backwards compatibility. In our case these two options are not in conflict as the only way to enable cors is weirdly to pass --no-cors=false in CLI. So, we need to do a couple of things here:

Enabled CORS in anvil does not imply host filtering like what was done in feat: add cli options for server configuration #487, it is purely about HTTP headers. We should do the same

We should understand if we are okay with enabling CORS by default. It's somewhat of a grey area for what I would consider a breaking change

@Romsters could you double check my understanding here and give your thoughts? If we are aligned I can make the change in this PR, no need to backport this for jsonrpc

@itegulov, @popzxc No, --no-cors is false by default, same as in anvil. default_missing_value is only applied if you specify --no-cors without value, which should be reasonably set to true.

Enabled CORS in anvil mirrors origin in response CORS headers, same as I've done. Disabled CORS in anvil doesn't check origin indeed, but it stops processing OPTIONS requests. I couldn't get the same behaviour with jsonrpc_http_server that's why I simply blocked all origins, which essentially is the same in terms of behaviour in browser, in both cases request will be blocked.

We definitely should enable CORS by default, it was enabled by default even before my changes. If you don't provide cors instructions to jsonrpc_http_server builder it still enables CORS by default.

default_missing_value is only applied if you specify --no-cors without value, which should be reasonably set to true

Yes, sorry I misunderstood that part after a long day of refactoring so the difference slipped my mind. I think what confused me is that it is IMHO a weird pattern to do Option<bool> with default missing value so I refactored it to just be bool with no extra clap options (except conflicts_with). The behaviour should be the same; hope you don't mind.

Disabled CORS in anvil doesn't check origin indeed, but it stops processing OPTIONS requests

I don't understand what you mean by this. CORS protocol relies on the client side (i.e. browser) respecting the access list options meaning server does not enforce them. Not sure what anvil's behaviour you are referring to but I just ran anvil with --allow-origin mydomain.com and made a curl request like this:

$ curl http://localhost:8011 -H "Origin: http://zksync-era.com" -X OPTIONS -v < HTTP/1.1 200 OK < access-control-allow-origin: mydomain.com < vary: origin, access-control-request-method, access-control-request-headers < access-control-allow-methods: GET,POST < access-control-allow-headers: content-type < allow: POST,GET,HEAD < content-length: 0 < date: Wed, 18 Dec 2024 04:44:40 GMT <

As you can see everything worked fine and nothing was blocked.

it was enabled by default even before my changes

I don't think this is true. I have just tried running era_test_node@0.1.0-alpha.33 and this is what I get:

$ curl http://localhost:8011 \ -X POST \ -H "Content-Type: application/json" \ --data '{"method":"eth_getBlockByNumber","params":["latest", true],"id":1,"jsonrpc":"2.0"}' -v ... * upload completely sent off: 82 bytes < HTTP/1.1 200 OK < content-type: application/json; charset=utf-8 < content-length: 1489 < date: Wed, 18 Dec 2024 04:31:19 GMT < ...

With OPTIONS:

$ curl http://localhost:8011 -X OPTIONS -v ... < HTTP/1.1 200 OK < content-type: application/json; charset=utf-8 < allow: OPTIONS, POST < accept: application/json < content-length: 0 < date: Wed, 18 Dec 2024 04:35:37 GMT <

The server only reports plain Allow, nothing about access control as supposed to happen when CORS is enabled.

In any case I aligned the code with anvil: I am using the same tower CORS middleware they are using. Please check out d7f6147. It also includes significant refactoring of integration tests as the behaviour I wanted to assert was different from what you implemented.

crates/cli/src/main.rs

popzxc · 2024-12-17T08:09:32Z

crates/types/Cargo.toml

+# TODO: Depending on `clap` here is not the worst thing for now but need to reconsider if this is strictly necessary
+clap.workspace = true


It's quite surprising indeed.

It's needed for clap-specific derivations. I could make it optional and hide it behind clap flag but at the same time I don't want to eventually arrive at the situation we have in zksync-era with the recently discussed feature flag hell.

I think the right approach here would be to define two types (one that derives ValueEnum in CLI and one agnostic in types) and implement conversion for them rather than introduce flags, but it can wait.

popzxc · 2024-12-17T08:10:55Z

crates/core/src/node/zks.rs

-            .into_boxed_future()
+impl InMemoryNode {
+    pub async fn estimate_fee_impl(&self, req: CallRequest) -> Result<Fee, Web3Error> {
+        // TODO: Burn with fire


e2e-tests-rust/tests/lib.rs

crates/cli/src/main.rs

# Conflicts: # crates/cli/src/cli.rs # crates/config/src/types/mod.rs # crates/core/src/node/eth.rs # crates/core/src/node/in_memory.rs # e2e-tests-rust/tests/lib.rs

popzxc · 2024-12-18T07:29:51Z

crates/types/Cargo.toml

+# TODO: Depending on `clap` here is not the worst thing for now but need to reconsider if this is strictly necessary
+clap.workspace = true


I think the right approach here would be to define two types (one that derives ValueEnum in CLI and one agnostic in types) and implement conversion for them rather than introduce flags, but it can wait.

crates/api_server/src/server.rs

itegulov added 8 commits December 17, 2024 15:35

migrate debug,evm,hardhat,net,web3 namespaces to jsonrpsee

ac1a809

migrate config namespace to jsonrpsee

736abfd

migrate zks namespace to jsonrpsee

2358c97

migrate eth namespace to jsonrpsee

deb40ec

return debug to node crate

585643c

fix e2e tests

cd35997

align impersonating return type to upstream anvil

7f1c48d

migrate CORS options to jsonrpsee

b5eebeb

itegulov requested review from Romsters, popzxc and dutterbutter December 17, 2024 06:22

itegulov requested a review from a team as a code owner December 17, 2024 06:22

itegulov added 2 commits December 17, 2024 17:28

clippy

d6879a9

prettier

2fe56af

popzxc reviewed Dec 17, 2024

View reviewed changes

Romsters reviewed Dec 17, 2024

View reviewed changes

e2e-tests-rust/tests/lib.rs Outdated Show resolved Hide resolved

Romsters reviewed Dec 17, 2024

View reviewed changes

e2e-tests-rust/tests/lib.rs Outdated Show resolved Hide resolved

Romsters reviewed Dec 17, 2024

View reviewed changes

crates/cli/src/main.rs Outdated Show resolved Hide resolved

itegulov added 5 commits December 18, 2024 15:25

full revamp of CORS

d7f6147

Merge branch 'main' into daniyar/refactor/jsonrpsee

345c492

# Conflicts: # crates/cli/src/cli.rs # crates/config/src/types/mod.rs # crates/core/src/node/eth.rs # crates/core/src/node/in_memory.rs # e2e-tests-rust/tests/lib.rs

move transaction_order to the types crate

b096f94

clippy

88f18e4

move server building logic to api_server crate

21cbcc5

itegulov requested review from popzxc and Romsters December 18, 2024 06:19

itegulov added 3 commits December 18, 2024 18:21

clippy

316893b

make api_server agnostic on the number of binds

6912d0c

rename github workflow for e2e-rust

c10dcd9

popzxc previously approved these changes Dec 18, 2024

View reviewed changes

clarify jsonrpsee caveat + link to docs

b6e9d01

itegulov dismissed popzxc’s stale review via b6e9d01 December 18, 2024 08:51

popzxc approved these changes Dec 18, 2024

View reviewed changes

itegulov merged commit dacf3fa into main Dec 18, 2024
13 checks passed

itegulov deleted the daniyar/refactor/jsonrpsee branch December 18, 2024 09:28

itegulov mentioned this pull request Dec 18, 2024

feat(cli): adds --dump-state cli cmd #496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate to `jsonrpsee` + `zksync_web3_decl` #497

refactor: migrate to `jsonrpsee` + `zksync_web3_decl` #497

itegulov commented Dec 17, 2024

popzxc left a comment

popzxc Dec 17, 2024

itegulov Dec 17, 2024 •

edited

Loading

Romsters Dec 17, 2024 •

edited

Loading

itegulov Dec 18, 2024

popzxc Dec 17, 2024

itegulov Dec 17, 2024

popzxc Dec 18, 2024

popzxc Dec 17, 2024

popzxc Dec 18, 2024

		# TODO: Depending on `clap` here is not the worst thing for now but need to reconsider if this is strictly necessary
		clap.workspace = true

refactor: migrate to jsonrpsee + zksync_web3_decl #497

refactor: migrate to jsonrpsee + zksync_web3_decl #497

Conversation

itegulov commented Dec 17, 2024

What 💻

Why ✋

popzxc left a comment

Choose a reason for hiding this comment

popzxc Dec 17, 2024

Choose a reason for hiding this comment

itegulov Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Romsters Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

itegulov Dec 18, 2024

Choose a reason for hiding this comment

popzxc Dec 17, 2024

Choose a reason for hiding this comment

itegulov Dec 17, 2024

Choose a reason for hiding this comment

popzxc Dec 18, 2024

Choose a reason for hiding this comment

popzxc Dec 17, 2024

Choose a reason for hiding this comment

popzxc Dec 18, 2024

Choose a reason for hiding this comment

refactor: migrate to `jsonrpsee` + `zksync_web3_decl` #497

refactor: migrate to `jsonrpsee` + `zksync_web3_decl` #497

itegulov Dec 17, 2024 •

edited

Loading

Romsters Dec 17, 2024 •

edited

Loading