Skip to content

Conversation

larry0x
Copy link
Contributor

@larry0x larry0x commented Feb 8, 2023

No description provided.

deps: &DepsMut,
env: &Env,
market: &mut Market,
liquidity_taken: Uint128,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous implementation, where utilization is computed based on the contract's coin balance, we need to subtract the amount of coins that will be sent out.

In this new implementation, we don't need this anymore, because the internally tracked params (collateral_total_scaled and liquidity_index) have already been properly updated in the apply_accumulated_interests function.

env: &Env,
market: &mut Market,
liquidity_taken: Uint128,
denom: &str,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also don't need this denom, which is already provided by market.denom

Comment on lines -290 to -292
if contract_current_balance < liquidity_taken {
return Err(ContractError::OperationExceedsAvailableLiquidity {});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really need to check whether "operation exceeds available liquidity". If it does, the bank send message will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I wonder if this will later make it harder to know why the transaction fails. Error messaging already vague as is in deployed contracts.

Comment on lines +836 to +847
let debt_total_scaled =
compute_scaled_amount(asset_initial_debt, Decimal::one(), ScalingOperation::Ceil).unwrap();

let asset_initial_collateral = asset_liquidity + asset_initial_debt;
let collateral_total_scaled =
compute_scaled_amount(asset_initial_collateral, Decimal::one(), ScalingOperation::Ceil)
.unwrap();

let initial_utilization_rate = Decimal::from_ratio(debt_total_scaled, collateral_total_scaled);
let borrow_rate = ir_model.get_borrow_rate(initial_utilization_rate).unwrap();
let liquidity_rate =
ir_model.get_liquidity_rate(borrow_rate, initial_utilization_rate, reserve_factor).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this test wasn't set up properly: the collateral_total_scaled was uninitialized (default to zero), and borrow/liquidity rates were just some made up numbers that do not correspond to the utilization rate.

Now, in order for the test to pass, we need to actually choose the logical values for these parameters.

I consider this a win - it forces us to improve the quality of our tests!

Comment on lines +900 to +903
// in this particular example, we have to subtract 1 from the total underlying
// collateral amount here, because of rounding error.
let expected_collateral = expected_liquidity + expected_debt - Uint128::new(1);
let expected_utilization_rate = Decimal::from_ratio(expected_debt, expected_collateral);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rounding error happens when converting the collateral's scaled amount to the underlying amount. Here I manually subtract the expected underlying amount by 1 to reflect the error. Not sure if there's a better way.

@piobab piobab changed the base branch from master to release/MP-2503 May 10, 2023 20:02
@piobab piobab force-pushed the release/MP-2503 branch from d7e2d4e to c1434c5 Compare May 29, 2023 12:54
@piobab piobab force-pushed the fix-utilization-rate branch from 15076cc to 53988e0 Compare May 30, 2023 12:11
@piobab piobab force-pushed the fix-utilization-rate branch from 53988e0 to 4f03a6c Compare May 30, 2023 12:12
@piobab piobab marked this pull request as ready for review May 30, 2023 12:13
@piobab piobab self-requested a review May 30, 2023 12:13
@piobab piobab requested a review from grod220 May 30, 2023 12:37
Comment on lines -290 to -292
if contract_current_balance < liquidity_taken {
return Err(ContractError::OperationExceedsAvailableLiquidity {});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I wonder if this will later make it harder to know why the transaction fails. Error messaging already vague as is in deployed contracts.

@piobab piobab merged commit 4329538 into release/MP-2503 Jun 1, 2023
piobab added a commit that referenced this pull request Jun 8, 2023
* Mp 2422 emergency role improve (#185)

* Mp 2460 reject unexpected payment (#186)

* Compute utilization rate using internally-tracked parameters (#180)

* Update owner pkg to 1.2.0 (#198)

Co-authored-by: larry <26318510+larry0x@users.noreply.github.com>
Co-authored-by: Gabe Rodriguez <grod220@gmail.com>
@piobab piobab deleted the fix-utilization-rate branch June 8, 2023 11:46
pacmanifold pushed a commit that referenced this pull request Jul 18, 2023
* Mp 2422 emergency role improve (#185)

* Mp 2460 reject unexpected payment (#186)

* Compute utilization rate using internally-tracked parameters (#180)

* Update owner pkg to 1.2.0 (#198)

Co-authored-by: larry <26318510+larry0x@users.noreply.github.com>
Co-authored-by: Gabe Rodriguez <grod220@gmail.com>
Signed-off-by: Pacman <pacman@apollo.farm>
larry0x pushed a commit that referenced this pull request Oct 7, 2023
* Add liquidation_related flag for withdraw. It is used by credit manager to indicate liquidation.

* Fix cargo audit.

* Use correct commit from red-bank.

* Update schema.

* Use --locked for grcov install.
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 this pull request may close these issues.

3 participants