Lender decides timelock + deprecate Lender0's interpret#46
Conversation
There was a problem hiding this comment.
Looking good! Thanks for cleaning my version up @luckysori :)
Add. deprecated API removal: we don't have to remove them at all - the internet states that "deprecation" means that the "feature is not supported anymore" [1]. So there might not be any fixes / support for issues around it.
It is good to mark things that we don't want to support anymore. For longer running products, where the product team has numbers on the usage of old version one can take decision on actually removing things.
That said: Given that we are not in production yet, feel free to remove this particular feature after the next release :)
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Looks good.
In regards to removal of deprecated APIs it is essentially up to you to set a policy. It is nice if deprecated APIs are supported for at least one entire follow-up version, i.e. something that is deprecated in 0.3 is only removed in the bump to 0.5.
If downstream has a policy of compiling without warnings, that should not be an issue.
The Rust language and its standard library have a stability guarantee which means there will never be a version 2. Hence, deprecated APIs are never removed.
| /// Get a reference to the borrower's address. | ||
| pub fn borrower_address(&self) -> &Address { | ||
| &self.borrower_address | ||
| } | ||
|
|
||
| /// Get a reference to the loan borrower's public key. | ||
| pub fn borrower_pk(&self) -> &PublicKey { | ||
| &self.borrower_pk | ||
| } | ||
|
|
||
| /// Get a reference to the fee sats per vbyte. | ||
| pub fn fee_sats_per_vbyte(&self) -> Amount { | ||
| self.fee_sats_per_vbyte | ||
| } | ||
|
|
||
| /// Get a reference to the borrower's collateral inputs. | ||
| pub fn collateral_inputs(&self) -> &[Input] { | ||
| self.collateral_inputs.as_slice() | ||
| } |
There was a problem hiding this comment.
If the API using LoanRequest is now deprecated, why are are extending the interface of LoanRequest? I would - in addition to interpret - deprecate the entire LoanRequest struct and all functions that deal with it.
There was a problem hiding this comment.
Yeah, I wasn't sure about this either. I thought this would be a reasonable first step before we get rid of the actor state structs. I should probably already move these to Borrower0 or get rid of Borrower0 altogether.
There was a problem hiding this comment.
or get rid of Borrower0 altogether.
Deprecate first :)
There was a problem hiding this comment.
But, don't deprecate until you offer an alternative solution.
There was a problem hiding this comment.
I've decided against removing any of the actor state structs for now because it is so painful to remodel things when we still have to construct the transactions manually (as opposed to using PSETs).
I will extend Borrower0 as mentioned above and deprecate LoanRequest.
There was a problem hiding this comment.
Actually annotating LoanRequest with deprecated creates lots of warnings inside the library because the type still exists. It should be sufficient to mark the APIs that use it as deprecated.
luckysori
force-pushed
the
lender-decides-timelock
branch
from
b9bb474 to
9c62c9e
Compare
By moving them to the crate's `tests` directory we ensure that we're testing the public API of the library. We've temporarily made public the `oracle` sub-module in `loan`, but since it's not fully developed we don't announce it in the changelog.
Borrower no longer sets it earlier on in the loan protocol. Co-authored-by: Daniel Karzel <daniel@comit.network>
Users of the library want to be able to control how and what they serialize. Before this change they had to live with passing around the `LoanRequest` object, but now they are free to decompose it and pass the individual fields to the new `build_loan_transaction` API on `Lender0`. Co-authored-by: Daniel Karzel <daniel@comit.network>
luckysori
force-pushed
the
lender-decides-timelock
branch
from
9c62c9e to
6f47882
Compare
3 participants
I've opened this PR since I cannot push to #39.
Does this cover what we discussed in the other PR? @da-kami @thomaseizinger
Since Thomas suggested it I've used this as an opportunity to practice deprecating APIs. Once we get in the habit of deprecating, what should be the strategy for removing the deprecated API?