feat(AMM)!: remove the AMM and cleanup bootstrap etc. dependencies #7074

Chris-Hibbert · 2023-02-24T22:20:17Z

closes: #7055
refs: #7000
refs: #7047

Description

Remove the AMM and cleanup dependencies in bootstrap and vaults.

The most visible consequence is to vault liquidation. Some pieces are left hanging, and will be repaired by #7047.

Security Considerations

removing code usually makes security simpler.

Scaling Considerations

None

Documentation Considerations

References to the AMM should be removed.

Testing Considerations

Some tests were marked test.skip because liquidation doesn't work until the revised Vault liquidation using the descending clock auction is added.

dckc · 2023-02-24T22:45:51Z

I wonder... if we ever want this back, can we somehow get the git history back with it such that git blame works?

turadg

Amazing. The package feels lighter already 😉

Some questions before approval.

Also there's more non-code to delete,

docs/threat_models/smart_contracts/amm/
node AMM in vaultFactory.puml
"take some action (i.e. add a new collateral type to the AMM)"
AMM in governance-params.md and inter-protocol-preview.md
AMM in zoe-catalog-of-storage.md
test.skip('collect fees from loan and AMM'
test-demoAMM.js

And I guess a subsequent PR will clear liquidateIncrementally.js and liquidateMinimum.js?

turadg · 2023-02-24T22:38:05Z

packages/SwingSet/misc-tools/measure-metering/measure.js

@@ -139,8 +134,9 @@ async function run() {
  const zoeInstallVaultFactory = await doCounted('zoeInstallVaultFactory');
  console.log(`zoe install (vaultFactory): ${zoeInstallVaultFactory}`);

-  const zoeInstallAMM = await doCounted('zoeInstallBundle', [autoswapBundle]);
-  console.log(`zoe install (AMM): ${zoeInstallAMM}`);
+  // XXX  Does this need to be replace by something else in order to be useful?


I don't think so. Who uses this tool… maybe @warner ?

Don't worry about it.

The misc-tools directory has a README-analysis-tools.md disclaimer:

They were built as-needed to investigate various performance issues, and vary drastically in quality and usability.

I think the PSM is a likely replacement, but I think it's fair to leave it in XXX state until somebody comes along and wants to use it again.

turadg · 2023-02-24T22:38:41Z

packages/inter-protocol/src/collectFees.js

@@ -1,4 +1,4 @@
-import { atomicTransfer } from '@agoric/zoe/src/contractSupport';
+import { atomicTransfer } from '@agoric/zoe/src/contractSupport/index.js';


TIL that ESM doesn't support the implicit index.js so we should have this everywhere. 👍

turadg · 2023-02-24T22:39:16Z

packages/inter-protocol/src/feeDistributor.js

@@ -41,7 +41,7 @@ export const customTermsShape = harden({
 */

 /**
- * wrapper to take the vaultFactory or AMM's creatorFacet, and make a call that
+ * wrapper to take the vaultFactory's creatorFacet, and make a call that


Suggested change

* wrapper to take the vaultFactory's creatorFacet, and make a call that

* wrapper to take a creatorFacet (e.g. vaultFactory) and make a call that

turadg · 2023-02-24T22:44:17Z

packages/inter-protocol/src/proposals/demoIssuers.js

@@ -462,33 +462,24 @@ export const poolRates = (issuerName, record, kits, central) => {
 *   consume: { mints }
 * }} powers
 */
+// XXX Is this dead? I can't find references to it. It seems to do more than


in master it's only used in SIM_CHAIN_MANIFEST. @michaelfig is it still needed for that? Does green CI imply "no"?

Either way this isn't XXX tech debt that should go into master. I'd make this comment DONOTMERGE and make sure it's resolved before approval.

turadg · 2023-02-24T22:45:57Z

packages/inter-protocol/src/reserve/assetReserve.js

 * {
- *   governedApis: ['addLiquidityToAmmPool'],
+ *   governedApis: ['burnFeesToReduceShortfall'],


👍 I missed including that.

turadg · 2023-02-24T22:47:47Z

packages/inter-protocol/src/vaultFactory/liquidateIncrementally.js

@@ -48,7 +48,7 @@ const trace = makeTracer('LiqI', false);

 /**
 * @typedef {{
- *   amm: XYKAMMPublicFacet,
+ *   amm: any,


consider a TODO comment linking the the issue or PR that will remove AMM entirely

Is it important to use any here? Can we use unknown instead?

it's going away. I felt free to be sloppy. I've updated.

packages/inter-protocol/src/vaultFactory/vaultManager.js

turadg · 2023-02-24T22:49:06Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -207,11 +200,6 @@ export const prepareVaultManagerKit = (
  );

  const poolIncrementSeat = provideEmptySeat(zcf, baggage, 'pool increment');
-  const retainedCollateralSeat = provideEmptySeat(


will the new liquidation system need something like this?

Yes, it re-appears in #7074. This was a quick way to make lint happy.

consider a lint suppression since it's temporary. that would retain the blame history

turadg · 2023-02-24T22:49:54Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -779,20 +609,6 @@ export const prepareVaultManagerKit = (
          return factoryPowers.getGovernedParams();
        },

-        /**
-         * In extreme situations, system health may require liquidating all vaults.


is this no longer a requirement?

The comment notwithstanding, this wasn't hooked up to an API, and I don't know how it ought to be connected to governance. If there's a future requirement for it, we can add something similar in the context of the new liquidation approach.

Chris-Hibbert · 2023-02-24T23:33:44Z

And I guess a subsequent PR will clear liquidateIncrementally.js and liquidateMinimum.js?

That's in #7047, which should arguably be stacked on top of this, and maybe be combined before merging this.

Chris-Hibbert · 2023-02-25T00:08:26Z

docs/threat_models/smart_contracts/amm/
node AMM in vaultFactory.puml
"take some action (i.e. add a new collateral type to the AMM)"
- I didn't understand this. I searched for it in the code. Was it a comment I made? Where does this go?
AMM in governance-params.md and inter-protocol-preview.md
AMM in zoe-catalog-of-storage.md
test.skip('collect fees from loan and AMM'
- Has an annotation: // TODO (7047) reinstate when vaults can use auction
test-demoAMM.js
- just tell me again, please. Should this really go away? Does it need to be replaced? (even if it's just a TODO or an issue.)

dckc

re fundAMM I'm not sure it can completely go. IOU more info when I find it.

dckc · 2023-02-25T03:17:26Z

packages/inter-protocol/src/interchainPool.js

@@ -1,26 +1,18 @@
 import { E, Far } from '@endo/far';


I suppose we should delete this whole contract.

Its only purpose is to support permissionless creation of AMM pools.

dckc · 2023-02-25T03:30:19Z

packages/inter-protocol/src/proposals/demoIssuers.js

+// TODO(cth) DONOTMERGE Is this dead? I can't find references to it. It seems to
+//  do more than fund the AMM, and I don't know if it's needed somewhere.


The reference was in SIM_CHAIN_MANIFEST above
https://github.com/Agoric/agoric-sdk/pull/7074/files#diff-892e7aad2dc384ce64eb722139fb97bf82a639fa2137ee6867141d8696348476L178

As you note, it does seem to do more than fund the AMM. It's at most a feature of the local development REPL environment. packages/solo/test/test-home.js should exercise it. I hope to chat with @michaelfig to jog my memory about exactly what it's supposed to do.

It creates (demo) price authorities. We have other mechanisms for that now:

real oracle providers

a default price

dckc · 2023-02-25T03:32:21Z

packages/inter-protocol/src/proposals/econ-behaviors.js

-  // AMM requires Reserve in order to add pools, but we can't provide it at startInstance
-  // time because Reserve requires AMM itself in order to be started.
-  // Now that we have the reserve, provide it to the AMM.
-  trace('Resolving the reserve public facet on the AMM');
-  await E(ammKit.creatorFacet).resolveReserveFacet(publicFacet);


good riddance!

dckc · 2023-02-25T03:34:11Z

packages/inter-protocol/src/reserve/assetReserve.js

@@ -56,18 +48,11 @@ const nonalphanumeric = /[^A-Za-z0-9]/g;

 /**
 * Asset Reserve holds onto assets for the Inter Protocol, and can
- * dispense it for various purposes under governance control. It currently
- * supports governance decisions to add liquidity to an AMM pool.
+ * dispense it for various purposes under governance control.


It can't really dispense until it's upgraded, right? Is that implicit in "under governance control"?

Yeah. that's what I was thinking.

dckc · 2023-02-25T03:39:01Z

packages/inter-protocol/src/vaultFactory/vaultDirector.js

-   * @param {Installation<unknown>} oldInstall
-   * @param {unknown} oldTerms
-   */
-  const watchGovernance = (vaultManager, oldInstall, oldTerms) => {


I guess you know where to look if you want this back when we add liquidation via auction?

The vaultManagers were responsible for starting the liquidation contract, but the Auction runs independently of the Vaults. There won't be a need to bring this back.

dckc · 2023-02-25T03:41:30Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -485,150 +462,6 @@ export const prepareVaultManagerKit = (
          updateQuote(outstandingQuote, highestDebtRatio, liquidationMargin);
          trace('update quote', collateralBrand, highestDebtRatio);
        },
-
-        async processLiquidations() {


It seems odd to take this out.

sure, we can put it back, but the git history will get obscured. hm.

liquidations will be invoked in a very different way. Connecting the new code to the previous didn't seem helpful, and I couldn't identify a subset of this code that I could retain and keep lint happy

dckc · 2023-02-25T03:42:53Z

packages/inter-protocol/test/reserve/setup.js

-    farZoeKit,
-  );
-  const { produce } =
+const setupReserveBootstrap = async (t, timer, farZoeKit) => {


This is the sort of test bootstrap setup that we could perhaps do away with using the swingset bootstrap testing approach @turadg presented today.

dckc · 2023-02-25T03:47:33Z

packages/inter-protocol/test/vaultFactory/test-storage.js

@@ -60,7 +60,6 @@ test('storage keys', async t => {
      'numActiveVaults',
      'numLiquidatingVaults',
      'numLiquidationsCompleted',
-      'retainedCollateral',


I don't see why this goes away. (most likely I skimmed something too fast)

It's an output of liquidation, which is temporarily missing.

dckc · 2023-02-25T03:48:21Z

packages/inter-protocol/test/vaultFactory/test-vaultFactory.js

-  runLiquidity,
-) => {
+/** @param {import('ava').ExecutionContext<Context>} t */
+const setupElectorateAndReserve = async t => {


more setup that could be replaced by swingset bootstrap testing

dckc · 2023-02-25T03:49:09Z

packages/inter-protocol/test/vaultFactory/test-vaultFactory.js

@@ -1727,7 +1615,9 @@ test('overdeposit', async t => {
 // Both loans will initially be over collateralized 100%. Alice will withdraw
 // enough of the overage that she'll get caught when prices drop. Bob will be
 // charged interest (twice), which will trigger liquidation.
-test('mutable liquidity triggers and interest', async t => {
+
+// TODO (7047) reinstate when vaults can use auction


ah. good. so we're keeping track of the tests.

yeah, though they migrate to a different file in #7047

dckc · 2023-02-27T20:48:06Z

testing consideration: deployment-test, loadgen

We're seeing deployment-test failures. This is unsurprising, since the deployment-test / loadgen depends on an AMM.

While looking into how to remove the dependency on an AMM, I'm reminded that the loadgen also uses ag-solo and mailbox access. I doubt this PR provides a configuration with mailbox access and no AMM.

dckc · 2023-02-27T20:50:23Z

I'm trying to confirm that this PR is consistent with smart wallet usage; I hit a vault vault1 not found bump...

packages/agoric-cli$ # list my vaults
bin/agops vaults list --from gov1 --keyring-backend="test"
published.vaultFactory.manager0.vaults.vault0

packages/agoric-cli$ OFFER=$(mktemp -t agops.XXX)
bin/agops vaults adjust --vaultId vault1 --giveCollateral 1.0 --from gov1 --keyring-backend="test" >|"$OFFER"
jq ".body | fromjson" <"$OFFER"
running with options {
  offerId: 1677530613923,
  vaultId: 'vault1',
  giveCollateral: 1,
  from: 'agoric1ldmtatp24qlllgxmrsjzcpe20fvlkp448zcuce'
}
(Error#1)
Error#1: vault vault1 not found

  at lookupOfferIdForVault (packages/agoric-cli/src/lib/vaults.js:168:13)
  at async Command.<anonymous> (packages/agoric-cli/src/commands/vaults.js:98:31)
  at async packages/agoric-cli/src/bin-agops.js:32:1

dckc · 2023-02-27T21:19:31Z

testing consideration: smoke test for open / adjust / close vault works

The bootstrap here does work in the context of a running chain. The packages/agoric-cli/test/agops-vaults-smoketest.sh test continues to work (with packages/inter-protocol/scripts/start-local-chain.sh).

2023-02-27T20:40:13.479Z SwingSet: vat: v26: wallet agoric1ldmtatp24qlllgxmrsjzcpe20fvlkp448zcuce offerStatus { id:  1677530392938, invitationSpec: { callPipe: [ [ 'getCollateralManager', [ [Object] ] ], [ 'makeVaultInvitation' ] ],  instancePath: [ 'VaultFactory' ], source: 'agoricContract' }, proposal: { give: { Collateral: { brand: Object [Alleged: IbcATOM brand] {}, value: 9_000_000n } }, want: { Minted: { brand: Object [Alleged: IST brand] {}, value: 5_000_000n } } }, result: 'UNPUBLISHED', numWantsSatisfied: 1, payouts: { Collateral: { brand: Object [Alleged: IbcATOM brand] {}, value: 0n }, Minted: { brand: Object [Alleged: IST brand] {}, value: 5_000_000n } } }

2023-02-27T20:57:00.877Z SwingSet: vat: v26: wallet agoric1ldmtatp24qlllgxmrsjzcpe20fvlkp448zcuce offerStatus { id: 1677531388345, invitationSpec: { invitationMakerName: 'AdjustBalances', previousOffer: '1677530392938', source: 'continuing' }, proposal: { give: { Collateral: { brand: Object [Alleged: IbcATOM brand] {}, value: 1_000_000n } }, want: {} }, result: 'We have adjusted your balances, thank you for your business', numWantsSatisfied: 1, payouts: { Collateral: { brand: Object [Alleged: IbcATOM brand] {}, value: 0n }, Minted: { brand: Object [Alleged: IST brand] {}, value: 0n } } }

2023-02-27T20:58:11.017Z SwingSet: vat: v26: wallet agoric1ldmtatp24qlllgxmrsjzcpe20fvlkp448zcuce offerStatus { id: 1677531479873, invitationSpec: { invitationMakerName: 'CloseVault', previousOffer: '1677530392938', source: 'continuing' }, proposal: { give: { Minted: { brand: Object [Alleged: IST brand] {}, value: 5_050_000n } }, want: {} }, result: 'your loan is closed, thank you for your business', numWantsSatisfied: 1, payouts: { Collateral: { brand: Object [Alleged: IbcATOM brand] {}, value: 11_000_000n }, Minted: { brand: Object [Alleged: IST brand] {}, value: 25_000n } } }

(in the run above, --vaultId vault1 didn't match the output of list vaults)

dckc

fundAMM and its test can go.

I suggest removing interchainPool.js

The most visible consequence is to vault liquidation. Some pieces are left hanging, and will be repaired by #7047.

Remove a few more things, mostly doc comments.

remove interchainPool and other bootstrap stuff connected to AMM Simplify liquidation contracts so they don't refer to dead code. other clean ups

turadg

✂️ 💥

turadg · 2023-02-28T22:47:11Z

packages/inter-protocol/src/vaultFactory/liquidateIncrementally.js

@@ -46,9 +45,11 @@ const trace = makeTracer('LiqI', false);
 * TODO integrate the reserve, including the above Reserve strategies.
 */

+// TODO: (#7047) this file goes away shortly, and is here to keep vaults happy


turadg · 2023-02-28T22:48:07Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -207,11 +200,6 @@ export const prepareVaultManagerKit = (
  );

  const poolIncrementSeat = provideEmptySeat(zcf, baggage, 'pool increment');
-  const retainedCollateralSeat = provideEmptySeat(


consider a lint suppression since it's temporary. that would retain the blame history

It'll be removed in #7074, I think, but for now it like a change in this PR.

Chris-Hibbert · 2023-02-28T23:47:06Z

consider a lint suppression since it's temporary. that would retain the blame history

I reverted it. It didn't need lint suppression. There's a seat. Its allocation is reported. It's empty during tests currently.

It'll be removed in #7074, I think, but for now it like a change in this PR.

* feat(auction): add an auctioneer to manage vault liquiditation Separate pieces include a scheduler that manages the phases of an auction, the AuctionBook that holds onto offers to buy, and the auctioneer, which accepts good for sale and has the APIs. Params are managed via governance. The classes are not durable. This is DRAFT, for early review. The changes to VaultFactory to make use of this approach to liquidation rather than the AMM are in a separate PR, which will be available before this PR is finalized. closes: #6992 * refactor: cleanups from working on vault liquidation * chore: cleanups suggested in review * refactor: simplify keys in sortOffers * chore: cosmetic cleanups from review * chore: auction was introduced to vaultfactory too early. It will be introduced with #7047 when vaultfactory makes use of the auction for liquidation * chore: correctly revert removal of param declaration It'll be removed in #7074, I think, but for now it like a change in this PR. * test: repair dependencies for startAuction * chore: responses to reviews * test: refactor proportional distribution test to use macros * refactor: correct sorting of orders by time Add a test for this case. make all auctionContract tests serial * chore: cleanups suggested in review for loops rather than foreach better type extraneous line * chore: clean up scheduler; add test for computeRoundTiming * chore: minor cleanups from review * chore: rename auctionKit to auctioneerKit * chore: drop auction from startPSM * chore: clean-ups from review * refactor: computeRoundTiming should not allow duration === frequency * chore: clean up governance, add invitation patterns in auctioneer * refactor: don't reschedule next if price is already locked

Chris-Hibbert added hygiene Tidying up around the house Inter-protocol Overarching Inter Protocol Vaults VaultFactor (née Treasury) MUST-HAVE labels Feb 24, 2023

Chris-Hibbert self-assigned this Feb 24, 2023

Chris-Hibbert requested review from turadg and dckc February 24, 2023 22:20

turadg reviewed Feb 24, 2023

View reviewed changes

Chris-Hibbert force-pushed the 7055-exciseAMM branch from afd8d53 to 0b697b7 Compare February 24, 2023 23:44

Chris-Hibbert added the force:integration Force integration tests to run on PR label Feb 25, 2023

dckc requested changes Feb 25, 2023

View reviewed changes

dckc mentioned this pull request Feb 25, 2023

replace ROLE with 3 bootstrap modules; audit contents #7049

Merged

dckc approved these changes Feb 27, 2023

View reviewed changes

Chris-Hibbert mentioned this pull request Feb 28, 2023

feat(auction): add an auctioneer to manage vault liquidation #7000

Merged

Chris-Hibbert added 5 commits February 28, 2023 14:43

feat(AMM)!: remove the AMM and cleanup bootstrap etc. dependencies

eaeb4c2

The most visible consequence is to vault liquidation. Some pieces are left hanging, and will be repaired by #7047.

chore: first follow-ups to initial comments

b7f8bca

Remove a few more things, mostly doc comments.

chore: clean-ups and todos to mark unfinished transition steps

c74d605

chore: use unknown rather than any in interim liquidation contracts

4d33194

chore: remove more stuff related to AMM

2a5e393

remove interchainPool and other bootstrap stuff connected to AMM Simplify liquidation contracts so they don't refer to dead code. other clean ups

Chris-Hibbert force-pushed the 7055-exciseAMM branch from 8523532 to 2a5e393 Compare February 28, 2023 22:45

turadg approved these changes Feb 28, 2023

View reviewed changes

Chris-Hibbert added a commit that referenced this pull request Feb 28, 2023

chore: correctly revert removal of param declaration

5d15c2e

It'll be removed in #7074, I think, but for now it like a change in this PR.

turadg approved these changes Mar 1, 2023

View reviewed changes

chore: lint cleanups and revert retainedCollateral

0f1b8bf

Chris-Hibbert force-pushed the 7055-exciseAMM branch from 99e0e75 to 0f1b8bf Compare March 1, 2023 00:21

test: update integration test to use faucet instead of amm & vault

d994a9d

Chris-Hibbert added the automerge:squash Automatically squash merge label Mar 1, 2023

mhofman mentioned this pull request Mar 1, 2023

Handle missing price authority from AMM Agoric/testnet-load-generator#92

Open

mergify bot merged commit ed5ee58 into master Mar 1, 2023

mergify bot deleted the 7055-exciseAMM branch March 1, 2023 01:01

dckc mentioned this pull request Mar 1, 2023

migrate AMM contract from agoric-sdk Agoric/dapp-amm#37

Draft

Chris-Hibbert added a commit that referenced this pull request Mar 2, 2023

chore: correctly revert removal of param declaration

4174ff1

It'll be removed in #7074, I think, but for now it like a change in this PR.

Chris-Hibbert added a commit that referenced this pull request Mar 5, 2023

chore: correctly revert removal of param declaration

4d07a53

It'll be removed in #7074, I think, but for now it like a change in this PR.

turadg mentioned this pull request May 4, 2023

remove stakeFactory #7608

Merged

turadg mentioned this pull request Oct 6, 2023

Add contracts Agoric/documentation#715

Closed

		@@ -1,4 +1,4 @@
		import { atomicTransfer } from '@agoric/zoe/src/contractSupport';
		import { atomicTransfer } from '@agoric/zoe/src/contractSupport/index.js';

	* wrapper to take the vaultFactory's creatorFacet, and make a call that
	* wrapper to take a creatorFacet (e.g. vaultFactory) and make a call that

		// TODO(cth) DONOTMERGE Is this dead? I can't find references to it. It seems to
		// do more than fund the AMM, and I don't know if it's needed somewhere.

feat(AMM)!: remove the AMM and cleanup bootstrap etc. dependencies #7074

feat(AMM)!: remove the AMM and cleanup bootstrap etc. dependencies #7074

Conversation

Chris-Hibbert commented Feb 24, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

dckc commented Feb 24, 2023 • edited Loading

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

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

Chris-Hibbert commented Feb 24, 2023

Chris-Hibbert commented Feb 25, 2023 • edited Loading

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

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

dckc commented Feb 27, 2023 • edited Loading

testing consideration: deployment-test, loadgen

dckc commented Feb 27, 2023

dckc commented Feb 27, 2023

testing consideration: smoke test for open / adjust / close vault works

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

Chris-Hibbert commented Feb 28, 2023

dckc commented Feb 24, 2023 •

edited

Loading

Chris-Hibbert commented Feb 25, 2023 •

edited

Loading

dckc commented Feb 27, 2023 •

edited

Loading