refactor(zk_toolbox): Minimize zkstack dependency on zksync_config (pt. 2)
#3543
Conversation
Unfortunately, it cannot be removed completely because gateway scripts depend on it.
slowli
requested review from
onyxet,
otani88,
iluwaa,
sanekmelnikov,
popzxc,
EmilLuta and
Artemka374
There was a problem hiding this comment.
Nice. As usual, suggest getting extra review from @sanekmelnikov
| optional uint32 ws_port = 3; // required; u16 | ||
| optional string ws_url = 4; // required |
There was a problem hiding this comment.
That's a bit of a controversial change; on the one hand, it's not required for running the server. But it does help for running other tools against the server
There was a problem hiding this comment.
- Even if these values would be beneficial in the config files, that doesn't mean that they should be retained in the config definitions. Compare with some wallet and contract params which are similarly not used by the server (but they aren't defined in the config).
- Are there cases when
ws_url/http_urldon't use the127.0.0.1domain? (I guess somewhere external, because I've replaced all the mentions in the repo, and, judging by CI, it seems to work.) If they are always constructed as(htttp|ws)://127.0.0.1/{corresponding_port}, then these params are redundant.
There was a problem hiding this comment.
Unfortunately, we sometimes want to change them. e.g. to ask get some remote server
| @@ -105,7 +105,25 @@ impl MainNodeClient for Box<DynClient<L2>> { | |||
| } | |||
|
|
|||
| async fn fetch_genesis_config(&self) -> EnrichedClientResult<GenesisConfig> { | |||
| self.genesis_config().rpc_context("genesis_config").await | |||
| let dto = self.genesis_config().rpc_context("genesis_config").await?; | |||
| Ok(GenesisConfig { | |||
There was a problem hiding this comment.
Why do we need both DTO and non dto?
There was a problem hiding this comment.
Single responsibility principle; it's weird to have a single type represent both a configuration and something used by API. More practically, it makes zksync_web3_decl depend on zksync_config, which is unnecessary and makes zkstack transitively depend on zksync_config as well (since it depends on zksync_web3_decl), even if the direct dependency is removed.
What ❔
zksync_*configdeps in zkstack.Why ❔
Part of preparations for integrating a new config system.
Checklist
zkstack dev fmtandzkstack dev lint.