From 9c9e5cf884e5f48554300d91ff7629773256f628 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 8 Aug 2024 15:12:56 -0700 Subject: [PATCH] refactor(ERTP,vats): AmountPatternShape (#9410) closes: XXX refs: https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1612477663 https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1612651947 https://github.com/Agoric/agoric-sdk/issues/9407 ## Description #9407 explains that sometimes we used a `.optional(AmountShape)` guard to describe an `optAmountShape` argument. https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1612477663 https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1612651947 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 https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1613503324 and https://github.com/Agoric/agoric-sdk/pull/9402#discussion_r1613622732 explain the upgrade concern that was likely the cause for omitting this fix from #9042 --- packages/ERTP/src/typeGuards.js | 19 ++++++- packages/vats/src/localchain.js | 9 +++- packages/vats/src/virtual-purse.js | 31 ++++++++++-- packages/vats/test/vpurse.test.js | 81 ++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 9 deletions(-) diff --git a/packages/ERTP/src/typeGuards.js b/packages/ERTP/src/typeGuards.js index 1b2ec30fcea..4e4f38f7f04 100644 --- a/packages/ERTP/src/typeGuards.js +++ b/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)), diff --git a/packages/vats/src/localchain.js b/packages/vats/src/localchain.js index 4637e0d12e1..bec7cf407da 100644 --- a/packages/vats/src/localchain.js +++ b/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( diff --git a/packages/vats/src/virtual-purse.js b/packages/vats/src/virtual-purse.js index 68addb78ceb..155f29e31a9 100644 --- a/packages/vats/src/virtual-purse.js +++ b/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,7 +49,9 @@ 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), }); @@ -56,7 +59,7 @@ export const makeVirtualPurseKitIKit = ( 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>} */ 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); }, diff --git a/packages/vats/test/vpurse.test.js b/packages/vats/test/vpurse.test.js index 053e65e06cc..ca0da5744ea 100644 --- a/packages/vats/test/vpurse.test.js +++ b/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');