feat: fusdc liquidity pool borrow and repay facets #10474

0xpatrickdev · 2024-11-13T21:20:55Z

refs: #10390

Description

Adds borrower and repayer facets to the Liquidity Pool
Track outstandingLends: Amount in state for checkPoolBalance invariant
Track PoolMetrics like cumulative borrows, repays, and fees

Security Considerations

Handles payment allocations, but is able to do so synchronously using a temporary seats provided by the callers.

Scaling Considerations

Makes a vstorage writes for each borrow and repay.

Documentation Considerations

Updates code comments for maintainers.

Testing Considerations

Tests use test-only methods on the createFacet to simulate fees from a borrow/repay sequence. Tests ensure zcf.atomicRearrange() in .borrow() and .repay() will not fail and trigger zcf.shutdownWithFailure().

Upgrade Considerations

None, unreleased

cloudflare-workers-and-pages · 2024-11-13T21:46:46Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3117eef`
Status:	✅ Deploy successful!
Preview URL:	https://a08a22b8.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fusdc-advancer-with-lp.agoric-sdk.pages.dev

View logs

packages/fast-usdc/src/exos/advancer.js

dckc · 2024-11-15T22:20:13Z

packages/fast-usdc/src/exos/liquidity-pool.js

+          // XXX COMMIT POINT?
+          const { USDC: paymentP } = await withdrawFromSeat(zcf, poolSeat, {
+            USDC: amount,
+          });


as discussed, rather than dealing with payments here, let's have the caller supply a seat.
The advancer can make a temp seat.

Then I think we can avoid turn boundaries in borrow and repay.

packages/fast-usdc/src/exos/liquidity-pool.js

dckc

this is before the fixups

dckc · 2024-11-18T21:09:55Z

packages/fast-usdc/src/exos/liquidity-pool.js

+  const virtualTotal = add(add(available, dust), outstandingLends);
+  isEqual(virtualTotal, shareWorth.numerator) ||


Suggested change

const virtualTotal = add(add(available, dust), outstandingLends);

isEqual(virtualTotal, shareWorth.numerator) ||

const encumberedBalance = add(add(available, dust), outstandingLends);

isEqual(encumberedBalance, shareWorth.numerator) ||

checkPoolBalance docstring needs updating

Since encumbered and unencumbered balance are mutually exclusive, I don't think encumberedBalance is a good name here.

I did rename outstandingLends to encumberedBalance and use unencumberedBalance to represent the current poolSeat allocation. I'm calling the sum of encumbered and unencumbered grossTotal, but am open to additional suggestions.

dckc · 2024-11-18T21:11:42Z

packages/fast-usdc/src/exos/liquidity-pool.js

  const available = poolSeat.getAmountAllocated('USDC', USDC);
  const dust = makeDust(USDC);
-  isEqual(add(available, dust), shareWorth.numerator) ||
+  const virtualTotal = add(add(available, dust), outstandingLends);
+  isEqual(virtualTotal, shareWorth.numerator) ||
    Fail`🚨 pool balance ${q(available)} inconsistent with shareWorth ${q(shareWorth)}`;


I wonder if this should crash the vat. (out of scope of this PR, I suppose)

dckc · 2024-11-18T21:14:21Z

packages/fast-usdc/src/exos/liquidity-pool.js

- * @param {Brand<'nat'>} USDC
+ * @param {object} powers
+ * @param {ZCF} powers.zcf
+ * @param {{brand: Brand<'nat'>; issuer: Issuer<'nat'>;}} powers.usdc


I don't see any use of usdc.issuer. Why this change?

vestigial, removed

dckc · 2024-11-18T21:17:08Z

packages/fast-usdc/src/exos/liquidity-pool.js

        /**
-         * @param {Amount<'nat'>} amount
-         * @param {Payment<'nat'>} payment
+         * @param {ZCFSeat} assetManagerSeat


assetManagerSeat seems like a left-over.

Suggested change

* @param {ZCFSeat} assetManagerSeat

* @param {ZCFSeat} toSeat

dckc · 2024-11-18T21:22:28Z

packages/fast-usdc/src/exos/liquidity-pool.js

      return {
-        shareMint,
-        shareWorth,
+        contractSeat,


It seems like there are lots of seats in this contract, so the name contractSeat doesn't help much. I had to look at the rest of the code to see what this one is for.

We've used feeSeat in other contracts. There's more than one kind of fee here, but I think I still prefer it.

dckc · 2024-11-18T21:26:32Z

packages/fast-usdc/src/exos/liquidity-pool.js

-        receive: M.call(AmountShape, PaymentShape).returns(M.promise()),
+      borrower: M.interface('borrower', {
+        lookupBalance: M.call().returns(AmountShape),
+        borrow: M.call(SeatShape, AmountKeywordRecordShape).returns(),


The method implementation assumes the keywords are right but this pattern doesn't ensure it.

Since the USDC brand is in scope, the pattern can constrain that too.

Good suggestion, done

dckc · 2024-11-18T21:31:25Z

packages/fast-usdc/src/exos/liquidity-pool.js

+                  assetManagerSeat,
+                  poolSeat,
+                  rest,
+                  { USDC: add(amounts.PoolFee, amounts.Principal) },


Add a test where the fromSeat doesn't have enough to cover these amounts, please? I think the atomicRearrange will fail, taking the contract down with it.

It looks like we should push this add() down into repayCalc. And repayCalc should get the seat allocation and the amounts. This isn't a zoe offer handler, so we shouldn't rely on the caller to ensure that the seat has enough.

Please see "repay fails when amounts do not match seat allocation" and "repay fails when seat allocation does not equal amounts".

I did not push the add() into repayCalc, but believe we can confidence atomicRearrange will not fail. There are additional tests that show the AKWR guards working as expected.

dckc · 2024-11-18T21:32:05Z

packages/fast-usdc/src/exos/liquidity-pool.js

          // Consumers of this .write() are off-chain / outside the VM.
          // And there's no way to recover from a failed write.
          // So don't await.
-          void recorder.write(shareWorth);
+          void recorder.write(
+            /** @type {PoolMetrics} */ ({


Why the cast here?

dckc · 2024-11-18T21:35:29Z

packages/fast-usdc/src/exos/liquidity-pool.js

-          void recorder.write(shareWorth);
+          void recorder.write(
+            /** @type {PoolMetrics} */ ({
+              availableBalance: poolSeat.getAmountAllocated('USDC', usdc.brand),


I wonder if that's a slight misnomer. If the poolSeat allocation is 1 USDC, you can't borrow all of it. The poolSeat can never go empty.

Per our convo, we'll publish encumberedBalance in PoolMetrics instead of availableBalance

dckc · 2024-11-18T21:38:57Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

+    const repayPayments = {};
+    for (const [kw, amount] of Object.entries(splits)) {
+      const pmt = await utils.pourPayment(amount);
+      repayPayments[kw] = pmt;
+    }


This should avoid the @ts-expect-error below:

Suggested change

const repayPayments = {};

for (const [kw, amount] of Object.entries(splits)) {

const pmt = await utils.pourPayment(amount);

repayPayments[kw] = pmt;

}

const repayPayments = await deeplyFulfilledObject(objectMap(splits, utils.pourPayment));

- factors existing makeLP test helper for reuse in other tests

dckc

isn't the encumbered balance the pool seat allocation minus outstanding borrows?

dckc · 2024-11-19T17:35:11Z

packages/fast-usdc/src/exos/liquidity-pool.js

-      const proposalShapes = makeProposalShapes({ USDC, PoolShares });
+      const proposalShapes = makeProposalShapes({
+        USDC,
+        PoolShares,
+      });


nit: superfluous whitespace change?

dckc · 2024-11-19T17:36:36Z

packages/fast-usdc/src/exos/liquidity-pool.js

+      /** used for `checkPoolBalance` invariant */
+      const encumberedBalance = makeEmpty(USDC);


Perhaps expand to say: poolSeat USDC allocation plus outstanding borrows.

I wonder if this docstring would get associated with state.encumberedBalance if it were moved down to the return statement.

Perhaps expand to say: ~~poolSeat USDC allocation plus outstanding borrows.~~ aka outstanding borrows

dckc · 2024-11-19T17:46:08Z

packages/fast-usdc/src/pool-share-math.js

+ * @param {Amount<'nat'>} poolSeatAllocation
+ * @param {Amount<'nat'>} encumberedBalance
+ * @param {PoolStats} poolStats
+ * @throws {Error} if conditions are not met


Let's either say what conditions or skip this @throws.

Suggested change

* @throws {Error} if conditions are not met

* @throws {Error} if too much is requested

dckc · 2024-11-19T17:49:47Z

packages/fast-usdc/src/pool-share-math.js

+  !isGTE(amounts.Principal, encumberedBalance) ||
+    isEqual(amounts.Principal, encumberedBalance) ||


why test both >= and = except to give separate errors?

if x = y, then x >= y is true, and !(x >= y) will fail. The isEqual branch can never be reached.

The isEqual branch can never be reached.

oops. this is an or.

but (!(x >= y) || x = y) is just y >= x, yes? so we can shorten this to isGTE(encumberedBalance, amounts.Principal)

Thanks, amended to simplify. I think soon™️ I'll have the hang of AmountMath. Looking at the source, it's interesting to see virtually every comparator is implemented with isGTE.

dckc · 2024-11-19T17:50:35Z

packages/fast-usdc/src/pool-share-math.js

+    Fail`Cannot repay. Principal ${q(amounts.Principal)} exceeds encumberedBalance ${q(encumberedBalance)}.`;
+
+  return harden({
+    shareWorth: withFees(shareWorth, amounts.PoolFee),


maybe withFees should be inlined... or at least not exported any more.

Agree, but there's an existing unit test that relies on it so I've left the export for now

dckc · 2024-11-19T17:53:28Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

+  // LPs can still withdraw (contract did not shutdown)
+  await lps.alice.withdraw(0.5);


dckc · 2024-11-19T17:56:29Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

+      encumberedBalance: {
+        value: 0n,
+      },


how did the encumberedBalance get to zero? and if it's zero, how can alice withdraw below?

We expect 0n here since all borrows have been returned

dckc · 2024-11-19T17:57:43Z

packages/fast-usdc/src/exos/liquidity-pool.js

+ * Verifies that the total pool balance (unencumbered + encumbered) matches the
+ * shareWorth numerator. The total pool balance consists of:
+ * 1. unencumbered balance - USDC available in the pool for borrowing
+ * 2. encumbered balance - USDC currently lent out


isn't the encumbered balance the pool seat allocation minus outstanding borrows?

ok. after discussion... the docs here are right.

dckc

some minor comments / suggestions

Sorry some of them were based on confusion.

strongest suggestion is to rewrite (!(x >= y) || x = y) as y >= x in pool-share-math

0xpatrickdev force-pushed the pc/fusdc-advancer branch from 37d455f to 1673c45 Compare November 13, 2024 21:31

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch from 30c1f02 to c2ad998 Compare November 13, 2024 21:46

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch from 6900cb1 to 1d6c5e8 Compare November 14, 2024 23:44

0xpatrickdev force-pushed the pc/fusdc-advancer branch 3 times, most recently from 165b3d7 to 99707ef Compare November 15, 2024 17:20

Base automatically changed from pc/fusdc-advancer to master November 15, 2024 18:07

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch from 1d6c5e8 to b0ec75a Compare November 15, 2024 18:08

0xpatrickdev changed the base branch from master to pc/fusdc-fees November 15, 2024 18:09

0xpatrickdev force-pushed the pc/fusdc-fees branch from 06ca4dd to 31a6f68 Compare November 15, 2024 18:09

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch 2 times, most recently from 98844c6 to 3a73737 Compare November 15, 2024 18:23

0xpatrickdev force-pushed the pc/fusdc-fees branch 4 times, most recently from 3ad5d52 to a1d931d Compare November 15, 2024 20:23

dckc reviewed Nov 15, 2024

View reviewed changes

packages/fast-usdc/src/exos/advancer.js Outdated Show resolved Hide resolved

dckc reviewed Nov 15, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-fees branch 2 times, most recently from cd0c2bf to 3771e7f Compare November 18, 2024 15:02

Base automatically changed from pc/fusdc-fees to master November 18, 2024 17:11

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch 2 times, most recently from 1f0314a to d91c88a Compare November 18, 2024 20:45

0xpatrickdev changed the title ~~feat: fusdc advancer integrates with liquidity pool~~ feat: fusdc liquidity pool borrow and repay facets Nov 18, 2024

0xpatrickdev marked this pull request as ready for review November 18, 2024 20:52

0xpatrickdev requested a review from a team as a code owner November 18, 2024 20:52

0xpatrickdev requested review from AgoricTriage and dckc November 18, 2024 20:52

0xpatrickdev commented Nov 18, 2024

View reviewed changes

packages/fast-usdc/src/exos/liquidity-pool.js Outdated Show resolved Hide resolved

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch from 033e2ea to 5da5aad Compare November 18, 2024 21:35

dckc reviewed Nov 18, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch 2 times, most recently from 3af5251 to b699b96 Compare November 19, 2024 15:38

0xpatrickdev added 2 commits November 19, 2024 10:38

refactor: smaller fees in mock FeeConfig

9d06522

refactor: makeLpTools test helper

65b9ec6

- factors existing makeLP test helper for reuse in other tests

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch 3 times, most recently from 78b4812 to a78e88c Compare November 19, 2024 17:01

0xpatrickdev requested a review from dckc November 19, 2024 17:07

dckc reviewed Nov 19, 2024

View reviewed changes

dckc approved these changes Nov 19, 2024

View reviewed changes

feat: liquidity pool borrower and repayer facets

3117eef

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch from a78e88c to 3117eef Compare November 19, 2024 20:16

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Nov 19, 2024

mergify bot merged commit 9c61393 into master Nov 19, 2024
90 of 91 checks passed

mergify bot deleted the pc/fusdc-advancer-with-lp branch November 19, 2024 21:00

		const virtualTotal = add(add(available, dust), outstandingLends);
		isEqual(virtualTotal, shareWorth.numerator) \|\|

	* @param {ZCFSeat} assetManagerSeat
	* @param {ZCFSeat} toSeat

		/** used for `checkPoolBalance` invariant */
		const encumberedBalance = makeEmpty(USDC);

	* @throws {Error} if conditions are not met
	* @throws {Error} if too much is requested

		!isGTE(amounts.Principal, encumberedBalance) \|\|
		isEqual(amounts.Principal, encumberedBalance) \|\|

		// LPs can still withdraw (contract did not shutdown)
		await lps.alice.withdraw(0.5);

feat: fusdc liquidity pool borrow and repay facets #10474

feat: fusdc liquidity pool borrow and repay facets #10474

Conversation

0xpatrickdev commented Nov 13, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 13, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

0xpatrickdev commented Nov 13, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 13, 2024 •

edited

Loading