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.

Sign up for GitHub

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

Jump to bottom

Get to 100% coverage for smart contracts #13831

Open
julien51 opened this issue May 13, 2024 · 10 comments · May be fixed by #14326
Open

Get to 100% coverage for smart contracts #13831

julien51 opened this issue May 13, 2024 · 10 comments · May be fixed by #14326
Assignees
@deepsea514
Labels
💰bounty There's a bounty on this issue!

Comments

@julien51
Copy link
Member

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 julien51 added the 💰bounty There's a bounty on this issue! label May 13, 2024
@julien51
Copy link
Member Author

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

@deepsea514
Copy link
Contributor

@julien51 I want to work on this issue.

@julien51
Copy link
Member Author

@deepsea514 we will attach a 300 USDC bounty here!

@deepsea514
Copy link
Contributor

deepsea514 commented Jul 24, 2024 •
edited
Loading

@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
Copy link
Contributor

deepsea514 commented Jul 29, 2024 •
edited
Loading

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

@deepsea514 deepsea514 linked a pull request Jul 29, 2024 that will close this issue
Open
6 tasks
@julien51
Copy link
Member Author

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

@deepsea514
Copy link
Contributor

I mean these contracts.

  • contracts/tokens/UDT/UnlockProtocolGoverner.sol and contracts/tokens/UP/UPGoverner.sol
  • contracts/tokens/UDT/UnlockProtocolTimelock.sol and contracts/tokens/UP/UPTimlock.sol

@deepsea514
Copy link
Contributor

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

@deepsea514
Copy link
Contributor

@julien51
Could you please comment?

@deepsea514
Copy link
Contributor

deepsea514 commented Aug 22, 2024 •
edited
Loading

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deepsea514 deepsea514

Labels
💰bounty There's a bounty on this issue!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update coverage of test deepsea514/unlock-protocol
2 participants
@julien51 @deepsea514