Skip to content
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

[WIP] "Just in time liquidity" AKA "splits" vault #1

Closed
wants to merge 5 commits into from

Conversation

wilsoncusack
Copy link
Contributor

@wilsoncusack wilsoncusack commented May 20, 2022

A "vault" which does not hold lenders funds but rather pulls the liquidity from lenders "just in time" to lend to a loan. Requires borrowers to pass in a set of lenders and lend amounts to use to fulfill the loan, and so assume there is off chain calculation done, tracking which lenders have approvals and enough funds to fill the loan. Also allows for reusing funds that have been paid back to a lender but not withdrawn.

This has a similar spirit to the Lend Offers PR - with-backed/backed-protocol#80 - but has a couple key differences

  • Multiple lenders can match to a single loan
  • There is a separate "terms" contract which provides lending and liquidation terms. Lenders approve terms contracts. This can also be thought of as lending strategies. E.g. you might approve a "Nouns terms contract" which lends to Nouns or a terms contract that uses oracle messages (passed in "data") to lend to nearly any NFT.

Concerns, some of these suggest changes to the core protocol:

  • I dislike the approvals calls required: approving facilitator to move NFT, approving facilitator to spend ERC20
  • In general lots of storage and checks and some loops. Feels like gas will be very high.

Roughly correct diagram of the case when the loan is created in the same transaction as the lend.

Screen Shot 2022-05-19 at 9 31 46 PM

(,,, uint40 lastAccumulatedTimestamp,,,,,,,) = loanFacilitator.loanInfo(loanId);

if(lastAccumulatedTimestamp != 0) {
require(terms.allowBuyouts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to think if this is really the best way to do this / what we want to do. Alternatively could

  • require(terms.amount >= loanFacilitator.totalOwed(loanId))
  • or just not do any check? If the terms contract is willing to lend to this asset, it should be willing to buyout? Expecting to get buyout amount back on repayment. (Off chain will have to ensure that terms include a 10% improvement over existing).

Choose a reason for hiding this comment

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

i think cool to not do any check. borrow ticket holder could then use UI to see if theres any way they could get better terms on their loan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well but this issue is, if you're a lender, are you OK more than X of your funds moving because you need to payback the previous lender, in addition to lending?


event SetTermsApproval(address indexed from, address indexed termsContract, bool approved);

function setTermsApproval(address termsContract, bool approved) external {
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'll probably want to have an amount based allowance and decrementing against it as funds go out.

}

// used to create loan and lend to it in one transaction
function onERC721Received(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using this onReceived hook allows us to prevent needing a multi call where users first createsLoan and then calls lend here. Although worse gas than multi call because NFT needs to go user -> LendController -> Loan Facilitator rather than user -> loan facilitator. But loan facilitator doesn't support multicall, anyway

Copy link
Contributor Author

@wilsoncusack wilsoncusack May 20, 2022

Choose a reason for hiding this comment

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

I think it would be cool if the core backed contract, on loanCreate allowed the borrower to send the lend ticket somewhere via a safeTransferFrom, counting on an immediate loan fulfillment as the result of that transfer. So the LendController would receive the lend ticket, instead, check terms and just return loan payment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would also be nice if we used like a fulfillLend hook to allow the lender to provide the liquidity by any means. Not necessarily a transfer from msg.sender.

terms.interestRate,
terms.amount,
terms.durationSeconds,
address(this)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this actually isn't quite right: we do not want to transfer the lend ticket to this contract. Because Backed has eager transfers for loan repayments, and we need to track whose funds are whose in the LendController, we will need to deploy a contract solely for the purpose of holding the lend ticket and receiving repayment. We will need one such contract for every lend ticket

Copy link
Contributor Author

@wilsoncusack wilsoncusack May 20, 2022

Choose a reason for hiding this comment

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

Non-eager payouts are better for composability but worse for gas in the EOA case: eats into lender returns to have to come back and withdraw payback

Choose a reason for hiding this comment

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

with regards to collateral seizure, are any of the lenders able to call seizeCollateral? does this newly deployed contract need to then know about liquidation strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the newly deployed contract would just allow the parent to make seize calls and send the collateral to itself/somewhere. parent (i.e. LendController) would control auction.

And yeah I think possibly anyone could call seizeCollateral, so long as what happens next (e.g. what kind of auction) is pre-agreed on

Copy link

@moodysalem moodysalem left a comment

Choose a reason for hiding this comment

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

just some somewhat higher level comments

uint16 interestRate;
uint128 amount;
uint32 durationSeconds;
address loanAsset;

Choose a reason for hiding this comment

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

why is this part of loan terms? it's included in the call

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 need on L147


for(uint i = 0; i < request.lendersSpecs.length; i++) {
LenderSpec memory info = request.lendersSpecs[i];
require(termsApprovals[info.lender][request.termsSource], 'lender has not approved terms source');

Choose a reason for hiding this comment

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

you could potentially borrow from lenders with different terms contracts, where the total lent must be less than the min

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but then need to do extra accounting on the way out? Can't assume everyone had the same rate, etc. But is interesting idea.

lenderLoanShares[loanId][info.lender] = info.amount * ONE / terms.amount;
}

loanAsset.approve(address(loanFacilitator), type(uint256).max);

Choose a reason for hiding this comment

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

ick from core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, as I said below

Would also be nice if we used like a fulfillLend hook to allow the lender to provide the liquidity by any means. Not necessarily a transfer from msg.sender.

struct BorrowRequest {
LenderSpec[] lendersSpecs;
address termsSource;
LoanTerms terms;

Choose a reason for hiding this comment

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

surprised the borrow request specifies the terms rather than it being derived from the terms source, but makes sense so the terms returned from the source can be validated. think these likely should be diff structs though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah was concerned about a race condition where terms contract is changed while borrow request tx is in the mempool. Agree could be different structs.

bool skipIsBuyoutCheck,
bytes calldata data
) internal {
LoanTerms memory terms = NFTLoanTermsSource(request.termsSource).terms(tokenId, msg.sender, request.terms.loanAsset, '');

Choose a reason for hiding this comment

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

should this method make some checks as to whether terms from terms source match up with request.terms? ideally they're the same yes, but someone could maliciously pass mismatching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will get checked in NFTLoanFacilitator, right? Must meet or beat.

}
lenderTotal += info.amount;

lenderLoanShares[loanId][info.lender] = info.amount * ONE / terms.amount;
Copy link

@joshieDo joshieDo May 23, 2022

Choose a reason for hiding this comment

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

lenderLoanShares[loanId] can be moved out of the loop for some gas savings for loops > 1 (example, example

mapping(address => uint256) storage loan = lenderLoanShares[loanId]
for(...) {
...
loan[info.lender] = info.amount * ONE / terms.amount;
}

and even accessing the fields on request might be better cached if used more than once, but i'm not 100% sure of that

@wilsoncusack
Copy link
Contributor Author

will come back to this with v2 done 😈

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.

4 participants