Upgrade Botimus to handle loan calc and validation #233

da-kami · 2021-08-11T10:15:54Z

Lender now includes the loan calculations and the validation of the loan.
I clicked it through and it looks sane :)

To be done in follow ups:

Send all amounts in sats and do conversion on UI side (did not tackle all amounts yet)
Validation step for the loan term
Upgrade the UI so the borrower can select values

bonomat · 2021-08-12T00:11:18Z

bobtimus/src/lib.rs

    ///
    /// We return the range of possible loan terms to the borrower.
    /// The borrower can then request a loan using parameters that are within our terms.
    pub async fn handle_loan_offer_request(&mut self) -> Result<LoanOffer> {


Does not look like it needs to be async anymore.

There is a lint you can enable to catch these automatically: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async

thomaseizinger · 2021-08-12T00:21:42Z

bobtimus/src/amounts.rs

 };
 use serde::{Deserialize, Serialize};
-use std::{convert::TryFrom, fmt::Debug};
+use std::{convert::TryFrom, fmt, fmt::Debug};


I tend to always import just the fmt module and refer to everything else from there. Makes it more consistent and reduces the number of imports.

The thing is, you will always need fmt::Fromatter and fmt::Result as well.

thomaseizinger · 2021-08-12T00:23:56Z

bobtimus/src/lib.rs

    /// Handle the borrower's loan request in which she puts up L-BTC as
    /// collateral and we lend L-USDt to her which she will have to
    /// repay in the future.


Similarly to commit messages, idiomatic rustdoc has a subject line with more fine-grained documentation in another paragraph: https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html#documenting-components

thomaseizinger · 2021-08-12T00:24:49Z

bobtimus/src/lib.rs

+            &loan_offer.terms,
+            &loan_offer.collateralizations,
+            loan_offer.base_interest_rate,
+        )?;


These might be worth extending with .context so you know which part failed.

thomaseizinger · 2021-08-12T00:30:01Z

extension/src/components/ConfirmLoan.tsx

+    let [timestamp, setTimestamp] = useState(Math.floor(Date.now() / 1000));
    useInterval(() => {
-        reloadBlockHeight();
+        setTimestamp(Math.floor(Date.now() / 1000));
    }, 6000); // 1 min


Could extract this into a useTime hook given that you are using it two times.

bonomat · 2021-08-12T04:01:02Z

bobtimus/src/loan.rs

+    for collateralization in collateralization_thresholds {
+        if borrower_collateralization >= collateralization.collateralization {
+            collateralization_interest_mod = collateralization.interest_mod;
+            continue;
+        }
+        break;
+    }


I don't understand the logic here :) Can you explain this please?

Overall idea: The lender can define different interest rates based on term and collateralization. The LoanOffer contains term and collateralization thresholds that may modify the interest rate of the loan. For collateralizations the assumption is that the modifier would usually be -x (if higher collateraliztion then lower interest rate), for the term it would be +x (if longer term then higher interest rate).

This piece of code: The modifier is set inside the if. As long as the collateralization falls into a higher threshold we keep looping. If the threshold is not higher we stop and use the modifier that was last set.

Note that for the term this has to be more restrictive - I will add a check to the loan acceptance criteria for that.

I feel dumb and don't understand it 😔

bobtimus/src/loan.rs

bonomat · 2021-08-12T04:06:43Z

bobtimus/src/loan.rs

+pub struct Term {
+    pub days: u32,
+    /// Interest to be added on top of the base interest rate for this term
+    pub interest_mod: Decimal,


What is this needed for?

see #233 (comment)

bonomat · 2021-08-12T04:09:46Z

bobtimus/src/loan.rs

+    use rust_decimal_macros::dec;
+
+    #[test]
+    fn test_calculate_interest_rate() {


I honestly cannot follow your tests.
Given that these calculations are critical can we have some code comments explaining what is happening?

I am not exactly sure what the problem is. From other comments I think there is confusion with the logic of making the interest rate configurable on top of loan term and collateralization.

Additionally, is it problematic that I splitted the calculation into small, testable functions?
At the moment there are tests that make sure that each calculation step is sane - but I did not add tests for the complete loan taking (yet).

I am not sure if it was the best idea to split the calculation into tiny chunks - I can combine more things again and we can write tests that cover bigger pieces of code - would that help or would you just like to have another bigger test on top?

Yes, it could be that the confusing part is how the tests are structured.

I have a hard time forming the code here into logic without grabbing another piece of paper and writing it out which makes it hard for me personally to see if the logic is sound.

What I would like to see is something like this (independent of this particular test):

// given: LTV: 80% exchange rate start: 10,000 interest_rate: 5%p.a. loan duration: 30 days principal amount: 10,000 overcollateralization_percent: 200% overcollateralization_btc: 3 // when do_some_math() // then collateralization_thresholds: X, interest: Y, ...

bonomat · 2021-08-13T03:26:02Z

bobtimus/src/loan.rs

+            rate: Rate {
+                ask: Default::default(),
+                bid: Default::default(),


It might still be useful to state it here so that one does not need to do the math on how the collateral_amount was chosen.

As a reference, that's what I came up with:

// collateral_amount / collateralization_percent = value of principal_amount 0.3675 / 1.4 = 0.2625 // so 0.2625 BTC = 10,000 // which means, 10,000 * 1/0.2625 = BTC price rate = 38095 USDT/BTC

now with this and the given max_ltv of 0.75 I get:

initial_price * LTV = liquidation_threshold 38095 * 0.75 = 28571.25

I ignored the 0.05% interest rate but the value seems to be close to what you have here.

bonomat · 2021-08-13T03:31:40Z

bobtimus/src/loan.rs

        }));
    }

+    if terms.iter().find(|a| a.days == request_term).is_none() {


Clippy is not complaining?

You could also .any here (but not that important):

if !terms.iter().any(|a| a.days == request_term) { return Ok(Err(LoanValidationError::TermNotAllowed { term: request_term, })); }

bonomat · 2021-08-13T03:42:18Z

bobtimus/src/loan.rs

+        let min_collateralization = sorted_collateralizations
+            .first()
+            .context("Unable to determine minimum collateralization")?
+            .collateralization;


You could get rid of this error/context handling here if you'd do :

let mut sorted_collateralizations = collateralizations.clone(); sorted_collateralizations.sort_by(|a, b| a.collateralization.cmp(&b.collateralization)); if let Some(Collateralization { collateralization, .. }) = sorted_collateralizations.first() ...

- Upgrade to baru 0.3 that allows to do the calculation of repay_amount, interest_rate (...) in waves - Loan calculation upon loan request, using static "current" loan offer - Laon validation upon loan request - Adapt the term and collateralization model of the loan offer and loan request to enable calculations (UI not extended yet but functional)

Blockheight was still used in extension instead of UNIX timestamp. It was previously accidentally not adapted because the term value was still hardcoded. Since we don't need the block-height anywhere else it was removed.

Includes a test for the overall calculation.

If we assume that the loan is paid back immediately the lender has to be covered for selling the asset upon repayment. Since the lender would sell the USDT at the current bid rate, that is what the collateral (covering the USDT repayment amount) should reflect.

Strict term validation, if the loan term given in the request is not specified (in days) in the current offer the loan is rejected.

Validate that the collateralization rate of a loan request is above the specified minimum collateralization threshold. There is not check for a maximum (the user can over-collateralize indefinitely). This commit includes some test refactoring to be able to specify tests faster.

The liquidation price has to reflect the over-collaterization of the borrower as well as the risk hunger of the lender. The lender's maxiumum LTV ratio has to be used for reflecting the risk hunger. The borrower is forced to over-collateralize heavily to be on the safe side.

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

bonomat · 2021-08-14T06:16:00Z

I see one issue (somehow related but not a blocker): in the UI we always show the principal amount instead of the repayment amount. Unfortunately it does not seem to be possible to get hold of this number from what is available in

waves/extension/wallet/src/wallet/extract_loan.rs

Line 11 in 06701f8

pub async fn extract_loan(

bonomat

Tests run locally. One issue detected and recorded in #242

da-kami requested review from bonomat and thomaseizinger August 11, 2021 10:15

da-kami changed the title ~~Upgrade bobtimus baru~~ Upgrade Botimus to handle loan calc and validation Aug 11, 2021

bonomat reviewed Aug 12, 2021

View reviewed changes

thomaseizinger reviewed Aug 12, 2021

View reviewed changes

bonomat reviewed Aug 12, 2021

View reviewed changes

bobtimus/src/loan.rs Show resolved Hide resolved

bonomat reviewed Aug 12, 2021

View reviewed changes

thomaseizinger mentioned this pull request Aug 12, 2021

Extract stateless functions into wallet library module #202

Merged

bonomat reviewed Aug 13, 2021

View reviewed changes

da-kami and others added 8 commits August 14, 2021 15:31

Fix timestamp display

b6db0e5

Blockheight was still used in extension instead of UNIX timestamp. It was previously accidentally not adapted because the term value was still hardcoded. Since we don't need the block-height anywhere else it was removed.

Loan calculations and validation in isolation

e340332

Includes a test for the overall calculation.

Term validation upon loan request

391f1b1

Strict term validation, if the loan term given in the request is not specified (in days) in the current offer the loan is rejected.

Add additional test with description

1cc5d7e

Signed-off-by: Philipp Hoenisch <philipp@hoenisch.at>

bonomat mentioned this pull request Aug 14, 2021

Use repayment_amount in extension when signing and repaying loan #242

Open

bonomat force-pushed the upgrade-bobtimus-baru branch from fa23aeb to 1cc5d7e Compare August 14, 2021 06:22

Clippy fixes

15675f8

bonomat approved these changes Aug 14, 2021

View reviewed changes

mergify bot merged commit c67ba51 into master Aug 14, 2021

mergify bot deleted the upgrade-bobtimus-baru branch August 14, 2021 06:29

Upgrade Botimus to handle loan calc and validation #233

Upgrade Botimus to handle loan calc and validation #233

Uh oh!

Conversation

da-kami commented Aug 11, 2021

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!

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!

da-kami Aug 12, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bonomat commented Aug 14, 2021

Uh oh!

bonomat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

da-kami Aug 12, 2021 •

edited

Loading