feat(fast-usdc): deposit, withdraw liquidity in exchange for shares #10400

dckc · 2024-11-05T00:11:02Z

closes: #10386

Description

math module with unit tests
exo with precious state (including shareMint)
public facet methods to make, handle invitations
postponed: representing the pool as an ICA

Security Considerations

integrity: the contract could issue more shares than a liquidity provider paid for, or otherwise allow them to get more USDC out than they are due
availability: the contract could fail to redeem shares for sufficient USDC liquidity

TODO:

exo interface guards

Scaling Considerations

state is O(1), as is compute, I'm pretty sure.

Documentation Considerations

proposal shapes should help clients make offers.

There are no offerArgs nor invitationArgs.

Note that the PoolShare brand is now created by the contract, as part of a ZCFMint, not passed to startInstance as part of the issuerKeywordRecord.

Testing Considerations

Consistent with plans to do multi-vat swingset tests later, we have:

unit tests for the math
basic contract tests with a mock zoe

Upgrade Considerations

The liquidity pool exo (and the public facet) are designed to survive upgrade; testing this is planned for later.

cloudflare-workers-and-pages · 2024-11-05T00:13:07Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`5ae543d`
Status:	🚫 Deploy failed.

View logs

dckc · 2024-11-05T15:45:23Z

idea: I think the deposit and withdraw math should return TransferPart[] for use with atomicRearrange.

packages/fast-usdc/src/type-guards.js

samsiegart · 2024-11-07T19:32:36Z

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

+  {
+    const proposal = harden({
+      give: { USDC: usdc.make(100n) },
+      want: { PoolShare: make(PoolShares, 20n) },


I assume a real client would need to:

Read shareWorth from the contract somehow.

Use pool-share-math.js to calculate the expected number of shares.

Does that sound realistic? If so, could we add a TODO or modify the test to follow that model more?

Good point. Thanks for the reminder, in fact. I was thinking about this.

That prompted me to flesh out a bunch of publish/subscribe stuff (83ed6c8). I like the result, though; I enhanced the story-telling aspect of the test while I was at it:

✔ LP deposits, earns fees, withdraws (1.6s) ℹ bootstrap vat dependencies ℹ Alice deposits 60 USDC ℹ Bob deposits 40 USDC ℹ Alice deposit payout 60 PoolShares ℹ Bob deposit payout 40 PoolShares ℹ contract accrues some amount of fees: 25 USDC ℹ Alice sees fees earned 15 USDC ℹ Alice withdraws 20 %: 12 PoolShares ℹ Bob sees fees earned 10 USDC ℹ Bob withdraws 80 %: 32 PoolShares ℹ Alice withdaw payout 15 USDC ℹ Bob withdaw payout 40 USDC

I didn't show shareWorth in the t.log(...) story, but fetching it is necessary for "Alice sees fees earned 15 USDC", which is explicit in the product spec.

To calculate the expected number of shares, divideBy(give.USDC, shareWorth) suffices. No need for anything special from pool-share-math.js.

I didn't bother to handle slippage here. I suppose pool-share-math.js could be enhanced to support that, though.

Ah yea this seems more like it!

No need for anything special from pool-share-math.js.

Ack

I didn't bother to handle slippage here. I suppose pool-share-math.js could be enhanced to support that, though.

Hmm yea that could be tricky. The pool could also fluctuate drastically as it makes advances and receives settlements, so they'd have to call withdraw at the right time between advancements. Unless we wrote some logic that could put things on hold until the withdrawal completes.

0xpatrickdev

Nice, need to audit the math more closely but here's feedback so far.

Aside - a lot of DeFi "Share Math" vulns seem to hinge on the ability to donate funds to the pool. Seems like another +1 for using a seat/purse instead of a (reachable) OrchAccount.

0xpatrickdev · 2024-11-07T18:58:44Z

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

+     * @param {ZCFMint<'nat'>} shareMint
+     */
+    shareMint => {
+      const { brand: PoolShares } = shareMint.getIssuerRecord();


nit: should we call it PoolShare? Or even PS? I've gotten really used to reading brands as 3-4 uppercase letters.

hardly a nit! the keys of agoricNames.brand are of tremendous consequence.

I'll follow up with product.
cc @sufyaankhan

I suppose the name here is local to this contract. But I'm also working on the core-eval (#10301), which will put a name in agoricNames.brand.

I've gotten really used to reading brands as 3-4 uppercase letters

Here's the current list of brands in agoricNames,

ATOM BLD DAI_axl IST Invitation KREAdCHARACTER USDC USDC_axl USDC_grv USDT_axl USDT_grv stATOM stTIA timer stkATOM

I think the abbreviations are generally for vbank brands. Using the more verbose style would be appropriate here, in part to convey it's not a vbank brand.

not a vbank brand? why not? we don't want folks sending it over IBC to trade on the open market for some reason?

I think we punted on it being a vbank brand, but with the option to make it one later. That suggests that the name be amenable to vbank.

But ay you also point out, it's easy to change later. We can defer to milestone 3: #10432

0xpatrickdev · 2024-11-07T19:06:56Z

packages/fast-usdc/src/type-guards.js

+export const makeProposalShapes = ({ PoolShares, USDC }) => {
+  /** @type {TypedPattern<USDCProposalShapes['deposit']>} */
+  const deposit = M.splitRecord(
+    { give: { USDC: makeNatAmountShape(USDC, 1n) } },


Beneficial to parameterize min here?

I'm struggling to see any benefit. Help?

(anticipated) product req: user must deposit minimum of 1e6 or 10e6 units, not 1n units, so initial share price is not weird

weird how?

note that share worth starts 1-1:

const dust = AmountMath.make(USDC, 1n); const shareWorth = makeParity(dust, PoolShares);

Other than that, there are no special cases, AFAICT.

0xpatrickdev · 2024-11-07T19:31:48Z

packages/fast-usdc/test/pool-share-math.test.ts

+  'sequence of deposits and withdrawals',
+  [
+    fc.array(
+      fc.record({
+        party: fc.nat(7),
+        action: fc.oneof(
+          fc.record({ In: arbUSDC }),
+          fc.record({ Part: arbPortion, Slip: arbDelta }),
+        ),
+      }),
+      { minLength: 3 },
+    ),
+  ],


turadg · 2024-11-08T18:28:30Z

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

 const contractFile = `${dirname}/../src/fast-usdc.contract.js`;
 type StartFn = typeof import('../src/fast-usdc.contract.js').start;

+const makeTestContext = async () => {
+  const bundleCache = await makeNodeBundleCache('bundles', {}, s => import(s));


Unfortunate the test has this burden. I've filed #10427

turadg · 2024-11-08T23:18:39Z

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

+     * @param {ZCFMint<'nat'>} shareMint
+     */
+    shareMint => {
+      const { brand: PoolShares } = shareMint.getIssuerRecord();


I think we punted on it being a vbank brand, but with the option to make it one later. That suggests that the name be amenable to vbank.

But ay you also point out, it's easy to change later. We can defer to milestone 3: #10432

turadg · 2024-11-08T23:22:10Z

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

+import { AmountMath } from '@agoric/ertp/src/amountMath.js';
+import {
+  AmountShape,
+  PaymentShape,
+  RatioShape,
+} from '@agoric/ertp/src/typeGuards.js';


These can and should be imported from the top level

yeah. I'm still suffering from bundle-size PTSD.

It doesn't make any difference in this case. 2.7M bundle size in either case. Something else is grabbing the rest of ERTP, or the difference is less than 1%.

If these imports are in a stand-alone entry point module, there is a noticeable difference (1.1M vs. 732K), but not as much as I expected.

I noticed that ERTP gets M and matches from @agoric/store. Changing that to @endo/patterns reduces it to 728K.

turadg · 2024-11-08T23:23:53Z

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

+  TopicsRecordShape,
+} from '@agoric/zoe/src/contractSupport/topics.js';
+import {
+  deposit as depositCalc,


this is a local module. please name the exports such that they don't need renaming to import

turadg · 2024-11-08T23:28:54Z

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

+/**
+ * @param {Zone} zone
+ * @param {ZCF} zcf
+ * @param {Record<'USDC', Brand<'nat'>>} brands


the brands param reads like it's the source of all brands. since this is just for one brand please just pass it in as a usdcBrand param, Or if you really want the destructuring something like externalBrands.

oops. yeah. left-over from when a PoolShare brand was passed in

turadg · 2024-11-08T23:29:40Z

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

+      const { zcfSeat: poolSeat } = zcf.makeEmptySeatKit();
+      const shareWorthRecorderKit = tools.makeRecorderKit(
+        node,
+        /** @type {TypedPattern<ShareWorth>} */ (RatioShape),


consider a ShareWorthShape constant

Since the shape/pattern doesn't help enforce the ShareWorth invariant, I'm inclined to go the other way: TypedPattern<Ratio>.

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

turadg · 2024-11-08T23:45:57Z

packages/fast-usdc/src/fast-usdc.contract.js

  assertAllDefined({ feed, settler, advancer, statusManager });

-  const creatorFacet = zone.exo('Fast USDC Creator', undefined, {});
+  const creatorFacet = zone.exo('Fast USDC Creator', undefined, {
+    simulateFeesFromAdvance(amount, payment) {


might be worth a louder call to remove

What will the interfaces be for the advancer? Would be great to see them here. It will need to:

withdraw a payment from a purse

return principal to pool (advanced funds)

return a fee

turadg · 2024-11-08T23:48:02Z

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

+ *
+ * Shares = ToPool * sharesOutstanding / poolBalance
+ *
+ * that is:


thanks for stepping through

turadg · 2024-11-08T23:49:36Z

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

+
+  const { denominator: sharesOutstanding, numerator: poolBalance } = shareWorth;
+
+  const PoolShare = divideBy(give.USDC, shareWorth);


please give this a more specific name. e.g.

Suggested change

const PoolShare = divideBy(give.USDC, shareWorth);

const fairPoolShare = divideBy(give.USDC, shareWorth);

then the conditional below reads more clearly,

isGTE(fairPoolShare, want.PoolShare) || Fail

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

packages/fast-usdc/src/fast-usdc.contract.js

turadg · 2024-11-09T00:08:21Z

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

+};
+const test: TestFn<Awaited<ReturnType<typeof makeTestContext>>> = anyTest;
+
+test.before('cache bundles', async t => (t.context = await makeTestContext()));


consider leaving caching for #10430

turadg · 2024-11-12T18:35:36Z

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

      const { zcfSeat: poolSeat } = zcf.makeEmptySeatKit();
      const shareWorthRecorderKit = tools.makeRecorderKit(
        node,
-        /** @type {TypedPattern<ShareWorth>} */ (RatioShape),
+        /** @type {TypedPattern<Ratio>} */ (RatioShape),


please annotate RatioShape instead

Ratio is defined in @agoric/zoe. Adding a (dev)dependency from ERTP to Zoe seems odd.

Does annotating shareWorthRecorderKit work for you?

turadg · 2024-11-12T18:38:07Z

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

+              harden([
+                // zoe guarantees lp has proposal.give allocated
+                [lp, poolSeat, proposal.give],
+                // mintGains() above establishes that mint has post.payouts


turadg · 2024-11-12T18:38:51Z

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

+          } catch (bug) {
+            const reason = Error('🚨 cannot commit deposit', { cause: bug });


bug is cute but a little distracting.

Suggested change

} catch (bug) {

const reason = Error('🚨 cannot commit deposit', { cause: bug });

} catch (cause) {

const reason = Error('🚨 cannot commit deposit', { cause });

oh. I didn't notice the cute part. I was trying to distinguish runtime problems from design-time bugs. but ok.

I had some vague notion of that. If you want to communicate that intent to the reader please use a code comment

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

turadg · 2024-11-12T18:56:51Z

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

+            harden({ USDC: payment }),
+          );
+          this.state.shareWorth = withFees(shareWorth, amount);
+          await external.publishShareWorth();


if the publishing fails you want receive to throw?

I don't know of a way to recover from failed publishing so I tend to make the publish methods sync (and void their .write())

Another reason to await is to get a signal that the write happened, but the only consumers are off-chain so I don't see why the contract should wait.

my brain is leaky. I meant to head that way but forgot to or something

turadg

Good stuff!

turadg · 2024-11-12T23:11:43Z

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

+        publishShareWorth() {
+          const { shareWorth } = this.state;
+          const { recorder } = this.state.shareWorthRecorderKit;
+          // Consumers of this .write() are off-chain / outside the VM.


helpful comment!

turadg · 2024-11-12T23:19:51Z

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

+            this.state.proposalShapes.withdraw,
+          );
+        },
+        getPublicTopics() {


I was surprised to see getPublicTopics on this Exo until I saw that this publicFacet is the publicFacet of the contract.

Consider making this public facet be its own zone.exo in the contract that calls out to a closely held LiquidityPool exo.

Good idea, but after looking into it, I'd like to postpone this until we get more pieces of the contract together.

I started on it, but the LP exo isn't available until after remote calls, which would mean doing the same gymnastics for the public facet.

turadg · 2024-11-12T23:21:05Z

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

+ */
+export const prepareLiquidityPoolKit = (zone, zcf, USDC, tools) => {
+  return zone.exoClassKit(
+    'Fast Liquidity Pool',


an exo kind is always per vat, right? no need to include Fast.

This is also pretty close to being a general capability for managing a liquidity pool, especially if you move out the public interface.

0xpatrickdev · 2024-11-13T00:48:57Z

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

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


Consider assertPoolBalance

Can you talk more about what this invariant check is doing? How will it work when an advance is in flight?

Consider assertPoolBalance

assertFoo(x) usually means: assert that x is a Foo.

Can you talk more about what this invariant check is doing?

The atomicRearranges that follow the call to depositCalc / withdrawCalc assume that the pool balance represented by the poolSeat USDC allocation is the same as the pool balance represented by the numerator of shareWorth. When I first added this check, my tests failed; they differ by the dust used to make the initial shareworth denominator non-zero.

The fact that this answer didn't come in the form of a pointer to existing docs says I should add more.

How will it work when an advance is in flight?

In flight in what sense? Its only use is in normal straight-line synchronous offer handlers: gather all the relevant info in one place and compute the outcome, synchronously. Things happening outside this vat may happen causally before or after, or in an incomparable causal order. In any case, the local computation does what it does.

- use on RatioShape

- using contractSupport/ratio upgrades zoe from devDependencies

- proposal shapes for deposit, withdraw - pool math with property testing

dckc force-pushed the dc-pool-shares branch 3 times, most recently from ea8a352 to 9c389e2 Compare November 5, 2024 21:08

dckc mentioned this pull request Nov 5, 2024

PoolAccount management with LP tokens #10386

Closed

dckc changed the title ~~feat(fast-usdc): pool manager~~ feat(fast-usdc): deposit, withdraw liquidity in exchange for shares Nov 5, 2024

dckc commented Nov 5, 2024

View reviewed changes

packages/fast-usdc/src/type-guards.js Outdated Show resolved Hide resolved

dckc force-pushed the dc-pool-shares branch 3 times, most recently from 51f7c6d to 300c524 Compare November 6, 2024 00:34

dckc marked this pull request as ready for review November 6, 2024 00:35

dckc requested a review from a team as a code owner November 6, 2024 00:35

dckc requested review from thams, samsiegart, 0xpatrickdev and turadg November 6, 2024 00:35

samsiegart reviewed Nov 7, 2024

View reviewed changes

0xpatrickdev reviewed Nov 7, 2024

View reviewed changes

turadg reviewed Nov 8, 2024

View reviewed changes

turadg reviewed Nov 9, 2024

View reviewed changes

dckc mentioned this pull request Nov 10, 2024

feat(fast-usdc): cli deposit command (WIP) #10441

Draft

6 tasks

dckc requested review from turadg and 0xpatrickdev November 12, 2024 18:02

turadg reviewed Nov 12, 2024

View reviewed changes

dckc requested a review from turadg November 12, 2024 21:41

turadg approved these changes Nov 12, 2024

View reviewed changes

dckc force-pushed the dc-pool-shares branch 2 times, most recently from 825c566 to 26f451b Compare November 13, 2024 00:08

dckc added the automerge:rebase Automatically rebase updates, then merge label Nov 13, 2024

0xpatrickdev reviewed Nov 13, 2024

View reviewed changes

dckc force-pushed the dc-pool-shares branch from 26f451b to b2a4037 Compare November 13, 2024 02:58

dckc added 3 commits November 13, 2024 03:13

docs(ERTP): Ratio of Amount

5bfec0f

- use on RatioShape

chore(deps): @fast-check/ava for pool math property testing

9649c0a

- using contractSupport/ratio upgrades zoe from devDependencies

feat(fast-usdc): deposit, withdraw liquidity in exchange for shares

5ae543d

- proposal shapes for deposit, withdraw - pool math with property testing

dckc force-pushed the dc-pool-shares branch from b2a4037 to 5ae543d Compare November 13, 2024 03:14

mergify bot merged commit 324bca1 into master Nov 13, 2024
80 of 81 checks passed

mergify bot deleted the dc-pool-shares branch November 13, 2024 03:50


		const { denominator: sharesOutstanding, numerator: poolBalance } = shareWorth;

		const PoolShare = divideBy(give.USDC, shareWorth);

	const PoolShare = divideBy(give.USDC, shareWorth);
	const fairPoolShare = divideBy(give.USDC, shareWorth);

		} catch (bug) {
		const reason = Error('🚨 cannot commit deposit', { cause: bug });

feat(fast-usdc): deposit, withdraw liquidity in exchange for shares #10400

feat(fast-usdc): deposit, withdraw liquidity in exchange for shares #10400

Conversation

dckc commented Nov 5, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

Deploying agoric-sdk with Cloudflare Pages

dckc commented Nov 5, 2024

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

0xpatrickdev 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

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

turadg 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

dckc commented Nov 5, 2024 •

edited

Loading

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

edited

Loading