Investigation profit calculation test

[KirillDogadin-std]

Closes #232

As #232 (comment) requested, adding the test that fails/

Fixes that were added:

1. Adjust the logic to already predict the approximateUnitPrice based on the next step (price drop) when the current step is close to the end.
2. Adjust/Reintroduce the calculateTransactionCollateralOutcome so that it does not directly compare the values but instead subtracts the values and compares them to close-to-zero value. In other words changed the line that duplicates the logic(owe > tab) from potentialOutcomeTotalPrice > auction.debtDAI to debtDai - PotentialOutcomeTotalPrice < 0.00000000000001

Checklist:

• issue number linked above after pound (#)
  • replace "Closes " with "Contributes to" or other if this PR does not close the issue
• issue checkboxes are all addressed
• manually checked my feature / not applicable
• wrote tests / not applicable
• attached screenshots / not applicable

[KirillDogadin-std]

run tests for different configurations

here's the summary

"with approx" means that the approximateUnitPrice is recomputed according to the price decrease logic. In technical terms: it means that calculateAuctionPrice remains unchanged.
"without" means that the calculateAuctionPrice was changes so that it directly returns unitPrice from the provided auction without adjusting it.

No.	Block	works with approx	works without approx	works with maximum	:index:
0	14068414	n	y	did not check	7
1	14068414	y	y	did not check	6
2	14068414	y	n	did not check	4
3	14068414	n	n	works without, did not check with	3
4	14068414	y	y	did not check	2
5	14068414	y	y	did not check	1
6	14068414	n	y	works with , without	0
7	14760072	n	y	with approx, without	1
8	14760072	y	y	did not check	2
9	14760072	y	y	did not check	3
10	14760072	y	y	did not check	4
11	14760072	y	y	did not check	9
12	14760072	y	n	did not check	10
13	14760072	y	n	did not check	11
14	14052140	y	n	did not check	0
15	14052140	y	y	did not check	2
16	14052140	n	n	work without, with	3

[KirillDogadin-std]

The observations from #364 (comment) :

1. Using calculateAuctionPrice is justified: it does adjust the price so that the prediction is more accurate at least for some cases which is proven with experiments number 2, 12, 13, 14
2. Sometimes it is harmful that is proven for the case number 0, 6, 7
3. Anomaly: predicting the price with the assumption that the whole collateralAmount is going to be purchased seems like valid approach sometimes - according to the records 3, 6, 7, 16

[KirillDogadin-std]

So apparently there're 2 problems:

1. Precision during comparison.
2. Speed of execution of the code - if we compute the approximate unit price value during moment of "transition" to the next step (when the price decreases), we end up with an incorrect value.

all test cases pass with 4b356e9 (works with approx)

[valiafetisov]

As discussed, please look out for much bigger issue than a few percent difference. In the bug report in #329 relevant line is:

keeper: auction "ETH-A:15" clear profit is 1643364586864524 DAI after transaction fees, moving on to the execution 

Note clear profit in this context is the net profit

[KirillDogadin-std]

Local tests pass, on the drone no :/

will address this later, moving on to #364 (comment)

[KirillDogadin-std]

Were you able to reproduce the high number of profit by "failing" fetch function that determines block time?

changing fetchDateByBlockNumber logic so that it returns null does not make the prediction fail in the bot on the same block number. Same goes if i throw a fake error in the middle of execution of this funciton so that it does not return.

also suggesting that date fetching function could fail does not entirely make sense because if the network is not a fork, we just return new Date()

[valiafetisov]

From the integration testing perspective this is a hack of "please work" nature IMO
Moreover, as i've noticed it's rather that memoizee breaks the tests because the cached info is not invalidated.

I think those statements contradict each other. Cashing on the frontend expects that it's running in the consistent environment. In case of the tests where unexpected changes can be quickly introduced, cache should be invalidated. So I would invalidate cache before each execution of the cached function (that's a lot of work) and after the network reset/warp. memoized.clear() should help according to https://github.com/medikoo/memoizee#cache-handling. Or even simpler, you can mock memoize function to return input function, so there is no cache at all inside tests.

[KirillDogadin-std]

Or even simpler, you can mock memoize function to return input function, so there is no cache at all inside tests.

Bare-result-wise. The cache is already being clear according to the suggestion. #364 (comment)

Regarding the other topics: what would be your take on the suggestions here?

[KirillDogadin-std]

as suggested regarding the clearing cache

[valiafetisov]

Regarding the other topics: what would be your take on the suggestions here?

Long-term, I think we should enable drone cache not only because it will decrease text execution speed, but also because we might reach alchemy quota or will need paid account otherwise. So please create an issue for devops.

For the short-term, I need to understand what exactly is failing: Some tests even time out (a) is quite different from it's due to prediction being made for one moment of time and the execution happening way later (b). a can be fixed via increased provider timeout (you can introduce env/runtime/mocked variable specific for tests). b should be fixed by cache invalidation, otherwise needs to be investigated further to come to a definitive description of the problem. For me, current theory doesn't sound convincing: actually, for the execution itself, we don't use predicted values. The predicted approximateUnitPrice going down earlier can only trigger failing execution "earlier": while in hardhat there will be no price drop due to no blocks being mined.

Therefore, I would suggest to refactor this whole logic of forked time based on the actual restrictions we have: eg do not predict approximateUnitPrice on a fork and instead return unitPrice under that variable.

[KirillDogadin-std]

should be fixed by cache invalidation

so i've went ahead and observed how to invalidate cache. Sadly i did not find a function that just wipes every single cached item in memozee package.

alternatively, i could go stub the memozee function there but encountered the problem with the fact that one has to invest time - research how to stub the default module export via some library. Sadly, i did not find fast & ez, but instead spotted sinonjs/sinon#1623

so for experimental purposes i just manually wrote the functionality that diables caching for tests, see the diff

f7b08cf

Assuming that there is not a killer-mistake and the introduced changes actually disable caching successfully, the result is that cache is not the reason for differing results in the runs with/without cache.

• so i guess i am going with the following step as mentioned:

needs to be investigated further

---

eg do not predict approximateUnitPrice on a fork

also tried this one out and this either did not yield any successful change in the results as by d9934f7

[valiafetisov]

research how to stub the default module export via some library

Does this https://stackoverflow.com/a/47058957 not do the trick?

[KirillDogadin-std]

During debugging i noticed that the hardhat logs differ depending on whether there's a cache prepared.

The difference is at least the presense of the error on the chain that originates from the dss-chainlog:

dss-chain-log/invalid-key

---

@valiafetisov

Any imminent idea about what it might hint towards? (would save me time researching)

[valiafetisov]

dss-chain-log/invalid-key error comes from the requesting contract address from the chainlog when there is no such contract. So it's probably some block number under which some of the collateral types defined in our config doesn't exist yet on the fork (which is pretty normal and expected, that's why it fails silently) – to be clear, I don't think that is a problem

[KirillDogadin-std]

Ok. The non-cached hardhat setup now is capable of actually successfully running tests.

The thought process:

1. Notice that when the first test from the profit calculation times out, the logs from it still are output into the console.
2. Increase the timeout time,
3. have the majority of the tests suddenly pass

Conclusion / hypothesis of what could be the reason:

The blockchain fork actually was super slow, the tests did not wait for its response and failed, the next test was started and the request to reset the chain was sent, thus the test runner and the hardhat fork were not "syncronized" and the fork did not behave as was expected because it was not yet done with the previously sent requests.

---

Additional sidenotes:

• the current main still has some bugs that we tried to fix with Minor price calculation adjustment #380
  • this is proven by the tests failing
  • the main problem there:
    • while computing the collateral needed to cover debt, the if with comarison was still not done properly. One has to subtract value, then the result has to be rounded toPrecision and compared against 0
• to not mess around with the real time that is going, the test forces the each consequent block to have the same timestamp as the previous one. This is ok in my opinion because the goal of the test is to assert the correct prediction for "now", so transactions that give allowances, ... are not the concern, however they do influence the timestamp of blocks and time on for in general.
• The block that is returned by usual provider (from ethers js) for some reason is null during chain date computation, however the typing from the library does not claim that this case is possible. For this reason the function is stubbed and hre (hardhat) functionality is used to figure out the block characteristics.

[LukSteib]

Closing this for the moment as we are not actively working on it