Health check by grod220 · Pull Request #7 · mars-protocol/rover

grod220 · 2022-07-21T22:43:26Z

The main feature is the health check assertion callback ensuring only healthy state changes are allowed. However, lots of little things are included here:

Oracle integration (using mock)
LTV and liquidation threshold now queried from red bank

contracts/credit-manager/src/health.rs

grod220 · 2022-07-21T22:45:35Z

contracts/credit-manager/src/health.rs

+}
+
+pub fn assert_health(deps: DepsMut, env: Env, token_id: NftTokenId) -> ContractResult<Response> {
+    let position = query_position(deps.as_ref(), &env, token_id)?;


I'm directly calling query_position here. Not sure if that is more normal or rather doing a query hitting that position msg endpoint.

contracts/credit-manager/src/execute.rs

contracts/credit-manager/src/health.rs

piobab · 2022-07-28T12:09:12Z

contracts/credit-manager/src/health.rs

+                &QueryMsg::Market {
+                    asset: asset_info.into(),
+                },
+            )?;


Some optimization could be to use QueryMsg::MarketsList to query markets in one call.

Ah, will investigate

In my case, MarketsList won't help me because it returns Vec<MarketInfo> which doesn't have max_tvl and liquidation_threshold fields. Will need to still call QueryMsg::Market individually.

We can fix that

I will work on red bank after finishing rewards collector. I will see what i can do

We could query by list of denoms but for now I don't know if it is worth

contracts/credit-manager/src/health.rs

piobab · 2022-07-28T12:20:50Z

contracts/credit-manager/src/health.rs

+    let margin = HEALTH_FACTOR_MARGIN.load(deps.storage)?;
+    let hf_str = position
+        .health_factor
+        .map_or("null".to_string(), |dec| dec.to_string());


To be honest I am not sure if this null is good. Maybe if it is null then we don't add it to attributes. Maybe we could add something more descriptive.

Would n/a be better? I wonder if it would be more difficult for the consumers of these attributes if the field sometimes wasn't present 🤔

N/a makes sense to me

n/a - perfect!

contracts/credit-manager/tests/borrow.rs

contracts/credit-manager/tests/create_credit_account.rs

piobab · 2022-07-28T14:08:40Z

contracts/mock-oracle/src/contract.rs

+    }
+}
+
+// TODO: After mars-core updates oracle contract to CosmWasm 1.0 should update this mock to return MarsDecimal


mars-core oracle already updated

I would need a version bump here: https://crates.io/crates/mars-core to import 🙏

Could you please use local deps for now ('lib = { path = "../lib" }')? Maybe it is better to update crates.io if we have everything ready (after testing and audit).

This type lives in mars-core so I don't have access to the file to add as a local dependency here. Maybe I'm missing something? Or are you suggesting copying over Mars' Decimal struct file to Rover (temporarily)?

New outpost doesn't use MarsDecimal (old one https://github.com/mars-protocol/mars-core/blob/master/packages/mars-core/src/red_bank/mod.rs#L13, new one https://github.com/mars-protocol/outposts/blob/master/packages/mars-outpost/src/red_bank/mod.rs#L4).

Anyway, for now you can leave it as it is but then we can bump on crates.io and use with correct https://github.com/mars-protocol/outposts/blob/master/packages/mars-outpost/src/red_bank/mod.rs#L43

oh! In that case, this code comment is invalid because there is not special Mars Decimal struct anyway. Happy to see it gone! Will remove this.

piobab · 2022-07-28T14:09:16Z

contracts/mock-red-bank/src/msg.rs

 }
+
+// Schema reference: https://github.com/mars-protocol/mars-core/blob/master/packages/mars-core/src/red_bank/mod.rs#L47
+// TODO: After mars-core updates oracle contract to CosmWasm 1.0 should update this mock to return MarsDecimal


grod220

Lots of changes from borrow PR relating to cw20s. Going to do a bit rebase with this.

grod220 · 2022-08-04T08:52:45Z

contracts/credit-manager/src/health.rs

+    let margin = HEALTH_FACTOR_MARGIN.load(deps.storage)?;
+    let hf_str = position
+        .health_factor
+        .map_or("null".to_string(), |dec| dec.to_string());


Would n/a be better? I wonder if it would be more difficult for the consumers of these attributes if the field sometimes wasn't present 🤔

contracts/credit-manager/tests/create_credit_account.rs

grod220 · 2022-08-04T09:00:37Z

contracts/mock-oracle/src/contract.rs

+    }
+}
+
+// TODO: After mars-core updates oracle contract to CosmWasm 1.0 should update this mock to return MarsDecimal


I would need a version bump here: https://crates.io/crates/mars-core to import 🙏

grod220 · 2022-08-04T09:02:59Z

contracts/credit-manager/src/health.rs

+                &QueryMsg::Market {
+                    asset: asset_info.into(),
+                },
+            )?;


Ah, will investigate

grod220 · 2022-08-05T17:05:31Z

contracts/credit-manager/src/health.rs

+    };
+
+    let liquidatable = lqdt_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());
+    let above_max_ltv = max_ltv_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());


This is different than fields. See: https://marslend.slack.com/archives/C03QSLT0LPJ/p1659677771072499. Having debt shares that are zero value is not a liquidatable event.

larry0x · 2022-08-11T02:55:51Z

contracts/credit-manager/src/execute.rs

+        ORACLE.save(deps.storage, &unchecked.check(deps.api)?)?;
+        response = response
+            .add_attribute("key", "oracle")
+            .add_attribute("value", unchecked.0);


not a fan of unchecked.0, because people will need to guess what "0" standards for. we know it stands for the contract's address, but it can well be something else.

instead, OracleBase can implement an address method that return the address:

struct OracleBase<T>(T); // T should be private impl<T> OracleBase<T> { pub fn address(&self) -> &T { return &self.0; } } response = response.add_attribute("value", unchecked.address());

Ah, this is much better. Actually think a getter would be even better, but those don't exist in rust. This will also require a new method since you can't use the constructor directly now.

larry0x · 2022-08-11T03:00:32Z

contracts/credit-manager/src/health.rs

+    let liquidatable = lqdt_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());
+    let above_max_ltv = max_ltv_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());


since false is a constant, you can use map_or instead of map_or_else

*_or_else methods are for non-constant values that need to be evaluated at runtime

Suggested change

let liquidatable = lqdt_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());

let above_max_ltv = max_ltv_health_factor.map_or_else(|| false, |hf| hf <= Decimal::one());

let liquidatable = lqdt_health_factor.map_or(false, |hf| hf <= Decimal::one());

let above_max_ltv = max_ltv_health_factor.map_or(false, |hf| hf <= Decimal::one());

Good to know that distinction

larry0x · 2022-08-11T03:03:05Z

contracts/credit-manager/src/health.rs

+pub fn assert_below_max_ltv(
+    deps: DepsMut,
+    env: Env,
+    token_id: NftTokenId,
+) -> ContractResult<Response> {
+    let position = compute_health(deps.as_ref(), &env, token_id)?;


Suggested change

pub fn assert_below_max_ltv(

deps: DepsMut,

env: Env,

token_id: NftTokenId,

) -> ContractResult<Response> {

let position = compute_health(deps.as_ref(), &env, token_id)?;

pub fn assert_below_max_ltv(

deps: Deps,

env: Env,

token_id: NftTokenId,

) -> ContractResult<Response> {

let position = compute_health(deps, &env, token_id)?;

larry0x · 2022-08-11T03:04:06Z

contracts/credit-manager/src/query.rs

        token_id: token_id.to_string(),
-        assets: get_assets(deps, token_id)?,
-        debt_shares: get_debt_shares(deps, token_id)?,
+        coin_assets: coin_asset_values,


why renaming assets to coin_assets? feels redundant. imo, it can either be coins or assets, but no coin_assets

I'm trying to distinguish the difference between:

Assets -> coins or vaults that are a positive contributor to your health factor

Debts -> coins you owe

But maybe renaming the state from ASSETS to COIN_BALANCES and this field to coins communicates this enough.

larry0x · 2022-08-11T03:08:00Z

contracts/credit-manager/src/query.rs

+                denom: denom.clone(),
+                amount: *amount,
+                price,
+                total_value,


imo this field should be called value instead of total_value. "total" means the sum of all coins' values. this is the value of just one coin.

larry0x · 2022-08-11T03:26:42Z

contracts/credit-manager/tests/create_credit_account_test.rs

        red_bank: RedBankBase("redbankaddr".to_string()),
+        oracle: OracleBase("oracle_contract".to_string()),


can you make the format of the mock contract addresses consistent

larry0x · 2022-08-11T03:35:29Z

contracts/credit-manager/tests/enumerate_assets_test.rs

+            CoinInfo {
+                denom: "coin_1".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_2".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_3".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_4".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_5".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_6".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_7".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_8".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_9".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_10".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_11".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_12".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_13".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },
+            CoinInfo {
+                denom: "coin_14".to_string(),
+                max_ltv: Decimal::from_atomics(7u128, 1).unwrap(),
+                liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(),
+                price: Decimal::from_atomics(10u128, 0).unwrap(),
+            },


may be a good idea to use a helper function to build this vector instead of typing it out, e.g.

let build_mock_coin_infos = |count: usize| -> Vec<CoinInfo> { (1..count) .into_iter() .map(|i| CoinInfo { denom: format!("coin_{}", i), max_ltv: Decimal::from_atomics(7u128, 1).unwrap(), liquidation_threshold: Decimal::from_atomics(78u128, 2).unwrap(), price: Decimal::from_atomics(10u128, 0).unwrap(), }) .collect() }; let coin_infos = build_mock_coin_infos(50);

larry0x · 2022-08-11T03:40:41Z

packages/rover/src/adapters/oracle.rs

+        let response: Decimal = querier.query(&QueryRequest::Wasm(WasmQuery::Smart {
+            contract_addr: self.0.to_string(),
+            msg: to_binary(&QueryMsg::AssetPrice {
+                denom: denom.to_string(),
+            })?,
+        }))?;
+        Ok(response)


a more succinct implementation. maybe works, haven't tried

Suggested change

let response: Decimal = querier.query(&QueryRequest::Wasm(WasmQuery::Smart {

contract_addr: self.0.to_string(),

msg: to_binary(&QueryMsg::AssetPrice {

denom: denom.to_string(),

})?,

}))?;

Ok(response)

querier.query_wasm_smart(

self.0.to_string(),

&QueryMsg::AssetPrice {

denom: denom.to_string(),

},

)

Indeed it does

larry0x · 2022-08-11T03:43:21Z

packages/rover/src/health.rs

+    pub assets_value: Decimal,
+    /// Total value of debts
+    pub debts_value: Decimal,
+    /// The sum of the value of all assets (multiplied by their liquidation threshold) over the


i feel the use of the term "liquidation threshold" is very confusing here.

imo "liquidation threshold" is a single constant number. if my health factor is below it, i'm liquidatable. otherwise i'm not liquidatable.

the term to be used here should be something like "collateral factor" or "collateral power"

Liquidation thresholds will always be different for different assets though.

Max LTV -- the most we'll allow a position to be taken out
Liquidation threshold -- the LTV where you get liquidated

There will be a wide variety of values for both of these.

larry0x · 2022-08-11T03:44:45Z

packages/rover/src/health.rs

+    /// Total value of assets
+    pub assets_value: Decimal,


imo if it's the total value of assets, it should be named total_asset_value

if it's the value of a single asset, it should be asset_value

at least this is how i name my variables it in Fields

That makes sense. However, would call it: total_assets_value

Uh i don’t think that’s correct english grammar

How so? Assets is the plural of asset. So the combined value of all assets could be: total_value_of_assets or total_assets_value.

grod220 · 2022-08-11T10:48:24Z

packages/rover/src/msg/query.rs

+    pub denom: String,
+    pub shares: Uint128,
+    pub total_value: Decimal,
+}


@larry0x so should we expose the shares or not here? I'm not sure it really communicates anything to the frontend. I wonder if instead we should do the math to calculate the amount underlying this and send that back instead.

i like returning the underlying amount instead. if someone wants to know the shares they can use raw query.

another benefit of returning the underlying amount is, instead of having two structs CoinValue and DebtShareValue, we can have only one (CoinValue) and use it for both assets and debts.

Yes, think this makes sense. But I want to create a follow up PR for this (given this one is nearly three weeks old).

grod220 commented Jul 21, 2022

View reviewed changes

contracts/credit-manager/src/health.rs Outdated Show resolved Hide resolved

grod220 commented Jul 21, 2022

View reviewed changes

kribby requested review from ahmadkaouk, dancreee, larry0x and piobab July 22, 2022 10:56

grod220 force-pushed the oracle-health branch 2 times, most recently from c07984e to 0fc8452 Compare July 25, 2022 08:12

piobab reviewed Jul 28, 2022

View reviewed changes

grod220 commented Aug 4, 2022

View reviewed changes

grod220 force-pushed the oracle-health branch from 0fc8452 to 7e9f487 Compare August 4, 2022 09:10

Base automatically changed from borrow to master August 4, 2022 18:12

grod220 force-pushed the oracle-health branch from 7e9f487 to 43fa0ba Compare August 4, 2022 18:18

grod220 commented Aug 5, 2022

View reviewed changes

grod220 force-pushed the oracle-health branch from 4b6edd3 to c8dc07e Compare August 5, 2022 17:10

grod220 added 3 commits August 8, 2022 08:34

Adding oracle & health check support

140477c

Adding liquidation threshold support

3d2cc07

More tests + debt value adjustment

6f530ce

grod220 force-pushed the oracle-health branch from 059442b to 6f530ce Compare August 8, 2022 06:34

piobab self-requested a review August 10, 2022 20:40

piobab approved these changes Aug 10, 2022

View reviewed changes

larry0x reviewed Aug 11, 2022

View reviewed changes

Review updates

3d49986

grod220 force-pushed the oracle-health branch from df73f62 to 3d49986 Compare August 11, 2022 10:40

grod220 commented Aug 11, 2022

View reviewed changes

grod220 requested a review from larry0x August 11, 2022 12:12

larry0x approved these changes Aug 11, 2022

View reviewed changes

grod220 merged commit 3e83ad2 into master Aug 11, 2022

grod220 deleted the oracle-health branch August 11, 2022 13:49

		let liquidatable = lqdt_health_factor.map_or_else(\|\| false, \|hf\| hf <= Decimal::one());
		let above_max_ltv = max_ltv_health_factor.map_or_else(\|\| false, \|hf\| hf <= Decimal::one());

		red_bank: RedBankBase("redbankaddr".to_string()),
		oracle: OracleBase("oracle_contract".to_string()),

Conversation

grod220 commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

piobab Aug 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grod220 left a comment

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!

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!

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

grod220 commented Jul 21, 2022 •

edited

Loading

piobab Aug 10, 2022 •

edited

Loading

larry0x Aug 11, 2022 •

edited

Loading

grod220 Aug 11, 2022 •

edited

Loading

larry0x Aug 11, 2022 •

edited

Loading