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

feat: compilation restrictions #8668

Merged
merged 15 commits into from
Nov 18, 2024
Merged

feat: compilation restrictions #8668

merged 15 commits into from
Nov 18, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Aug 14, 2024

Closes #6099
Closes #5715

This builds on top of foundry-rs/compilers#170 and allows the following configuration options in foundry.toml:

# add via_ir profile
additional_compiler_profiles = [ { name = "via-ir", via_ir = true } ]

# enforce compiling tests without via_ir and only some heavy contract with via_ir
# note: it will only work if tests are using vm.deployCode to deploy contract
compilation_restrictions = [
    { paths = "test/**", via_ir = false },
    { paths = "src/Counter.sol", via_ir = true },
]

compilation_restrictions allows users to configure how we compile individual files or directories. Currently accepted keys are min_evm_version, max_evm_version, min_optimizer_runs, max_optimizer_runs, cancun. Configured restrictions apply to sources importing the restricted file as well.

Once we have a set of restrictions, we need to somehow construct settings objects to satisfy them. Right now those should be constructed manually via additional_compiler_profiles. It is configured a mapping from profile name to settings overrides. In example above we add a single profile named via-ir, which uses default settings with via_ir enabled, making it possible to compile a single contract under src/ with via-ir while all other contracts, including tests are compiled with default profile.

We need names for all profiles to include them into artifacts names in cases when same source file is compiled with multiple different profiles. Right now such artifacts would be named as Counter.{profile}.json.

When choosing profile, first profile (starting with default one), satisfying restrictions of the source file and all of its imports will be used.

@bowd
Copy link

bowd commented Aug 22, 2024

Not sure if it's related to this or some other unreleased code, but on this branch I've noticed some behaviour where forge test hangs if it needs to build contracts. Then if I run forge build separately it works ok.

bowd added a commit to mento-protocol/mento-core that referenced this pull request Aug 26, 2024
## Description

As I told some of you at dinner, I've had a bad case of insomnia over
the last week. This PR resulted from a couple of late night coding
sessions and the incessant need to make things nicer in our repos.

It's a big PR but 𝚫code is negative so 🤷 .

What happens in this mondo-PR: 

1. All `celo` contracts are removed form the repo and replaced with the
`@celo/contracts` NPM package. With a small caveat.
2. All `interfaces` are made to work with `0.8` and cover more of the
contract functions.
3. All `tests` contracts are updated to `0.8` and use `deployCode`
helpers to deploy lower-version contracts, and the use the interfaces to
interact with them.
4. ~We make use of this foundry-rs/foundry#8668
to improve compile time.~
5. Update `solhint` and `prettier` to a recent version and fix/ignore
all issues.
6. Fix solc compiler warnings.
7. Fix/ignore slither > informational warnings.
8. Slight Refactor of ForkTests to make them better to work with.
9. Restructuring of the tests folder.

#### `@celo/contracts`

The only caveat of using the `@celo/contracts` package is that
`UsingRegistry.sol` in there needs to import things from `mento-core`
and I didn't manage to get it working ok with remappings, so I kept a
copy of `UsingRegistry.sol` in `contracts/common`. It's only used in
`swap/Reserve.sol` and when we remove it from there we can completely
kill `common`.

#### The foundry WIP PR

A better solution was found here #502, which removes the need for
`via-ir` completely.

~The reason it takes so long to compile our code is because we need
`via-ir` for `Airgrab.sol` and `StableTokenV2.sol`, and `via-ir` is
super slow. But with the compiler restrictions implemented in
foundry-rs/foundry#8668 we can have multiple
compiler profile settings for subgraphs of the source-graph, which
compile in parallel with different settings.~

~You can easily install that version of foundry locally, (you have to
have rust installed tho):~
```
foundryup -P 8668
```

~With this version of foundry and the settings in `foundry.toml`, if
you're not working in a part of the source graph that contains
`Airgrab.sol` or `StableTokenV2.sol`, compilation will happen without
`via-ir` and will be super snappy. However if you do touch that source
graph it will still take noticeably long. Right now on my machine full
clean compilation takes 80seconds. It used to take >3minutes.~

#### ForkTest Refactoring

Our fork tests can get a bit heavy because they're running test
assertions for all the exchanges in a single test call. I've refactor it
a bit and split out exchange assertions into their own tests contracts
that need to be manually created when new exchanges are deployed.
There's a chain-level assertion on the number of exchanges that helps us
catch this and keep them up to date.

This work was continued here #503 for a much cleaner solution.

### Other changes

> _Describe any minor or "drive-by" changes here._

no minor changes here, no :))

### Tested

Tested? Tested!

### Related issues

Fixes the issues in my head.

### Backwards compatibility

What even is that?

### Documentation

Holy Bible

---------

Co-authored-by: Bayological <6872903+bayological@users.noreply.github.com>
Co-authored-by: chapati <philip.paetz@me.com>
Co-authored-by: philbow61 <80156619+philbow61@users.noreply.github.com>
@smol-ninja
Copy link

Thanks for the PR @klkvr. I have a question (related to #4979):

Does compilation_restrictions also dictate what artifacts will be generated? In your example, will it only generate artifacts for src/Counter.sol and test/** while ignoring the rest?

@klkvr
Copy link
Member Author

klkvr commented Oct 12, 2024

Does compilation_restrictions also dictate what artifacts will be generated? In your example, will it only generate artifacts for src/Counter.sol and test/** while ignoring the rest?

it doesn't. Each project file would still get compiled with either default or custom profile unless you specify --skip flag or config key which dictates which files should be included

@smol-ninja
Copy link

smol-ninja commented Oct 12, 2024

Thanks for the reply. Can you please point me to the config key that can be used to dictate which artifacts would get included?

@klkvr klkvr marked this pull request as ready for review November 18, 2024 19:56
@klkvr klkvr changed the title [wip] feat: compilation restrictions feat: compilation restrictions Nov 18, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, optional suggestions

crates/config/src/compilation.rs Show resolved Hide resolved
crates/config/src/compilation.rs Show resolved Hide resolved
optimizer_runs: Option<usize>,
max_optimizer_runs: Option<usize>,

#[serde(default, with = "serde_helpers::display_from_str_opt")]
Copy link
Member

Choose a reason for hiding this comment

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

unrelated,
worth checking which of the serde_utils we'd like to have in alloy-serde

@klkvr klkvr enabled auto-merge (squash) November 18, 2024 21:02
@klkvr klkvr merged commit 547d8a5 into master Nov 18, 2024
22 checks passed
@klkvr klkvr deleted the klkvr/compilation-restrictions branch November 18, 2024 21:12
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* [wip] feat: compilation restrictions

* Cargo.lock

* update patch

* fixes

* update patch

* update patch

* wip

* deps

* bytecode hash

* fixes

* rm patches

* pub
@frontier159
Copy link
Contributor

frontier159 commented Dec 2, 2024

@klkvr Could you please help describe the behaviour when setting min_solc_version | max_solc_version ?

Here's a sample repo:
https://github.com/frontier159/forge-compilation-restrictions/blob/main/foundry.toml

Which as it stands uses 0.8.19 for all

Behaviour I would like:

  • All files by default to compile with 0.8.22
    • Might have pragma solidity ^0.8.13; or pragma solidity ^0.8.21;
  • Counter-0.8.19.sol to compile with 0.8.19
    • Since it has a fixed pragma solidity 0.8.19;

It's also not clear to me what additional_compiler_profiles does and what name should be given to it?

@klkvr
Copy link
Member Author

klkvr commented Dec 2, 2024

hey @frontier159

min_solc_version/max_solc_version have no effect, you should use version to specify the solc version restrictions. In your case it would be something like

compilation_restrictions = [
    { paths = "src/Counter-0.8.19.sol", version = ">=0.8.19, <=0.8.22" },
]

additional_compiler_profiles are only needed if you're altering the compiler settings as this feature is not yet smart enough to resolve the additional profiles on its own (like we do with versions)

So for this example if you only need to specify the version restrictions you can just not add any profiles, the version keys in restrictions are treated in the same way as version pragmas in source files.

@frontier159
Copy link
Contributor

frontier159 commented Dec 3, 2024

hey @frontier159

min_solc_version/max_solc_version have no effect, you should use version to specify the solc version restrictions. In your case it would be something like

compilation_restrictions = [
    { paths = "src/Counter-0.8.19.sol", version = ">=0.8.19, <=0.8.22" },
]

@klkvr In this example repo, if I set to:

[profile.default]
src = "src"
out = "out"
libs = ["lib"]
solc_version = '0.8.22'

compilation_restrictions = [
    { paths = "src/Counter-0.8.19.sol", version = ">=0.8.19, <=0.8.22" },
]

Then I get the error:

Error: Encountered invalid solc version in src/Counter-0.8.19.sol: No solc version exists that matches the version requirement: =0.8.19, >=0.8.19, <=0.8.22

Sorry I might need to be spoon fed with the full example

@klkvr
Copy link
Member Author

klkvr commented Dec 3, 2024

you need to remove the solc_version key from the root, it requires all sources to get compiled with the 0.8.22 version thus we can't match the =0.8.19 requirement

@frontier159
Copy link
Contributor

So how do I fully specify that src/Counter-0.8.19.sol should use 0.8.19, and everything else should be 0.8.22?

@klkvr
Copy link
Member Author

klkvr commented Dec 3, 2024

something like this should work

[profile.default]
src = "src"
out = "out"
libs = ["lib"]

compilation_restrictions = [
    { paths = "src/**[!1][!9].sol", version = "=0.8.22" },
    { paths = "src/Counter-0.8.19.sol", version = "=0.8.19" },
]

we don't yet have full regex support for glob patterns, so the first restriction is a bit ugly, you could use a separate directory for this to make it nicer

@frontier159
Copy link
Contributor

OK great thanks @klkvr
I found the glob syntax: https://github.com/devongovett/glob-match?tab=readme-ov-file#syntax

It would be nice to still be able to specify the default version for any unmatched paths (eg via solc_version in the default profile)

It will get complicated if we have more than one or two upstream dependencies (eg in libs or node_modules) which need specific versions but then for everything else we want to specify the version.

eg

[profile.default]
src = "src"
out = "out"
libs = ["lib"]

# It would be nice to specify here instead of the last (getting ugly) path below
# solc_version = 0.8.22

compilation_restrictions = [
    { paths = "src/Counter-0.8.19.sol", version = "=0.8.19" },
    { paths = "src/Counter-0.8.20.sol", version = "=0.8.20" },
    { paths = "src/**[!{19,20}].sol", version = "=0.8.22" }
]

Alternatively implement where the order matters, where if a file is matched in an earlier list item it it won't be matched in future ones, so where this could work:

compilation_restrictions = [
    { paths = "src/Counter-0.8.19.sol", version = "=0.8.19" },
    { paths = "src/Counter-0.8.20.sol", version = "=0.8.20" },

    # picks up src/CounterAny.sol, but not src/Counter-0.8.19.sol or src/Counter-0.8.20.sol
    { paths = "**", version = "=0.8.22" } 
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: Completed
6 participants