Add unit tests to increase test coverage

[garyghayrat]

closes #117

Before:

After:

The uncovered lines and statements in StakerPermitAndStake.sol seems to be due to the try/catch block in

  try IERC20Permit(address(STAKE_TOKEN)).permit(
      msg.sender, address(this), _amount, _deadline, _v, _r, _s
    ) {} catch {}

which we already test with valid and invalid signatures.

[apbendi]

Hey @garyghayrat, have not done a full review, but just wanted to address your comment about the remaining untested blocks:

The uncovered lines and statements in StakerPermitAndStake.sol seems to be due to the try/catch block in which we already test with valid and invalid signatures.

This is not quite right! The point of the Try/Catch is to prevent a griefing attack where someone pulls out the signature and executes the permit directly ahead of the actual permitAnd function call. This would cause the original transaction to fail, even though the allowance had been created, which is why we include the try/catch blocks—with them included, the transaction would proceed despite the fact the permit call was frontrun.

To test this, you should create a test that simulates this, i.e. generate the signature, the submit it directly on the token contract first before calling the permitAnd function. The test actually wouldn't need an assert, since we're just ensure the call does not revert.

[github-actions]

Coverage after merging chore/increase-coverage into main will be

99.54%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
DelegationSurrogate.sol	100%	100%	100%	100%
DelegationSurrogateVotes.sol	100%	100%	100%	100%
Staker.sol	100%	100%	100%	100%
src/calculators
BinaryEligibilityOracleEarningPowerCalculator.sol	100%	100%	100%	100%
IdentityEarningPowerCalculator.sol	100%	100%	100%	100%
src/extensions
StakerCapDeposits.sol	100%	100%	100%	100%
StakerDelegateSurrogateVotes.sol	100%	100%	100%	100%
StakerOnBehalf.sol	100%	100%	100%	100%
StakerPermitAndStake.sol	88.24%	100%	100%	84.62%	50, 78
src/notifiers
MintRewardNotifier.sol	100%	100%	100%	100%
RewardTokenNotifierBase.sol	100%	100%	100%	100%
TransferFromRewardNotifier.sol	100%	100%	100%	100%
TransferRewardNotifier.sol	100%	100%	100%	100%

[garyghayrat]

Hey @garyghayrat, have not done a full review, but just wanted to address your comment about the remaining untested blocks:

The uncovered lines and statements in StakerPermitAndStake.sol seems to be due to the try/catch block in which we already test with valid and invalid signatures.

This is not quite right! The point of the Try/Catch is to prevent a griefing attack where someone pulls out the signature and executes the permit directly ahead of the actual permitAnd function call. This would cause the original transaction to fail, even though the allowance had been created, which is why we include the try/catch blocks—with them included, the transaction would proceed despite the fact the permit call was frontrun.

To test this, you should create a test that simulates this, i.e. generate the signature, the submit it directly on the token contract first before calling the permitAnd function. The test actually wouldn't need an assert, since we're just ensure the call does not revert.

That's good to know, and I've added the test cases you mentioned in 2eaa2b5. But I meant the coverage report summary shows that there are 4 statements (I assume the 2 try catch blocks) are not covered in StakerPermitAndStake.sol. And I've tried using a token contract that doesn't have the permit function so permitAndStake reverts and the catch block is run, but the test coverage report is still showing the try/catch lines as uncovered.

[apbendi]

Hey @garyghayrat we have these same try/catch blocks in UniStaker and the lines are covered there. I'd suggest testing out the test suite there to see how it's done.

[apbendi]

This looks good @garyghayrat. Given we have to wait for Foundry to fix foundry-rs/foundry#9921 before we can get up to 100, let's create an issue to track that work so we remember to come back to it. Otherwise, this is good to merge IMO!