Add ValidatorWalletTest.t.sol #511
Conversation
mahsamoosavi
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #511 +/- ##
==========================================
- Coverage 61.86% 60.58% -1.28%
==========================================
Files 43 43
Lines 6236 6394 +158
==========================================
+ Hits 3858 3874 +16
- Misses 2329 2471 +142
Partials 49 49 |
There was a problem hiding this comment.
Left some comment then realized you are trying to replicate the existing ts tests, it is good on that regard but I think we can also improve the test coverage here. Try to test all the function and error in the ValidatorWallet contract while reduce redundancy when appropriate.
|
|
||
| contract RollupMock { | ||
| event WithdrawTriggered(); | ||
| event ZombieTriggered(); |
There was a problem hiding this comment.
unused event
| event ZombieTriggered(); |
| RollupMock rollupMock1; | ||
| RollupMock rollupMock2; |
There was a problem hiding this comment.
I don't think we need 2 rollup, if you want to test batching you can simply call the same rollup twice.
| address public executor = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4; | ||
| address public owner = 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2; |
There was a problem hiding this comment.
These are some very specific addresses lol usually we can just use sth like addr(1337); no big deal tho
There was a problem hiding this comment.
We already have this in
./contracts/src/test-helpers/RollupMock.sol
| emit ExecutorUpdated(executor, true); | ||
| vm.prank(owner); | ||
| wallet.setExecutor(newExecutors, isExecutor); | ||
| require(wallet.executors(executor), "Executor should be marked as executor"); |
There was a problem hiding this comment.
instead of require(cond, reason), use assertEq(a,b,reason)
Thanks Harry! Yep, the idea was to replicate the existing ts tests at first just to get the hang of it, and then I'll expand things. I'll definitely add more test cases once I tackle your comments. |
This PR adds a solidity test file for the ValidatorWallet.