Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement vault liquidation core logic #460

Closed
7 tasks done
valiafetisov opened this issue Sep 12, 2022 · 28 comments · Fixed by #457
Closed
7 tasks done

Implement vault liquidation core logic #460

valiafetisov opened this issue Sep 12, 2022 · 28 comments · Fixed by #457
Assignees

Comments

@valiafetisov
Copy link
Contributor

valiafetisov commented Sep 12, 2022

Goal

The PM is able to see specific vault and liquidate it using store actions

Context

After the conclusion of the related investigation issue, where the implementation proposal was outlined, we would implement this functionality in this issue. Current issue may also keep the discussion on the new edge cases.

The basic outline of the proposal:

  • Fetch individual vaults from the blockchain directly
  • Liquidate a vault
  • Provide store functionality

Assets

Tasks

  • Discuss exact implementation order
    • Eg order of implementation (simulation, liquidation)
  • Implement simulation
  • Implement fetching of a single vault
  • Implement liquidation
  • Implement store-related functionality
    • Provide test commands in the PR description
@KirillDogadin-std
Copy link
Contributor

Edge case for price logs:

  • https://etherscan.io/address/0x81FE72B5A8d1A857d176C3E7d5Bd2679A9B85763

  • the price is stored in the private variable

  • the price is exposed via the events

  • so the intent here is to fetch the events for the true price.

  • the problem:

    • the price was not successfully updated for a long time (20000+ blocks)
    • via ethers.js there is a limit of events one can fetch
    • potentially we risk to encounter a state where there were no events for a long time and we can't reach it because it has happened too long ago.
    • what should we do at this case?

options:

  • display current market price as the current unit price instead even if we risk to lie
  • try to fetch the information from the contract via directly reading the slot value
    • risk that we bear in this case: we rely on the value to always be present in the same slot. (e.g. current price is always at slot no. 4). if it's not the case, we fail to extract the vulue once again.
  • introduce this edgecase as something that the user has to figure out on their own via other means (e.g. etherscan)

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 12, 2022

Implement simulation

what should it do?

  • just warp time to the block number where the state X of the vault V is present?
  • or create the vault and simulate its lifecycle artificially?

@valiafetisov
Copy link
Contributor Author

what should it do?
or create the vault and simulate its lifecycle artificially?

Yes, I think it's more helpful long-term. I actually forgot that we previously agreed to write tests using specific vault on a specific block, so I created a separate issue for this: #464

via ethers.js there is a limit of events one can fetch

  • What does increasing the limit implies technically (please link the docs/source code)? Does the limit just sent to the RPC or does ethers have to fetch X blocks and parse them under the hood in order to "fetch events"?
  • Can we simply increase the limit geometrically (if we don't want to fetch thousands of events for some contracts and 0 for others)?

try to fetch the information from the contract via directly reading the slot value

Would it be the same slot address for all collateral types?

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 12, 2022

Would it be the same slot address for all collateral types?

does not seem so. i found a contract that differs from the one at https://github.com/makerdao/osm/blob/master/src/osm.sol regarding the slot address of the cur feed struct.

https://etherscan.io/address/0x25D03C2C928ADE19ff9f4FFECc07d991d0df054B#code

What does increasing the limit implies technically

it apparently executes the eth_getLogs https://github.com/ethers-io/ethers.js/blob/ec1b9583039a14a0e0fa15d0a2a6082a2f41cf5b/packages/providers/src.ts/json-rpc-provider.ts#L604

sadly the docs here are not of much help (not too much description)
here's the most sound example i found :https://docs.ethers.io/v5/getting-started/#getting-started--history

Can we simply increase the limit geometrically (if we don't want to fetch thousands of events for some contracts and 0 for others)?

i dont think it makes much sense to increase it geometrically, instead i would propose to shift the from-to window because the error gives the following description:

You can make eth_getLogs requests with up to a 2K block range and no limit on
 the response size, or you can request any block range with a cap of 10K logs in the response.

so i would say we request as follows:

let windowNumber = 0
const maxLogs = 2000
let events = []
const latestBlockNumber = getLatestBlockNumber()
while(events.length === 0 && latestBlockNumber > (windowNumber+1)*maxLogs){
  events = contract.queryFilter(filter, -(1 + windowNumber)*maxLogs, -windowNumber*maxLogs)
}

queryFilter docs

But even then we potentially risk to have a contract that has no events yet. What shall we do then?

actually forgot that we previously agreed to write tests using specific vault on a specific block, so I created a separate issue for this:

then in this pr i will not add a simulation but instead just do the tests with specific block numbers. Later in the linked pr i will add the simulations

@valiafetisov
Copy link
Contributor Author

it apparently executes the eth_getLogs https://github.com/ethers-io/ethers.js/blob/ec1b9583039a14a0e0fa15d0a2a6082a2f41cf5b/packages/providers/src.ts/json-rpc-provider.ts#L604

According to the docs the topics are also send to the RPC endpoint and are filtered on their side.

instead i would propose to shift the from-to window because the error gives the following description:

This doesn't contradict each other. If we need only one last event of the kind and it's updated every ~hour, we might want to first request a very small page of a few blocks and only then increase it to the limit and start pagination.

Regarding the original edge case, how many pages would it take to fetch the last event?

But even then we potentially risk to have a contract that has no events yet. What shall we do then?

Good point, maybe we can check the deployment block # of the contract to be the latest block to fetch?

Would it be the same slot address for all collateral types?

Can you determine the slot address automatically? Or what is the current process for this?

then in this pr i will not add a simulation but instead just do the tests with specific block numbers. Later in the linked pr i will add the simulations

Please also make a simulation with fixed block number, same as you're using for a test, so it's manually testable too and is usable on the frontend/bot.

Regarding the implementation order, I propose to implement separate service to cache vaults and fetch data from DICU in another PR after the rest is tested and merged, so we can validate core logic and unblock UI integration earlier. Do you have any objections?

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 12, 2022

Regarding the #460 (comment), how many pages would it take to fetch the last event?

6 pages, it's possible to fetch 100k events per request

Can you determine the slot address automatically?

i can't imagine such a move in the current issue:

  • there's a specific logic to determine which slot the variable takes
    • one needs a source code for this -> have to fetch it, have to use it as input to some tool
    • not every contract has a source code available
    • implementing the determination logic manually is not feasible / is time consuming & is it's own separate package.

Good point, maybe we can check the deployment block # of the contract to be the latest block to fetch?

good optimization move, but still does not solve the original problem where there could be no events at all in the logs.
sadly i did not discover the way to figure out this block number ( from ethers.js 5 documentation ) and additionally there's a claim that this is not possible via the standartized tool - ethers-io/ethers.js#2716 (comment)

Regarding the implementation order, I propose to implement separate service to cache vaults and fetch data from DICU in another PR after the rest is tested and merged, so we can validate core logic and unblock UI integration earlier. Do you have any objections?

no objections

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 12, 2022

one needs a source code for this

As far as I know, the source code doesn't guarantee the order, but the compiler. That's why I am asking, what is the current process, how do you determine the slot address? I can imagine that it can be a simple trail and error loop: modify slot 0, check the public method, modify slot 1, check the method output, etc.

Maybe there is already a package for this?

but still does not solve the original problem where there could be no events at all in the logs

Well, we can set this price to undefined or NaN until we can fetch it.

there's a claim that this is not possible via the standartized tool - ethers-io/ethers.js#2716 (comment)

Oh wow, wouldn't expect this to be difficult. If it doesn't work generically on etherium, you can use CHAINLOG events to filter UpdateAddress events by the (encoded) contract name, get the hash and the block number of this even. Would this solve the problem?

I want to first find a solution that works generically and only later optimize for less requests. The worst-case scenario is the hardcoded slot numbers inside our COLLATERALS.ts file, that we need to somehow keep updated with every new change of the PIP_* contract?

@valiafetisov
Copy link
Contributor Author

You:

6 pages, it's possible to fetch 100k events per request

Earlier, also you:

You can make eth_getLogs requests with up to a 2K block range

Which of this is true?

@KirillDogadin-std
Copy link
Contributor

also you

not me, but the error message :)

Which of this is true?

the truth is that i've tried to set the page size to 100k and it works: so the error message is apparently incorrect :o

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 12, 2022

That's why I am asking, what is the current process, how do you determine the slot address?

up until now i've been using the approach that is most commonly met all over the place (yt guides, googlable articles): count from top to bottom starting from the first declaration, skip structs' definitions. Trial and error obviously also works.

Maybe there is already a package for this?

did not find one within 6 min search. Also it does not make sense to me. We (humans) know that there's a private variable that represents "current price" and in the src code it is defined as cur, the contract itself does not expose it under this name, the contract itself is only aware of the pointer to the value. I can't imagine how it would be possible to craft the package that determines the memory address of the variable based on a unique contract interface. For contrast: humans interact with the interface and compare the output with expectations in order to determine wether slot overwrite actually changed the desired variable. The package would either have to do the same, or would have to have the source code + compiler identifier. We can't be sure that we have any of those on hands.

Well, we can set this price to undefined or NaN until we can fetch it.

so i interpet this as "Set the type to currentPrice?: BigNumber and process the case where it is undefined correctly.

Oh wow, wouldn't expect this to be difficult. If it doesn't work generically on etherium, you can use CHAINLOG events to filter UpdateAddress events by the (encoded) contract name, get the hash and the block number of this even. Would this solve the problem?

yes, so i assume i implement this.

actually as i looked deeper into this: the UpdateAddress has no indexed values so we can't filter there :D. Seems like we have to filter page by page

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 13, 2022

so i interpet this as "Set the type to currentPrice?: BigNumber and process the case where it is undefined correctly.

Yes, the other option is to keep the type, set currentPrice to NaN and let all other computations (that depend on it) automatically turn into NaN (otherwise you will end up with some other keys of the vault to also being optional) – which should already automatically result in Unknown inside frontend components. I think this approach requires less work and a bit cleaner 🙂

Also it does not make sense to me

Agreed

actually as i looked deeper into this: the UpdateAddress has no indexed values so we can't filter there :D. Seems like we have to filter page by page

Ahh, I don't know why PE is doing that. Let's still proceed with this, but after you implement it, please count how many requests is the overhead for all vaults.

@KirillDogadin-std
Copy link
Contributor

how many requests is the overhead for all vaults.

please rephrase the question to avoid the misinterpretation from my side.
Are you asking to count the max possible number of requests to fetch all vaults? fetch all vaults' oracle prices? something else?

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 13, 2022

Are you asking to count the max possible number of requests to fetch all vaults? fetch all vaults' oracle prices? something else?

There were two proposed solutions to the situation with private variables:
A. Fetch the events via paginated event filters and chainlog-derived determination of total number of pages (if needed)
B. Hardcode slot address for each contract and come up with mechanism to validate it hasn't changed/still valid

We went with A so far, but I want to know in the end, how much overhead (in terms of requests per minute) A brings over B.

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 13, 2022

The overhead now is 225 requests. potentially can go up to 250

Also after going over every entry in our COLLATERALS const i found the following edgecases via the etherscan:

So it seems like going over and hardcoding things is inevitable. Might as well find and hardcode the slots instead of hardcoding the event names :)

regarding the missing oracles: without those there will be no price and consequently we won't be able to tell if the vault is liquidatable right now or in the near future. This is the case for example with 15093 vault where the cdp manager claims the type to be 0x555344432d41 and the osm mom does not have the oracle address. The spotter on the other hand has. Should i introduce it as a backup option or replace the osm_mom with spotter?

@valiafetisov
Copy link
Contributor Author

some collateral types do not have an oracle listed in the osm mom contract

Where is it then?

Might as well find and hardcode the slots instead of hardcoding the event names

What would be the mechanism to validate slot hasn't changed/still valid? The mechanism to validate the events is simple: to fetch at least one event.

@KirillDogadin-std
Copy link
Contributor

Where is it then?

spot contract contains them as well. i guess it's a more reliable option since it's used in the price update mechanisms unlike the osm-mom contract.

What would be the mechanism to validate slot hasn't changed/still valid?

this definitely requires a simulation in place since the inner state is not exposed. I see it as follows:

  1. overwrite the slot of bud variable in order to add the address to the whitelist
  2. call peek and peep to get current and next values
  3. overwrite the current and the next values,
  4. validate the overwrite was successful via peep and peek again.

If we have a wrong bud slot, step (2) fails. If we have a wrong price slot, assertion in (4) fails.

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 13, 2022

One more edge case that speaks for hardcoding: not all contracts comply with osm abi, some reference dsthing as their oracles. Should i lay out the proposal?

So i guess we have to come up with a config that determines how the prices are being read.

@valiafetisov
Copy link
Contributor Author

Yes, please. Also, please try to better explain what is the edge case, I can't follow atm

@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 14, 2022

3 following links lead to the oracles of different collateral types:

https://etherscan.io/address/0xf45ae69cca1b9b043dae2c83a5b65bc605bec5f5#code
https://etherscan.io/address/0x81FE72B5A8d1A857d176C3E7d5Bd2679A9B85763#events
https://etherscan.io/address/0x25D03C2C928ADE19ff9f4FFECc07d991d0df054B#code

Upon closer examination you can find a variety of differences. Some of those are:

  • presence of the whitelist mapping (that forbids execution of some function if the caller was not authorized to do so)
  • presence of the variable that contains "next price" (which means that we potentially will not be able to predict the price change)
  • slot addresses of the variables.
  • event names

So i would have to manually configure at least 3 of these types of oracles and fetch prices based on the oracle configurations.

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 14, 2022

Thanks! Does this affect our ability to generically fetch BOTH current and next prices or only the next one? Do you want to propose how to extend COLLATERALS config with some parameters on osm types/slots/something else?

@valiafetisov valiafetisov changed the title Implement Vault Liquidation core logic Implement vault liquidation core logic Sep 14, 2022
@KirillDogadin-std
Copy link
Contributor

KirillDogadin-std commented Sep 14, 2022

Does this affect our ability to generically fetch BOTH current and next prices or only the next one? Do you want to propose how to extend COLLATERALS config with some parameters on osm types/slots/something else?

Missed it in the previous iterations: we actually should cover the case where we have no prices available anyway because there's also a flag that indicates wether the price is valid (if it's not, the contracts that depend on the oracle will not update their parameters & transaction fails )

I propose to do this first:

  • i find out which slot is applicable to which type of contract with hope that there're just these 3 types and each type has the same slots for prices.
  • i set the collection of oracle types with the slots for: next price, current price, whitelist, price validity flags. Looks like export const oracleTypeToConfig: Record<OracleType, OracleConfig>

If this does not cover, then we're forced to manually go over the contracts where this approach has failed and hardcode the slots for them. Potentially all the collateral types could have different slots so it's a lot of "manual labor"

@valiafetisov
Copy link
Contributor Author

Missed it in the previous iterations: we actually should cover the case where we have no prices available anyway
here's also a flag that indicates wether the price is valid

Alright, so it's another edge case, but this doesn't answer my first question. Do those different contracts complicates fetching of the current price, next price or both?

Record<OracleType, OracleConfig>

What is the OracleType, what is OracleConfig and why do you need to create a separate entities? Hardcoding slotAddress per collateralType seems to be more flexible already, right?

@KirillDogadin-std
Copy link
Contributor

Do those different contracts complicates fetching of the current price, next price or both?

differing contracts complicate fetching any value from them since the slot layout is different everywhere.

What is the OracleType, what is OracleConfig and why do you need to create a separate entities? Hardcoding slotAddress per collateralType seems to be more flexible already, right?

since they're 3 types of contracts that i've seen that supposedly have the same slot layout per type, i would think that having less duplication is more adequate.

Hardcoding slotAddress per collateralType seems to be more flexible already, right?

yes, but this seems to cover for theoretical (as of now) case where each collateral has unique set of values. If they don't , then it clutters the config. I can nevertheless just put the values right into the config if you'd like. On top of that (as a weak hypothetical case) if 3 types are distinguishable we may utilise the type id for future logic.

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 14, 2022

any value from them

I am still lacking the technicality where which value is located and which of them are private, which of them are not private, but still affected, so please be more specific! Does it affect ALL values of the VaultTransaction type?

since they're 3 types of contracts that i've seen that supposedly have the same slot layout per type

Again, I was asking to be more specific and provide the missing parts, since OracleType OracleConfig doesn't explain what they actually contain. OracleType = 'one' | 'two' | '3' and OracleConfig = { slotAddress: string; }?

having less duplication is more adequate

Obviously, not always, sometimes less duplication is overoptimization. But we can also discuss it after the code is there.

I can nevertheless just put the values right into the config if you'd like

I can't strongly suggest one over the other at the moment

@KirillDogadin-std
Copy link
Contributor

types are as follows

export type PriceOracleType = 'dsValue' | 'osm' | 'univ';
export declare interface PriceOracleConfig {
    currentPriceSlotAddress: string;
    currentPriceValidityFlagSlotAddress: string;
    nextPriceSlotAddress?: string;
    whitelistSlotAddress?: string;
    nextPriceValidityFlagSlotAddress?: string;
}

am still lacking the technicality where which value is located and which of them are private,

all of the relevant values are private for osm and univ and we're forced to use the slot access (osm, univ)

for dsValue we can access the current price since there's a public method.

Does it affect ALL values of the VaultTransaction type?

no, i'm currently only speaking about the oracle prices which are part of OraclePrices type.


the code related points (the rest) i agree with

@KirillDogadin-std
Copy link
Contributor

Additional changes that turns out are needed:

the vault type needs to receive fields:

    liquidationPenalty: BigNumber;
    minimalAuctionedDai: BigNumber;

where liquidationPenalty is the value that influences the dai that will be on the auction after liquidation in case partial liquidation occurs.
and minimalAuctionedDai sets the lower limit for the dai that is going to end up on the auction (relevant during partial liquidation as well).

@valiafetisov
Copy link
Contributor Author

valiafetisov commented Sep 15, 2022

Unfortunately, I can't follow. Also, looking at the types, I can see that there is a new LiquidationEvent type. My suggestion was to outline the changes to the other team members, eg:

  • @zoey-kaiser, please pay attention to the new LiquidationEvent type. It turns out that a vault can be liquidated several times. Therefore, there are now VaultTransactionLiquidated.pastLiquidations key containing information about previous liquidations
  • ...

@KirillDogadin-std
Copy link
Contributor

  • Added Oracle types to indicate various configurations of price sources (contracts). Depending on the type, collateral unit price will be extracted in various ways:
    • OracleCurrentAndNextPrices
    • OracleCurrentPriceOnly
  • Extended LiquidationLimits with values since the previous assumption about profit calculation logic (@zoey-kaiser , @valiafetisov, @LukSteib ) was not entirely correct. Partial liquidation of a vault can occur which influences the incentive and thus gross profit is different in this case. The added values are related to partial liquidation logic and serve the purpose of determinig what amount of vault's debt is going to be covered in auction:
    • liquidationPenaltyRatio
    • minimalAuctionedDai
  • Since there apparently can be several liquidation events per vault, the type not has a list of liquidation events instead. Added interface LiquidationEvent (@zoey-kaiser)
  • Changed type of the following VaultTransaction properties to BigNumber from number (@zoey-kaiser):
    • liquidationRatio: BigNumber;
    • collateralizationRatio: BigNumber;
    • proximityToLiquidation: BigNumber;

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 a pull request may close this issue.

2 participants