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

Audit #30

Merged
merged 7 commits into from
Apr 15, 2022
Merged

Audit #30

merged 7 commits into from
Apr 15, 2022

Conversation

pegahcarter
Copy link
Contributor

No description provided.

Copy link
Member

@0xAlcibiades 0xAlcibiades left a comment

Choose a reason for hiding this comment

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

Aside from removing the slither output, which we can run at any time, or even add to the CI pipeline (ideally) this looks good for an incremental, but hasn't really increased coverage.

@@ -43,18 +43,27 @@ contract OptionSettlementTest is DSTest, NFTreceiver {
OptionSettlementEngine public engine;

address public immutable ac = 0x36273803306a3C22bc848f8Db761e974697ece0d;
address public immutable wethAddress =
address public WETH =
Copy link
Member

Choose a reason for hiding this comment

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

Any reason these cant be immutable?

@@ -0,0 +1,105 @@
```
Copy link
Member

Choose a reason for hiding this comment

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

No reason to include this here, can run slither on demand as needed, but also not useful to run on the libraries/false positive on the base64 encode

Copy link
Member

Choose a reason for hiding this comment

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

See the commented out slither command in the CI pipeline, would be preferable to just get that working and have the output on each pipeline.

.solhintignore Outdated
@@ -0,0 +1 @@
test/OptionSettlement.t.sol
Copy link
Member

Choose a reason for hiding this comment

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

Ignoring the tests is not the way, why not just make them pass lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough - I will revise as so

@0xAlcibiades 0xAlcibiades merged commit 28fc1be into valorem-labs-inc:master Apr 15, 2022
@pegahcarter pegahcarter deleted the audit branch April 19, 2022 15:53
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.

2 participants