matter-labs / foundry-zksync

Fork of Foundry tailored for zkSync environment
Apache License 2.0
299 stars 130 forks source link

refactor(config): split zk-related configuration #430

Closed Karrq closed 5 months ago

Karrq commented 5 months ago

Motivation

All zksync related configurations are flattened into the top level Config struct, having to deal with duplicated names and need to keep relatively simplified values to encode wanted behavior.

Solution

Encapsulate zksync related configuration as to not pollute the top level Config struct, allowing a more structured approach to configuration.

This is how foundry.toml can look like, with the changes:

[zksync] #all configuration below lives inside the `zksync` item
enable = false # can still be enabled from the cli
zksolc = "1.5.0"
force_evmla = true
fallback_oz = true
solc_path = "./solc-0.8.23-1.0.1"
optimizer = true
optimizer_mode = 'z'

before:

# no section
zksync = true
zksolc = "1.5.0"
force_evmla = true
fallback_oz = true
zk_solc_path = "./solc-0.8.23-1.0.1"
zk_optimizer = true
mode = 'z'
Karrq commented 5 months ago

Unfortunately there's a bit of attrition on converting the cli args to the config values, maybe we can find a way to improve that so future changes to zksync configuration can be further simplified or at least be done in the same context

elfedy commented 5 months ago

My two cents: Having everything under [zksync] might be a good idea as it provides a clean separation for the config. The further nesting might be a bit more controversial because:

  1. It's hard to get the "right" level of nesting on the first try, more so when we are in an alpha stage and options come and go all the time, we would have to change categories a bit and might provide more confusion than clarity in the end.
  2. We don't have THAT many zksync specific options, a single level might even be cleaner
  3. (This is the most important to me) Foundry's API does not use this nesting and we should probably aim to mirror Foundry's style at least in spirit for a more cohesive experience
Karrq commented 5 months ago

Okay, I can refactor to flatten the structure so we keep everything under one [zksync] section.

I'd still opt to keep the optimizer details nested, as that's already a structure we depend on from elsewhere, and the various values would have been nested also originally

elfedy commented 5 months ago

If we merge this we should probably notify somewhere about the API change