Skip to content

Conversation

@Padraic-O-Mhuiris
Copy link
Contributor

@Padraic-O-Mhuiris Padraic-O-Mhuiris commented Feb 13, 2023

This PR adds unit tests for MultiToken._transferFrom(). In addition it also provides some framework guidance on how "combinatorial" unit tests are to be constructed differing to the design in protocolv2 to better structure and reduce line count.

The tests are constructed with a fix sender from (Alice) and receiver to (Bob). Also all transfers contain a caller context which will either be Alice for a direct transfer or Eve for an approved transfer of which there are two kinds - a global approval isApprovedForAll and a perTokenApproval which is self-evident. Error cases involve an underflow on the non-infinite approvalm, the from balance updates and an overflow on to balance updates.
To note we may want to consider a more explicative error messaging especially in context of the non-infinite approval underflow which would be more considerate for frontends.

As stated, this PR should highlight an arguably cleaner approach to our combinatorial unit testing framework which we have used previously in protocolV2.
As shown, a combinatorial test should be 1 test per file and in so far as possible define 4 functions, __setup, __fail, __success and __log which ought to provide a template for test construction.

  • __setup - teardown and setup of contract state in context of each test case
  • __fail - validates if the test case ought to produce an error and attempts to catch that error. Must return a boolean so that the success phase can be short-circuited if an error is expected.
  • __success - If applicable, event registration and state caching can be made before the call to the function under test. State validation follows.
  • __log - logs test case values for debugging purposes

@coveralls
Copy link
Collaborator

coveralls commented Feb 13, 2023

Pull Request Test Coverage Report for Build 4186351439

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 180 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+3.3%) to 3.311%

Files with Coverage Reduction New Missed Lines %
contracts/libraries/AssetId.sol 3 0%
contracts/AaveYieldSource.sol 11 0%
contracts/BondWrapper.sol 28 0%
contracts/Hyperdrive.sol 138 0%
Totals Coverage Status
Change from base Build 4175784911: 3.3%
Covered Lines: 15
Relevant Lines: 453

💛 - Coveralls

Base automatically changed from via-ir-2 to main February 14, 2023 16:14
@Padraic-O-Mhuiris Padraic-O-Mhuiris marked this pull request as draft February 14, 2023 18:36
@Padraic-O-Mhuiris Padraic-O-Mhuiris marked this pull request as ready for review February 14, 2023 21:46
@Padraic-O-Mhuiris Padraic-O-Mhuiris changed the title MultiToken tests Combinatorial MultiToken _transferFrom unit test Feb 14, 2023
Copy link
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

I think you did a great job with this framework. It's a great addition to the repo. Aside from the other comments that I left, I have two requests. First, I'd appreciate using combinatorial instead of combi. Since every shell worth its salt has tab auto-completion for files, there isn't a compelling reason to use the abbreviation IMO. Second, I'd like to see more comments in the combinatorial test. Specifically in the functions that really matter (__fail and __success).

My comments should be addressed before merging, but I will approve since it LGTM once those comments are addressed.

@Padraic-O-Mhuiris Padraic-O-Mhuiris merged commit 9cfee95 into main Feb 15, 2023
@Padraic-O-Mhuiris Padraic-O-Mhuiris deleted the multi-token-tests branch February 15, 2023 18:01
jalextowle added a commit that referenced this pull request Mar 6, 2024
* Use safe functions more consistently in `calculateNetCurveTradeSafe`

* Fail if `calculateSharesOutGivenBondsInDownSafe` underflows

* Fixed the rust tests

* Improved the system's liveness when the present value can't be computed

* Improved the liveness properties of `removeLiquidity`

* Ignore `calculateLPSharePrice` failures in `removeLiquidity`

* Fixed small typo

* Addressed review feedback from @cmichel

* Added a check on `addLiquidity` in `test_netting_extreme_positive_interest_time_elapsed`

* Post merge fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants