Extract AMM from Vaults (ex-Treasury) #4026

Chris-Hibbert · 2021-11-03T19:15:07Z

closes: #3861
closes: #3019
closes: #4032
closes: #2877

Description

Vaults switch to using constantProduct AMM
Rewards are no longer collected from the AMM in the Vaults
Vaults consistently specify rounding mode with Ratios
Vaults is unaware of AMM governed params
Vaults uses feeMintAccess for access to RUN mint
governance tokens in Vaults are no longer supported
fix up install-on-chain

Security Considerations

The normal for a core part of our economy

Documentation Considerations

Documentation for the constantProduct AMM is in 566
An update to Treasury docs has been prepared.

Testing Considerations

tests are included and updated.

ref: Agoric/agoric-sdk#4026

dckc

I've looked at all the non-test changes; I think it's worth sharing my comments so far.

packages/treasury/bundles/install-on-chain.js

dckc · 2021-11-12T22:55:21Z

packages/treasury/bundles/install-on-chain.js

@@ -63,7 +129,7 @@ export async function installOnChain({
  /** @type {Array<[string, SourceBundle]>} */
  const nameBundles = [
    ['liquidate', liquidateBundle],
-    ['autoswap', autoswapBundle],
+    ['amm', ammBundle],


Using string literals suggests we're free to choose any string we like, when actually, there are external constraints. IMO, it makes at least as much sense to use a manifest constant here as for DEFAULT_POOL_FEE. But we didn't do that before this PR, so I'm OK leaving it out of scope.

agoricNames is somewhat under-documented, like much of the wallet / REPL API. I should ensure that we have an issue about that...

I'd be very happy to conform to some standard practice about consistent names for the wallet and other user-facing code.

I opened #4084 in case any of us gets inspired.

dckc · 2021-11-12T22:56:48Z

packages/treasury/bundles/install-on-chain.js

+  }
+
+  return {
+    governor: g,


why return governor and then ignore it below?

I return it here because someone will need it to change AMM parameters. This is the code that knows that it has a valuable facet. I ignore it below because I don't have a use for it in install-on-chain. I can imagine that there's an appropriate place to put it, but it's surely not in agoric-names. Is there a plausible place to put it currently? I expect we'll eventually want someone to have that facet, but I don't know who it'll be.

Hm... setupAmm isn't exported. So it's not possible for anyone to use governor without changing this file. My style would be to leave this out until then, but I suppose it's not critical.

Looking around to see if there's an issue to straighten this out... #3924 looks close enough, though it sure would be nice to see a plan to actually make sense of the bootstrap process.

dckc · 2021-11-12T23:02:47Z

packages/treasury/bundles/install-on-chain.js

+  )
+
+  // todo(hibbert): set up an initial AMM pool with RUN and BLD
+  // const liqSeat = createLiquidityPool(...);


I made a note to track this TODO in #2877

I guess that's now happening in bootstrap.js, so maybe this todo is done now. It took me a while to figure out that bootstrap was that place.

dckc · 2021-11-12T23:08:16Z

packages/treasury/src/liquidateMinimum.js

        trace(`onSwapoutFail`);
-        assert(
-          error.message.match(AutoswapInsufficientMsg),


good riddance to that one. errors are best treated as sealed to normal callers.

Hmm. I don't have that rule of thumb in my repertoire. Why is it a mistake to say "if that call failed for this reason, here's a fallback plan"?

That might not be a mistake, but there's a slippery slope to peeking at "this reason" and getting stuff that you shouldn't; it was enough of a risk that Joe-E ruled out catching exceptions altogether.

@erights I'm not sure we have a good answer to this question documented. The closest I can find is:

A pervasive concern is hiding diagnostic information—both for confidentiality and for deterministic replay. Code that obtains access to an error object, for example by catching it, should not have access this hidden diagnostic information. -- https://github.com/endojs/endo/tree/master/packages/ses/src/error#hiding-and-revealing-local-diagnostic-information

The preferred idiom in E was ejectors: if you think something might fail, you pass in an ejector that the callee can use to signal that yup, it failed for one of the expected reasons. The callee can pass args to the ejector in the clear. Adding those to agoric-sdk / endo is on the todo list endojs/endo#414 ->
https://github.com/Agoric/SES/blob/0.5.0/src/old/ejectorsGuardsTrademarks.js

Ooh, I like the idea of ejectors. Declaring code to be executed in case of particular expected exceptions seems a cleaner way to deal with this.

packages/treasury/src/stablecoinMachine.js

dckc · 2021-11-12T23:36:10Z

packages/treasury/src/stablecoinMachine.js

-      // Create new governance tokens, trade them with the incoming offer for
-      // collateral. The offer uses the keywords Collateral and Governance.
-      // govSeat stores the collateral as Secondary. We then mint new RUN for
-      // govSeat and store them as Central. govSeat then creates a liquidity
-      // pool for autoswap, trading in Central and Secondary for governance
-      // tokens as Liquidity. These governance tokens are held by govSeat


This looks like a contortion that we are well rid of too.

packages/treasury/src/stablecoinMachine.js

dckc · 2021-11-12T23:47:54Z

packages/treasury/src/vaultManager.js

-      divideBy(getAmountOut(quote), liquidationMargin),
+      ceilDivideBy(getAmountOut(quote), liquidationMargin),


I tried a little mutation testing (changed this to floorDivideBy): our tests are not sensitive to this change.

That seems relatively important, if not critical.

I added a test in test-stablecoin that fails when this is floorDivideBy and succeeds with ceilDivideBy. The test asserts that Alice retains enough collateral that her vault doesn't get liquidated.

dckc

Good stuff! Especially deleting a bunch of iffy code.

I'm comfortable with the changes that move stuff around; less so with the rounding changes. Our tests are not sensitive to changing one of the ceilMultiply to floorMultiply. Hm.

dckc · 2021-11-13T00:16:07Z

packages/treasury/test/test-interest.js

-    interest: AmountMath.make(brand, 6n),
-    newDebt: AmountMath.make(brand, 100006n),
+    interest: AmountMath.make(brand, 7n),
+    newDebt: AmountMath.make(brand, 100007n),


mixing this rounding change in with this PR is hurting my brain a little. It feels like a very different angle to review from...

Yeah, that turned out to be a mistake. I'll try not to mix that kind of change in for the future.

dckc · 2021-11-13T00:22:15Z

packages/vats/src/bootstrap.js

+    // Ask the treasury for enough RUN to fund both AMM and bank.
+    const bootstrapPaymentValue = bankBootstrapSupply + ammDepositValue;


Thanks, @michaelfig , for the assist.

dckc · 2021-11-13T00:24:37Z

packages/vats/src/bootstrap.js

    // NOTE: no use of the voteCreator. We'll need it to initiate votes on
    // changing Treasury parameters.
-    const { treasuryCreator, _voteCreator } = await installEconomy(
+    const { treasuryCreator, _voteCreator, ammFacets } = await installEconomy(


This voteCreator NOTE looks like a TODO; is it covered by an issue? #3924 looks close enough.

It was intended more as a highlight. Until we connect up governance on chain, it's important to be able to tell that the facet is unused. But you're right that eventually we want to make use of it.

dckc · 2021-11-13T00:25:53Z

packages/vats/src/bootstrap.js

      // We still create demo currencies, but not obviously fake ones unless
      // $FAKE_CURRENCIES is given.
-      ...demoIssuerEntries(noFakeCurrencies),
+      ...demoIssuers,


update / delete the comment with this change?

I moved the call to create demoIssuers earlier, since it wants to be shared, but this is the place that cares that rawIssuerEntries excludes the fakes.

The tension that kept me from being able to initialize rawIssuerEntries earlier is that it wants the bootstrapPayment, which depends on other info from issuers.js. What a tangle!

Yes, what a tangle. I suppose #4021 represents at least some of the work we should do in this area.

dckc · 2021-11-13T00:28:21Z

packages/vats/src/bootstrap.js

+                  // TODO: what happens if unusedBankPayment doesn't have enough
+                  // remaining?


It looks like we fail safe in this case (i.e. the whole thing comes crashing down) so I'm not checking for an issue.

My preference would have been to not add the amount the bank wants to the bootstrap payment if there won't be a bank. I think this code would have used a mint instead of splitting payments, and the coupling would have been less and the risk I mention would go away, but that change would touch even more of this file.

dckc · 2021-11-13T00:32:17Z

packages/vats/test/test-distributeFees.js

@@ -58,12 +63,13 @@ test('fee distribution', async t => {
  const brand = brands.get('moola');
  const { feeDepositFacet, getPayments } = makeFakeFeeDepositFacet(issuer);
  const treasury = makeFakeTreasury();
+  const amm = makeFakeTreasury();


awkward naming here; I can read it as makeFakeFeeProducer or some such, right? (not critical)

Yes, that would be a better name. changed.

dckc

looks good.

dckc · 2021-11-16T15:39:15Z

packages/treasury/bundles/install-on-chain.js

+  }
+
+  return {
+    governor: g,


Hm... setupAmm isn't exported. So it's not possible for anyone to use governor without changing this file. My style would be to leave this out until then, but I suppose it's not critical.

Looking around to see if there's an issue to straighten this out... #3924 looks close enough, though it sure would be nice to see a plan to actually make sense of the bootstrap process.

dckc · 2021-11-16T15:52:11Z

packages/treasury/bundles/install-on-chain.js

@@ -63,7 +129,7 @@ export async function installOnChain({
  /** @type {Array<[string, SourceBundle]>} */
  const nameBundles = [
    ['liquidate', liquidateBundle],
-    ['autoswap', autoswapBundle],
+    ['amm', ammBundle],


I opened #4084 in case any of us gets inspired.

dckc · 2021-11-16T16:11:31Z

packages/treasury/src/liquidateMinimum.js

        trace(`onSwapoutFail`);
-        assert(
-          error.message.match(AutoswapInsufficientMsg),


That might not be a mistake, but there's a slippery slope to peeking at "this reason" and getting stuff that you shouldn't; it was enough of a risk that Joe-E ruled out catching exceptions altogether.

@erights I'm not sure we have a good answer to this question documented. The closest I can find is:

A pervasive concern is hiding diagnostic information—both for confidentiality and for deterministic replay. Code that obtains access to an error object, for example by catching it, should not have access this hidden diagnostic information. -- https://github.com/endojs/endo/tree/master/packages/ses/src/error#hiding-and-revealing-local-diagnostic-information

The preferred idiom in E was ejectors: if you think something might fail, you pass in an ejector that the callee can use to signal that yup, it failed for one of the expected reasons. The callee can pass args to the ejector in the clear. Adding those to agoric-sdk / endo is on the todo list endojs/endo#414 ->
https://github.com/Agoric/SES/blob/0.5.0/src/old/ejectorsGuardsTrademarks.js

dckc · 2021-11-16T16:44:36Z

packages/vats/src/bootstrap.js

      // We still create demo currencies, but not obviously fake ones unless
      // $FAKE_CURRENCIES is given.
-      ...demoIssuerEntries(noFakeCurrencies),
+      ...demoIssuers,


Yes, what a tangle. I suppose #4021 represents at least some of the work we should do in this area.

See also: Agoric/agoric-sdk#4026 So far, I've updated the README to discard fungible faucet and add directions for dealing with Zoe fees.

dckc

I don't understand why Promise.all won't work.

dckc · 2021-11-17T21:14:36Z

packages/treasury/src/liquidateMinimum.js

      // await deposited, but we don't need the value. We'll need it to have
      // resolved in both branches, so can't put it in Promise.all.
      await deposited;
      await E(liqSeat).getOfferResult();


I don't understand why Promise.all doesn't work; is there an ordering constraint between the two?

Suggested change

// await deposited, but we don't need the value. We'll need it to have

// resolved in both branches, so can't put it in Promise.all.

await deposited;

await E(liqSeat).getOfferResult();

await Promise.all([deposited, E(liqSeat).getOfferResult()]);

There is an ordering constraint. deposited is a promise that resolves when offerTo's transfer is complete, and thus it's safe to call getOfferResult.

https://agoric.com/documentation/zoe/api/zoe-helpers.html#offerto-zcf-invitation-keywordmapping-proposal-fromseat-toseat

OK; I didn't get that from "We'll need it to have resolved in both branches"; maybe rephrase the code comment to state the ordering constraint as you did here?

The name of the AMM changed from autoswap to amm in Agoric/agoric-sdk#4026. Pick whichever is available for backwards compatibility

Vaults switch to using constantProduct AMM Rewards are no longer collected from the AMM in the Vaults Vaults consistently specify rounding mode with Ratios Vaults is unaware of AMM governed params Vaults uses feeMintAccess for access to RUN mint governance tokens in Vaults are no longer supported collect fees from AMM rename autoswap to amm pervasively install-on-chain needed some help initializing AMM pools, since the RUN funds had toj come from somewhere different

add a test remove some outdated comments deposit more plausible values in AMM pools

Mostly adding missing harden() calls. Also added collateralIssuer to startInstance() (!)

ref: Agoric/agoric-sdk#4026

Chris-Hibbert added hygiene Tidying up around the house Medium Core Economy OBSOLETE in favor of INTER-protocol Inter-protocol Overarching Inter Protocol labels Nov 3, 2021

Chris-Hibbert added this to the Mainnet: Phase 1 - Treasury Launch milestone Nov 3, 2021

Chris-Hibbert self-assigned this Nov 3, 2021

Chris-Hibbert force-pushed the treasWExternalAMM branch 2 times, most recently from 99234ed to 516c4a2 Compare November 3, 2021 20:30

Chris-Hibbert added a commit to Agoric/documentation that referenced this pull request Nov 3, 2021

Update Treasury doc for extracted AMM

077c450

ref: Agoric/agoric-sdk#4026

This was referenced Nov 3, 2021

Update Treasury doc for extracted AMM Agoric/documentation#570

Merged

Collect fees from AMM #4032

Closed

Base automatically changed from constProdParamGovernance to master November 5, 2021 18:48

This was referenced Nov 6, 2021

chore: add swingset tests of multiple run mints #4034

Merged

chore: make an escrow purse for RUN (aka: feeIssuer) at Zoe startup #4050

Merged

Chris-Hibbert force-pushed the treasWExternalAMM branch 2 times, most recently from e806367 to 88c2d18 Compare November 10, 2021 23:00

Chris-Hibbert changed the base branch from master to 4026-runEscrowPurse November 10, 2021 23:01

Base automatically changed from 4026-runEscrowPurse to master November 10, 2021 23:25

Chris-Hibbert force-pushed the treasWExternalAMM branch 5 times, most recently from fbd685c to dca1757 Compare November 12, 2021 17:44

Chris-Hibbert requested a review from dckc November 12, 2021 18:33

dckc mentioned this pull request Nov 12, 2021

Treasury: clean up new collateral addition #2877

Closed

dckc reviewed Nov 12, 2021

View reviewed changes

dckc approved these changes Nov 13, 2021

View reviewed changes

Chris-Hibbert force-pushed the treasWExternalAMM branch from 83792fc to 242d94a Compare November 16, 2021 00:47

Chris-Hibbert added the automerge:squash Automatically squash merge label Nov 16, 2021

Chris-Hibbert force-pushed the treasWExternalAMM branch from 242d94a to 090aee4 Compare November 16, 2021 01:21

Chris-Hibbert removed the automerge:squash Automatically squash merge label Nov 16, 2021

Chris-Hibbert force-pushed the treasWExternalAMM branch from 090aee4 to 103c16d Compare November 16, 2021 01:22

This was referenced Nov 16, 2021

agoricNames: managing naming consistency between dApps and chain #4084

Open

Start a contract instance in cosmic-swingset bootstrap #1932

Closed

dckc approved these changes Nov 16, 2021

View reviewed changes

Chris-Hibbert mentioned this pull request Nov 16, 2021

Update for extracted AMM Agoric/dapp-treasury#46

Merged

Chris-Hibbert mentioned this pull request Nov 17, 2021

Update for compatibility with treasury changes Agoric/dapp-card-store#44

Merged

dckc reviewed Nov 17, 2021

View reviewed changes

Chris-Hibbert force-pushed the treasWExternalAMM branch from 4a9c831 to ebc7865 Compare November 17, 2021 22:17

mhofman mentioned this pull request Nov 18, 2021

Update loadgen for changes to AMM in PR 4026 Agoric/testnet-load-generator#33

Merged

Chris-Hibbert added the automerge:squash Automatically squash merge label Nov 18, 2021

Chris-Hibbert added 4 commits November 18, 2021 12:58

chore: respond to review comments

e797d1c

add a test remove some outdated comments deposit more plausible values in AMM pools

refactor: rearrange to remove a top-level await

81950f2

chore: minor type correction

d80429c

Chris-Hibbert force-pushed the treasWExternalAMM branch from ebc7865 to d80429c Compare November 18, 2021 21:08

chore: integration to main

fe3b845

Mostly adding missing harden() calls. Also added collateralIssuer to startInstance() (!)

Chris-Hibbert mentioned this pull request Nov 19, 2021

feat!: remove newSwap; replace with constantProduct AMM where needed #4097

Merged

Merge branch 'master' into treasWExternalAMM

a5349c7

mergify bot merged commit b605a6a into master Nov 19, 2021

mergify bot deleted the treasWExternalAMM branch November 19, 2021 01:44

Chris-Hibbert added a commit to Agoric/documentation that referenced this pull request Nov 19, 2021

Update Treasury doc for extracted AMM (#570)

c0fe07a

ref: Agoric/agoric-sdk#4026

mhofman mentioned this pull request Nov 23, 2021

Loadgen vault close causes amountGiven or amountWanted must be greater than 0 #4114

Closed

dckc mentioned this pull request Dec 22, 2021

separate production vat bootstrap from testing mechanisms #4165

Closed

		divideBy(getAmountOut(quote), liquidationMargin),
		ceilDivideBy(getAmountOut(quote), liquidationMargin),

		// Ask the treasury for enough RUN to fund both AMM and bank.
		const bootstrapPaymentValue = bankBootstrapSupply + ammDepositValue;

		// TODO: what happens if unusedBankPayment doesn't have enough
		// remaining?

Extract AMM from Vaults (ex-Treasury) #4026

Extract AMM from Vaults (ex-Treasury) #4026

Conversation

Chris-Hibbert commented Nov 3, 2021 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Nov 3, 2021 •

edited

Loading