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

feat(coverage): library support #3128

Merged
merged 3 commits into from Sep 8, 2022
Merged

feat(coverage): library support #3128

merged 3 commits into from Sep 8, 2022

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Sep 8, 2022

Motivation

Adds coverage support for libraries, of which there are two types:

  • Libraries with external functions boil down to delegatecall operations from the user of the library
  • Libraries with internal functions have their functions inlined/are called via a jump instruction

Of course some libraries may be a mix.

For the first type of libraries, we were already kind of supporting it except we needed to replace all library address placeholders with a dummy address in order to produce a valid bytecode we can analyze. This was added previously but a minor error in the implementation made it non functional.

For internal libraries we need to walk through the AST of contracts that use them and find calls to the library. This is not super trivial to do since there are many ways to use libraries - calling them directly, importing them using an alias, using using for etc.

At first I wanted to finish the strongly typed AST in ethers-rs, but I've become somewhat uncertain that it's a good idea for now since breaking changes in unrelated parts of the AST might render coverage completely unusable for new versions of Solidity, and I haven't found a good way around that yet.

Solution

  • Fix the dummy link step to add coverage to libraries with external functions
  • Walk through the AST to find calls to library functions for internal libraries and note down the AST node ID of the called library. Currently all of these are assumed to be in FunctionCall nodes where the expression attribute of those nodes are a MemberAccess node.

This does not currently handle cases where using for is used for internal libraries since it seems like we lose information on what libraries were bound to what types..

Ref #2567

This PR can be validated with the updated tests I've added to https://github.com/onbjerg/forge-coverage-test (or the report can be viewed here https://f3128.tiiny.site/), note that the using for test case shows up as uncovered (InternalLibrary.subPlusOne) even though it is covered (called from the contract in Contract2.sol).

Taking the link references makes it impossible to link
the object, which broke calls to external library
functions
Attempt at collecting calls to libraries without
using the strongly typed AST in ethers-rs until
I find a less fragile way to implement that

This *should* catch most library calls, but
some edge cases need to be tested:

- Referencing across files
- Referencing with an alias
- Calls to libraries that have been bound
  using `using for`
@onbjerg onbjerg changed the title Onbjerg/coverage libs feat(coverage): library support Sep 8, 2022
@onbjerg onbjerg added the T-feature Type: feature label Sep 8, 2022
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!

@onbjerg onbjerg merged commit e93144b into master Sep 8, 2022
@onbjerg onbjerg deleted the onbjerg/coverage-libs branch September 8, 2022 14:23
@gakonst
Copy link
Member

gakonst commented Sep 8, 2022

@transmissions11 wanna give this a spin?

@ZeroEkkusu
Copy link
Contributor

ZeroEkkusu commented Sep 8, 2022

Some feedback

I wanted to make a simple contract that exposes library functions for coverage to pick up.

The following approaches do not work. The reported coverage for the library is 0%:

contract LibHelper {
    function increment(uint256 a) external returns (uint256) {
        return Lib.increment(a);
    }
}
contract LibHelper {
    function increment(uint256 a) external returns (uint256 r) {
        r = Lib.increment(a);
    }
}

This approach works:

contract LibHelper {
    function increment(uint256 a) external returns (uint256) {
        uint256 r = Lib.increment(a);
        return r;
    }
}

This also works, but is not useful in testing:

contract LibHelper {
    function increment(uint256 a) external {
        Lib.increment(a);
    }
}

@gakonst
Copy link
Member

gakonst commented Sep 8, 2022

@onbjerg suspect we're not catching the typedef in the function sig?

@onbjerg
Copy link
Member Author

onbjerg commented Sep 9, 2022

I think we're just not walking the expression on the right side of an assignment

@gakonst
Copy link
Member

gakonst commented Sep 9, 2022

Should we track this in a separate ticket for followup?

iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* fix(coverage): fix dummy linking

Taking the link references makes it impossible to link
the object, which broke calls to external library
functions

* feat(coverage): collect library calls

Attempt at collecting calls to libraries without
using the strongly typed AST in ethers-rs until
I find a less fragile way to implement that

This *should* catch most library calls, but
some edge cases need to be tested:

- Referencing across files
- Referencing with an alias
- Calls to libraries that have been bound
  using `using for`

* fix(coverage): base contract determinism
antoncoding added a commit to lyra-finance/v2-core that referenced this pull request Dec 12, 2022
## Summary

* Deposit USDC into the Lending contract and create balance

## Details

* Update to `handleAdjustment`: add manager check
* the `deposit` function simply convert token amount => 18 decimals, and
update that value in `Account`.
* Add `DecimalMath` library that convert decimals for `uint256`
* in `DecimalMathTest` test: it needs to setup a "helper contract" to
let the forge coverage work properly with internal library.
([reference](foundry-rs/foundry#3128 (comment)))

## Todo

- [x] decimal conversion for `deposit`
- [x] Unit tests for `deposit`
- [x] 100% coverage for `deposit` and `convertDecimals`

## Checklist

Ensure you completed **all of the steps** below before submitting your
pull request:

- [x] Add
[natspec](https://docs.soliditylang.org/en/latest/natspec-format.html)
for all functions / parameters
- [x] Ran `forge snapshot`
- [x] Ran `forge fmt`
- [x] Ran `forge test`
- [x] [Triage Slither issues](../README.md#triage-issues), and post
uncertain ones in the PR
- [x] 100% test coverage on code changes

### Slither Issues (Optional)

If you're unsure about a new issue reported by Slither, copy them here
so others can verify as well.
@PaulRBerg
Copy link
Contributor

PaulRBerg commented Feb 8, 2023

Should we track this in a separate ticket for followup?

Definitely. Opened an issue here:

#4305

0xrajath added a commit to llamaxyz/llama that referenced this pull request Mar 30, 2023
**Motivation:**

Currently the coverage report for the Checkpoints Library shows up as
0%.
We need to fix it by refactoring the Checkpoints Mock in
`Checkpoints.t.sol` as stated in
foundry-rs/foundry#3128 (comment)

**Modifications:**

* Refactoring Checkpoints Mock from:

```
contract LibHelper {
    function increment(uint256 a) external returns (uint256 r) {
        r = Lib.increment(a);
    }
}
```

to 

```
contract LibHelper {
    function increment(uint256 a) external returns (uint256) {
        uint256 r = Lib.increment(a);
        return r;
    }
}
```

* Removing `using for` notation in Checkpoints Mock
* Setting coverage threshold back to 80%

**Result:**

Increased code coverage for Checkpoints Library. Closes #184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants