New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(AMM)!: make amm.addPool() require minimum collateral #5377

Merged

mergify merged 14 commits into master from 4643-gateAddPool

May 24, 2022

Contributor

Chris-Hibbert commented May 16, 2022

closes: #4643

Description

When creating a new pool for the AMM, there will be a minimum amount
of IST. The caller is expected to provide an equivalent value of the
collateral to balance the AMM pool. The liquidity tokens corresponding
to that minimum will be added to the AssetReserve. If the caller adds
more liquidity than the minimum, they get the extra liquidity tokens.

This required updating a number of tests to initialize their AMMs with
a minimal amount.

Security Considerations

This adds to the economic security of the INTER protocol, but doesn't have much to do with system security.

Documentation Considerations

MinInitialPoolLiquidity needs to be added to the doc for governance parameters, and the contract description.

Testing Considerations

updated existing tests.

Chris-Hibbert added Inter-protocol AMM labels

Chris-Hibbert added this to the Mainnet 1 milestone

Chris-Hibbert requested a review from turadg as a code owner

May 16, 2022 21:20

Chris-Hibbert self-assigned this

Chris-Hibbert marked this pull request as draft

May 16, 2022 21:20

Chris-Hibbert force-pushed the 4643-gateAddPool branch from ff2ba9d to ae00101 Compare

May 16, 2022 21:50

turadg removed their request for review

May 16, 2022 21:51

Chris-Hibbert force-pushed the 4643-gateAddPool branch from 34b0947 to bbf019b Compare

May 18, 2022 00:55

turadg mentioned this pull request

liquidation penalty proceeds to reserve #5387

Merged

This was referenced May 18, 2022

Make amm.addPool require initial assets #4643

Closed

feat: permissionless AMM pools from Interchain assets #5397

Merged

Chris-Hibbert marked this pull request as ready for review

May 19, 2022 17:16

Contributor Author

Chris-Hibbert commented May 19, 2022

@turadg this is now ready for review. #5397 should address the failing test.

turadg reviewed

View reviewed changes

Member

turadg left a comment

Took a pass through src/. Haven't reviewed test/ yet but have to get to a meeting.

packages/run-protocol/src/vpool-xyk-amm/AttackersGuide.md

               ## Multiple Pools
               `addPool` and `addLiquidity` are publicly available. That means someone could maliciously
-              create pools or provide insufficient liquidity. The ability to add liquidity in arbitrary
-              proportions should be enough to prevent misuse of these APIs from hurting us.
+              create pools or provide insufficient liquidity. The requirement of a minimum initial

Member

turadg May 19, 2022

👍 updating related docs

packages/run-protocol/src/vpool-xyk-amm/addPool.js Outdated

Comment on lines 11 to 21

		export const makeAddIssuer = (zcf, isInSecondaries, brandToLiquidityMint) => {
		return async (secondaryIssuer, keyword) => {

Member

turadg May 19, 2022

param types, s'il vous plaît.

from the function signature isInSecondaries looks like a boolean

Contributor Author

Chris-Hibbert May 19, 2022

done

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

+                    X`${keyword} asset not fungible (must use NAT math)`,
+                  );
+                  const liquidityKeyword = `${keyword}Liquidity`;
+                  zcf.assertUniqueKeyword(liquidityKeyword);

Member

turadg May 19, 2022

for my learning: is this giving earlier feedback or checking something that wouldn't be otherwise? (I assume makeZCFMint won't let you override a keyword.)

Contributor Author

Chris-Hibbert May 19, 2022

It's an early check so we don't make the mint if saveIssuer will fail.

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

+                    zcf.makeZCFMint(
+                      liquidityKeyword,
+                      AssetKind.NAT,
+                      harden({ decimalPlaces: 6 }),

Member

turadg May 19, 2022

magic number

Contributor Author

Chris-Hibbert May 19, 2022

Do we have a canonical version of this number? Can I let it default?

packages/run-protocol/src/vpool-xyk-amm/addPool.js Outdated

-                  ]);
+                  const { zcfSeat: poolSeat } = zcf.makeEmptySeatKit();
+                  /** @type {PoolFacets} */
+                  // @ts-expect-error cast

Member

turadg May 19, 2022

something still awry with singlePool.getPriceForInput type differing between PoolFacets and ReturnType<makePool>. Which one is correct? If it's PoolFacets let's move this cast over to makePool's signature. If it's makePool, let's define PoolFacets using it.

Contributor Author

Chris-Hibbert May 20, 2022

Fixed? Now the bad magic is concentrated around helper in pool.js. help?

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

-                  return liquidityZCFMint.getIssuerRecord().issuer;
+                  // in addLiquidityInternal, funder provides centralAmount & secondaryAmount,
+                  // and receives liquidity tokens equal to centralAmount. Afterward, we'll

Member

turadg May 19, 2022

thanks for explaining this. any risk in something failing before that happens? (not atomic)

Contributor Author

Chris-Hibbert May 20, 2022

It's a valid question, but I think I've covered all the cases.

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

                 const quoteIssuerKit = makeIssuerKit('Quote', AssetKind.SET);
                 // For now, this seat collects protocol fees. It needs to be connected to
                 // something that will extract the fees.
                 const { zcfSeat: protocolSeat } = zcf.makeEmptySeatKit();
+                // todo(hibbert): give reserve the abilty to collect this.

Member

turadg May 19, 2022

is this before MN-1? should have a ticket so we track it to release

Contributor Author

Chris-Hibbert May 20, 2022

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

@@ @@ -112,6 +108,23 @@ const helperBehavior = { @@
                   } else {
                     zcf.reallocate(poolSeat, zcfSeat);
                   }
+                },
+                addLiquidityActual: (

Member

turadg May 19, 2022

can keep this, no?

Suggested change

      
              addLiquidityActual: (
          
              /** @type {import('@agoric/vat-data/src/types').PlusContext<MethodContext, AddLiquidityActual>} */
          
                addLiquidityActual: (

Contributor Author

Chris-Hibbert May 20, 2022

done

Member

turadg May 20, 2022

I pushed some other type fixes in a71f732

Contributor Author

Chris-Hibbert May 20, 2022

thanks

packages/run-protocol/src/vpool-xyk-amm/pool.js Outdated

@@ @@ -140,6 +153,7 @@ const poolBehavior = { @@
                   assert(isNatValue(secondaryIn.value), 'User Secondary');
                   if (state.liqTokenSupply === 0n) {
+                    // @ts-expect-error TS confused about parameter count

Member

turadg May 19, 2022

without an annotation it doesn't know that feeSeat is optional

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

    
            @@ -83,8 +91,9 @@
          
               */

              /**

               * @typedef {object} XYKAMMPublicFacet

               * @property {(issuer: ERef<Issuer>, keyword: Keyword) => Promise<Issuer>} addPool

               * @property {() => Promise<Invitation>} addPoolInvitation

Member

turadg May 19, 2022

the Invitation can be typed with the offer args and result. I don't know it offhand but I'd be happy to help.

Member

turadg May 20, 2022

nevermind, I tried a bit and it's more effort than I thought.

turadg approved these changes

View reviewed changes

Member

turadg left a comment

Code LGTM. I pushed a few types fix commits.

I see the tests aren't passing yet but assume it's a matter of tweaking. Please re-request review if more substantial changes are pushed.

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

+                  feeSeat,
+                ) => {
+                  const { updater } = state;
+                  const { helper } = facets;

Member

turadg May 20, 2022

this is a work-around for the cyclic typedef error that happens when a method destructures a facet that it's part of defining. that's a bit of a gotcha. thoughts on how/where to raise awareness?

Contributor Author

Chris-Hibbert May 20, 2022

I don't have an idea other than spreading it through code reviews. I get bitten by this while trying to make the types work, and understanding these common workarounds will help me get past that.

packages/run-protocol/src/vpool-xyk-amm/types.js Outdated

    
               * @property {{addLiquidityActual: AddLiquidityActual}} helper

               * @property {VirtualPool} singlePool

               * @property {{addLiquidityActual: AddLiquidityActual, addLiquidityInternal: AddLiquidityInternal}} helper

               * @property {SinglePool} singlePool

Member

turadg May 20, 2022 •

edited

Loading

👍

I think the VirtualPool types will be all sorted out with 856aef

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

    
            @@ -83,8 +91,9 @@
          
               */

              /**

               * @typedef {object} XYKAMMPublicFacet

               * @property {(issuer: ERef<Issuer>, keyword: Keyword) => Promise<Issuer>} addPool

               * @property {() => Promise<Invitation>} addPoolInvitation

Member

turadg May 20, 2022

nevermind, I tried a bit and it's more effort than I thought.

packages/run-protocol/test/amm/vpool-xyk-amm/test-liquidity.js

@@ @@ -196,7 +236,11 @@ test('amm add and remove liquidity', async t => { @@
                 await assertPayouts(l1, 50_000n, c1, 0n, s1, 0n);
                 t.deepEqual(
                   await E(amm.ammPublicFacet).getPoolAllocation(moolaR.brand),
-                  allocation(50_000n, 0n, 10000n),
+                  allocation(
+                    centralLiquidityValue + 50_000n,

Member

turadg May 20, 2022

+1 to regular arithmetic in tests

packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js Outdated

                   `bob gets the same price as when he called the getInputPrice method`,
                 );
+                // @ts-ignore it doesn't know both are Nat

Member

turadg May 20, 2022

solved by 1a7c0a5

packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js Outdated

Comment on lines 1029 to 1033

+                // await t.throwsAsync(
+                //   () => E(amm.ammPublicFacet).getPoolAllocation(moolaR.brand),
+                //   { message: /"secondaryBrand" not found: / },
+                //   "The pool hasn't been created yet",
+                // );

Member

turadg May 20, 2022

💀

Contributor Author

Chris-Hibbert May 20, 2022

uncommented. This is valid at this point in the test .

packages/run-protocol/test/reserve/test-reserve.js

    
            @@ -15,11 +15,71 @@ import { unsafeMakeBundleCache } from '../bundleTool.js';
          
              // Some notifier updates aren't propogating sufficiently quickly for the tests.

              // This invocation (thanks to Warner) waits for all promises that can fire to

              // have all their callbacks run

              async function waitForPromisesToSettle() {

              const waitForPromisesToSettle = async () => {

Member

turadg May 20, 2022

import from test/supports.js

This was referenced May 20, 2022

Reserve collects liquidity held by AMM from creating new pools #5408

Closed

Reserve contract receives JS metering fees; IST fees distributed to reward pool and reserve #4399

Closed

Chris-Hibbert force-pushed the 4643-gateAddPool branch from efc19a7 to 40d98d7 Compare

May 20, 2022 19:42

Contributor Author

Chris-Hibbert commented May 20, 2022 •

edited

Loading

Blocked by #5397

[corrected]

Chris-Hibbert and others added 10 commits

May 23, 2022 17:19


          feat: make amm.addPool() require minimum collateral

f808cba

When creating a new pool for the AMM, there will be a minimum amount
of IST. The caller is expected to provide an equivalent value of the
collateral to balance the AMM pool. The liquidity tokens corresponding
to that minimum will be added to the AssetReserve. If the caller adds
more liquidity than the minimum, they get the extra liquidity tokens.

Closes: #4643

This required updating a number of tests to initialize their AMMs with
a minimal amount.


          fix: don't create AMM pools in addAssetToVault

9ddd185


          docs: update references to addPool in documentation

61a7163


          fix: update demoIssuers to use addPoolInvitation.

555a1ce


          refactor: turn minInitialPoolLiquidity into an Amount

48fa5c4


          chore: cleanups. variable names and typescript annotations.

26f720b


          fix pool helper types

18ebee2


          parameterize VirtualPool type

c9ebef4


          pool amounts are nat

a4c6a21


          chore: update TODO to include issue #

c50d9c3

Chris-Hibbert added 2 commits

May 23, 2022 17:24


          test: uncomment a test assertion before pool creation

26833c7


          refactor: integrate with #5397

2d536b8

update interchainPool tests

switch minInitialPoolLiquidity back to Nat in startupAmm since it has
to be driven from text values in environment variables.

Chris-Hibbert force-pushed the 4643-gateAddPool branch from 40d98d7 to 2d536b8 Compare

May 24, 2022 04:55


          chore: lint errors (unreferenced names)

8d6a1da

Contributor Author

Chris-Hibbert commented May 24, 2022

I made relatively straightforward changes to integrate with #5397. I plan to merge this morning when everything is green.

Chris-Hibbert added the automerge:squash label


          Merge branch 'master' into 4643-gateAddPool

c8b651c

mhofman added a commit to Agoric/testnet-load-generator that referenced this pull request


          feat(loadgen): handle AMM method renames

a7cd874

Agoric/agoric-sdk#5377

mhofman mentioned this pull request

feat(loadgen): handle AMM method renames Agoric/testnet-load-generator#84

Merged

mhofman added a commit to Agoric/testnet-load-generator that referenced this pull request


          feat(loadgen): handle AMM method renames (#84)

1a9c9c2

Handles the upcoming rename in Agoric/agoric-sdk#5377

Member

mhofman commented May 24, 2022

@Mergifyio refresh

Contributor

mergify bot commented May 24, 2022

refresh

✅ Pull request refreshed

mergify bot merged commit 2fedea6 into master

mergify bot deleted the 4643-gateAddPool branch

May 24, 2022 19:02

Chris-Hibbert added a commit that referenced this pull request


          test: integrate tests with changes from #5377

66a6fc3

mhofman added a commit to Agoric/testnet-load-generator that referenced this pull request


          feat(loadgen): handle AMM method renames (#84)

2c03000

Handles the upcoming rename in Agoric/agoric-sdk#5377

mergify bot added a commit that referenced this pull request


          feat: publish econ stats from AMM (#5420)

87a9e62

* feat: public econ stats from AMM

closes: #4648

add notifier and subscription to state

* test: integrate tests with changes from #5377

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

dckc mentioned this pull request

InstAgoric network for Econ Stress Test (xnet) #5375

Closed

17 tasks

Chris-Hibbert mentioned this pull request

AMM enforce minimum liquidity by wrapping addPool with a new contract #5634

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AMM automerge:squash Inter-protocol