feat(amm): support `maxOut` optional arg to `swap` #5125

dtribble · 2022-04-16T03:52:09Z

refs: #4856

Description

Implements the new API in terms of the existing underlying implementation.

Move swap implementation into shared swap.js
Implement maxOut using existing pricing operations
implement swapOut in terms of swapIn with maxOut
remove now-unneeded code in pool implementations
make allocateGainsAndLosses API the same for single and double pools

Security Considerations

N/A

Documentation Considerations

Need to deprecate the old API and add the new argument to Swap.

Testing Considerations

SwapOut is defined in terms of the new API, so it gets lot of testing now. Additional specific cases should be added for which the want is not the same as the maxOut.

Test cases from the liquidation branch

turadg

I think the test failure is legit. Debt is empty in this block:

  // An emergency liquidation got less than full value
  const debtAfterLiquidation = await E(vault).getCurrentDebt();

turadg · 2022-04-16T15:27:27Z

packages/run-protocol/src/vaultFactory/liquidateMinimum.js

    );

-    // Three (!) awaits coming here. We can't use Promise.all because
+    // Multiple awaits coming here. We can't use Promise.all because


just one now, right?

packages/run-protocol/src/vpool-xyk-amm/swap.js

turadg · 2022-04-16T15:42:38Z

packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js

@@ -193,6 +193,7 @@ const start = async (zcf, privateArgs) => {
    }

    const pool = isSecondary(brandOut) ? getPool(brandOut) : getPool(brandIn);
+    // @ts-expect-error cast


@Chris-Hibbert is it safe to cast to DoublePoolInternalFacet here?

Lines 184-193 are the doublePool case. This is handling a single pool, so the renaming of VPoolInternalFacet to DoublePoolInternalFacet looks like a mistake. This correctly returns VpoolWrapper.

renaming of VPoolInternalFacet to DoublePoolInternalFacet looks like a mistake

All properties of VPoolInternalFacet specified double,

/** * @typedef {Object} VPoolInternalFacet - virtual pool for price quotes and trading * @property {GetDoublePoolSwapQuote} getPriceForInput * @property {GetDoublePoolSwapQuote} getPriceForOutput * @property {(ZCFSeat, DoublePoolSwapResult) => string} allocateGainsAndLosses */

Is there a type for VPoolInternalFacet that doesn't presume pool type?

As far as I can tell, no. So the types are not generic here, even though the code kinda is.

It looks like someone made the type a union type, so it's now actually accurate.

Chris-Hibbert · 2022-04-17T22:43:38Z

packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js

@@ -193,6 +193,7 @@ const start = async (zcf, privateArgs) => {
    }

    const pool = isSecondary(brandOut) ? getPool(brandOut) : getPool(brandIn);
+    // @ts-expect-error cast


Lines 184-193 are the doublePool case. This is handling a single pool, so the renaming of VPoolInternalFacet to DoublePoolInternalFacet looks like a mistake. This correctly returns VpoolWrapper.

Chris-Hibbert · 2022-04-18T16:12:38Z

packages/run-protocol/src/vpool-xyk-amm/swap.js

+      AmountMath.coerce(amountOut.brand, maxOut);
+      prices = pool.getPriceForOutput(amountIn, maxOut);
+    }
+    if (!prices || !AmountMath.isGTE(prices.swapperGets, maxOut)) {


I think there are scenarios where current calls to swapOut() would fail to trade, and this code would fall through to getPriceForInput() and sell anyway. One I found is if there is insufficient liquidity to satisfy the swapOut. In that case, I think the failure to trade is the correct outcome, even though that's not what you want in the liquidation case.

I would also like to see reasonably thorough testing of the swapIn(amountIn, { maxOut }) cases.

Since it also passes the want.Out as a want.Out for the swapIn, I believe in all those cases it should properly fail to reallocate because the want is not satisfied.

dtribble · 2022-04-19T19:41:35Z

Test cases from the liquidation branch

Chris-Hibbert · 2022-04-19T22:05:13Z

switching between getPriceForInput and getPriceForOutput when the user calls either swapIn or swapOut breaks the assumptions laid out in the doc.

The first consequence I can see is that the way fees are calculated is supposed to depend on whether they called swapIn or swapOut ("The pool fee is charged against the side not specified by the user" assumes that swapIn specifies the input price as base, and swapOut specifies the output price as the base.) In some cases this means the pool fee will be charged in the opposite currency.

That's probably defensible, but we haven't done so yet.

I'm not sure those are the only consequences. I am convinced that offer safety constraints are not broken by this approach. When the user specifies amountOut, there won't be a transaction unless they get at least that much. If they specify stopAfter instead, they don't get offer safety.

You suggested that we could change to an approach where we charge a flat fee on top of the transaction and allocate it to pool and protocol fees separately from calculating prices. That would also be defensible, but that's not what this change does.

* Move swap implementation into shared `swap.js` * Implement `maxOut` using existing pricing operations * implement `swapOut` in terms of `swapIn` with `maxOut` * remove now-unneeded code in pool implementations * make `allocateGainsAndLosses` API the same for single and double pools

* remove fallback `swapOut` since it's not needed anymore * suppress problematic typescript warnings

* to preclude any difference from documented behavior fro `swapOut` keep using the direct implementation for now

Chris-Hibbert

Thanks for listening to my concerns.

This is good.

Chris-Hibbert · 2022-04-19T22:53:41Z

packages/run-protocol/src/vpool-xyk-amm/swap.js


 /**
 * @param {ZCF} zcf
- * @param {(brandIn: Brand, brandOut: Brand) => VPoolWrapper<unknown>} provideVPool
+ * @param {(brandIn: Brand, brandOut: Brand) => VPoolWrapper<DoublePoolInternalFacet>} provideVPool


provideVPool can provide either singlePool or doublePool, though no one should care. I'd make it VPoolWrapper<unknown>, which allows future extensions and declares that we don't care.

dtribble requested a review from Chris-Hibbert April 16, 2022 03:52

dtribble requested a review from turadg as a code owner April 16, 2022 03:52

turadg reviewed Apr 16, 2022

View reviewed changes

Chris-Hibbert requested changes Apr 18, 2022

View reviewed changes

dtribble force-pushed the 4856_simple_amm_api branch from cad92bb to e1688f5 Compare April 19, 2022 19:07

dtribble requested a review from Chris-Hibbert April 19, 2022 19:18

dtribble force-pushed the 4856_simple_amm_api branch from 2def151 to 1aa1371 Compare April 19, 2022 21:37

dtribble and others added 8 commits April 19, 2022 15:47

feat(vault): use maxOut for liquidation

d6c2c14

* remove fallback `swapOut` since it's not needed anymore * suppress problematic typescript warnings

chore(typescript): suppress type ignorance

76c2b69

chore(amm): types

702536c

chore(amm): merge fixes from liquidation work

ae796af

chore(amm): rename maxOut to stopAfter

aa1f4b8

types

261331a

chore(amm): roll-back swapOut change

83a84f6

* to preclude any difference from documented behavior fro `swapOut` keep using the direct implementation for now

dtribble force-pushed the 4856_simple_amm_api branch from 2b33b8b to 83a84f6 Compare April 19, 2022 22:47

Chris-Hibbert approved these changes Apr 19, 2022

View reviewed changes

dtribble added the automerge:squash Automatically squash merge label Apr 19, 2022

Merge branch 'master' into 4856_simple_amm_api

d2b7184

mergify bot merged commit 54151ac into master Apr 20, 2022

mergify bot deleted the 4856_simple_amm_api branch April 20, 2022 00:07

dtribble mentioned this pull request Apr 22, 2022

Simplify AMM Swap API #4856

Closed

This was referenced Apr 26, 2022

AMM support for selling enough to repay a debt, or everything if there isn't enough #4321

Closed

AMM Roadmap #1673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(amm): support `maxOut` optional arg to `swap` #5125

feat(amm): support `maxOut` optional arg to `swap` #5125

dtribble commented Apr 16, 2022 •

edited

Loading

turadg left a comment

turadg Apr 16, 2022

dtribble Apr 19, 2022

turadg Apr 16, 2022

Chris-Hibbert Apr 17, 2022

turadg Apr 18, 2022

dtribble Apr 18, 2022

dtribble Apr 19, 2022

Chris-Hibbert Apr 17, 2022

Chris-Hibbert Apr 18, 2022

dtribble Apr 18, 2022 •

edited

Loading

dtribble commented Apr 19, 2022

Chris-Hibbert commented Apr 19, 2022

Chris-Hibbert left a comment

Chris-Hibbert Apr 19, 2022

feat(amm): support maxOut optional arg to swap #5125

feat(amm): support maxOut optional arg to swap #5125

Conversation

dtribble commented Apr 16, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtribble Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

dtribble commented Apr 19, 2022

Chris-Hibbert commented Apr 19, 2022

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(amm): support `maxOut` optional arg to `swap` #5125

feat(amm): support `maxOut` optional arg to `swap` #5125

dtribble commented Apr 16, 2022 •

edited

Loading

dtribble Apr 18, 2022 •

edited

Loading