test: property 8 and 9 #188
Conversation
--------- Co-authored-by: Hex <[email protected]>
* chore: improve assertions * feat: add ghost vars contract
* fix: weth invariants assertion * fix: actors flow
…e to medusa error * refactor: mint super tokens on setup and remove handler to mint them * fix: some math on superweth properties
This reverts commit c37ea22.
packages/contracts-bedrock/test/invariants/interop/FuzzTest.t.sol
Outdated
Show resolved
Hide resolved
|
|
||
| assertWithMsg( | ||
| Utils.checkOverflow(address(SUPER_WETH).balance, _message.amount) | ||
| || !Utils.checkBalance(ethLiquidityEthBalanceBefore, _message.amount) |
There was a problem hiding this comment.
No need to use a function for this check
There was a problem hiding this comment.
Is more self-explanatory to just have the line there, also the name doesn't exactly reflect what the check does
| }); | ||
|
|
||
| assertWithMsg( | ||
| Utils.checkOverflow(address(SUPER_WETH).balance, _message.amount) |
There was a problem hiding this comment.
I don't like these functions. I don't think it harms to repeat that line wherever needed, since now you have to read the checkOverflow function to fully understand the check, and what is _a, what is _b?
This looks cleaner and simpler IMO
// Check overflow
address(SUPER_WETH).balance > type(uint256).max - _message.amount
| if (success) { | ||
| assert(address(ETH_LIQUIDITY).balance == ethLiquidityEthBalanceBefore + _amount); | ||
| } else { | ||
| assertWithMsg(false, "Unknown Revert Error"); |
There was a problem hiding this comment.
Jic it reverts with another error
| assertWithMsg(false, "Unknown Revert Error"); | |
| assert(false); |
There was a problem hiding this comment.
Also, why assert(false) and not checking overflow here?
There was a problem hiding this comment.
Ah, becasue it is a precondition. I'd rather remove it from pre-condition and add it to the assertion since that's the property we're testing
| Utils.checkOverflow(address(SUPER_WETH).balance, _message.amount) | ||
| || !Utils.checkBalance(ethLiquidityEthBalanceBefore, _message.amount) | ||
| || L2_TO_L2_MESSENGER.successfulMessages(messageHash), // Already relayed | ||
| "Unkonwn Revert Error" |
There was a problem hiding this comment.
I'm unsure about this. Why do we need to check for the "Unkonwn Revert Error" msg?
Does it have any advantage? I think we could be omitting the case where it reverts with one of the expected conditions, but with another log - and we don't really care about the log of Medusa on that check.
Wdyt?
Co-authored-by: Disco <[email protected]>
… into test/property-8
CLOSES OPT-656