Bridgehub Test Coverage Increase #494
Conversation
neotheprogramist
requested review from
vladbochok and
StanislavBreadless
as code owners
l1-contracts/test/foundry/unit/concrete/Bridgehub/experimental_bridge.t.sol
Outdated
Show resolved
Hide resolved
l1-contracts/test/foundry/unit/concrete/Bridgehub/experimental_bridge.t.sol
Outdated
Show resolved
Hide resolved
neotheprogramist
changed the title
neotheprogramist
changed the title
tommysr
force-pushed
the
bridgehub-test-coverage-increase
branch
from
159fb4f to
f799fa0
Compare
tommysr
force-pushed
the
bridgehub-test-coverage-increase
branch
2 times, most recently
from
417a1d6 to
9ea11f4
Compare
|
Coverage: 88.01% (lines) | 88.60% (statements) | 75.98% (branches) | 85.09% (funcs) |
| secondBridge.acceptOwnership(); | ||
|
|
||
| sharedBridgeAddress = address(sharedBridge); | ||
| secondBridgeAddress = address(secondBridge); | ||
| testToken = new TestnetERC20Token("ZKSTT", "ZkSync Test Token", 18); |
There was a problem hiding this comment.
Can we aim for tests with tokens that have decimals other than 18, such as 6 (USDC) , cTokens from Compound and WBTC have 8, etc.
There was a problem hiding this comment.
Added tokens with different decimals, I've used modifier to choose random token.
| _chainId: 1, | ||
| _stateTransitionManager: address(mockSTM), | ||
| _baseToken: address(testToken), | ||
| _salt: uint256(123), |
There was a problem hiding this comment.
Please fuzz the tests as much as you can. In this case, try and provide the _chainId parameter as a fuzzed number bounded to be between 1 and type(uint48).max and similarly for the _salt parameter.
There was a problem hiding this comment.
I've fuzzed chain ids. Can you confirm if salt spans the whole uint256 range?
|
|
||
| vm.deal(randomCaller, l2TxnReqDirect.mintValue); | ||
| vm.txGasPrice(0.05 ether); |
There was a problem hiding this comment.
Good approximation, but ideally we would this value to be fuzzed as well. You can upper bound the values to be in a realistic range.
| @@ -641,8 +879,15 @@ contract ExperimentalBridgeTest is Test { | |||
| mockChainContract.setBridgeHubAddress(address(bridgeHub)); | |||
| assertTrue(mockChainContract.getBridgeHubAddress() == address(bridgeHub)); | |||
|
|
|||
| vm.txGasPrice(0.05 ether); | |||
| if (msgValue != mockMintValue) { | |||
There was a problem hiding this comment.
can it be if(msgValue < mockMintValue) instead of the strict equality used here?
There was a problem hiding this comment.
Require statement has strict equality, so I guess strict inequality fits here. I've separated success/revert case, moved revert into separate function and used vm.assume to discard equal values.
tommysr
force-pushed
the
bridgehub-test-coverage-increase
branch
from
8ef6604 to
be07ab8
Compare
* revert bridgehub changes * removed dummy bridge from erc20bridge tests * replaced dummy shared bridge with actual bridge
tommysr
force-pushed
the
bridgehub-test-coverage-increase
branch
from
e8f1972 to
ebc08c1
Compare
What ❔
Increased coverage in bridgehub unit tests, add some cases for two bridges deposit.
Changed order of assert in Bridgehub contract.
Why ❔
Previous tests had low coverage on branches, asserts and types of deposits, theirs permutations.
Checklist