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

THE BIG ONE 🐘 💣 🚝 #501

Merged
merged 62 commits into from
Aug 26, 2024
Merged

Conversation

bowd
Copy link
Contributor

@bowd bowd commented Aug 22, 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 feat: compilation restrictions 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

@RyRy79261
Copy link
Contributor

yarn fork-test

image

@RyRy79261
Copy link
Contributor

yarn fork-test:alfajores
image

@bowd
Copy link
Contributor Author

bowd commented Aug 26, 2024

@RyRy79261 tackled the fork tests here: #503 because it felt like a bigger beast and worth keeping separate. The reasons these were failing here weren't related to the changes in the branch, they were just a bit flakey, Hopefully you should see all green on the other PR.

bowd and others added 2 commits August 26, 2024 12:56
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 requested a review from bayological August 26, 2024 13:52
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.

🫨 🫨 🚢 🚢
I noticed it sometime takes time for already built contracts between running the forge test and compilation starts(2-3 mins) and also between compilation ends and tests run(2-3 mins). It basically stays there without any feedback. I guess that is caused by the foundry version and won't be an issue after the latest PR of eliminating the need for viaIR.

contracts/oracles/ChainlinkRelayerV1.sol Outdated Show resolved Hide resolved
@bowd
Copy link
Contributor Author

bowd commented Aug 26, 2024

@baroooo yeah I noticed that as well (and commented on the foundry WIP branch about it) but yeah like you said it's not a problem cose it will go away completely with the other PR.

@bowd bowd merged commit 45a551d into develop Aug 26, 2024
5 checks passed
@bowd bowd deleted the feat/upgrade-tests-solidity-version branch August 26, 2024 15:58
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