Python Pools Contracts Testing

[hepnerthomas]

Description

• Invariant testing in test_RevenueGeneratingContracts (2, 3, 5) with name format test_funcName_invariant_{name_of_test}
  • 2. (invariant) all self.owner rev is claimable by self.rev_recipient
  • 3. (invariant) claimable rev == sum of self.owner fees events emitted
  • 5. (invariant) max_uint claim_rev is claimable_rev

Future Work:

• Test pool share prices in test_pool_share_price.py
• Implement _virtual_apr in Vyper
• Go through TODO_TEST comments in smart contracts

Related Issue

Python Pools Contracts Testing

Motivation and Context

Help Kiba with testing Pools contracts.

How Has This Been Tested?

Running pytest tests/boa/pool/test_RevenueGeneratingContract.py -m "not slow" locally.

Screenshots (if appropriate):

[hepnerthomas]

@kibagateaux Questions for you:

1. Why are there two tests for test_rev_recipient_cant_overclaim_rev? I'm assuming one of these is an unintentional duplicate?
2. What is the purpose of @pytest.mark.rev_generator in the test_RevenueGeneratingContract.py? It's a decorator above all of the tests, but I don't think it's being used for anything. For instance, there are numerous tests that still pass if you comment it out.
3. There are multiple failing tests in test_RevenueGeneratingContract.py that are failing because they are trying to get args_map from an Event object (e.g. e.args_amp) which does not exist in the Event class. Is there a reason you are trying to utilize something that doesn't exist and is not in the Vyper contract for the Event?

[kibagateaux]

1. two test_rev_recipient_cant_overclaim_rev` tests is probably copypasta. U can delete second (but make sure i didnt change the internal test logic while forgetting to change the name
2. its just a tag so we can tell pytest exactly which tests to run instead of all of them. not used for actual testing logic
3. Thats a custom thing i've added to titanoboa. Looks like i have dev-requirements.txt pointing to git+https://github.com/kibagateaux/titanoboa instead of git+https://github.com/kibagateaux/titanoboa#fix/event-data-access or i could just push my changes to my master branch

[kibagateaux]

We already have a test for what u wrote here in test_rev_recipient_can_claim_rev. I want to test that all RevenueGenerated events emitted with self.owner as the receiver are claimable by rev_recipient. So should randomly generate fees (any type or just the ones that pay to owner idc) and then test that claimable rev == event amounts

[hepnerthomas]

done. rewrote the test and created a helper function called gen_rev_events that calls different functions (mint, deposit, redeem, withdraw, etc) that log the RevenueGenerated event.

[hepnerthomas]

Some comments before merging code:

1. The _gen_rev function within confttest.py fails when fee_type == 'flash with an eth_abi encoding type error. I see this failure in both test_self_owner_rev_claimable_by_rev_recipient and test_claimable_rev_equals_sum_of_self_owner_fee_events_emitted tests.
   

This appears to be because "" is not a valid input in this line:
pool.flashLoan(flash_borrower, base_asset, amount, "")

2. The test_self_owner_rev_claimable_by_rev_recipient fails for fee_type == 'flash' for reason described above, but test_claimable_rev_equals_sum_of_self_owner_fee_events_emitted fails for withdraw and collector fee types as well as pool_claimable_rev(pool) is greater than total_revenue.

[kibagateaux]

1. Ah ya for flashLoan i havent figured out how to do raw byte inputs with boa yet. Thats an issue for another function too somewhere.
2. interesting, is it only on withdraw, not redeem? Might have to do with how we calculate shares before calculating fees which we dont have to do for redeem. Collector fees dont go to pool.accrued_fees tho, the go straight to the caller (if its not the owner)

[hepnerthomas]

2. interesting, is it only on withdraw, not redeem?

Both tests are iterating through pool_fee_types which does not include redeem, even though it creates a RevenueGenerated event, because in the contracts both the _redeem and _withdraw functions include the same FEE_TYPE on the emitted event. See below:

_redeem : log RevenueGenerated(_owner, self, withdraw_fee, _shares, convert(FEE_TYPES.WITHDRAW, uint256), self)
_withdraw: log RevenueGenerated(_owner, self, withdraw_fee, shares, convert(FEE_TYPES.WITHDRAW, uint256), self)

[kibagateaux]

ya interesting. gen_rev was really only made to test the different fee types, not every single 4626 function. So might have to manually call deposit/withdraw, mint/redeem, and collect interest for this test

[hepnerthomas]

2. interesting, is it only on withdraw, not redeem? Might have to do with how we calculate shares before calculating fees which we dont have to do for redeem. Collector fees dont go to pool.accrued_fees tho, the go straight to the caller (if its not the owner)

To answer your question, yes, this is an issue only with withdraw and not redeem. Total revenue and claimable revenue are the same for redeem, but not withdraw.