A constant product AMM that charges pool and protocol fees #3791

Chris-Hibbert · 2021-09-03T21:20:23Z

Uses Ratios pervasively in calculating prices and fees
Uses virtual pools to calculate prices and do swaps for two hop (aka "doubleswap") trades

Constant Product calculations for an AMM. (ref: #3791) type declarations Allow swapOut() to have an empty amountIn representing "no restriction" Drop enforcement in swapIn/swapOut that the quotes are in the forward direction doublePool will need to call them in the reverse direction extract common code in swapIn & swapOut drop reporting on price improvements reduce duplicated code in swap.js Co-authored-by: Kate Sills <kate@agoric.com>

dckc

I'm struggling to read the code for getInputPrice. I'd like to discuss it.

packages/zoe/test/unitTests/contracts/vpool-xyk-amm/test-xyk-amm-swap.js

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

packages/zoe/src/contracts/vpool-xyk-amm/doublePool.js

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

dckc

I'd like to discuss the use of Far on pools.

dckc · 2021-10-01T05:16:01Z

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

+  // This contract must have a "Central" keyword and issuer in the
+  // IssuerKeywordRecord.
+  // poolFee is the portion of the fees that go into the pool (in BP).
+  // protocolFee is the portion of the fees that are shared with validators


with validators? is that right? Is that the same as the stability pool? We discussed this but now I don't recall your answer.

Mentioning the validators here is too specific. stability pool is one of the names that has been bandied about without resolution, but I'll use it as an improvement over what I wrote.

dckc · 2021-10-01T05:22:34Z

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

+ * @param {(brand: Brand) => boolean} isSecondary
+ * @param {(brand: Brand, pool: XYKPool) => void} initPool
+ * @param {Brand} centralBrand
+ * @param {Timer} timer


Requiring a Timer rather than an ERef<Timer> is awkward. When working on the governance demo, I discovered it's not just a matter of type declarations. Zoe or something really requires a Timer in some places. This makes no sense to me, since all calls to it are remote.

changed to ERef<>.

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

dckc · 2021-10-01T15:56:12Z

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

+    };
+
+    /** @type {XYKPool} */
+    const pool = Far('pool', {


This Far declaration suggests this might be exposed to callers from other vats, so I'm checking much more closely for defensive correctness.

Is it actually exposed? If not, why Far? (perhaps using Far to be conservative is good... I'm still thinking thru it...)

Not exposed outside the module. I'll drop Far here and in related places.

dckc

I'd like to discuss the case of of a zcfSeat with a non-fungible Central allocation in XYKPool.addLiquidity.

dckc · 2021-10-01T20:36:04Z

packages/zoe/src/contracts/vpool-xyk-amm/doublePool.js

+ * @param {bigint} protocolFee
+ * @param {bigint} poolFee


I take it we're 99.9999% sure we don't need more resolution than 1 in 10,000 for these fees, but I'd like confirmation. Noone in the next several years will want / need a fee ratio of, say, 0.035%?

I suspect there is a AMM design document that I missed where I could confirm that @rowgraus , @btulloh , @dtribble are content that we don't require finer resolution than a basis point for fee ratios. Got a pointer handy?

I don't have a link for an AMM design doc.

I'm confident that we don't need finer resolution than a basis point. I don't have 6 nines confidence.

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

dckc

a couple more notes... finishing some stuff up...

dckc · 2021-10-01T22:24:16Z

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

+      liquidityZcfMint.mintGains({ Liquidity: liquidityAmountOut }, poolSeat);
+      liqTokenSupply += liquidityValueOut;
+
+      poolSeat.incrementBy(
+        zcfSeat.decrementBy({
+          Central: zcfSeat.getCurrentAllocation().Central,
+          Secondary: secondaryAmount,
+        }),
+      );
+
+      zcfSeat.incrementBy(
+        poolSeat.decrementBy({ Liquidity: liquidityAmountOut }),
+      );
+      zcf.reallocate(poolSeat, zcfSeat);
+      zcfSeat.exit();
+      updateState(pool);


The stuff between mintGains and exit (or maybe reallocate) feels like some sort of critical section. @erights were you looking into some idiom for those?

@Chris-Hibbert in this case, if something in this section throws, did rights get minted and lost? Or are we safe? Or are we certain that nothing throws?

It's not critical. The only place that users' assets are changed is in the reallocate() call. If something breaks between mint and reallocate, we've leaked RUN into an internal Pool that will ignore it, but the user is protected by offer safety.

leaking RUN seems pretty bad; I was under the impression we wanted to be able to account for every last jot and tiddle at all times. But I can ask around about that separately.

packages/zoe/src/contracts/vpool-xyk-amm/pool.js

dckc · 2021-10-01T22:48:49Z

packages/zoe/src/contracts/vpool-xyk-amm/multipoolMarketMaker.js

+   * @param {Ratio} poolFeeRatio
+   * @returns {VPool}
+   */
+  const provideVPool = (brandIn, brandOut, poolFeeRatio) => {


You suggested I study this code until I see how until you can see how singlePool and doublePool polymorphically solve the previously complicated problem. I think I get it now. This seems to be where they meet.

dckc

looks good.

I have a couple suggested changes and a few comments to consider, but none of them are critical.

To confirm my understanding of what's stored in some of the variables, I added some type declarations as I reviewed the code. They're available in #3920. Feel free to integrate them or not now; if not, I may submit them later.

Uses Ratios pervasively in calculating prices and fees Uses virtual pools to calculate prices and do swaps for two hop trades

Chris-Hibbert added enhancement New feature or request Beta Core Economy OBSOLETE in favor of INTER-protocol AMM labels Sep 3, 2021

Chris-Hibbert added this to the Beta Phase 4: Governance milestone Sep 3, 2021

Chris-Hibbert self-assigned this Sep 3, 2021

Chris-Hibbert mentioned this pull request Sep 3, 2021

Evaluate improvements to protocol fee calculation #3018

Closed

Chris-Hibbert requested a review from katelynsills September 3, 2021 21:40

Chris-Hibbert force-pushed the constantProductAMM branch from fd77182 to 1e903b9 Compare September 23, 2021 00:25

Chris-Hibbert force-pushed the constant-Product branch from 51d0f43 to 57e8f50 Compare September 23, 2021 00:36

Chris-Hibbert force-pushed the constantProductAMM branch 2 times, most recently from 2349786 to aeebce0 Compare September 23, 2021 00:53

dckc self-requested a review September 29, 2021 22:20

Chris-Hibbert force-pushed the constant-Product branch from d3dd05c to cf3f73a Compare September 30, 2021 20:54

Chris-Hibbert force-pushed the constantProductAMM branch from aeebce0 to 4584a65 Compare September 30, 2021 21:08

Base automatically changed from constant-Product to master September 30, 2021 21:22

Chris-Hibbert force-pushed the constantProductAMM branch from 4584a65 to 7b769ac Compare September 30, 2021 22:57

dckc reviewed Oct 1, 2021

View reviewed changes

dckc mentioned this pull request Oct 1, 2021

store.get throws on not found: same name as Map.get but different semantics #3917

Closed

dckc reviewed Oct 1, 2021

View reviewed changes

dckc force-pushed the constantProductAMM branch from 2d72e7c to 7b769ac Compare October 1, 2021 23:01

dckc reviewed Oct 1, 2021

View reviewed changes

dckc approved these changes Oct 1, 2021

View reviewed changes

feat: a constant product AMM that charges pool and protocol fees

c551739

Uses Ratios pervasively in calculating prices and fees Uses virtual pools to calculate prices and do swaps for two hop trades

Chris-Hibbert added 3 commits October 2, 2021 10:11

chore: update vpool AMM for new location of swapIn/swapOut

43d0f1f

chore: integrate with renaming of calcSwapInPrices/calcSwapOutPrices

de27595

chore: cleanups, renamings, type improvements

3c078fe

Chris-Hibbert force-pushed the constantProductAMM branch from 4d836a5 to 3c078fe Compare October 2, 2021 18:03

Chris-Hibbert merged commit 3f53592 into master Oct 2, 2021

Chris-Hibbert deleted the constantProductAMM branch October 2, 2021 18:26

dckc mentioned this pull request Oct 7, 2021

Fix protocol vs. pool fee calculations #3340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A constant product AMM that charges pool and protocol fees #3791

A constant product AMM that charges pool and protocol fees #3791

Chris-Hibbert commented Sep 3, 2021 •

edited by dckc

Loading

dckc left a comment

dckc left a comment

dckc Oct 1, 2021

Chris-Hibbert Oct 1, 2021

dckc Oct 1, 2021

Chris-Hibbert Oct 1, 2021

dckc Oct 1, 2021

Chris-Hibbert Oct 1, 2021

dckc left a comment

dckc Oct 1, 2021

dckc Oct 1, 2021

Chris-Hibbert Oct 1, 2021

dckc left a comment

dckc Oct 1, 2021

Chris-Hibbert Oct 1, 2021

dckc Oct 2, 2021

dckc Oct 1, 2021

dckc left a comment

A constant product AMM that charges pool and protocol fees #3791

A constant product AMM that charges pool and protocol fees #3791

Conversation

Chris-Hibbert commented Sep 3, 2021 • edited by dckc Loading

dckc left a 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

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

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

dckc left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Sep 3, 2021 •

edited by dckc

Loading