search

ubiquity / ubiquity-dollar

Ubiquity Dollar (UUSD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
34 stars 89 forks source link

feat: switch to OpenZeppelin implementation of EnumerableSet #861

Closed gitcoindev closed 9 months ago

gitcoindev commented 9 months ago

Hi @molecula451 , @rndquu , @pavlovcik this pull request is an interesting case.

I discovered that we could potentially use EnumerableSet directly from OpenZeppelin and it would automatically increase coverage as we do not have currently tests for this library. An another solution is to implement / port tests.

I tried both solutions, ported tests in another branch here : https://github.com/ubiquity/ubiquity-dollar/compare/development...gitcoindev:ubiquity-dollar:test-enumerable-set

The tests that I ported all pass, but they do not increase coverage due to https://github.com/foundry-rs/foundry/issues/6308

Quote:

Internal library functions can be inlined by the compiler and won't get recorded in coverage. To get full coverage you'll need to make them public. We'll revisit coverage later and make it better. Closing for now.

I did change locally library functions to public and they are indeed included in coverage but I am not sure if this is what we want. I checked that currently EnumerableSet is used in dollar/libraries/LibCollectableDust.sol, dollar/libraries/LibAccessControl.sol and dollar/access/AccessControlInternal.sol .

There is one difference in the EnumerableSet library interface, the dollar implementation uses toArray() and OpenZeppelin's uses values() . Since toArray() is not used anywhere in dollar sources I switched the library to OpenZeppelin, builds fine and all tests pass. OpenZeppelin contracts are audited and are used anyway in dollar libraries.

Unless there is anything against switching to OpenZeppelin implementation of EnumerableSet I suggest to review this pull request. We can also discuss here and come up with the best solution.

Resolves: #838

gitcoindev commented 9 months ago

Development branch coverage: 70.9 PR branch coverage: 73.0

molecula451 commented 9 months ago

@gitcoindev can you open another PR with actually implementing the tests/port for the library that we already have? we'll keep this open to check bounds

rndquu commented 9 months ago

@gitcoindev can you open another PR with actually implementing the tests/port for the library that we already have? we'll keep this open to check bounds

This PR is not even about the coverage but making the code cleaner.