From b9130cc0c619f1b63f54c72a2587cb5a11214383 Mon Sep 17 00:00:00 2001 From: Dan Connolly Date: Tue, 21 Nov 2023 10:16:47 -0600 Subject: [PATCH] fix(smartWallet): handle upgrade disconnects from purse notifiers observeNotifier() would handle upgrade disconnects if the notifiers were durable. But in @agoric/ertp, they're ephemeral, so we open-code the loop. When the offerWatchers detect an upgrade, they reschedule the watchers. We needed to ensure the wallet itself doesn't do its cleanup in that case. The test updates purse balance across upgrade - provision a smartWallet for an oracle operator - upgrade zoe; reproduce smartWallet bug - check for new invitation --- .../bootstrapTests/test-wallet-upgrade.js | 224 ++++++++++++++++++ .../test/bootstrapTests/wallet-scripts.js | 87 +++++++ .../test/smartWallet/test-psm-integration.js | 6 +- packages/smart-wallet/src/offerWatcher.js | 4 +- packages/smart-wallet/src/smartWallet.js | 99 ++++++-- 5 files changed, 390 insertions(+), 30 deletions(-) create mode 100644 packages/boot/test/bootstrapTests/test-wallet-upgrade.js create mode 100644 packages/boot/test/bootstrapTests/wallet-scripts.js diff --git a/packages/boot/test/bootstrapTests/test-wallet-upgrade.js b/packages/boot/test/bootstrapTests/test-wallet-upgrade.js new file mode 100644 index 00000000000..d1279fd576a --- /dev/null +++ b/packages/boot/test/bootstrapTests/test-wallet-upgrade.js @@ -0,0 +1,224 @@ +// @ts-check +import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js'; +import { eventLoopIteration } from '@agoric/internal/src/testing-utils.js'; +import { makeAgoricNamesRemotesFromFakeStorage } from '@agoric/vats/tools/board-utils.js'; +import { Offers } from '@agoric/inter-protocol/src/clientSupport.js'; +import { makeWalletFactoryDriver } from '../../tools/drivers.ts'; +import { makeSwingsetTestKit } from '../../tools/supports.ts'; +import { + restartWalletFactoryScript, + sendInvitationScript, + upgradeZoeScript, +} from './wallet-scripts.js'; + +const { Fail } = assert; + +/** + * @type {import('ava').TestFn< + * Awaited> + * >} + */ +const test = anyTest; + +// main/production config doesn't have initialPrice, upon which 'open vaults' depends +const PLATFORM_CONFIG = '@agoric/vm-config/decentral-itest-vaults-config.json'; + +const makeTestContext = async t => { + const swingsetTestKit = await makeSwingsetTestKit(t.log, 'bundles/wallet', { + configSpecifier: PLATFORM_CONFIG, + }); + + const { runUtils, storage } = swingsetTestKit; + console.timeLog('DefaultTestContext', 'swingsetTestKit'); + const { EV } = runUtils; + + // vaultFactoryKit is one of the last things produced in bootstrap. + await EV.vat('bootstrap').consumeItem('vaultFactoryKit'); + + await eventLoopIteration(); + // wait for bootstrap to settle before looking in storage for brands etc. + const agoricNamesRemotes = makeAgoricNamesRemotesFromFakeStorage( + swingsetTestKit.storage, + ); + agoricNamesRemotes.brand.ATOM || Fail`ATOM brand not yet defined`; + + const walletFactoryDriver = await makeWalletFactoryDriver( + runUtils, + storage, + agoricNamesRemotes, + ); + + return { + walletFactoryDriver, + runUtils, + agoricNamesRemotes, + }; +}; + +test.before(async t => (t.context = await makeTestContext(t))); + +/** + * @param {import('ava').ExecutionContext>>} t + */ +const makeScenario = async t => { + const { agoricNamesRemotes } = t.context; + + const findPurse = (current, _brand = agoricNamesRemotes.brand.Invitation) => { + // getCurrentWalletRecord and agoricNamesRemotes + // aren't using the same marshal context. hmm. + // return ( + // current.purses.find(p => p.brand === brand) || + // Fail`brand ${brand} not found` + // ); + return current.purses[0]; + }; + + const { EV } = t.context.runUtils; + /** @type {ERef} */ + const coreEvalBridgeHandler = await EV.vat('bootstrap').consumeItem( + 'coreEvalBridgeHandler', + ); + + const runCoreEval = async evals => { + const bridgeMessage = { + type: 'CORE_EVAL', + evals, + }; + await EV(coreEvalBridgeHandler).fromBridge(bridgeMessage); + }; + + return { findPurse, runCoreEval }; +}; + +test('update purse balance across zoe upgrade', async t => { + const { walletFactoryDriver } = t.context; + const { findPurse, runCoreEval } = await makeScenario(t); + + t.log('provision a smartWallet for an oracle operator'); + const oraAddr = 'agoric1oracle-operator'; + const oraWallet = await walletFactoryDriver.provideSmartWallet(oraAddr); + + t.log('upgrade zoe'); + await runCoreEval([ + { + json_permits: JSON.stringify({ + consume: { vatStore: true, vatAdminSvc: true }, + }), + js_code: `(${upgradeZoeScript})()`, + }, + ]); + + t.log('send an invitation to the oracle operator'); + await runCoreEval([ + { + json_permits: JSON.stringify({ + consume: { namesByAddressAdmin: true, zoe: true }, + instance: { consume: { reserve: true } }, + }), + js_code: `(${sendInvitationScript})()`.replace('ADDRESS', oraAddr), + }, + ]); + + const current = oraWallet.getCurrentWalletRecord(); + t.log( + 'invitation balance after sending invitation', + findPurse(current).balance, + ); + t.notDeepEqual(findPurse(current).balance.value, [], 'invitation set'); +}); + +test('update purse balance across walletFactory upgrade', async t => { + const { walletFactoryDriver } = t.context; + const { findPurse, runCoreEval } = await makeScenario(t); + + t.log('provision a smartWallet for another oracle operator'); + const oraAddr = 'agoric1oracle-operator2'; + const oraWallet = await walletFactoryDriver.provideSmartWallet(oraAddr); + + t.log('upgrade (restart) walletFactory'); + await runCoreEval([ + { + json_permits: JSON.stringify({ + consume: { instancePrivateArgs: true, walletFactoryStartResult: true }, + }), + js_code: `(${restartWalletFactoryScript})()`.replace('import(', 'XXX'), + }, + ]); + + t.log('send an invitation to the oracle operator'); + await runCoreEval([ + { + json_permits: JSON.stringify({ + consume: { namesByAddressAdmin: true, zoe: true }, + instance: { consume: { reserve: true } }, + }), + js_code: `(${sendInvitationScript})()`.replace('ADDRESS', oraAddr), + }, + ]); + + const current = oraWallet.getCurrentWalletRecord(); + t.log( + 'invitation balance after sending invitation', + findPurse(current).balance, + ); + t.notDeepEqual(findPurse(current).balance.value, [], 'invitation set'); +}); + +test.todo('smartWallet created before upgrade works after'); + +test('offer lasts across zoe upgrade', async t => { + const { walletFactoryDriver, agoricNamesRemotes } = t.context; + const { runCoreEval } = await makeScenario(t); + + t.log('provision a smartWallet for a bidder'); + const bidderAddr = 'agoric1bidder'; + const bidderWallet = await walletFactoryDriver.provideSmartWallet(bidderAddr); + + const checkLive = (label, expected) => { + const { liveOffers } = bidderWallet.getCurrentWalletRecord(); + const ids = Object.keys(Object.fromEntries(liveOffers)); + t.log(bidderAddr, 'liveOffers', label, ids); + t.is(liveOffers.length, expected, label); + + const update = bidderWallet.getLatestUpdateRecord(); + if (update.updated !== 'offerStatus') return; + const { error } = update.status; + t.is(error, undefined); + }; + + checkLive('before IST swap', 0); + + await bidderWallet.sendOffer( + Offers.psm.swap( + agoricNamesRemotes, + agoricNamesRemotes.instance['psm-IST-USDC_axl'], + { offerId: `print-ist1`, wantMinted: 1_000, pair: ['IST', 'USDC_axl'] }, + ), + ); + + checkLive('between IST swap and bid', 0); + + const offerId = 'bid1'; + await bidderWallet.sendOfferMaker(Offers.auction.Bid, { + offerId, + give: '10 IST', + maxBuy: '1000000 ATOM', + price: 5, + }); + + checkLive('between bid and zoe upgrade', 1); + + await runCoreEval([ + { + json_permits: JSON.stringify({ + consume: { vatStore: true, vatAdminSvc: true }, + }), + js_code: `(${upgradeZoeScript})()`, + }, + ]); + + checkLive('between upgrade and exit', 1); + + await bidderWallet.tryExitOffer(offerId); + checkLive('after exit', 0); +}); diff --git a/packages/boot/test/bootstrapTests/wallet-scripts.js b/packages/boot/test/bootstrapTests/wallet-scripts.js new file mode 100644 index 00000000000..2eca0478e54 --- /dev/null +++ b/packages/boot/test/bootstrapTests/wallet-scripts.js @@ -0,0 +1,87 @@ +// @ts-check + +import { E } from '@endo/far'; + +/** + * @typedef {>>( + * obj: T, + * ) => Promise<{ [K in keyof T]: Awaited }>} AllValues + */ + +export const upgradeZoeScript = () => { + /** + * @param {VatAdminSvc} vatAdminSvc + * @param {any} adminNode + * @param {string} bundleCapName + * @param {unknown} vatParameters + */ + const upgradeVat = async ( + vatAdminSvc, + adminNode, + bundleCapName, + vatParameters = {}, + ) => { + const bcap = await E(vatAdminSvc).getNamedBundleCap(bundleCapName); + const options = { vatParameters }; + const incarnationNumber = await E(adminNode).upgrade(bcap, options); + console.log('upgraded', bundleCapName, 'to', incarnationNumber); + }; + + const upgradeZoe = async powers => { + const { vatStore, vatAdminSvc } = powers.consume; + const { adminNode } = await E(vatStore).get('zoe'); + console.log('zoe admin node', adminNode); + await upgradeVat(vatAdminSvc, adminNode, 'zoe'); + }; + return upgradeZoe; +}; + +export const restartWalletFactoryScript = () => { + const { entries, fromEntries } = Object; + + /** @type {AllValues} */ + const allValues = async obj => { + const resolved = await Promise.all( + entries(obj).map(([k, vP]) => Promise.resolve(vP).then(v => [k, v])), + ); + return harden(fromEntries(resolved)); + }; + + /** @param {BootstrapPowers} powers } */ + const restartWalletFactory = async powers => { + const { instancePrivateArgs, walletFactoryStartResult } = powers.consume; + const kit = await walletFactoryStartResult; + console.log(kit); + const { adminFacet } = kit; + /** @type {Parameters[1]} */ + // @ts-expect-error instancePrivateArgs maps to unknown + const privateArgs = await E(instancePrivateArgs).get(kit.instance); + const settledPrivateArgs = await allValues(privateArgs); + await E(adminFacet).restartContract(settledPrivateArgs); + }; + return restartWalletFactory; +}; + +export const sendInvitationScript = () => { + const addr = 'ADDRESS'; + const sendIt = async powers => { + // namesByAddress is broken #8113 + const { + consume: { namesByAddressAdmin, zoe }, + instance: { + consume: { reserve }, + }, + } = powers; + const pf = E(zoe).getPublicFacet(reserve); + const anInvitation = await E(pf).makeAddCollateralInvitation(); + await E(namesByAddressAdmin).reserve(addr); + // don't trigger the namesByAddressAdmin.readonly() bug + const addressAdmin = E(namesByAddressAdmin).lookupAdmin(addr); + await E(addressAdmin).reserve('depositFacet'); + const addressHub = E(addressAdmin).readonly(); + const addressDepositFacet = E(addressHub).lookup('depositFacet'); + await E(addressDepositFacet).receive(anInvitation); + }; + + return sendIt; +}; diff --git a/packages/inter-protocol/test/smartWallet/test-psm-integration.js b/packages/inter-protocol/test/smartWallet/test-psm-integration.js index 84053f87140..b5bdeaa4ffb 100644 --- a/packages/inter-protocol/test/smartWallet/test-psm-integration.js +++ b/packages/inter-protocol/test/smartWallet/test-psm-integration.js @@ -93,10 +93,8 @@ test('null swap', async t => { t.is(await E.get(getBalanceFor(anchor.brand)).value, 0n); t.is(await E.get(getBalanceFor(mintedBrand)).value, 0n); - t.deepEqual(currents[0].liveOffers, []); - t.deepEqual(currents[1].liveOffers, []); - t.deepEqual(currents[2].liveOffers, [['nullSwap', offer]]); - t.deepEqual(currents[3].liveOffers, []); + const found = currents.find(c => c.liveOffers.length > 0); + t.deepEqual(found?.liveOffers, [['nullSwap', offer]]); }); // we test this direction of swap because wanting anchor would require the PSM to have anchor in it first diff --git a/packages/smart-wallet/src/offerWatcher.js b/packages/smart-wallet/src/offerWatcher.js index 5c7511c54af..5e14abe06cc 100644 --- a/packages/smart-wallet/src/offerWatcher.js +++ b/packages/smart-wallet/src/offerWatcher.js @@ -235,7 +235,9 @@ export const prepareOfferWatcher = baggage => { */ onRejected(err, seat) { const { facets } = this; - void watchForNumWants(facets, seat); + if (isUpgradeDisconnection(err)) { + void watchForNumWants(facets, seat); + } }, }, }, diff --git a/packages/smart-wallet/src/smartWallet.js b/packages/smart-wallet/src/smartWallet.js index d5f89e0b6ab..c6c1cb37bad 100644 --- a/packages/smart-wallet/src/smartWallet.js +++ b/packages/smart-wallet/src/smartWallet.js @@ -13,7 +13,7 @@ import { objectMap, StorageNodeShape, } from '@agoric/internal'; -import { observeNotifier } from '@agoric/notifier'; +import { isUpgradeDisconnection } from '@agoric/internal/src/upgrade-api.js'; import { M, mustMatch } from '@agoric/store'; import { appendToStoredArray, @@ -22,8 +22,10 @@ import { import { makeScalarBigMapStore, makeScalarBigWeakMapStore, + prepareExoClass, prepareExoClassKit, provide, + watchPromise, } from '@agoric/vat-data'; import { prepareRecorderKit, @@ -271,6 +273,57 @@ export const prepareSmartWallet = (baggage, shared) => { const makeOfferWatcher = prepareOfferWatcher(baggage); + const NotifierShape = M.remotable(); + const updateShape = { + value: AmountShape, + updateCount: M.bigint(), + }; + const amountWatcherGuard = M.interface('paymentWatcher', { + onFulfilled: M.call(updateShape, NotifierShape).returns(), + onRejected: M.call(M.any(), NotifierShape).returns(M.promise()), + }); + const prepareAmountWatcher = () => + prepareExoClass( + baggage, + 'AmountWatcher', + amountWatcherGuard, + /** + * @param {Purse} purse + * @param {ReturnType['helper']} helper + */ + (purse, helper) => ({ purse, helper }), + { + /** + * @param {{ value: Amount, updateCount: bigint | undefined }} updateRecord + * @param { Notifier } notifier + * @returns {void} + */ + onFulfilled(updateRecord, notifier) { + const { helper, purse } = this.state; + helper.updateBalance(purse, updateRecord.value); + helper.watchNextBalance( + this.self, + notifier, + updateRecord.updateCount, + ); + }, + /** + * @param {unknown} err + * @returns {Promise} + */ + onRejected(err) { + const { helper, purse } = this.state; + if (isUpgradeDisconnection(err)) { + return helper.watchPurse(purse); // retry + } + helper.logWalletError(`failed amount observer`, err); + throw err; + }, + }, + ); + + const makeAmountWatcher = prepareAmountWatcher(); + /** * @param {UniqueParams} unique * @returns {State} @@ -356,7 +409,8 @@ export const prepareSmartWallet = (baggage, shared) => { .returns(M.promise()), publishCurrentState: M.call().returns(), watchPurse: M.call(M.eref(PurseShape)).returns(M.promise()), - repairUnwatchedSeats: M.call().returns(M.promise()), + watchNextBalance: M.call(M.any(), NotifierShape, M.bigint()).returns(), + repairUnwatchedSeats: M.call().returns(), updateStatus: M.call(M.any()).returns(), addContinuingOffer: M.call( M.or(M.number(), M.string()), @@ -476,32 +530,23 @@ export const prepareSmartWallet = (baggage, shared) => { /** @type {(purse: ERef) => Promise} */ async watchPurse(purseRef) { - const { facets } = this; - const purse = await purseRef; // promises don't fit in durable storage const { helper } = this.facets; // publish purse's balance and changes - void E.when( - E(purse).getCurrentAmount(), - balance => helper.updateBalance(purse, balance), - err => - facets.helper.logWalletError( - 'initial purse balance publish failed', - err, - ), - ); - void observeNotifier(E(purse).getCurrentAmountNotifier(), { - updateState(balance) { - helper.updateBalance(purse, balance); - }, - fail(reason) { - facets.helper.logWalletError( - '⚠️ failed updateState observer', - reason, - ); - }, - }); + // This would seem to fit the observeNotifier() pattern, + // but purse notifiers are not necessarily durable. + // If there is an error due to upgrade, retry watchPurse(). + const notifier = await E(purse).getCurrentAmountNotifier(); + + const handler = makeAmountWatcher(purse, helper); + const startP = E(notifier).getUpdateSince(undefined); + watchPromise(startP, handler, notifier); + }, + + watchNextBalance(handler, notifier, updateCount) { + const nextP = E(notifier).getUpdateSince(updateCount); + watchPromise(nextP, handler, notifier); }, /** @@ -902,7 +947,11 @@ export const prepareSmartWallet = (baggage, shared) => { } catch (err) { facets.helper.logWalletError('OFFER ERROR:', err); // Notify the user - if (watcher) { + + if (err.upgradeMessage === 'vat upgraded') { + // The offer watchers will reconnect. Don't reclaim or exit + return; + } else if (watcher) { watcher.helper.updateStatus({ error: err.toString() }); } else { facets.helper.updateStatus({