chore: fusdc recovery paths #10659

0xpatrickdev · 2024-12-09T19:42:52Z

closes: #10624

Description

handles potential failure in zoeTools.localTransfer during FUSDC Advance flow by returning funds to LP
documents why it's OK to fail during a potential zoeTools.withdrawToSeat failure in .disperse()
removes zcf.shutdownWithFailure() in catch blocks around zcf.atomicRearrange. This is only necessary when commits are not atomic.

Security Considerations

Improves handling of Payments during unexpected, but potential, failure paths.
Ensures FUSDC contract will not shutdown during unexpected failures.

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

Includes a unit test that ensures the advancer performs the expected calls after zoeTools.localTransfer fails.

Does not include unit tests for the new liquidity pool borrower.returnToPool as those have been deemed too expensive. The risk here seems low as the call is effectively passed through to repayer.repay, which has tests, modulo the KWR arrangement which is protected by the interface guard.

Upgrade Considerations

None, FUSDC is unreleased.

0xpatrickdev · 2024-12-09T19:44:42Z

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

+          zcf.atomicRearrange(
+            harden([[borrowSeat, repaySeat, amountKWR, returnAmounts]]),
+          );


I don't think we need try/catch here since we are protected by the borrower.repay interface guard.

LMK if that thinking is off base and/or a code comment is warranted,

The interface guard doesn't prevent calling this function with a borrowSeat that doesn't have enough USDC.

Check isGTE(borrowSeat.getAmountAllocated('USDC'), amountToRepay) before calling atomicRearrange.

And I think we need a try/catch here exactly as much as in the other functions that reallocate. I can see arguments either way (the commit phase is synchronous for all of them), but I don't see how this one should be different.

re-reading Prepare / Commit, the try / catch is only in order in case the commit phase is not atomic.

Check isGTE(borrowSeat.getAmountAllocated('USDC'), amountToRepay) before calling atomicRearrange

Agree, added.

re-reading Prepare / Commit, the try / catch is only in order in case the commit phase is not atomic.

I think this means we should remove the zcf.shutdownWithFailure(reason);'s in liquidity-pool? If yes, please see 1d8c44e

please remove all the shutdowns; leave >= 1 UNTIL #10684 comment.

In some cases, the try part of the commit has multiple synchronous steps. Part of me was thinking of that as atomic, since it's all 1 turn. But it's not atomic. Regardless, recent discussion suggests removing all shutdownWithFailure calls UNTIL #10684.

see 1d8c44e

I tried; no joy.

cloudflare-workers-and-pages · 2024-12-09T19:49:40Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`50c1bd1`
Status:	✅ Deploy successful!
Preview URL:	https://0c47bf2a.agoric-sdk.pages.dev
Branch Preview URL:	https://10624-fusdc-recovery-paths.agoric-sdk.pages.dev

View logs

dckc

I don't see any correctness problems, but I'm not ready to approve without some discussion.

dckc · 2024-12-09T22:20:39Z

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

-          // TODO #10510 (comprehensive error testing) determine
-          // course of action here
+        onRejected(error, { tmpSeat, advanceAmount, ...restCtx }) {
+          // we don't expect this to be a common failure. if it happens, return funds to LP


Is how common it is relevant? We're talking user funds here, after all.

Good point, updated the comment with "how"

dckc · 2024-12-09T22:21:49Z

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

-            'TODO live payment on seat to return to LP',
-            q(tmpSeat).toString(),
+            '⚠️ deposit to localOrchAccount failed, attempting to return payment to LP',
+            q(error).toString(),


why stringify the error? why not let log do that?

this will lose stack info, no?

Good point, done

dckc · 2024-12-09T22:28:15Z

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

+         * If something fails during advance, return funds to the pool.
+         *
+         * @param {ZCFSeat} borrowSeat
+         * @param {{ USDC: Amount<'nat'>}} amountKWR


Is there any reason to make this a record?

Only in as much localTransfer/withdrawToSeat and their ~counterparts depositToSeat, withdrawToSeat expect amountKWR.

I agree this could be dropped. If we did, I would do the same in borrow. Let me know if we should make this change.

Yes, I think we should.

I'm OK with leaving it if it would cost you another ci round trip.

dckc · 2024-12-09T22:33:27Z

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

+          zcf.atomicRearrange(
+            harden([[borrowSeat, repaySeat, amountKWR, returnAmounts]]),
+          );


The interface guard doesn't prevent calling this function with a borrowSeat that doesn't have enough USDC.

Check isGTE(borrowSeat.getAmountAllocated('USDC'), amountToRepay) before calling atomicRearrange.

And I think we need a try/catch here exactly as much as in the other functions that reallocate. I can see arguments either way (the commit phase is synchronous for all of them), but I don't see how this one should be different.

dckc · 2024-12-09T22:39:27Z

packages/fast-usdc/test/exos/advancer.test.ts

+      console.log('LpBorrowFacet.borrow called with', amounts);
+      mockBorrowerFacetCalls.push(['borrow', seat, amounts]);


given that we're now actually consuming the args and checking them, is logging them still useful?

Agree, removed

dckc · 2024-12-09T22:42:38Z

packages/fast-usdc/test/exos/advancer.test.ts

+    borrowCall[1],
+    'same temp borrowSeat is supplied to LP during repay',
+  );
+  t.deepEqual(repayCall[2], borrowCall[2], 'advance amount is returned to LP');


check repayCall[2] against some literal data? maybe the value of an amount?

Good feedback, done. Refactored this test a bit as it was confusing. Also added alerts if returnToPool fallback fails

dckc · 2024-12-09T22:46:16Z

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

@@ -88,6 +88,10 @@ export const prepareLiquidityPoolKit = (zone, zcf, USDC, tools) => {
          SeatShape,
          harden({ USDC: makeNatAmountShape(USDC, 1n) }),
        ).returns(),
+        repay: M.call(


using the same name (repay) with different semantics (sometimes fees, sometimes without) is asking for trouble, IME.

maybe...

Suggested change

repay: M.call(

returnToPool: M.call(

Suggested change

repay: M.call(

repayFailed: M.call(

Noted, updated to returnToPool

- only use `zcf.shutdownWithFailure` in a catch block around `atomicRearrange()` and when the commit phase is _not_ atomic - see #10659 (comment) - note: try/catch/finally remains for `borrow()` and `deposit()` so we can exit seats

dckc

Good stuff.

I feel some obligation to give the whole thing a fresh look, but I don't have energy for that today, so you might want to stand by for more eyes.

- wait until #10684, where we can terminate an incarnation without terminating the contract - see #10659 (comment) - note: try/catch/finally remains for `borrow()` and `deposit()` so we can exit seats

turadg · 2024-12-12T22:13:38Z

packages/fast-usdc/test/exos/advancer.test.ts

-  t.deepEqual(inspectLogs(1), [
-    'txHash already seen:',
-    '0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702',
+  const [, , ...remainingLogs] = inspectLogs();


TIL array destructuring doesn't have to name elements

I think I relearned that yesterday. Same with assignment while destructuring. JavaScript is fun 🙂

0xpatrickdev requested a review from a team as a code owner December 9, 2024 19:42

0xpatrickdev requested a review from AgoricTriage December 9, 2024 19:42

0xpatrickdev commented Dec 9, 2024

View reviewed changes

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from db3ced7 to 4db6510 Compare December 9, 2024 19:49

0xpatrickdev requested review from turadg and dckc December 9, 2024 20:02

dckc reviewed Dec 9, 2024

View reviewed changes

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from 4db6510 to 85ce625 Compare December 12, 2024 19:46

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from 85ce625 to 1d8c44e Compare December 12, 2024 19:51

0xpatrickdev added 3 commits December 12, 2024 15:05

chore: resolve withdrawToSeat TODO with comment

12510e9

fix: do not stringify logs

d04c5ea

chore: return advance payment to LP if zoeTools.localTransfer fails

b7065e7

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from 1d8c44e to a8a0cf1 Compare December 12, 2024 20:05

0xpatrickdev requested a review from dckc December 12, 2024 20:09

dckc approved these changes Dec 12, 2024

View reviewed changes

chore: do not call shutdownWithFailure() if atomicRearrange() fails

81517e2

- wait until #10684, where we can terminate an incarnation without terminating the contract - see #10659 (comment) - note: try/catch/finally remains for `borrow()` and `deposit()` so we can exit seats

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from a8a0cf1 to 81517e2 Compare December 12, 2024 21:53

turadg approved these changes Dec 12, 2024

View reviewed changes

refactor: LP.borrowerFacet expects Amount, not AmountKWR

50c1bd1

0xpatrickdev force-pushed the 10624-fusdc-recovery-paths branch from 7fac172 to 50c1bd1 Compare December 12, 2024 22:25

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Dec 12, 2024

mergify bot merged commit 4feddb0 into master Dec 12, 2024
91 checks passed

mergify bot deleted the 10624-fusdc-recovery-paths branch December 12, 2024 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fusdc recovery paths #10659

chore: fusdc recovery paths #10659

0xpatrickdev commented Dec 9, 2024 •

edited

Loading

0xpatrickdev Dec 9, 2024 •

edited

Loading

dckc Dec 9, 2024

dckc Dec 10, 2024

0xpatrickdev Dec 12, 2024 •

edited

Loading

dckc Dec 12, 2024

cloudflare-workers-and-pages bot commented Dec 9, 2024 •

edited

Loading

dckc left a comment

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc Dec 12, 2024

dckc Dec 9, 2024

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc Dec 9, 2024

0xpatrickdev Dec 12, 2024

dckc left a comment

turadg Dec 12, 2024

0xpatrickdev Dec 12, 2024

		console.log('LpBorrowFacet.borrow called with', amounts);
		mockBorrowerFacetCalls.push(['borrow', seat, amounts]);

chore: fusdc recovery paths #10659

chore: fusdc recovery paths #10659

Conversation

0xpatrickdev commented Dec 9, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

0xpatrickdev Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Dec 9, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev commented Dec 9, 2024 •

edited

Loading

0xpatrickdev Dec 9, 2024 •

edited

Loading

0xpatrickdev Dec 12, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 9, 2024 •

edited

Loading