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

Add ValidatorWalletTest.t.sol #511

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mahsamoosavi
Copy link

@mahsamoosavi mahsamoosavi commented Dec 22, 2023

This PR adds a solidity test file for the ValidatorWallet.

@mahsamoosavi mahsamoosavi changed the title Add ValidatorWalletTest.sol Add ValidatorWalletTest.t.sol Dec 22, 2023
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5223eb4) 61.86% compared to head (f029d5f) 60.58%.
Report is 5 commits behind head on main.

❗ Current head f029d5f differs from pull request most recent head f4522ea. Consider uploading reports for the commit f4522ea to get more accurate results

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              

Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

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();
Copy link
Member

Choose a reason for hiding this comment

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

unused event

Suggested change
event ZombieTriggered();

Comment on lines +16 to +17
RollupMock rollupMock1;
RollupMock rollupMock2;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need 2 rollup, if you want to test batching you can simply call the same rollup twice.

Comment on lines +20 to +21
address public executor = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
address public owner = 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2;
Copy link
Member

Choose a reason for hiding this comment

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

These are some very specific addresses lol usually we can just use sth like addr(1337); no big deal tho

Copy link
Member

Choose a reason for hiding this comment

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

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");
Copy link
Member

Choose a reason for hiding this comment

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

instead of require(cond, reason), use assertEq(a,b,reason)

@mahsamoosavi
Copy link
Author

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.

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.

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