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

Colin@verif/setup #19

Merged
merged 37 commits into from
Sep 30, 2024
Merged

Colin@verif/setup #19

merged 37 commits into from
Sep 30, 2024

Conversation

colin-morpho
Copy link
Contributor

@colin-morpho colin-morpho commented Sep 20, 2024

This PR provides a verification setup for the PreLiquidation contract.

Please make sure of the following before accepting this PR:

  • the verification succeeds;
  • the README file has been updated to provide some documentation;
  • the CI has been updated as required.

Related issue #12 .

@colin-morpho colin-morpho added 👷‍♀️ work in progress verification Fromal verification with Certora labels Sep 20, 2024
@colin-morpho colin-morpho self-assigned this Sep 20, 2024
@colin-morpho colin-morpho linked an issue Sep 20, 2024 that may be closed by this pull request
8 tasks
@colin-morpho colin-morpho mentioned this pull request Sep 20, 2024
4 tasks
@colin-morpho colin-morpho linked an issue Sep 20, 2024 that may be closed by this pull request
4 tasks
@colin-morpho colin-morpho changed the base branch from main to feat/licence September 20, 2024 12:55
.gitignore Outdated Show resolved Hide resolved
certora/README.md Outdated Show resolved Hide resolved
certora/README.md Outdated Show resolved Hide resolved
certora/README.md Outdated Show resolved Hide resolved
certora/README.md Show resolved Hide resolved
@colin-morpho colin-morpho marked this pull request as draft September 20, 2024 15:25
MathisGD
MathisGD previously approved these changes Sep 23, 2024
Copy link
Contributor

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

Setup is good

Base automatically changed from feat/licence to main September 25, 2024 14:54
@peyha peyha dismissed MathisGD’s stale review September 25, 2024 14:54

The base branch was changed.

@MathisGD
Copy link
Contributor

MathisGD commented Sep 27, 2024

the CI isn't passing anymore. Btw, now we could merge on main directly to avoid having to sync all the time

@colin-morpho
Copy link
Contributor Author

The CI isn't passing because, in this branch, the implementation of PreLiquidation is out of date with the harness which has been removed in newer branches. Said newer branches are blocking because a verification is failing. The Certora team should provide assistance on the failing task. I'll tidy up all of this so that we can merge this branch.

@QGarchery
Copy link
Contributor

Said newer branches are blocking because a verification is failing. The Certora team should provide assistance on the failing task.

What's the task that is blocking ?

I think you can merge main into this branch anyway right ? No need to wait to resolve other failures

certora/README.md Outdated Show resolved Hide resolved
certora/README.md Outdated Show resolved Hide resolved
certora/confs/Reentrancy.conf Show resolved Hide resolved
certora/specs/Reentrancy.spec Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@colin-morpho
Copy link
Contributor Author

colin-morpho commented Sep 29, 2024

@QGarchery

What's the task that is blocking ?

The goal that is failing is the property that shows that a pre-liquidation is created iff the market exists in the PR #37 .

I think you can merge main into this branch anyway right ? No need to wait to resolve other failures

The branch main has been merged in this branch, this is not the issue. Perhaps the situation is unclear.

As talked over with @MathisGD, this branch is meant as a develop branch that lives in parallel to the solidity development. This helps me maintaining a clean setup and to stay close to upstream branches whilst syncing with upstream changes only when they are somewhat stable. That means that fixes with respect to upstream changes are done in branches that target colin@verif/setup, namely #37. You may think of it as a the principal branch of the verification to target pull requests.

If you want to me adopt another workflow just let me know which are your recommendations to organize the work. I chose this as there was no given workflow or recommended setup.

@MathisGD
Copy link
Contributor

MathisGD commented Sep 30, 2024

I was probably unclear when we discussed this, but this isn't what I had in mind. To me now we should merge this branch in main. This has the benefit of making it clear for other devs and you that we are introducing a change that's breaking the verification, and avoid you to have to merge main in this branch all the time.

The parallel branch was for when the code wasn't even on main, so having the verif in the same PR would have been weird.

Does that work for you?

@colin-morpho
Copy link
Contributor Author

Sure! I'll fix this so we can merge it then.

@colin-morpho
Copy link
Contributor Author

colin-morpho commented Sep 30, 2024

@MathisGD and @QGarchery CI is fixed in 7463846.

@QGarchery QGarchery marked this pull request as ready for review September 30, 2024 09:57
@colin-morpho colin-morpho merged commit 1be0ead into main Sep 30, 2024
3 checks passed
@colin-morpho colin-morpho deleted the colin@verif/setup branch September 30, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done verification Fromal verification with Certora
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formal verification Setup Certora
3 participants