Lender decides the absolute block timelock#39
Conversation
| pub fn new( | ||
| collateral_amount: Amount, | ||
| collateral_inputs: Vec<Input>, | ||
| fee_sats_per_vbyte: Amount, | ||
| borrower_pk: PublicKey, | ||
| borrower_address: Address, | ||
| ) -> Self { | ||
| Self { | ||
| collateral_amount, | ||
| collateral_inputs, | ||
| fee_sats_per_vbyte, | ||
| borrower_pk, | ||
| borrower_address, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🤔 I think we shouldn't need to expose this to consumers of the library.
I agree with the maker being able to set the absolute timelock, rather than taking whatever they get from the borrower in the LoanRequest. But I think you shouldn't need to reconstruct this protocol message.
Looking at the matching PR on waves, you should be able to send your own application-specific message which looks like this:
WavesLoanRequest {
baru_loan_request: LoanRequest,
loan_term: u32, // in days
}Then you can just take the loan request field as is and you don't need to implement From which requires exposing a constructor for this struct.
There was a problem hiding this comment.
I deliberately wanted to avoid re-using baru's loan request when communicating with Bobtimus. I think the LoanRequest is an implementation detail within Bobtimus - you could change baru, but that would not necessarily have to change Bobtimus' interface. With using the LoanRequest in Bobtimus' interface we have very tight coupling. Is that on purpose?
There was a problem hiding this comment.
In the long-run, I think this state machine of Borrower0 etc should likely be removed from this library. Instead, baru should concern itself with making it easy to create transactions given the appropriate parameters. How those parameters have been created is not really a concern. Once baru provides a wallet, these APIs should get a lot simpler and the state machine can nicely move to the extension.
Instead of making constructors for these types, we could remove them from baru and change all the APIs to just take the data that was contained within these types. That would give the extension the necessary control over the wire messages sent.
API evolution tip (may not be worth it for such a small change): To make the upgrade smoother, instead of just removing APIs like Borrower0::loan_request, mark them as #[deprecated] and add new APIs that should be used instead.
For Borrower0::interpret, you can add a new API that takes all the values directly and have the old, deprecated API forward to the new one. Would need to find a new name because Rust doesn't support fn overloading but that shouldn't be too difficult.
There was a problem hiding this comment.
I deliberately wanted to avoid re-using baru's loan request when communicating with Bobtimus. I think the LoanRequest is an implementation detail within Bobtimus - you could change baru, but that would not necessarily have to change Bobtimus' interface. With using the LoanRequest in Bobtimus' interface we have very tight coupling.
I think that makes sense, @da-kami. Although in this instance I would have just treated the protocol message LoanRequest as a black box. That way if baru changes its format you wouldn't have to do anything. You can still have control over the message that you send between your applications, but LoanRequest would be opaque.
Instead of making constructors for these types, we could remove them from baru and change all the APIs to just take the data that was contained within these types. That would give the extension the necessary control over the wire messages sent.
I prefer @thomaseizinger's solution over the constructor because introducing the constructor makes the library confusing to use: when are you even supposed to construct the LoanRequest?
There was a problem hiding this comment.
Happy to go for a solution that offers a different / better API.
The only important thing for me is, that Bobtimus' communication and Waves' (as in the web application) communication with baru may want to be more loosely coupled.
@luckysori as an in-between solution we could also go with the solution you proposed.
Is it OK if I hand over this problem to you guys and you come up with a better solution? Happy to adhere to that later.
There was a problem hiding this comment.
@luckysori as an in-between solution we could also go with the solution you proposed.
Is it OK if I hand over this problem to you guys and you come up with a better solution? Happy to adhere to that later.
Absolutely.
Given that we can have multiple txins the loan contract might not always be at index position 1. Since we push the loan contract txin into the vec as last element we can be sure it is the last element in the vec when we satisfy the loan repayment.
da-kami
force-pushed
the
lender-decides-timelock
branch
from
782ac49 to
9bb512a
Compare
|
Note: rebased on top of #42 |
|
Superseded by #46 (couldn't push to this one D:) |
3 participants
needed for comit-network/waves#208