-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update solidity, hardhat, slither CI #632
Conversation
could you fix coverage? |
b284ad9
to
98a6d30
Compare
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.
LGTM
82df34f
to
50ecd95
Compare
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.
LGTM
// Reverts with default message | ||
revert(FUNCTION_CALL_NOT_SUCCESSFUL); | ||
} | ||
} |
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.
this seems like a step backwards, I wonder why this is like this ?
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.
returnData
contains some bytes at the beginning, which are not the actual revert reason.
When it's cast to string (string(returnData)
) solidity does not really care that the bytes are not valid string bytes. So if that's then passed as a revert message, the client cannot decode it into a valid string.
For some reason, this wasn't a problem in the past (probably hardhat/ethers ignored the invalid part of the string), but now it is.
In any case, this is now the correct implementation for bubbling up the error, i.e. the error is exactly the same as the one, raised by the contract that reverted.
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.
And without this code change, some tests fail.
revert(string(error)); | ||
assembly { | ||
revert(add(32, error), mload(error)) | ||
} |
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.
This happens in a few places ...
Closes #611
The latest solidity version is 0.8.20, but 0.8.18 is the latest fully supported by hardhat (and also recommended by slither).
In addition to updating the compiler version I made these changes:
hardhat-waffle
tohardhat-toolbox
. This will allow us to use predicates when testing events. Migration lead to the following changes.JewelerLib.sol
,MetaTransactionsHandlerFacet.sol
andProtocolInitializationHandlerFacet.sol
hardhat.config.js
we don't need to require separate packages anymore@ethereum-waffle/mock-contract
so mocked contracts inbosonVoucherTest.js
still workI tried compilation viaIR, which resulted in a larger contract size. Before using that feature we need to investigate what is the proper optimization sequence.