feat: permissionless AMM pools from Interchain assets #5397

dckc · 2022-05-19T04:49:51Z

refs: #5075, #4643

Description

Given sufficient IST, create an issuer for an IBC denom and an invitation to add a pool for it to the AMM.

There are 2 steps:

offer IST (Central) and provide a denom in the offer args; get back the issuer for that denom in the VBANK
offer the new asset (Secondary) and get back liquidity.

Un-ties knots discussed in #4643 (comment) , #5075 (comment)

TODO:

bankManager should have a method like addAsset that does not propagate to every wallet in the system.
- postponed as refine bankManager / VBANK to provide purses on demand #5412
bootstrap wiring

Security Considerations

Does not provide offer safety, due to usual propoerties of offerTo.

Requires bankManager as a private arg.

Documentation Considerations

Yet Another Contract to document / review.
cc @jessysaurusrex

Testing Considerations

Lightly tested. Reviewers will please suggest further tests.

dckc · 2022-05-19T16:17:55Z

packages/run-protocol/src/interchainPool.js

+    );
+    kwNonce += 1;
+
+    const liquidityIssuer = await E(amm).addPool(


@Chris-Hibbert the addPool call would move to step 2 when we require funds (#5377). I can't move it yet because the AMM requires the liquidity brand in the want when adding liquidity, even though it doesn't use it.

Maybe I should fix that in this PR?

@Chris-Hibbert the addPool call would move to step 2 when we require funds (#5377). I can't move it yet because the AMM requires the liquidity brand in the want when adding liquidity, even though it doesn't use it.

Maybe I should fix that in this PR?

Before and after #5377 the APIs for adding a pool are just different. Our choices are to reconcile in whichever PR is second, or merge this into #5377 once approved.

I did fix the odd AMM constraint and move addPool to step 2. That way, when #5377 lands, we'll have the collateral on hand when we want to addPool.

Chris-Hibbert · 2022-05-19T17:50:19Z

packages/run-protocol/src/interchainPool.js

+ * }} pa
+ *
+ * @typedef {{
+ *   minimumCentral: Amount<'nat'>,


If this should eventually be governed, let's at least add a comment to that effect here.

I added a TODO to get this value dynamically from the AMM (once #5377 lands)

Chris-Hibbert · 2022-05-19T18:21:53Z

packages/run-protocol/src/interchainPool.js

+      return deposited.then(_ => {
+        seat.exit();
+        seat2.exit();


I think there should be a .catch() clause here that returns the money from seat2 to seat1 and exits the seat.

I would test it by setting the AMM's minimum higher than the minimum enforced here.

I plan to get the AMM's minimum on demand, so that test wouldn't last.

What is it that could throw? seat.exit() or deposited? In either case, what difference does it make which seat the money is returned in?

what difference does it make which seat the money is returned in?

I thought seat2 was purely internal, but I now see that I was wrong.

It still seems surprising that the caller has to check both seats to see if there's money there.

If someone has already saved an issuer on the AMM and called it 'Interchain5', then the saveIssuer() call will fail, and the allocation will be stuck on seat2.

It still seems surprising that the caller has to check both seats to see if there's money there.

In discussion with @dtribble and co., we didn't see an obvious improvement. (There are 2 offers, so having 2 seats to check on failure never seemed odd, to me.)

If someone has already saved an issuer on the AMM and called it 'Interchain5', then the saveIssuer() call will fail

The saveIssuer() call here doesn't depend on issuers in the AMM, as we discussed. (There could be a collision at addPool, but that's before the saveIssuer() here.)

dckc · 2022-05-20T22:29:02Z

@Chris-Hibbert @turadg I wired up everything to get test-gov-collateral.js and such working. Please take a look.

turadg

Nothing stands out to me as a concern but given the security risks and my understanding of the interchain I don't feel comfortable approving and leave that to @Chris-Hibbert .

turadg · 2022-05-20T22:35:07Z

packages/run-protocol/src/interchainPool.js

+      });
+    };
+
+    const keyword = denom; // ISSUE: should not show up in all wallets.


what does ISSUE mean? something to do or just know about?

what does ISSUE mean?

um... something to do, I suppose.

In this case, it originated from discussion with @dtribble , who was OK postponing it; I didn't turn it into a separate issue, #5412 , until I got a chance to run it by @michaelfig , where we decided it's feasible in due course. Now that I have a number for it, I can add it to that comment.

turadg · 2022-05-20T22:36:54Z

packages/run-protocol/test/test-gov-collateral.js

+          receive: () => {},
+        }),
+    });
+    // @ts-ignore mock doesn't have all the methods


@ts-expect-error

(not worth a push bc I plan to lint this soon)

Chris-Hibbert · 2022-05-20T23:41:23Z

packages/run-protocol/src/interchainPool.js

+      return deposited.then(_ => {
+        seat.exit();
+        seat2.exit();


what difference does it make which seat the money is returned in?

I thought seat2 was purely internal, but I now see that I was wrong.

It still seems surprising that the caller has to check both seats to see if there's money there.

If someone has already saved an issuer on the AMM and called it 'Interchain5', then the saveIssuer() call will fail, and the allocation will be stuck on seat2.

too much coupling here, still

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

* feat: make amm.addPool() require minimum collateral 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` * docs: update references to addPool in documentation * fix: update demoIssuers to use addPoolInvitation. * refactor: turn minInitialPoolLiquidity into an Amount * chore: cleanups. variable names and typescript annotations. * fix pool helper types * parameterize VirtualPool type * pool amounts are nat * chore: update TODO to include issue # * test: uncomment a test assertion before pool creation * refactor: integrate with #5397 update interchainPool tests switch minInitialPoolLiquidity back to Nat in startupAmm since it has to be driven from text values in environment variables. * chore: lint errors (unreferenced names) Co-authored-by: Turadg Aleahmad <turadg@agoric.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

dckc requested review from michaelfig, dtribble and Chris-Hibbert May 19, 2022 04:49

dckc requested a review from turadg as a code owner May 19, 2022 04:49

turadg removed their request for review May 19, 2022 12:56

dckc commented May 19, 2022

View reviewed changes

Chris-Hibbert mentioned this pull request May 19, 2022

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

Merged

Chris-Hibbert approved these changes May 19, 2022

View reviewed changes

dckc marked this pull request as draft May 20, 2022 16:18

dckc force-pushed the dc-ibc-pool branch 6 times, most recently from 8b3791b to 2088c49 Compare May 20, 2022 22:13

dckc requested a review from Chris-Hibbert May 20, 2022 22:22

dckc mentioned this pull request May 20, 2022

refine bankManager / VBANK to provide purses on demand #5412

Closed

dckc removed the request for review from michaelfig May 20, 2022 22:27

dckc marked this pull request as ready for review May 20, 2022 22:29

turadg reviewed May 20, 2022

View reviewed changes

Chris-Hibbert approved these changes May 20, 2022

View reviewed changes

dckc added 4 commits May 23, 2022 13:14

fix(amm): don't require {want:{Liquidity}} when adding liquidity

e768c91

chore(vats): include interchainPool in bootstrap types

c76140a

too much coupling here, still

test(run-protocol): factor out mintRunPayment

34ca287

test(AMM): produce agoricNamesAdmin too

1f0337a

dckc force-pushed the dc-ibc-pool branch from 2088c49 to da78268 Compare May 23, 2022 18:17

dckc added the automerge:no-update (expert!) Automatically merge without updates label May 23, 2022

dckc added 2 commits May 23, 2022 13:52

feat: permissionless interchain AMM pool creation

5e2a8d0

fix: vault proposal relies on permissionless IBC AMM pool creation

4e711b1

dckc force-pushed the dc-ibc-pool branch from da78268 to 4e711b1 Compare May 23, 2022 18:52

mergify bot merged commit 3d0d9de into master May 23, 2022

mergify bot deleted the dc-ibc-pool branch May 23, 2022 19:32

Chris-Hibbert added a commit that referenced this pull request May 24, 2022

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.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: permissionless AMM pools from Interchain assets #5397

feat: permissionless AMM pools from Interchain assets #5397

dckc commented May 19, 2022 •

edited

Loading

dckc May 19, 2022

Chris-Hibbert May 19, 2022

dckc May 20, 2022

Chris-Hibbert May 19, 2022

dckc May 20, 2022

Chris-Hibbert May 19, 2022

dckc May 20, 2022

Chris-Hibbert May 20, 2022

dckc May 23, 2022

dckc commented May 20, 2022

turadg left a comment

turadg May 20, 2022

dckc May 20, 2022

turadg May 20, 2022

Chris-Hibbert May 20, 2022

feat: permissionless AMM pools from Interchain assets #5397

feat: permissionless AMM pools from Interchain assets #5397

Conversation

dckc commented May 19, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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 commented May 20, 2022

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

dckc commented May 19, 2022 •

edited

Loading