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

fix(ctb): N-03 Assert for impossible condition #3449

Merged
merged 5 commits into from
Sep 19, 2022
Merged

fix(ctb): N-03 Assert for impossible condition #3449

merged 5 commits into from
Sep 19, 2022

Conversation

maurelian
Copy link
Contributor

Description

Uses assert rather than a require statements to check for conditions we believe are unreachable.

This is more semantically explicit, and should enable us to more effectively use some advanced analysis methods in our testing.

IMO it's also more readable.

Tests

We don't have a good harness in place for verifying that code is unreachable. Making use of Solidity's SMTChecker would be good to do eventually.

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2022

🦋 Changeset detected

Latest commit: f873a73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts-bedrock Patch
@eth-optimism/actor-tests Patch
@eth-optimism/sdk Patch
@eth-optimism/drippie-mon Patch
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes
Copy link
Contributor

tynes commented Sep 13, 2022

100% on getting the SMT checker going. Do you know of anything better than the solidity docs for an intro on getting going with it?

@maurelian
Copy link
Contributor Author

This looks interesting, though not directly useful without installing dapp: https://fv.ethereum.org/2021/12/01/smtchecker-dapptools/

For what little I've used it in the past I've just referred to the official docs.

@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2022

Hey @maurelian! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Sep 15, 2022
@tynes
Copy link
Contributor

tynes commented Sep 19, 2022

This looks like it needs a rebase before merge, but the code looks good to me

@mergify mergify bot removed the conflict label Sep 19, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit 88dde7c into develop Sep 19, 2022
@mergify mergify bot deleted the ctb/assert branch September 19, 2022 22:41
@mergify
Copy link
Contributor

mergify bot commented Sep 19, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pkg-contracts-bedrock Area: packages/contracts-bedrock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants