-
Notifications
You must be signed in to change notification settings - Fork 6
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
Loan offer #194
Loan offer #194
Conversation
bobtimus/src/lib.rs
Outdated
/// Handle Alice's loan offer request | ||
/// | ||
/// We return the possible loan terms that we offer to Alice | ||
/// Alice can then request a loan that is within the parameters that were offered | ||
pub async fn handle_loan_offer_request(&mut self) -> Result<LoanOffer> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we should remove the name "Alice" (and "Bob") from bobtimus and replace it with "borrower" (and "lender" respectively).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good to keep it in mind, I started doing it now (i.e. where I touched code that contains Alice/Bob I change it where needed) but I did not clean up all instances. Might be a bit tricky - in a protocol context we will want to leave it I suppose.
bobtimus/src/loan.rs
Outdated
pub max_principal: LiquidUsdt, | ||
|
||
/// The minimum ltv that is acceptable in percent | ||
pub min_ltv: Decimal, // TODO: is u64 appropriate here - how is ltv depicted internally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I point this out:
LTV => Loan To Value
Since we want to over-collateralize the loan the LTV ratio will be smaller than 0.
e.g.
- current rate: 1 BTC = $100
- principal amount = $100
- collateral amount = 2 BTC
--> LTV = 100 / 2*100 = 0.5
The higher this value is the higher the risk for the lender.
which means LTV value should be a floating point number and a max.
Note: In the loan contract the liquidation is decided according to the exchange rate (https://github.com/comit-network/baru/blob/a7743b538c91ff3f055c3a1f22a14502be7e275b/src/loan.rs#L68) (cc @luckysori ).
We will need to derive the value we need to put into liquidation function from the agreed max LTV ratio.
c5a3e77
to
892322e
Compare
8e4fb03
to
df76ae4
Compare
Should have run the tests first... those still need some fixing / extending. |
45ea181
to
8a520b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, definitely well seen changes :)
/// The maximum LTV that defines at what point the lender liquidates | ||
/// | ||
/// LTV ... loan to value | ||
/// LTV = principal_amount/loan_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% correct would be
principal_amount+interest_rate/loan_value
but let's ignore that for now.
) -> Result<JsValue, JsValue> { | ||
let collateral = map_err_from_anyhow!(parse_to_bitcoin_amount(collateral))?; | ||
// TODO: Change the UI to handle SATs not BTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this. Shall we record a ticket for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -225,10 +225,22 @@ pub async fn make_sell_create_swap_payload( | |||
pub async fn make_loan_request( | |||
wallet_name: String, | |||
collateral: String, | |||
fee_rate: String, | |||
timeout: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of changing the API in a way that we always deal with days until we get back the loan response. Thatway Bobtimus has to the calculation to actual blockheight and we only need to confirm it again in the UI when we sign it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I should have done that in the first place. The absolute timelock being decided at the very start is not optimal, because the relative time may already decline while the user is still deciding on the loan. This calculating the timelock upon taking that is not an issue anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do this in a follow-up that includes baru upgrades (because to do this properly some minor changes in baru will be needed that I'll just do on the way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only change request from me is on renaming the error
to debug
.
This requires changes and is not just a comment that explains the piece of code.
Get rid of this warning ``` WARNING: .dprintrc.json will be deprecated soon. Please rename it to dprint.json ```
The UI now properly display the values given by Bobtimus. When taking a loan the UI values are not sent yet though, it still uses hardcoded values from within the extension wallet.
Fixes #187
We are reaching a mergeable state given what is descibed in the ticket.
Overall the lending feature is still a bit brittle. Id say we record tickets for specific "bugs" rather than keep extending this PR.