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

Remove the need for --via-ir in our contracts #502

Merged
merged 82 commits into from
Aug 27, 2024

Conversation

bowd
Copy link
Contributor

@bowd bowd commented Aug 23, 2024

Description

After getting more and more annoyed with the CI time and other small issues around this I decided to really understand how big is our need for --via-ir compilation. Turns out it's really small. I mean really small.

There are just a couple of places where this is causing issues:

  1. Airgrab#claim has too many variables. I fixed this with a struct for the fractal proof, and an internal function to do the lock. Changing it is ok in my opinion. If we want to reuse this contract we'll have to redeploy it anyway, we have a tag for the version when it was deployed, all is well.

  2. GovernanceFactory#createGovernance has too many variables. I just broke it down in internal functions. Again, I don't see a problem in changing this, even if it's deployed and etc.

  3. StableTokenV2#initialize has too many arguments. When we made StableTokenV2 we wanted to keep the same interface for the initialize method to keep the interfaces between stable tokens consistent, but it was actually a bad call. There's no good reason why we have it that way and it's a method that can anyway only be called once. I would say it's ok to also change this without making a StableTokenV3 for it. Basically we will have a few contracts which will have a different signature for the initialize method that anyway can't be called anymore. But this is the one change the feels least nice.

  4. A couple of tests with many variables where I used structs for grouping or broke down into internal functions.

What happens afterwards, well:

╰─ forge test
[⠊] Compiling...
[⠒] Compiling 64 files with Solc 0.5.17
[⠢] Compiling 55 files with Solc 0.8.26
[⠒] Compiling 223 files with Solc 0.8.18
[⠒] Solc 0.5.17 finished in 307.90ms
[⠘] Solc 0.8.26 finished in 1.75s
[⠒] Solc 0.8.18 finished in 5.50s
Compiler run successful with warnings:

The whole fucking shabang runs in 5.5s. Jesus.
CI takes ~1minute instead of ~15minutes.

P.S. Thank you Claude for the bash script.

Other changes

It changed my life.

Tested

Thoroughly.

Related issues

Sleep deprivation.

Backwards compatibility

Who are you calling backward?

Documentation

I read it, yes.

bowd and others added 10 commits August 23, 2024 04:28
This reverts commit ac1c540.

Brings back the maxTimestampSpread setting. A visual explanation here:

<img width="918" alt="image"
src="https://github.com/user-attachments/assets/02dfaadb-f029-4f38-8bd5-965644b4a4b5">

We need it because CELO/USD has a heartbeat of 24hr and PHP/USD
5minutes, therefore we could be in a situation where the spread between
reports is quite large, and if we rely only on max timestamp spread we
could be reporting with stale data. The rule of thumb is:
- Set `maxTimestampSpread` by considering the largest heartbeat
frequency.
- Set `tokenExpiry` in SortedOracles by considering the shortest
heartbeat frequency.

N/A

Yup

N/A

N/A

N/A

---------

Co-authored-by: chapati <philip.paetz@me.com>
Co-authored-by: philbow61 <80156619+philbow61@users.noreply.github.com>
@bowd bowd mentioned this pull request Aug 26, 2024
@bowd bowd requested review from chapati23 and baroooo August 26, 2024 13:29
bowd added a commit 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>
Base automatically changed from feat/upgrade-tests-solidity-version to develop August 26, 2024 15:58
contracts/governance/GovernanceFactory.sol Show resolved Hide resolved
contracts/oracles/ChainlinkRelayerV1.sol Outdated Show resolved Hide resolved
bin/check-contracts.sh Show resolved Hide resolved
.github/workflows/lint_test.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@baroooo baroooo left a comment

Choose a reason for hiding this comment

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

@chapati23
Copy link
Contributor

image

nice new compile time

Copy link
Contributor

@chapati23 chapati23 left a comment

Choose a reason for hiding this comment

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

lgtm, just one outdated-looking outcommented line

bin/check-contracts.sh Outdated Show resolved Hide resolved
Co-authored-by: chapati <philip.paetz@me.com>
@bowd bowd merged commit d916e50 into develop Aug 27, 2024
5 checks passed
@bowd bowd deleted the feat/remove-need-for-via-ir branch August 27, 2024 11: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.

4 participants