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 (most) forge compilation warnings #960

Merged
merged 2 commits into from
Jul 11, 2022
Merged

Fix (most) forge compilation warnings #960

merged 2 commits into from
Jul 11, 2022

Conversation

eswak
Copy link
Contributor

@eswak eswak commented Jul 7, 2022

This PR fixes most compilation warnings when running npm run compile (forge compilation).

Only .sol changes that won't change the contract's bytecode are included.

Most warnings were in Mock contracts or forge test files.

There are a bunch of warnings left during compilation, detailed below :

  • 12 warnings in node_modules/@orcaprotocol/contracts/...., not much we can do about it without copying the Solidity in our own repo, so I did not take this decision. Tom notified Orca about it (but it's not a big deal).

  • 5 warnings in WETH9.sol, it's using an old version of Solidity and uses this.balance instead of address(this).balance and does not prefix event emissions with the emit keyword, but I didn't change because this contract compiles in Solidity 0.4.18

  • 1 warning in PSMRouter.sol "This contract has a payable fallback function, but no receive ether function. Consider adding a receive ether function.", I did not fix because it would change the bytecode of the contract. PSMRouter is not used anywhere and the only mainnet deployment is deprecated because it's a wrapper above the ETH PSM that is itself deprecated (I'm doing a separate PR to deprecate the ethPSMRouter)

  • 2 warnings in FuseGuardian.sol, I think these are important warnings because the Solidity code does not have the intended behavior and I think this was missed because of all the "warnings pollution" and nobody reading warnings anymore. The functions _setBorrowPaused and _setMintPausedByUnderlying return booleans, but they always return false, instead of the result of the internal functions. I don't think there's any harm in it, and I did not fix the warnings because it would introduce bytecode changes, but it's definitely not the intended behavior. Reducing the amounts of warnings will allow us to catch this kind of errors in the future.

@eswak eswak requested a review from a team as a code owner July 7, 2022 10:04
@eswak eswak changed the title Fix (most) compilation warnings Fix (most) forge compilation warnings Jul 7, 2022
@eswak
Copy link
Contributor Author

eswak commented Jul 11, 2022

Rebased to remove commits that fixed compilation warnings that were on live contracts.

Only Mock & Test contracts are fixed here.

Copy link
Contributor

@thomas-waite thomas-waite left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for fixing these! I've also given Orca the heads up about their warnings, they're taking a look

@eswak eswak merged commit f8e7ddb into develop Jul 11, 2022
@eswak eswak deleted the qol/fix-warnings branch July 11, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants