Refactor `core/` for multi-callee support by aomafarag · Pull Request #511 · sidestream-tech/unified-auctions-ui

aomafarag · 2022-10-26T13:06:29Z

Closes #497 .

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

core/src/calleeFunctions/index.ts

bot/src/keepers/collateral.ts

core/src/types.ts

core/src/auctions.ts

core/src/calleeFunctions/CurveLpTokenUniv3Callee.ts

core/src/calleeFunctions/UniswapV2LpTokenCalleeDai.ts

core/src/calleeFunctions/index.ts

core/src/auctions.ts

core/src/calleeFunctions/CurveLpTokenUniv3Callee.ts

core/src/types.ts

core/src/fees.ts

…t + other adjustments

core/src/calleeFunctions/index.ts

…per unit

valiafetisov · 2022-11-03T08:13:24Z

core/src/auctions.ts

+        let marketDataRecords = await getMarketData(network, auction.collateralSymbol, collateralToCoverDebt);
+        const exchangeFees = await getDefaultMarketFee(network);
+        const exchangeFeePerUnitDAI = exchangeFees.exchangeFeeDAI.dividedBy(collateralToCoverDebt);
+        for (let marketData of Object.values(marketDataRecords)) {


Currently, you put the whole loop into the try/catch here. So in case any market fails (which is expected, see comment below catch) the auction will be returned without any market data. Please make so that each market data is checked separately

Nit: you might want to move the whole loop into a separate function to simplify try/catching it.

We use the loop to populate marketData after getting them. If there is a place where the code would fail/throw an error, it's when calling getMarketData(). Looking at the code again, I still don't understand why we should push the try/catch inside the loop; we would be basically trying to catch assignments. Or maybe I don't understand what you mean concretely.

We normally should not care if functions can or can not throw errors, we should assume they always can. My main point is that if one of the markets fail (which we expect to happen), the full "list" of markets should still be sent to the frontend:

Single marketData can't be null or undefined

The maketDataRecords itself can't be null or undefined

So the question then boils down to: why do you need try catch above the whole loop then if you catch market problems on the level deeper? As it will return incorrect state, where marketDataRecords === undefined.

The proposed solution is to wrap the parts into try/catch at the place where you need to replace them with other "error"-like objects

The whole thing is still in the try/catch, right?

core/src/auctions.ts

valiafetisov · 2022-11-03T08:26:31Z

core/src/calleeFunctions/index.ts

+        try {
+            marketUnitPrice = await allCalleeFunctions[marketData.callee].getMarketPrice(
+                network,
+                collateral,
+                marketId,
+                amount
+            );
+        } catch {
+            marketUnitPrice = new BigNumber(NaN);
+        }


Why do you need to try/catch it here? (if your function can still throw)

In other words, if you want to return market data records in any case, you need to move Unsupported collateral symbol "${collateralSymbol}" to be thrown on the market level?

I also suggest to add new optional key marketData.errorMessage which will contain those errors (because previously it was cached on the frontend level and we were able to display this error)

We throw above to make sure that there is at least one valid callee name (i.e. contained in allCalleeFunctions), because otherwise we can't call getMarketPrice(). On the other hand, we try/catch below because the getMarketPrice() method of each callee can be different (encoding routes vs. fetching token reserves...) and is prone to error, so if we encounter one, we return NaN.

I also updated the code to continue the loop in the case of an invalid callee name (latest commit).

I still don't see any errorMessage inside marketData

core/src/calleeFunctions/index.ts

aomafarag · 2022-11-03T15:54:55Z

Although the loop in enrichAuctionWithMarketData is virtually the same, I added a new function, getMarketDataById() in index.ts and updated the logic of getMarketData() accordingly. Together they ensure that a market is always returned (if an error gets thrown, they return a dummy market).

valiafetisov · 2022-11-03T16:13:06Z

core/src/calleeFunctions/index.ts

+    let marketUnitPrice: BigNumber;
+    try {
+        marketUnitPrice = await allCalleeFunctions[marketData.callee].getMarketPrice(
+            network,
+            collateral,
+            marketId,
+            amount
+        );
+    } catch (error: any) {
+        const errorMessage = error;
+        marketUnitPrice = new BigNumber(NaN);
+        return {
+            ...marketData,
+            marketUnitPrice,
+            errorMessage,
+        };
+    }
+    return {
+        ...marketData,
+        marketUnitPrice: marketUnitPrice ? marketUnitPrice : new BigNumber(NaN),
+    };


Why do you need to try catch it here if you already catching it in the parent function getMarketData (within the loop)?

valiafetisov · 2022-11-03T16:15:31Z

core/src/calleeFunctions/index.ts

+            marketData = {
+                errorMessage: error,
+                marketUnitPrice: new BigNumber(NaN),
+                route: [],
+            };


Why does this suddenly returns a route?

valiafetisov · 2022-11-03T16:21:30Z

core/src/auctions.ts

+        let marketDataRecords = await getMarketData(network, auction.collateralSymbol, collateralToCoverDebt);
+        const exchangeFees = await getDefaultMarketFee(network);
+        const exchangeFeePerUnitDAI = exchangeFees.exchangeFeeDAI.dividedBy(collateralToCoverDebt);
+        for (let marketData of Object.values(marketDataRecords)) {


The whole thing is still in the try/catch, right?

valiafetisov · 2022-11-03T16:25:52Z

core/src/auctions.ts

+            marketData = {
+                ...marketData,
+                marketUnitPrice: marketData.marketUnitPrice.plus(exchangeFeePerUnitDAI),
+                marketUnitPriceToUnitPriceRatio: auction.approximateUnitPrice
+                    .minus(marketData.marketUnitPrice)
+                    .dividedBy(marketData.marketUnitPrice),
+                ...exchangeFees,
+                transactionGrossProfit: calculateTransactionGrossProfit(
+                    marketData.marketUnitPrice,
+                    collateralToCoverDebt,
+                    auction.approximateUnitPrice
+                ),
+                transactionGrossProfitDate: calculateTransactionGrossProfitDate(
+                    auction,
+                    marketData.marketUnitPrice,
+                    currentDate
+                ),
+            };


The idea of the last suggestion was to move this whole block with calculations into the getMarketDataById. The reason is: it's extremely confusing that everything is a marketData and there is no types to enforce proper keys. You call makertData collateral.exchanges[marketId] in one place, { errorMessage: error, marketUnitPrice: new BigNumber(NaN), route: [], } in another, then here you call marketData something else, but those are all different thing and we have typescript to enforce them

core/src/auctions.ts

LukSteib

Tested on hardhat fork looking at an active ETH-A auction

Running the bot resulted in the following errors:

When testing on current block: Not able to participate in any auction (similar to the problem seen when using the UI below) -> needs to be addressed

ℹ collateral keeper: auction "ETH-A:836" is not tradable                                                                  19:00:25

Testing the frontend:

Currently unable to swap any collateral into profit as the button to access transaction page is disabled (see below)

LukSteib

Tested via hardhat fork, simulation ETH-A auction

The bot throws an error when trying to execute the swap transaction (see terminal output below). While authorisation steps work as expected there seems to be a problem with callee contract determination
Similar problem on frontend (see console output below)

terminal output for bot execution:

ℹ collateral keeper: auction "ETH-A:836" net profit is 6154 DAI after transaction fees, moving on to the execution        12:16:04
ℹ keeper: wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" has not been authorized yet. Attempting authorization now...
ℹ Transaction is preparing to be mined...                                                                                 12:16:04
ℹ Transaction is pending to be mined...                                                                                   12:16:05
ℹ Transaction was mined into block "15939548"                                                                             12:16:05
ℹ keeper: wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" successfully authorized via "0x76f9eed11c31cd9fcfe3226af1dd21613f9f2fbed4f724b211cdfe55bc3ee9c3" transaction
ℹ collateral keeper: auction "ETH-A:836" net profit is 6154 DAI after transaction fees, moving on to the execution        12:16:05
ℹ keeper: wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" has already been authorized                                 12:16:05
ℹ keeper: collateral "ETH-A" has not been authorized on wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" yet. Attempting authorization now...
ℹ Transaction is preparing to be mined...                                                                                 12:16:05
ℹ Transaction is pending to be mined...                                                                                   12:16:05
ℹ Transaction was mined into block "15939549"                                                                             12:16:05
ℹ keeper: collateral "ETH-A" successfully authorized on wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" via "0x6c4ee203e3f0d4810f93a9ea76c37d6af77f0b254f5cc3133bd64c3932ed9706" transaction
ℹ collateral keeper: auction "ETH-A:836" net profit is 6154 DAI after transaction fees, moving on to the execution        12:16:05
ℹ keeper: wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8" has already been authorized                                 12:16:05
ℹ keeper: collateral "ETH-A" has already been authorized on wallet "0x70997970C51812dc3A010C7d01b50e0d17dc79C8"           12:16:05
ℹ collateral keeper: auction "ETH-A:836": attempting swap execution                                                       12:16:05

 ERROR  collateral keeper: unexpected error: No callee address found for the "ETH-A" collateral on "custom" network

console output with stack trace:

Bidding error Error: No callee address found for the "ETH-A" collateral on "custom" network
    at getCalleeAddressByCollateralType (CALLEES.ts?96e2:45:1)
    at _callee15$ (auctions.ts?450c:334:1)
    at tryCatch (router-sdk.cjs.development.js?ce8d:423:1)
    at Generator.invoke [as _invoke] (router-sdk.cjs.development.js?ce8d:654:1)
    at Generator.eval [as next] (router-sdk.cjs.development.js?ce8d:479:1)
    at asyncGeneratorStep (asyncToGenerator.js?1da1:3:1)
    at _next (asyncToGenerator.js?1da1:25:1)
    at eval (asyncToGenerator.js?1da1:32:1)
    at new Promise (<anonymous>)
    at eval (asyncToGenerator.js?1da1:21:1)

LukSteib · 2022-11-04T13:09:28Z

Next round of functional review (again based on hardhat fork with ETH-A simulation)

was able to execute swap transaction via the bot

frontend execution is not working. The console is flooded with warnings (see s1 below). And when trying to execute the swap transaction I receive same error as reported above

s1:

console error on swap execution:

Bidding error Error: No callee address found for the "ETH-A" collateral on "custom" network
    at getCalleeAddressByCollateralType (CALLEES.ts?96e2:45:1)
    at _callee15$ (auctions.ts?450c:341:1)
    at tryCatch (router-sdk.cjs.development.js?ce8d:423:1)
    at Generator.invoke [as _invoke] (router-sdk.cjs.development.js?ce8d:654:1)
    at Generator.eval [as next] (router-sdk.cjs.development.js?ce8d:479:1)
    at asyncGeneratorStep (asyncToGenerator.js?1da1:3:1)
    at _next (asyncToGenerator.js?1da1:25:1)
    at eval (asyncToGenerator.js?1da1:32:1)
    at new Promise (<anonymous>)
    at eval (asyncToGenerator.js?1da1:21:1)

aomafarag · 2022-11-04T13:55:18Z

I removed the dummy console.warn() I used for testing locally. This should resolve the warning statements issue.

Bidding error Error: No callee address found for the "ETH-A" collateral on "custom" network
    at getCalleeAddressByCollateralType (CALLEES.ts?96e2:45:1)
    at _callee15$ (auctions.ts?450c:341:1)
    at tryCatch (router-sdk.cjs.development.js?ce8d:423:1)
    at Generator.invoke [as _invoke] (router-sdk.cjs.development.js?ce8d:654:1)
    at Generator.eval [as next] (router-sdk.cjs.development.js?ce8d:479:1)
    at asyncGeneratorStep (asyncToGenerator.js?1da1:3:1)
    at _next (asyncToGenerator.js?1da1:25:1)
    at eval (asyncToGenerator.js?1da1:32:1)
    at new Promise (<anonymous>)
    at eval (asyncToGenerator.js?1da1:21:1)

The above bidding error, however, will resolve after MarketPriceSelection is merged: bidWithCallee is awaiting a marketId which gets passed here (link is from the MarketPriceSelection PR) on the frontend.

valiafetisov · 2022-11-04T14:02:45Z

The above bidding error, however, will resolve after MarketPriceSelection is merged

The main/staging should never be in a broken state. So if we want to merge a refactoring PR, current functionality should stay intact. You can achieve that by temporary passing the suggest market id in the modified store action.

LukSteib

Was able to execute swap transaction via frontend ✔️
Also successfully retested the bot ✔️

valiafetisov · 2022-11-04T15:03:33Z

core/src/calleeFunctions/index.ts

+        try {
+            marketData = await getMarketDataById(network, collateral, marketId, amount);
+        } catch (error: any) {
+            marketData = {
+                errorMessage: error?.message,
+                marketUnitPrice: new BigNumber(NaN),
+                route: [], // dummy value: MarketData requires either a route or tokens
+            };
+        }


If you remember, we want to show the route on the frontend. In case there is no "marketData", the route will be empty and there will be nothing to display.

If you move try/catch down the getMarketDataById, it can return correct route or token based on the collateral config, not just a dummy empty value.

Please fix this in the next frontend PR

getMarketDataById can throw an error if the callee is invalid. That's why I'm catching it here. So, what do you mean exactly by moving try/catch down?

Good question. I guess linked throw condition shouldn't even be possible (eg via static typing), or should be validated during tests as it's strictly depends on the static config, not on the external conditions.

aomafarag added 2 commits October 21, 2022 17:34

WIP: Refactor core for multi-callee support

5a690fa

Add exchange fee and store logic

86a64c9

aomafarag commented Oct 26, 2022

View reviewed changes

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

aomafarag added 5 commits October 26, 2022 15:17

Merge branch 'main' into multi-callee-core

5c7534a

Refactor rETH

2764e92

Use suggestedMarketId in keeper

ec7808d

Type assertion

e3ad2f6

Lint

681143c

aomafarag self-assigned this Oct 26, 2022

valiafetisov reviewed Oct 27, 2022

View reviewed changes

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

aomafarag added 9 commits October 28, 2022 13:24

Validate suggestedMarketId in bot

5b2d9be

Move fee calculation to fees.ts

c56ecbd

Use marketId to return a single callee address

c73b2e0

Enrich auction with market data in a separate function

9c3a3f9

Refactor types.ts and add new fee

fd2fe2d

Move new fee calculation to fees.ts

671e8fc

Remove typecasts

8830104

Add marketId parameter to getCalleeData() function

e163276

Quick fixes and adjustments

86fe41a

valiafetisov reviewed Oct 31, 2022

View reviewed changes

core/src/auctions.ts Outdated Show resolved Hide resolved

core/src/auctions.ts Outdated Show resolved Hide resolved

core/src/auctions.ts Outdated Show resolved Hide resolved

core/src/calleeFunctions/CurveLpTokenUniv3Callee.ts Outdated Show resolved Hide resolved

Validate callee name

6e75ecc

valiafetisov reviewed Oct 31, 2022

View reviewed changes

core/src/types.ts Outdated Show resolved Hide resolved

core/src/fees.ts Outdated Show resolved Hide resolved

core/src/fees.ts Outdated Show resolved Hide resolved

Add marketId parameter to getCalleeData and getMarketPrice, and use i…

ac7a19d

…t + other adjustments

valiafetisov reviewed Nov 2, 2022

View reviewed changes

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

core/src/calleeFunctions/index.ts Outdated Show resolved Hide resolved

aomafarag added 2 commits November 2, 2022 14:59

Implement suggestions

41dae71

Refacor indexing callee.exxhanges in own const, and add exchange fee …

0d4d1df

…per unit

valiafetisov reviewed Nov 3, 2022

View reviewed changes

aomafarag added 2 commits November 3, 2022 12:12

Implement suggested modifications

04735cb

Skip with invalid callee

2bf25ae

Refactor for loop and calculate net profit

7c3ad35

valiafetisov reviewed Nov 3, 2022

View reviewed changes

LukSteib requested changes Nov 3, 2022

View reviewed changes

Refactor

fe8b419

LukSteib requested changes Nov 4, 2022

View reviewed changes

Add marketId to index records

630dc07

Remove dummy console.warn

7b9acc1

Pass suggestedMarketId in the frontend

9402ef9

LukSteib reviewed Nov 4, 2022

View reviewed changes

valiafetisov approved these changes Nov 4, 2022

View reviewed changes

Merge branch 'main' into multi-callee-core

0708c95

LukSteib approved these changes Nov 4, 2022

View reviewed changes

valiafetisov merged commit 24a1932 into main Nov 4, 2022

valiafetisov deleted the multi-callee-core branch November 4, 2022 15:24

valiafetisov mentioned this pull request Nov 7, 2022

Extend multi-callee with auto router #527

Closed

11 tasks

Conversation

aomafarag commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aomafarag commented Nov 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukSteib left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukSteib left a comment

Choose a reason for hiding this comment

Uh oh!

LukSteib commented Nov 4, 2022

Uh oh!

aomafarag commented Nov 4, 2022

Uh oh!

valiafetisov commented Nov 4, 2022

Uh oh!

LukSteib left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valiafetisov Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aomafarag commented Oct 26, 2022 •

edited

Loading

LukSteib left a comment •

edited

Loading

LukSteib left a comment •

edited

Loading

valiafetisov Nov 4, 2022 •

edited

Loading