From 186fcf5c76c6ed41a33a0f1f77081da64ebabb4d Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 2 Nov 2023 20:46:50 -0700 Subject: [PATCH 1/4] feat(ertp): upgraded quotes incrementally empty recovery-sets --- packages/ERTP/src/issuerKit.js | 73 ++++++++++++++--- packages/ERTP/src/paymentLedger.js | 19 ++++- packages/ERTP/src/purse.js | 81 +++++++++++++++++-- packages/ERTP/src/types-ambient.js | 37 ++++++++- .../src/price/fluxAggregatorContract.js | 1 + .../priceAuthorityQuoteMint.js | 1 + 6 files changed, 190 insertions(+), 22 deletions(-) diff --git a/packages/ERTP/src/issuerKit.js b/packages/ERTP/src/issuerKit.js index 028ba4fbcd6..ecf6f282b2c 100644 --- a/packages/ERTP/src/issuerKit.js +++ b/packages/ERTP/src/issuerKit.js @@ -1,6 +1,6 @@ // @jessie-check -import { assert } from '@agoric/assert'; +import { assert, Fail } from '@agoric/assert'; import { assertPattern } from '@agoric/store'; import { makeScalarBigMapStore } from '@agoric/vat-data'; @@ -28,6 +28,8 @@ import './types-ambient.js'; * * @template {AssetKind} K * @param {IssuerRecord} issuerRecord + * @param {RecoverySetsOption} recoverySetsState Omitted from issuerRecord + * because it was added in an upgrade. * @param {Baggage} issuerBaggage * @param {ShutdownWithFailure} [optShutdownWithFailure] If this issuer fails in * the middle of an atomic action (which btw should never happen), it @@ -40,6 +42,7 @@ import './types-ambient.js'; */ const setupIssuerKit = ( { name, assetKind, displayInfo, elementShape }, + recoverySetsState, issuerBaggage, optShutdownWithFailure = undefined, ) => { @@ -65,6 +68,7 @@ const setupIssuerKit = ( assetKind, cleanDisplayInfo, elementShape, + recoverySetsState, optShutdownWithFailure, ); @@ -80,6 +84,12 @@ harden(setupIssuerKit); /** The key at which the issuer record is stored. */ const INSTANCE_KEY = 'issuer'; +/** + * The key at which the issuerKit's `RecoverySetsOption` state is stored. + * Introduced by an upgrade, so may be absent on a predecessor incarnation. See + * `RecoverySetsOption` for defaulting behavior. + */ +const RECOVERY_SETS_STATE = 'recoverySetsState'; /** * Used _only_ to upgrade a predecessor issuerKit. Use `makeDurableIssuerKit` to @@ -94,14 +104,32 @@ const INSTANCE_KEY = 'issuer'; * unit of computation, like the enclosing vat, can be shutdown before * anything else is corrupted by that corrupted state. See * https://github.com/Agoric/agoric-sdk/issues/3434 + * @param {RecoverySetsOption} [recoverySetsOption] Added in upgrade, so last + * and optional. See `RecoverySetsOption` for defaulting behavior. * @returns {IssuerKit} */ export const upgradeIssuerKit = ( issuerBaggage, optShutdownWithFailure = undefined, + recoverySetsOption = undefined, ) => { const issuerRecord = issuerBaggage.get(INSTANCE_KEY); - return setupIssuerKit(issuerRecord, issuerBaggage, optShutdownWithFailure); + const oldRecoverySetsState = issuerBaggage.has(RECOVERY_SETS_STATE) + ? issuerBaggage.get(RECOVERY_SETS_STATE) + : 'hasRecoverySets'; + if ( + oldRecoverySetsState === 'noRecoverySets' && + recoverySetsOption === 'hasRecoverySets' + ) { + Fail`Cannot (yet?) upgrade from 'noRecoverySets' to 'hasRecoverySets'`; + } + const recoverySetsState = recoverySetsOption || oldRecoverySetsState; + return setupIssuerKit( + issuerRecord, + recoverySetsState, + issuerBaggage, + optShutdownWithFailure, + ); }; harden(upgradeIssuerKit); @@ -121,8 +149,14 @@ export const hasIssuer = baggage => baggage.has(INSTANCE_KEY); * typically, the amount of an invitation payment is a singleton set. Such a * payment is often referred to in the singular as "an invitation".) * + * `recoverySetsOption` added in upgrade. Note that `IssuerOptionsRecord` is + * never stored, so we never need to worry about inheriting one from a + * predecessor predating the introduction of recovery sets. See + * `RecoverySetsOption` for defaulting behavior. + * * @typedef {Partial<{ * elementShape: Pattern; + * recoverySetsOption: RecoverySetsOption; * }>} IssuerOptionsRecord */ @@ -163,11 +197,23 @@ export const makeDurableIssuerKit = ( assetKind = AssetKind.NAT, displayInfo = harden({}), optShutdownWithFailure = undefined, - { elementShape = undefined } = {}, + { elementShape = undefined, recoverySetsOption = undefined } = {}, ) => { - const issuerData = harden({ name, assetKind, displayInfo, elementShape }); + const issuerData = harden({ + name, + assetKind, + displayInfo, + elementShape, + }); issuerBaggage.init(INSTANCE_KEY, issuerData); - return setupIssuerKit(issuerData, issuerBaggage, optShutdownWithFailure); + const recoverySetsState = recoverySetsOption || 'hasRecoverySets'; + issuerBaggage.init(RECOVERY_SETS_STATE, recoverySetsState); + return setupIssuerKit( + issuerData, + recoverySetsState, + issuerBaggage, + optShutdownWithFailure, + ); }; harden(makeDurableIssuerKit); @@ -211,12 +257,19 @@ export const prepareIssuerKit = ( options = {}, ) => { if (hasIssuer(issuerBaggage)) { - const { elementShape: _ = undefined } = options; - const issuerKit = upgradeIssuerKit(issuerBaggage, optShutdownWithFailure); + const { elementShape: _ = undefined, recoverySetsOption = undefined } = + options; + const issuerKit = upgradeIssuerKit( + issuerBaggage, + optShutdownWithFailure, + recoverySetsOption, + ); // TODO check consistency with name, assetKind, displayInfo, elementShape. // Consistency either means that these are the same, or that they differ - // in a direction we are prepared to upgrade. + // in a direction we are prepared to upgrade. Note that it is the + // responsibility of `upgradeIssuerKit` to check consistency of + // `recoverySetsOption`, so continue to not do that here. // @ts-expect-error Type parameter confusion. return issuerKit; @@ -274,7 +327,7 @@ export const makeIssuerKit = ( assetKind = AssetKind.NAT, displayInfo = harden({}), optShutdownWithFailure = undefined, - { elementShape = undefined } = {}, + { elementShape = undefined, recoverySetsOption = undefined } = {}, ) => makeDurableIssuerKit( makeScalarBigMapStore('dropped issuer kit', { durable: true }), @@ -282,6 +335,6 @@ export const makeIssuerKit = ( assetKind, displayInfo, optShutdownWithFailure, - { elementShape }, + { elementShape, recoverySetsOption }, ); harden(makeIssuerKit); diff --git a/packages/ERTP/src/paymentLedger.js b/packages/ERTP/src/paymentLedger.js index b11ecc925a7..dd7c99625c4 100644 --- a/packages/ERTP/src/paymentLedger.js +++ b/packages/ERTP/src/paymentLedger.js @@ -2,7 +2,7 @@ /* eslint-disable no-use-before-define */ import { isPromise } from '@endo/promise-kit'; -import { mustMatch, M, keyEQ } from '@agoric/store'; +import { mustMatch, M, keyEQ } from '@endo/patterns'; import { provideDurableWeakMapStore, prepareExo, @@ -79,6 +79,7 @@ const amountShapeFromElementShape = (brand, assetKind, elementShape) => { * @param {K} assetKind * @param {DisplayInfo} displayInfo * @param {Pattern} elementShape + * @param {RecoverySetsOption} recoverySetsState * @param {ShutdownWithFailure} [optShutdownWithFailure] * @returns {PaymentLedger} */ @@ -88,6 +89,7 @@ export const preparePaymentLedger = ( assetKind, displayInfo, elementShape, + recoverySetsState, optShutdownWithFailure = undefined, ) => { /** @type {Brand} */ @@ -162,6 +164,12 @@ export const preparePaymentLedger = ( * - A purse's recovery set only contains payments withdrawn from that purse and * not yet consumed. * + * If `recoverySetsState === 'noRecoverySets'`, then nothing should ever be + * added to this WeakStore. If upgrading from a previous state with recovery + * sets, whether implicitly or explicitly, then this WeakStore should + * eventually become empty. But because this store is weak, the responsibility + * emptying it out lies elsewhere (purse.js). + * * @type {WeakMapStore>} */ const paymentRecoverySets = provideDurableWeakMapStore( @@ -178,6 +186,9 @@ export const preparePaymentLedger = ( * @param {SetStore} [optRecoverySet] */ const initPayment = (payment, amount, optRecoverySet = undefined) => { + if (recoverySetsState === 'noRecoverySets') { + assert(optRecoverySet === undefined); + } if (optRecoverySet !== undefined) { optRecoverySet.add(payment); paymentRecoverySets.init(payment, optRecoverySet); @@ -283,14 +294,14 @@ export const preparePaymentLedger = ( * @param {(newPurseBalance: Amount) => void} updatePurseBalance - commit the * purse balance * @param {Amount} amount - the amount to be withdrawn - * @param {SetStore} recoverySet + * @param {SetStore} [recoverySet] * @returns {Payment} */ const withdrawInternal = ( currentBalance, updatePurseBalance, amount, - recoverySet, + recoverySet = undefined, ) => { amount = coerce(amount); AmountMath.isGTE(currentBalance, amount) || @@ -322,6 +333,8 @@ export const preparePaymentLedger = ( depositInternal, withdrawInternal, }), + recoverySetsState, + recoverySetsState === 'noRecoverySets' ? undefined : paymentRecoverySets, ); /** @type {Issuer} */ diff --git a/packages/ERTP/src/purse.js b/packages/ERTP/src/purse.js index 74451170fe0..13e98879eeb 100644 --- a/packages/ERTP/src/purse.js +++ b/packages/ERTP/src/purse.js @@ -1,4 +1,4 @@ -import { M } from '@agoric/store'; +import { M, makeCopySet } from '@endo/patterns'; import { prepareExoClassKit, makeScalarBigSetStore } from '@agoric/vat-data'; import { AmountMath } from './amountMath.js'; import { makeTransientNotifierKit } from './transientNotifier.js'; @@ -9,6 +9,8 @@ import { makeTransientNotifierKit } from './transientNotifier.js'; const { Fail } = assert; +const EMPTY_COPY_SET = makeCopySet([]); + /** * @param {Baggage} issuerBaggage * @param {string} name @@ -22,6 +24,8 @@ const { Fail } = assert; * depositInternal: any; * withdrawInternal: any; * }} purseMethods + * @param {RecoverySetsOption} recoverySetsState + * @param {WeakMapStore>} [paymentRecoverySets] */ export const preparePurseKind = ( issuerBaggage, @@ -30,6 +34,8 @@ export const preparePurseKind = ( brand, PurseIKit, purseMethods, + recoverySetsState, + paymentRecoverySets = undefined, ) => { const amountShape = brand.getAmountShape(); @@ -42,6 +48,59 @@ export const preparePurseKind = ( updateBalance(purse, purse.getCurrentAmount()); }; + /** + * How may payments to clean out of the recoverySet on each call to + * `cleanerRecoverySet`. + */ + const CLEANING_BUDGET = 10; + + /** + * If `recoverySetsState === 'hasRecoverySets'` (the normal state), then just + * return `state.recoverySet`. + * + * If `recoverySetsState === 'noRecoverySets'`, then first delete up to + * `CLEANING_BUDGET` payments from `state.recoverySet`, to eventually clean it + * out. Then return `undefined`. Callers must be aware that the `undefined` + * return happens iff `recoverySetsState === 'noRecoverySets'`, and to avoid + * avoid storing or retrieving anything from the actual recovery set. + * + * @param {{ recoverySet: SetStore }} state + * @returns {SetStore | undefined} + */ + const cleanerRecoverySet = state => { + const { recoverySet } = state; + if (recoverySetsState === 'hasRecoverySets') { + return recoverySet; + } else { + assert(recoverySetsState === 'noRecoverySets'); + assert(paymentRecoverySets !== undefined); + let i = 0; + for (const payment of recoverySet.keys()) { + if (i >= CLEANING_BUDGET) { + break; + } + i += 1; + // The stateShape constraint and the current lack of support for schema + // upgrade means that we cannot upgrade `state.recoverySet` to + // `undefined` or any non-remotable. + // + // At the time of this writing, SwingSet's liveSlots package does not + // yet incrementalize the gc work of virtual and durable objects. + // To avoid depending on that, this code does the incremental removal + // of payments from recovery sets here. Doing so means that the cleanup + // only happens when touched, which would be a potential problem if + // an idle purse's recovert set held onto a lot of unneeded payments. + // However, we currently only have this problem for quote issuers, + // which we know store minted payments only in the mintRecoveryPurse's + // recovery purse, which we also know to be perpetually active. + assert(paymentRecoverySets.get(payment) === recoverySet); + paymentRecoverySets.delete(payment); + recoverySet.delete(payment); + } + return undefined; + } + }; + // - This kind is a pair of purse and depositFacet that have a 1:1 // correspondence. // - They are virtualized together to share a single state record. @@ -83,13 +142,14 @@ export const preparePurseKind = ( }, withdraw(amount) { const { state } = this; + const optRecoverySet = cleanerRecoverySet(state); // Note COMMIT POINT within withdraw. return withdrawInternal( state.currentBalance, newPurseBalance => updatePurseBalance(state, newPurseBalance, this.facets.purse), amount, - state.recoverySet, + optRecoverySet, ); }, getCurrentAmount() { @@ -107,18 +167,29 @@ export const preparePurseKind = ( }, getRecoverySet() { - return this.state.recoverySet.snapshot(); + const { state } = this; + const optRecoverySet = cleanerRecoverySet(state); + if (optRecoverySet === undefined) { + return EMPTY_COPY_SET; + } + return optRecoverySet.snapshot(); }, recoverAll() { const { state, facets } = this; let amount = AmountMath.makeEmpty(brand, assetKind); - for (const payment of state.recoverySet.keys()) { + const optRecoverySet = cleanerRecoverySet(state); + if (optRecoverySet === undefined) { + // Note that even this case does only the gc work implied by the + // call to `cleanerRecoverySet` above. + return amount; // empty at this time + } + for (const payment of optRecoverySet.keys()) { // This does cause deletions from the set while iterating, // but this special case is allowed. const delta = facets.purse.deposit(payment); amount = AmountMath.add(amount, delta, brand); } - state.recoverySet.getSize() === 0 || + optRecoverySet.getSize() === 0 || Fail`internal: Remaining unrecovered payments: ${facets.purse.getRecoverySet()}`; return amount; }, diff --git a/packages/ERTP/src/types-ambient.js b/packages/ERTP/src/types-ambient.js index 0bdc50acee0..95f1751c94b 100644 --- a/packages/ERTP/src/types-ambient.js +++ b/packages/ERTP/src/types-ambient.js @@ -178,8 +178,8 @@ * @template {AssetKind} [K=AssetKind] * @typedef {object} PaymentLedger * @property {Mint} mint - * @property {Purse} mintRecoveryPurse Useful only to get the recovery set - * associated with minted payments that are still live. + * @property {Purse} mintRecoveryPurse Externally useful only if this issuer + * uses recovery sets. * @property {Issuer} issuer * @property {Brand} brand */ @@ -188,8 +188,8 @@ * @template {AssetKind} [K=AssetKind] * @typedef {object} IssuerKit * @property {Mint} mint - * @property {Purse} mintRecoveryPurse Useful only to get the recovery set - * associated with minted payments that are still live. + * @property {Purse} mintRecoveryPurse Externally useful only if this issuer + * uses recovery sets. * @property {Issuer} issuer * @property {Brand} brand * @property {DisplayInfo} displayInfo @@ -222,6 +222,31 @@ // /////////////////////////// Purse / Payment ///////////////////////////////// +/** + * Issuers first became durable with recovery sets and no option to suppress + * them. Thus, absence of a `RecoverySetsOption` state is equivalent to + * `'hasRecoverySets'`. By contrast, the absence of `RecoverySetsOption` provide + * parameter defaults to the predecessor's `RecoverySetsOption` state, or + * `'hasRecoverySets'` if none. + * + * The `'noRecoverySets'` state, if used for the first incarnation, makes an + * issuer without recovery sets. If used for a successor incarnation, no matter + * whether the predecessor was `'hasRecoverySets'` or `'noRecoverySets'`, + * + * - will start emptying recovery sets, + * - will prevent any new payments from being added to recovery sets, + * - and (controversially) will not provide access via recovery sets of any + * payments that have not yet been emptied out. + * + * At this time, a `'noRecoverySets'` predecessor cannot be upgraded to a + * `'hasRecoverySets'` successor. If it turns out this transition is needed, it + * can likely be supported in a future upgrade. + * + * @typedef {'hasRecoverySets' | 'noRecoverySets'} RecoverySetsOption + */ + +// /////////////////////////// Purse / Payment ///////////////////////////////// + /** * @callback DepositFacetReceive * @param {Payment} payment @@ -281,10 +306,14 @@ * can spend the assets at stake on other things. Afterwards, if the recipient * of the original check finally gets around to depositing it, their deposit * fails. + * + * Returns an empty set if this issuer does not support recovery sets. * @property {() => Amount} recoverAll For use in emergencies, such as coming * back from a traumatic crash and upgrade. This deposits all the payments in * this purse's recovery set into the purse itself, returning the total amount * of assets recovered. + * + * Returns an empty amount if this issuer does not support recovery sets. */ /** diff --git a/packages/inter-protocol/src/price/fluxAggregatorContract.js b/packages/inter-protocol/src/price/fluxAggregatorContract.js index d8050c26a18..84f6b8eb17d 100644 --- a/packages/inter-protocol/src/price/fluxAggregatorContract.js +++ b/packages/inter-protocol/src/price/fluxAggregatorContract.js @@ -74,6 +74,7 @@ export const start = async (zcf, privateArgs, baggage) => { 'set', undefined, undefined, + { recoverySetsOption: 'noRecoverySets' }, ); const { diff --git a/packages/zoe/src/contractSupport/priceAuthorityQuoteMint.js b/packages/zoe/src/contractSupport/priceAuthorityQuoteMint.js index 769b08643ac..46531ffc125 100644 --- a/packages/zoe/src/contractSupport/priceAuthorityQuoteMint.js +++ b/packages/zoe/src/contractSupport/priceAuthorityQuoteMint.js @@ -17,6 +17,7 @@ export const provideQuoteMint = baggage => { AssetKind.SET, undefined, undefined, + { recoverySetsOption: 'noRecoverySets' }, ); return issuerKit.mint; }; From a5999ea4db375a9da9811fb48e6b0e9f35dec576 Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Tue, 7 Nov 2023 10:49:57 -0800 Subject: [PATCH 2/4] fix: include paymentRecoverySets when calling preparePurseKind() cleanerRecoverySet() asserts paymentRecoverySet !== undefined when recoverSetState === noRecoverySets, so paymentLedger must provide it when calling preparePurseKind(). --- packages/ERTP/src/paymentLedger.js | 2 +- packages/ERTP/src/purse.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/ERTP/src/paymentLedger.js b/packages/ERTP/src/paymentLedger.js index dd7c99625c4..634a4d6cb0e 100644 --- a/packages/ERTP/src/paymentLedger.js +++ b/packages/ERTP/src/paymentLedger.js @@ -334,7 +334,7 @@ export const preparePaymentLedger = ( withdrawInternal, }), recoverySetsState, - recoverySetsState === 'noRecoverySets' ? undefined : paymentRecoverySets, + paymentRecoverySets, ); /** @type {Issuer} */ diff --git a/packages/ERTP/src/purse.js b/packages/ERTP/src/purse.js index 13e98879eeb..d8487703ca5 100644 --- a/packages/ERTP/src/purse.js +++ b/packages/ERTP/src/purse.js @@ -25,7 +25,7 @@ const EMPTY_COPY_SET = makeCopySet([]); * withdrawInternal: any; * }} purseMethods * @param {RecoverySetsOption} recoverySetsState - * @param {WeakMapStore>} [paymentRecoverySets] + * @param {WeakMapStore>} paymentRecoverySets */ export const preparePurseKind = ( issuerBaggage, @@ -35,7 +35,7 @@ export const preparePurseKind = ( PurseIKit, purseMethods, recoverySetsState, - paymentRecoverySets = undefined, + paymentRecoverySets, ) => { const amountShape = brand.getAmountShape(); From e5cfa654d5de58372eef2147e25b10603ac8218e Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Tue, 7 Nov 2023 11:17:52 -0800 Subject: [PATCH 3/4] chore: fix minor typos in comments --- packages/ERTP/src/paymentLedger.js | 2 +- packages/ERTP/src/purse.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ERTP/src/paymentLedger.js b/packages/ERTP/src/paymentLedger.js index 634a4d6cb0e..1e9ed5ea209 100644 --- a/packages/ERTP/src/paymentLedger.js +++ b/packages/ERTP/src/paymentLedger.js @@ -168,7 +168,7 @@ export const preparePaymentLedger = ( * added to this WeakStore. If upgrading from a previous state with recovery * sets, whether implicitly or explicitly, then this WeakStore should * eventually become empty. But because this store is weak, the responsibility - * emptying it out lies elsewhere (purse.js). + * for emptying it out lies elsewhere (purse.js). * * @type {WeakMapStore>} */ diff --git a/packages/ERTP/src/purse.js b/packages/ERTP/src/purse.js index d8487703ca5..e4ae153118b 100644 --- a/packages/ERTP/src/purse.js +++ b/packages/ERTP/src/purse.js @@ -49,7 +49,7 @@ export const preparePurseKind = ( }; /** - * How may payments to clean out of the recoverySet on each call to + * How many payments to clean out of the recoverySet on each call to * `cleanerRecoverySet`. */ const CLEANING_BUDGET = 10; @@ -62,7 +62,7 @@ export const preparePurseKind = ( * `CLEANING_BUDGET` payments from `state.recoverySet`, to eventually clean it * out. Then return `undefined`. Callers must be aware that the `undefined` * return happens iff `recoverySetsState === 'noRecoverySets'`, and to avoid - * avoid storing or retrieving anything from the actual recovery set. + * storing or retrieving anything from the actual recovery set. * * @param {{ recoverySet: SetStore }} state * @returns {SetStore | undefined} @@ -89,7 +89,7 @@ export const preparePurseKind = ( // To avoid depending on that, this code does the incremental removal // of payments from recovery sets here. Doing so means that the cleanup // only happens when touched, which would be a potential problem if - // an idle purse's recovert set held onto a lot of unneeded payments. + // an idle purse's recovery set held onto a lot of unneeded payments. // However, we currently only have this problem for quote issuers, // which we know store minted payments only in the mintRecoveryPurse's // recovery purse, which we also know to be perpetually active. From fcde617279d16eb4ad13af0c7cb8bb3dd007fc6f Mon Sep 17 00:00:00 2001 From: Chris Hibbert Date: Tue, 19 Dec 2023 11:08:48 -0800 Subject: [PATCH 4/4] refactor: don't add zero-value payments to recoverySet --- packages/ERTP/src/paymentLedger.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ERTP/src/paymentLedger.js b/packages/ERTP/src/paymentLedger.js index 1e9ed5ea209..11233f108b4 100644 --- a/packages/ERTP/src/paymentLedger.js +++ b/packages/ERTP/src/paymentLedger.js @@ -189,7 +189,7 @@ export const preparePaymentLedger = ( if (recoverySetsState === 'noRecoverySets') { assert(optRecoverySet === undefined); } - if (optRecoverySet !== undefined) { + if (optRecoverySet !== undefined && !AmountMath.isEmpty(amount)) { optRecoverySet.add(payment); paymentRecoverySets.init(payment, optRecoverySet); }