refactor(ERTP,vats): AmountPatternShape (#9410)

closes: XXX
refs: #9402 (comment) #9402 (comment)  #9407

## Description

#9407 explains that sometimes we used a `.optional(AmountShape)` guard to describe an `optAmountShape` argument.

#9402 (comment) #9402 (comment) discussed ways to fix this that were ultimately omitted from #9402 . This PR provides a form of the fix discussed in those comments. Three remaining controversies I'd like my reviewers to comment on:
- Whether the name of the exported pattern should be `AmountShapeShape` or `AmountPatternShape`. See my doc-comment on `AmountPatternShape` for my weak reasoning about why I chose this name.
- If we do go with `AmountPatternShape` for the stated reasons, should the corresponding parameter variables be changed from `optAmountShape` to `optAmountPattern`.
- Whatever the name of the exported pattern, should it be defined as 
   - `M.pattern()`
   - `harden({ brand: M.pattern(), value: M.pattern() })`
   - `harden({ brand: BrandShape, value: M.pattern() })`
   - something else

### Security Considerations

For almost all of the choices of resolving the above controversies, this PR remains a pure bug fix. All correct code that used to work will continue working the same way, and some correct code that used to incorrectly fail due to this bug will now start working correctly.

The exception would be if we both accepting the renaming of some existing occurrences of `M.pattern()` to `AmountPatternShape`/`AmountShapeShape`, and define this name as a pattern narrower than `M.pattern()`. In that case, existing (hypothetical) code that, for example, used `M.any()` in such an argument position would start failing.

### Scaling Considerations

None
### Documentation Considerations

None
### Testing Considerations

#9407 mentions how to reproduce the bug it reports. This PR should add that as a test, to verify that this PR fixes that bug.
### Upgrade Considerations

#9402 (comment) and #9402 (comment) explain the upgrade concern that was likely the cause for omitting this fix from #9042

packages/ERTP/src/typeGuards.js

@@ -77,6 +77,19 @@ export const AmountShape = harden({
   value: AmountValueShape,
 });
 
+/**
+ * To be used to guard an amount pattern argument, i.e., an argument which is a
+ * pattern that can be used to test amounts. Since amounts are keys, anywhere an
+ * amount pattern is expected, an amount can be provided and will match only
+ * that concrete amount, i.e., amounts that are `keyEQ` to that amount.
+ *
+ * The `AmountShape` guard above is an amount pattern. But not all amount
+ * patterns are like `AmountShape`. For example, `M.any()` is a valid amount
+ * pattern that will admit any amount, but is does not resemble the
+ * `AmountShape` pattern above.
+ */
+export const AmountPatternShape = M.pattern();
+
 export const RatioShape = harden({
   numerator: AmountShape,
   denominator: AmountShape,
@@ -178,7 +191,7 @@ export const makeIssuerInterfaces = (
     isLive: M.callWhen(M.await(PaymentShape)).returns(M.boolean()),
     getAmountOf: M.callWhen(M.await(PaymentShape)).returns(amountShape),
     burn: M.callWhen(M.await(PaymentShape))
-      .optional(M.pattern())
+      .optional(AmountPatternShape)
       .returns(amountShape),
   });
 
@@ -205,7 +218,9 @@ export const makeIssuerInterfaces = (
     // `srcPayment` is a remotable, leaving it
     // to this raw method to validate that this remotable is actually
     // a live payment of the correct brand with sufficient funds.
-    deposit: M.call(PaymentShape).optional(M.pattern()).returns(amountShape),
+    deposit: M.call(PaymentShape)
+      .optional(AmountPatternShape)
+      .returns(amountShape),
     getDepositFacet: M.call().returns(DepositFacetShape),
     withdraw: M.call(amountShape).returns(PaymentShape),
     getRecoverySet: M.call().returns(M.setOf(PaymentShape)),

packages/vats/src/localchain.js

@@ -2,7 +2,12 @@
 import { Fail } from '@endo/errors';
 import { E } from '@endo/far';
 import { M } from '@endo/patterns';
-import { AmountShape, BrandShape, PaymentShape } from '@agoric/ertp';
+import {
+  AmountPatternShape,
+  AmountShape,
+  BrandShape,
+  PaymentShape,
+} from '@agoric/ertp';
 import { Shape as NetworkShape } from '@agoric/network';
 
 const { Vow$ } = NetworkShape;
@@ -49,7 +54,7 @@ export const LocalChainAccountI = M.interface('LocalChainAccount', {
   getAddress: M.callWhen().returns(Vow$(M.string())),
   getBalance: M.callWhen(BrandShape).returns(Vow$(AmountShape)),
   deposit: M.callWhen(PaymentShape)
-    .optional(M.pattern())
+    .optional(AmountPatternShape)
     .returns(Vow$(AmountShape)),
   withdraw: M.callWhen(AmountShape).returns(Vow$(PaymentShape)),
   executeTx: M.callWhen(M.arrayOf(M.record())).returns(

packages/vats/src/virtual-purse.js

@@ -1,16 +1,17 @@
 import { Fail } from '@endo/errors';
 import { E } from '@endo/far';
 import { isPromise } from '@endo/promise-kit';
-import { getInterfaceGuardPayload } from '@endo/patterns';
+import { getInterfaceGuardPayload, matches } from '@endo/patterns';
 
 import { M } from '@agoric/store';
 import {
+  AmountPatternShape,
   AmountShape,
   BrandShape,
   DepositFacetShape,
   NotifierShape,
   PaymentShape,
-} from '@agoric/ertp/src/typeGuards.js';
+} from '@agoric/ertp';
 
 /**
  * @param {Pattern} [brandShape]
@@ -35,7 +36,7 @@ export const makeVirtualPurseKitIKit = (
     // to this raw method to validate that this remotable is actually
     // a live payment of the correct brand with sufficient funds.
     deposit: M.callWhen(PaymentShape)
-      .optional(M.pattern())
+      .optional(AmountPatternShape)
       .returns(amountShape),
     getDepositFacet: M.callWhen().returns(DepositFacetShape),
     withdraw: M.callWhen(amountShape).returns(PaymentShape),
@@ -48,15 +49,17 @@ export const makeVirtualPurseKitIKit = (
   });
 
   const RetainRedeemI = M.interface('RetainRedeem', {
-    retain: M.callWhen(PaymentShape).optional(amountShape).returns(amountShape),
+    retain: M.callWhen(PaymentShape)
+      .optional(AmountPatternShape)
+      .returns(amountShape),
     redeem: M.callWhen(amountShape).returns(PaymentShape),
   });
 
   const UtilsI = M.interface('Utils', {
     retain: getInterfaceGuardPayload(RetainRedeemI).methodGuards.retain,
     redeem: getInterfaceGuardPayload(RetainRedeemI).methodGuards.redeem,
     recoverableClaim: M.callWhen(M.await(PaymentShape))
-      .optional(amountShape)
+      .optional(AmountPatternShape)
       .returns(PaymentShape),
   });
 
@@ -102,6 +105,22 @@ export const makeVirtualPurseKitIKit = (
  *   current balance iterable for a given brand.
  */
 
+/**
+ * Until https://github.com/Agoric/agoric-sdk/issues/9407 is fixed, this
+ * function restricts the `optAmountShape`, if provided, to be a concrete
+ * `Amount` rather than a `Pattern` as it is supposed to be.
+ *
+ * TODO: Once https://github.com/Agoric/agoric-sdk/issues/9407 is fixed, remove
+ * this function and all calls to it.
+ *
+ * @param {Pattern} [optAmountShape]
+ */
+const legacyRestrictAmountShapeArg = optAmountShape => {
+  if (optAmountShape && !matches(optAmountShape, AmountShape)) {
+    throw Fail`optAmountShape if provided, must still be a concrete Amount due to https://github.com/Agoric/agoric-sdk/issues/9407`;
+  }
+};
+
 /** @param {import('@agoric/zone').Zone} zone */
 const prepareVirtualPurseKit = zone =>
   zone.exoClassKit(
@@ -139,6 +158,7 @@ const prepareVirtualPurseKit = zone =>
          * @returns {Promise<Payment<'nat'>>}
          */
         async recoverableClaim(payment, optAmountShape) {
+          legacyRestrictAmountShapeArg(optAmountShape);
           const {
             state: { recoveryPurse },
           } = this;
@@ -166,6 +186,7 @@ const prepareVirtualPurseKit = zone =>
       minter: {
         /** @type {Retain} */
         async retain(payment, optAmountShape) {
+          legacyRestrictAmountShapeArg(optAmountShape);
           !!this.state.mint || Fail`minter cannot retain without a mint.`;
           return E(this.state.issuer).burn(payment, optAmountShape);
         },

packages/vats/test/vpurse.test.js

@@ -1,3 +1,4 @@
+import { M } from '@endo/patterns';
 import { test as rawTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';
 import { reincarnate } from '@agoric/swingset-liveslots/tools/setup-vat-data.js';
 
@@ -176,6 +177,86 @@ test('makeVirtualPurse', async t => {
     .then(checkWithdrawal);
 });
 
+// TODO Once https://github.com/Agoric/agoric-sdk/issues/9407 is fixed,
+// This test should replace the similar one above.
+test.failing('makeVirtualPurse with optAmountShape pattern', async t => {
+  t.plan(22);
+  const { baggage } = t.context;
+  const zone = makeDurableZone(baggage).subZone('makeVirtualPurse');
+
+  const { expected, balanceUpdater, issuer, mint, brand, vpurse } = setup(
+    t,
+    zone,
+  );
+
+  const payment = mint.mintPayment(AmountMath.make(brand, 837n));
+
+  const notifier = E(vpurse).getCurrentAmountNotifier();
+  let nextUpdateP = E(notifier).getUpdateSince();
+
+  const checkNotifier = async () => {
+    const { value: balance, updateCount } = await nextUpdateP;
+    t.assert(
+      AmountMath.isEqual(await E(vpurse).getCurrentAmount(), balance),
+      `the notifier balance is the same as the purse`,
+    );
+    nextUpdateP = E(notifier).getUpdateSince(updateCount);
+  };
+
+  balanceUpdater.updateState(AmountMath.makeEmpty(brand));
+  await checkNotifier();
+  t.assert(
+    AmountMath.isEqual(
+      await E(vpurse).getCurrentAmount(),
+      AmountMath.makeEmpty(brand),
+    ),
+    `empty purse is empty`,
+  );
+  t.is(await E(vpurse).getAllegedBrand(), brand, `purse's brand is correct`);
+  const fungible837 = AmountMath.make(brand, 837n);
+
+  const checkDeposit = async newPurseBalance => {
+    t.assert(
+      AmountMath.isEqual(newPurseBalance, fungible837),
+      `the amount returned is the payment amount`,
+    );
+    await checkNotifier();
+    t.assert(
+      AmountMath.isEqual(await E(vpurse).getCurrentAmount(), fungible837),
+      `the new purse balance is the payment's old balance`,
+    );
+  };
+
+  const performWithdrawal = () => {
+    expected.pullAmount(fungible837);
+    return E(vpurse).withdraw(fungible837);
+  };
+
+  const checkWithdrawal = async newPayment => {
+    await issuer.getAmountOf(newPayment).then(amount => {
+      t.assert(
+        AmountMath.isEqual(amount, fungible837),
+        `the withdrawn payment has the right balance`,
+      );
+    });
+    await checkNotifier();
+    t.assert(
+      AmountMath.isEqual(
+        await E(vpurse).getCurrentAmount(),
+        AmountMath.makeEmpty(brand),
+      ),
+      `the purse is empty again`,
+    );
+  };
+
+  expected.pushAmount(fungible837);
+  await E(vpurse)
+    .deposit(payment, M.and(fungible837))
+    .then(checkDeposit)
+    .then(performWithdrawal)
+    .then(checkWithdrawal);
+});
+
 test('makeVirtualPurse withdraw from escrowPurse', async t => {
   const { baggage } = t.context;
   const zone = makeDurableZone(baggage).subZone('withdraw from escrowPurse');