-
Notifications
You must be signed in to change notification settings - Fork 3
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
refactor: using require instead of revert where possible #107
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
===========================================
- Coverage 98.62% 84.43% -14.20%
===========================================
Files 12 12
Lines 364 379 +15
Branches 11 67 +56
===========================================
- Hits 359 320 -39
- Misses 5 25 +20
- Partials 0 34 +34 ☔ View full report in Codecov by Sentry. |
emit PacketCommitted(path, commitment); | ||
commitments[path] = commitment; | ||
emit PacketCommitted(path, commitment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes more logical sense for this to be at the end. This event should probably be removed on another issue to clean up unnecessary events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added an issue for implementing spec events, and specified in that issue that we should clean up events all over the place at the same time: #108
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I very much like this syntax better. Mostly reads a lot better, which I love :D
LGTM!
.github/workflows/foundry.yml
Outdated
@@ -80,8 +80,11 @@ jobs: | |||
uses: ./.github/setup | |||
|
|||
- name: "Run coverage" | |||
run: "bash scripts/checks/coverage.sh" | |||
run: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes more sense
@@ -9,6 +9,7 @@ | |||
"named-parameters-mapping": "warn", | |||
"no-console": "off", | |||
"not-rely-on-time": "off", | |||
"one-contract-per-file": "off" | |||
"one-contract-per-file": "off", | |||
"gas-custom-errors": "off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why this one is necessary right now, but I created an issue to re-introduce this one as soon as this is fixed in solhint: #109
Description
closes: #7
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.