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

refactor(zk_toolbox): Minimize dependency on zksync_config #3436

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Jan 7, 2025

What ❔

Largely removes zkstack dependency on zksync_config.

Why ❔

This dependency significantly complicates new config system integration. While the alternative is less typesafe, it can be made more typesafe in the future once the config system integration is complete (a node binary can then be used to validate the changed config values).

Checklist

  • 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.

@slowli slowli marked this pull request as draft January 7, 2025 13:03
Copy link
Contributor Author

@slowli slowli left a comment

Choose a reason for hiding this comment

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

The remaining zksync_config uses in the config crate are related to GatewayConfig and GatewayChainConfig. Not sure what to do with these mentions because both configs are created from scratch in migrate_from_gateway / migrate_to_gateway modules (rather than being created from a template); so using the same approach as for GeneralConfig etc. is somewhat problematic. OTOH, these configs do not adhere to Protobuf serialization format anyway 🤷 I guess they could be dealt with in a separate PR (still before the config system integration).

update_from_chain_config(&mut genesis_config, &chain_config)?;
// FIXME: config isn't saved; why?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subj.

Copy link
Member

Choose a reason for hiding this comment

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

Have no answer, unfortunately.

zkstack_cli/crates/config/src/raw.rs Show resolved Hide resolved
.http_url,
)?;
let general_config = gateway_chain_config.get_general_config().await?;
let l2_rpc_url = general_config.get::<String>("api.web3_json_rpc.http_url")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uses of http_url / ws_url are somewhat problematic because these config params are never actually used by the node. Other similarly unused params are postgres.test.{server_url, prover_url}. I've left them as-is so far, but IMO they should be removed.

@slowli slowli marked this pull request as ready for review January 7, 2025 15:12
popzxc
popzxc previously approved these changes Jan 8, 2025
Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

Yes! Nice!

I'd still recommend getting a review from @sanekmelnikov before merging.

@slowli slowli requested a review from sanekmelnikov January 8, 2025 08:44
@slowli slowli requested a review from a team as a code owner January 13, 2025 10:56
@slowli slowli requested a review from yorik January 13, 2025 10:56
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.

2 participants