Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: watch ERTP purse balances across zoe upgrades (release branch) #8557

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
58 changes: 37 additions & 21 deletions packages/smart-wallet/src/smartWallet.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
PurseShape,
} from '@agoric/ertp';
import { StorageNodeShape, makeTracer } 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,
Expand All @@ -27,6 +27,7 @@ import {
prepareRecorderKit,
} from '@agoric/zoe/src/contractSupport/index.js';
import { E } from '@endo/far';

import { makeInvitationsHelper } from './invitations.js';
import { makeOfferExecutor } from './offers.js';
import { shape } from './typeGuards.js';
Expand All @@ -36,6 +37,24 @@ const { Fail, quote: q } = assert;

const trace = makeTracer('SmrtWlt');

/**
* Like `subscribeLatest` but that swallows the upgrade error that this module needs to detect and handle.
*
* @template T
* @param {Notifier<T>} notifierP
*/
async function* subscribeLatestSimple(notifierP) {
let lastUpdate;
await null; // for jessie
while (true) {
const updateRecord =
// eslint-disable-next-line no-await-in-loop
await E(notifierP).getUpdateSince(lastUpdate);
lastUpdate = updateRecord.updateCount;
yield updateRecord.value;
}
}

/**
* @file Smart wallet module
*
Expand Down Expand Up @@ -418,29 +437,26 @@ export const prepareSmartWallet = (baggage, shared) => {
/** @type {(purse: ERef<RemotePurse>) => Promise<void>} */
async watchPurse(purseRef) {
const { address } = this.state;
const { helper } = this.facets;

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 =>
console.error(
address,
'initial purse balance publish failed',
err,
),
);
void observeNotifier(E(purse).getCurrentAmountNotifier(), {
updateState(balance) {
helper.updateBalance(purse, balance);
},
fail(reason) {
console.error(address, `failed updateState observer`, reason);
},
});
// This would seem to fit the observeNotifier() pattern,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valuable comment

// but purse notifiers are not (always) durable.
// If there is an error due to upgrade, retry watchPurse().
const notifier = E(purse).getCurrentAmountNotifier();
try {
// eslint-disable-next-line no-await-in-loop
for await (const newBalance of subscribeLatestSimple(notifier)) {
helper.updateBalance(purse, newBalance);
}
} catch (err) {
if (isUpgradeDisconnection(err)) {
helper.watchPurse(purse); // retry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No return? Not sure who is the initial caller but this looks like a tail recursion, and it feels weird without a return.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually thinking about this more, this seem too simplistic, and doesn't handle the same disconnection error being replayed, which is why tools like reconnectAsNeeded exist which check the incarnation in the disconnect error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't handle the same disconnection error being replayed

What am I missing? The way I understand things, the test is compelling evidence that it does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is in case of a proxy between the notifier in the upgraded vat, and the subscriber. It's possible that this is not the case here, but you could get in an infinite loop if the subscription request somehow rejects with the same disconnect error. The increasing incarnation number is there to help differentiate the case of replayed disconnect error, and should be checked when re-attempting subscription.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No return? Not sure who is the initial caller but this looks like a tail recursion, and it feels weird without a return.

Agreed. #8573 has a return.

@dckc which PR should be reviewed? I support this one first and the port be in draft until this lands, so that it can match whatever changes are made here

}
console.error(`*** ${address} failed amount observer, ${err} ***`);
throw err;
}
},

/**
Expand Down
165 changes: 165 additions & 0 deletions packages/vats/test/bootstrapTests/test-wallet-upgrade.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// @ts-check
import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';
import { E } from '@endo/far';
import { eventLoopIteration } from '@agoric/notifier/tools/testSupports.js';
import { makeAgoricNamesRemotesFromFakeStorage } from '../../tools/board-utils.js';
import { makeWalletFactoryDriver } from './drivers.js';
import { makeSwingsetTestKit } from './supports.js';

const { Fail } = assert;

/**
* @type {import('ava').TestFn<
* Awaited<ReturnType<typeof makeTestContext>>
* >}
*/
const test = anyTest;

// main/production config doesn't have initialPrice, upon which 'open vaults' depends
const PLATFORM_CONFIG = '@agoric/vats/decentral-itest-vaults-config.json';

const makeTestContext = async t => {
const swingsetTestKit = await makeSwingsetTestKit(t, '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)));

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;
};

const sendInvitationScript = () => {
const addr = 'agoric1oracle-operator';
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;
};

test('update purse balance across upgrade', async t => {
const oraAddr = 'agoric1oracle-operator';
const { walletFactoryDriver, agoricNamesRemotes } = t.context;
t.log('provision a smartWallet for an oracle operator');
const oraWallet = await walletFactoryDriver.provideSmartWallet(oraAddr);

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<import('../../src/types.js').BridgeHandler>} */
const coreEvalBridgeHandler = await EV.vat('bootstrap').consumeItem(
'coreEvalBridgeHandler',
);

const runCoreEval = async evals => {
const proposal = { evals };
const bridgeMessage = {
type: 'CORE_EVAL',
evals: proposal.evals,
};
await EV(coreEvalBridgeHandler).fromBridge(bridgeMessage);
};

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})()`,
},
]);

const current = oraWallet.getCurrentWalletRecord();
t.log(
'invitation balance after sending invitation',
findPurse(current).balance,
);
t.notDeepEqual(findPurse(current).balance.value, [], 'invitation set');
});
Loading