Investigate profit calculation

[LukSteib]

Context:
We experienced our profit calculation to not be accurate all the time.
See recording here -> LOCAL TESTING video

We saw that similar problems were tackled in the auction demo keeper implementation here

First conclusion by @valiafetisov:

We should definitely use this sophisticated calculateTransactionCollateralOutcome mirroring of the contract to calculate our profit in swap transaction as well and also account for the states when auction can't be executed for some strange reason

Tasks:

• Look into profit calculation (especially for swap transaction) and potential change to the more sophisticated calculateTransactionCollateralOutcome approach

[valiafetisov]

So I see two problems in the existing calculation:

Problem 1: We get market price for swapping the whole amount of collateral in the vault (auction.collateralAmount), while in fact the contract will swap only enough collateral to cover the debt (auction.debtDAI):

unified-auctions-ui/core/src/auctions.ts

Line 52 in 1f3aa05

const marketUnitPrice = await getMarketPrice(network, auction.collateralSymbol, auction.collateralAmount);

We can calculate how much collateral will actually be sold by using calculateTransactionCollateralOutcome implemented for the Bid with DAI flow and passing debtDAI to it (as maximum possible amount the contract will take anyway)

But there is a problem, that calculateTransactionCollateralOutcome sometimes returns NaN:

unified-auctions-ui/core/src/price.ts

Lines 81 to 84 in 1f3aa05

	if (
	// if tab > _chost
	auction.debtDAI.isLessThanOrEqualTo(auction.minimumBidDai)
	) {

Therefore, in some rare occasions, when auction can not be executed, the market price wouldn't be available as well. Not sure it's desirable, since till now we used unavailability of market price as an indication that it's "not tradable". We should maybe catch those two errors differently?

Problem 2 (that was already tackled before via calculateTransactionGrossProfit):

unified-auctions-ui/core/src/price.ts

Lines 44 to 55 in 1f3aa05

	export const calculateTransactionGrossProfit = function (auction: Auction): BigNumber {
	if (!auction.marketUnitPrice) {
	return new BigNumber(0);
	}
	const totalMarketPrice = auction.collateralAmount.multipliedBy(auction.marketUnitPrice);
	if (totalMarketPrice <= auction.debtDAI) {
	return totalMarketPrice.minus(auction.totalPrice);
	}
	const collateralAmountLimitedByDebt = auction.debtDAI.dividedBy(auction.approximateUnitPrice);
	const totalMarketPriceLimitedByDebt = collateralAmountLimitedByDebt.multipliedBy(auction.marketUnitPrice);
	return totalMarketPriceLimitedByDebt.minus(auction.debtDAI);
	};

The difference between total selling prices should be computed on the debtDAI amount. (sounds similar to the problem 1, but it's actually not)

[valiafetisov]

I think the best way to address it would be to recreate take function line by line https://github.com/makerdao/dss/blob/master/src/clip.sol#L335-L415

[valiafetisov]

Once again: we want Transaction Gross Profit to display the amount of DAI we receive after execution of the swap. Currently this number can deviate from what the user can actually receive.

Context

There are can be many reasons for it, but known problems are described above:

• Bidding with DAI and Swap transactions are actually the same function calls under the hood: they both call take function of a clipper contract
• Bidding with DAI implemented calculateTransactionCollateralOutcome function which properly computes how much collateral the user will receive if they bid X amount of DAI. This function almost recreates contract line by line and works well

How to reproduce

1. Start hardhat simulation
2. Open one auction, execute both authorizations using hardhat wallet
3. Record / screenshot Transaction Gross Profit displayed in the UI
4. Execute auction
5. Check wallet popup for how much DAI you actually received

Known problems

1. marketUnitPrice depends on the amount you want to swap: the bigger amount you sell, the lower the price goes. But currently it's retrieved based on the whole amount of collateral available in the wallet – while we actually know that even if you bid unlimited amount of DAI, you will only receive amount of collateral enough to cover the debt from the calculateTransactionCollateralOutcome. Therefore, we should only use amount returned by the calculateTransactionCollateralOutcome to retrieve marketUnitPrice
2. calculateTransactionGrossProfit should also use more precise calculateTransactionCollateralOutcome under the hood instead (but look ahead of the not tradable error)

[valiafetisov]

Since we now have test setup in place, I suggest to open a PR where we first write a failing test:

1. Fork chain on the block where there were auctions
2. Execute both authorizations
3. Refetch/re-enrich auction data (as every mined block can change auction price, since abacus computes prices based on time passed from the auction start)
4. Execute swap and (approximately) compare amount of received DAI with the amount of gross profit, computed in the previous step
5. We can write several tests (for different collateral types, or using time warping/later block numbers) to have several reference points

This will open the ground for the further investigation based on actual number and reproducible way to test our theories.

[KirillDogadin-std]

fun fact (aka observation) which does not make any definite conclusions and just raises a discussion

i've merged my test #364 with #247 and changed minimum to maximum in the line

unified-auctions-ui/core/src/price.ts

Line 64 in c139af9

const potentialOutcomeCollateralAmount = BigNumber.minimum(collateralToBuyForTheBid, auction.collateralAmount); // slice

then all the auctions on the default blocknumber (now on main) have an accurate prediction OR can't be swapped due to insuffiecient profit.

all cases on 14760072 block have the same result
all except for one on 14068414 block have the asme result.

so among ~25 test cases there's only single outlier that has an inaccurate prediction.

compared to the case with the original minimum that has quite some failed cases.

As of now, i am struggling to explain "why" this change gives better result. and is struggle to understand why there's one exception in the maximum case which does not pass the test.

[valiafetisov]

and changed minimum to maximum in the line
which does not make any definite conclusions

This means that whatever way the collateralToBuyForTheBid value is computed it is wrong, since auction.collateralAmount is the maximum amount of collateral what can be exchanged.

all cases on 14760072 block have the same result

When do you say "the same" how same are they? Down to the last digit?

all except for one on 14068414 block have the asme result

Can please you provide predicted gross profit for the one that fails?

[KirillDogadin-std]

When do you say "the same" how same are they? Down to the last digit

Within 1 percent is the guarantee of the test
Manual inspection of several cases - precise at least as int. Did not check after .

Can please you provide predicted gross profit for the one that fails?

actual profit 336.713869870931917693
expected profit 141.01489056908999527329470327581048527017518956919695418059549804701035940201931437

This means that whatever way the collateralToBuyForTheBid value is computed it is wrong, since auction.collateralAmount is the maximum amount of collateral what can be exchanged.

after looking at the tested function bidWithCallee i see that the argument that is passed as amt (Upper limit on amount of collateral to buy) is collateralAmount which is extracted from lot (available collateral), which in its turn is compared against amt via minimum.

This makes me a bit confused regarding why we have to find minimum during transaction collateral outcome computation.

[KirillDogadin-std]

The investigation result for the one failing case there:

My current hypothesis is that unitPrice (or in other term approximateUnitPrice) value gets updated at some point to a higher value and then the outcome is "more actual profit than expected".

[valiafetisov]

why we have to find minimum during transaction collateral outcome computation

Good question. We are sending auction.collateralAmount as Upper limit on amount of collateral to buy because:

1. We always want to make full swap (but potentially the contract allows us to swap partially)
2. That's a valid value for the full swap

But even if we aim to sell complete amount of collateral in the auction (eg 100 ETH) the amount of collateral that will be sold will not be defined by the value we send, but by the debtDAI that needs to be covered (eg in case the debt is 1000 DAI, only ~1 ETH will be sold to cover it). But what is the user profit in this case?

To answer you question "why": the function where minimum is used is implemented one-to-one to the contract function even if some parts are not used at the moment to make sure that the logic is the same.

My current hypothesis is that unitPrice (or in other term approximateUnitPrice) value gets updated at some point to a higher value and then the outcome is "more actual profit than expected".

I think that is most probable cause for the #329
But it's not likely the problem with 2-3 times difference like in your failed test case. Regarding approximateUnitPrice, I can imagine that this can be related to getCurrentDate or fetchLatestBlockDate. Btw, how do you want to validate it?

[valiafetisov]

Regarding the dates, please note that there are two exact files core/src/time.ts and core/src/date.ts where one is unused. Please remove unused one (I believe, it was introduced after the improperly resolved merge conflict)

[LukSteib]

Another input to this issue: #561 (comment)