feat(zkstack): Split chain and ecosystem layers #3199
Conversation
…refactor-chain-commands
…bs/zksync-era into matias/refactor-chain-commands
matias-gonz
changed the title
matias-gonz
requested review from
yorik,
alexandrst88,
artmakh,
hatemosphere and
onyxet
…refactor-chain-commands
matias-gonz
changed the title
| @@ -26,7 +26,7 @@ use crate::{ | |||
| }; | |||
|
|
|||
| pub async fn run(args: InitConfigsArgs, shell: &Shell) -> anyhow::Result<()> { | |||
| let ecosystem_config = EcosystemConfig::from_file(shell)?; | |||
| let ecosystem_config = ZkStackConfig::ecosystem(shell)?; | |||
There was a problem hiding this comment.
we shouldn't use it here.
We need it only for port reallocation.
It'd be better to pass zkstack config here and if ecosystem is available do reallocation and if not, use default one
There was a problem hiding this comment.
We also need ecosystem for contracts config, this is fixed in a next PR:
#3231
…refactor-chain-commands
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
## What ❔ <!-- What are the changes this PR brings about? --> <!-- Example: This PR adds a PR template to the repo. --> <!-- (For bigger PRs adding more context is appreciated) --> ## Why ❔ <!-- Why are these changes done? What goal do they contribute to? What are the principles behind them? --> <!-- Example: PR templates ensure PR reviewers, observers, and future iterators are in context about the evolution of repos. --> ## Checklist <!-- Check your PR fulfills the following items. --> <!-- For draft PRs check the boxes as you complete them. --> - [ ] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [ ] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [ ] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
matias-gonz
changed the title
There was a problem hiding this comment.
@matias-gonz, would it be reasonable to ask you to fill in the description [What, Why]? :)
Since it's a breaking change, I think it would be useful to list all the commands that will stop working (zkstack server -> zkstack chain server, etс.)
@Deniallugo, @matias-gonz
So I've been trying out these changes and I guess I don't understand what problem are we trying to solve or what use-case are we trying to improve?
- If we want to deploy a brand-new ecosystem, I don't see much value separating ecosystem and chain. imho, the current setup works really well for that case.
- If we are working with the ecosystem, that already exists:
2.1) if ecocsystem is on localhost: I don't see howzkstack chain create/initis easier to use outside of the ecosystem - because I need to provide contracts/wallets file and path to code. It's much easier to run the same commands from the ecosystem folder.
2.2) if ecosystem is ZKSync's Sepolia/Mainnet:zkstack ecosystem import --from mainnetin my view would be better way to go since contracts/wallet (addresses) can be populated for you
2.3) if ecosystem is not ZKSync's Sepolia/Mainnet: that's probably the only use-case that will be improved, although I'd still preferzkstack ecosystem import --from [FILE]followed byzkstack chain createetc. and also that's the least common use-case.
I think the fundamental problem is that we have ecosystem and chain command tightly coupled together, but making it easy to import existing ecosystem with a separate command is a more straightforward way to go?
The biggest problem with this PR is that all of these currently aren't working for chain-only folder:
zkstack portalzkstack explorerzkstack updatezkstack external-nodezkstack containerszkstack prover
As well as:
zkstack chain init configs
As for zkstack chain server vs zkstack server: do we really think it's worth breaking for all the users? For chain-only folders zkstack chain server makes less sense than zkstack server since obviously everything we're going to run will be chain-related. Can we support both?
| .map(|ecosystem| ecosystem.list_of_chains().len() as u32) | ||
| .unwrap_or(0); | ||
|
|
||
| let internal_id = ecosystem.as_ref().map_or(0, |_| number_of_chains + 1); |
There was a problem hiding this comment.
[minor]: 0 -> 1, i'd use the same way we numerate in ecosystem
There was a problem hiding this comment.
Also, is it used anywhere apart from calculating ports?
| #[clap(long, help = MSG_ECOSYSTEM_CONTRACTS_PATH_HELP)] | ||
| pub ecosystem_contracts_path: Option<String>, | ||
| #[clap(long, help = MSG_WALLETS_PATH_HELP)] | ||
| pub wallets_path: Option<String>, |
There was a problem hiding this comment.
is it ecosystem_wallets_path?
| let rocks_db_path = chain_path.join(LOCAL_DB_PATH); | ||
| let artifacts = chain_path.join(LOCAL_ARTIFACTS_PATH); |
There was a problem hiding this comment.
I think for chain-only folders this leads to the structure:
{chain_name}/{chain_name}/db
vs in ecosystem based:
chains/{chain_name}/db
Are we intentionally adding one more layer?
|
Ah, with chain registrar changes though, I can see the value for external chains. |
What ❔
Checklist
zkstack dev fmtandzkstack dev lint.