search

unlock-protocol / unlock

Ʉnlock is a protocol for memberships built on a blockchain.
https://unlock-protocol.com
MIT License
837 stars 246 forks source link

Get to 100% coverage for smart contracts #13831

Open julien51 opened 4 months ago

julien51 commented 4 months ago

Our smart-contracts have a very good test suite, but the coverage is not 100%. Here is the report as of May 13th:

------------------------------|----------|----------|----------|----------|----------------|
File                          |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------------------|----------|----------|----------|----------|----------------|
 contracts/                   |    66.17 |    53.64 |    77.78 |    65.08 |                |
  CardPurchaser.sol           |        0 |        0 |        0 |        0 |... 171,197,198 |
  KeyManager.sol              |      100 |       90 |      100 |      100 |                |
  PublicLock.sol              |      100 |      100 |      100 |      100 |                |
  Unlock.sol                  |    78.82 |    63.51 |    84.38 |    77.78 |... 650,679,704 |
  UnlockProtocolTimelock.sol  |      100 |       50 |      100 |      100 |                |
 contracts/hooks/             |    86.05 |    79.73 |    75.44 |    85.98 |                |
  CaptchaHook.sol             |    82.35 |       50 |       70 |    82.61 |    18,35,60,64 |
  DiscountCodeHook.sol        |    90.91 |       80 |    77.78 |     87.1 |  27,30,102,106 |
  ERC1155BalanceOfHook.sol    |      100 |      100 |      100 |      100 |                |
  ERC20BalanceOfHook.sol      |      100 |      100 |      100 |      100 |                |
  ERC721BalanceOfHook.sol     |      100 |      100 |      100 |      100 |                |
  GitcoinHook.sol             |     87.5 |       75 |       80 |    90.91 |          59,63 |
  GuildHook.sol               |    82.35 |       75 |       80 |    86.96 |       35,60,64 |
  PasswordRequiredHook.sol    |    86.67 |       75 |    77.78 |    86.96 |       22,68,72 |
  TokenUriHook.sol            |        0 |        0 |        0 |        0 |... 25,35,36,38 |
 contracts/interfaces/        |      100 |      100 |      100 |      100 |                |
  IMintableERC20.sol          |      100 |      100 |      100 |      100 |                |
  IPermit2.sol                |      100 |      100 |      100 |      100 |                |
  IPublicLock.sol             |      100 |      100 |      100 |      100 |                |
  ISwapBurner.sol             |      100 |      100 |      100 |      100 |                |
  IUSDC.sol                   |      100 |      100 |      100 |      100 |                |
  IUniswapOracleV3.sol        |      100 |      100 |      100 |      100 |                |
  IUniversalRouter.sol        |      100 |      100 |      100 |      100 |                |
  IUnlock.sol                 |      100 |      100 |      100 |      100 |                |
  IWETH.sol                   |      100 |      100 |      100 |      100 |                |
 contracts/interfaces/hooks/  |      100 |      100 |      100 |      100 |                |
  ILockKeyCancelHook.sol      |      100 |      100 |      100 |      100 |                |
  ILockKeyExtendHook.sol      |      100 |      100 |      100 |      100 |                |
  ILockKeyGrantHook.sol       |      100 |      100 |      100 |      100 |                |
  ILockKeyPurchaseHook.sol    |      100 |      100 |      100 |      100 |                |
  ILockKeyTransferHook.sol    |      100 |      100 |      100 |      100 |                |
  ILockTokenURIHook.sol       |      100 |      100 |      100 |      100 |                |
  ILockValidKeyHook.sol       |      100 |      100 |      100 |      100 |                |
 contracts/mixins/            |    98.78 |    91.82 |    98.92 |    97.29 |                |
  MixinConvenienceOwnable.sol |      100 |      100 |      100 |      100 |                |
  MixinDisable.sol            |      100 |      100 |      100 |      100 |                |
  MixinERC721Enumerable.sol   |      100 |      100 |      100 |      100 |                |
  MixinErrors.sol             |      100 |      100 |      100 |      100 |                |
  MixinFunds.sol              |      100 |    66.67 |      100 |    88.89 |             32 |
  MixinGrantKeys.sol          |      100 |      100 |      100 |      100 |                |
  MixinKeys.sol               |    96.34 |    95.31 |    96.67 |     96.3 |... 147,159,160 |
  MixinLockCore.sol           |      100 |    95.83 |      100 |    98.11 |            121 |
  MixinLockMetadata.sol       |      100 |      100 |      100 |      100 |                |
  MixinPurchase.sol           |      100 |    94.44 |      100 |    98.95 |            263 |
  MixinRefunds.sol            |      100 |      100 |      100 |      100 |                |
  MixinRoles.sol              |      100 |    66.67 |      100 |      100 |                |
  MixinTransfer.sol           |    98.28 |    79.41 |      100 |     93.9 |... 202,299,328 |
------------------------------|----------|----------|----------|----------|----------------|
All files                     |    88.64 |    79.21 |    87.18 |    87.76 |                |
------------------------------|----------|----------|----------|----------|----------------|

IMO the priority should be on Unlock.sol, as well as contracts/mixins/. Once these are cleared, we should look at CardPurchaser.sol and contracts/hooks/

julien51 commented 4 months ago

Checking coverage can be done with yarn workspace @unlock-protocol/smart-contracts coverage

deepsea514 commented 2 months ago

@julien51 I want to work on this issue.

julien51 commented 2 months ago

@deepsea514 we will attach a 300 USDC bounty here!

deepsea514 commented 2 months ago

@julien51 There are some codes uncovered because of mainnet features. And also deprecated functionalities because of upgrading. This will prevent to make coverage 100%. How should I handle it?

deepsea514 commented 2 months ago

@julien51 I don't understand the usage of UnlockProtocolGoverner.sol and UnlockProtocolTimeLock.sol. It seems they are duplicated.

julien51 commented 2 months ago

They are not. Please read our docs and the open Zeppelin docs to distinguish between a governor and a time lock contract.

deepsea514 commented 2 months ago

I mean these contracts.

deepsea514 commented 2 months ago

And tests for CardPurchaser.sol is working on mainnet only. It is skipped by you. @julien51

deepsea514 commented 2 months ago

@julien51 Could you please comment?

deepsea514 commented 1 month ago

@julien51 Here is the lines to prevent the make coverage 100%

Unlock.sol

Deprecated Impl (L502) Mainnet Feature (L623)

hooks/GitcoinHook.sol

Unused Private Functions (L59)

hooks/GuildHook.sol

Unused Private Functions (L60)

hooks/PasswordRequiredHook.sol

Unused Private Functions (L68)

mixins/MixinKeys.sol

Mainnet Feature (L139)

mixins/MixinLockMetadata.sol

Cannot be called in hardhat because migrate is done only mainnet (L70)

tokens/UP/UPGoverner.sol

Unused private function (L132)

I have left same comment on the pr