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.

Sign up for GitHub

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: stop accepting offers if zcf.shutdown is called #1772

Merged
merged 1 commit into from
Sep 16, 2020
Merged

fix: stop accepting offers if zcf.shutdown is called #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions packages/zoe/src/internal-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@
* @property {() => IssuerKeywordRecord} getIssuers
* @property {() => BrandKeywordRecord} getBrands
* @property {() => Object} getTerms
* @property {() => boolean} hasShutdown
* @property {() => void} shutdown
*/

/**
Expand Down
23 changes: 14 additions & 9 deletions packages/zoe/src/zoeService/zoe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
const makeInstanceAdmin = () => {
/** @type {Set<ZoeSeatAdmin>} */
const zoeSeatAdmins = new Set();
let hasShutdown = false;

/** @type {InstanceAdmin} */
return {
Expand All @@ -234,8 +235,11 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
getIssuers: () => instanceRecord.terms.issuers,
getBrands: () => instanceRecord.terms.brands,
getInstance: () => instance,
exitAllSeats: () =>
zoeSeatAdmins.forEach(zoeSeatAdmin => zoeSeatAdmin.exit()),
hasShutdown: () => hasShutdown,
shutdown: () => {
hasShutdown = true;
zoeSeatAdmins.forEach(zoeSeatAdmin => zoeSeatAdmin.exit());
},
};
};

Expand All @@ -246,8 +250,8 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
E(adminNode)
.done()
.then(
() => instanceAdmin.exitAllSeats(),
() => instanceAdmin.exitAllSeats(),
() => instanceAdmin.shutdown(),
() => instanceAdmin.shutdown(),
Copy link
Member

Choose a reason for hiding this comment

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

On seats, we have exit vs kickOut to distinguish between exiting for "normal" reasons vs exiting because of a problem. Such problems include circumstances in addition to the contract calling kickOut. For example, if an offerHandler's returned promise rejects, the seat is kicked out with the rejection reason, which is good.

IIUC, if the zcf vat crashes, the rejection handler above is called with an error representing the reason for crash. But the resulting shutdown ignores this reason and exits all the seats as if by exit rather than as if by kickOut. This is a false-successful-exit inconsistency.

Going the other way, on an explicit call to zcf.shutdown(), all the seats already created exit as if by exit rather than kickOut, which seems good. But any offer made after shutdown causes the offer to be rejected. We might view the later as a false-problematic-exit inconsistency. Or at least race condition.

I suggest that we fix the first inconsistency. But I suggest we document rather than "fix" the latter race condition, as I don't see a fix that's actually better than the current behavior.

I note that the one fix I suggest, to the rejection-handler behavior above, is not new as of this PR. Therefore, it is ok if it is not fixed as of this PR.

When we do address these, we may also want to introduce a shutdown variant that the contract can explicitly call to say "I'm shutting down abnormally because of the following problem", where that variant would also result in a kickOut of all seats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds good to me. The shutdown variant you describe should be a good fix for the first issue, in that we can hook it up to the rejection handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created here: #1774

);

// Unpack the invitationKit.
Expand Down Expand Up @@ -303,7 +307,7 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
return { userSeat, notifier, zoeSeatAdmin };
},
shutdown: () => {
instanceAdmin.exitAllSeats();
instanceAdmin.shutdown();
E(adminNode).terminate();
},
makeZoeMint,
Expand Down Expand Up @@ -368,6 +372,11 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
invitationAmount.value.length === 1,
'Only one invitation can be redeemed at a time',
);
const {
value: [{ instance, handle: invitationHandle }],
} = invitationAmount;
const instanceAdmin = instanceToInstanceAdmin.get(instance);
assert(!instanceAdmin.hasShutdown(), `No further offers are accepted`);

const proposal = cleanProposal(getAmountMath, uncleanProposal);
const { give, want } = proposal;
Expand All @@ -390,9 +399,6 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
return getAmountMath(proposal.want[keyword].brand).getEmpty();
}
});
const {
value: [{ instance, handle: invitationHandle }],
} = invitationAmount;

return Promise.all(paymentDepositedPs).then(amountsArray => {
const initialAllocation = arrayToObj(amountsArray, proposalKeywords);
Expand All @@ -403,7 +409,6 @@ function makeZoe(vatAdminSvc, zcfBundleName = undefined) {
const exitObjPromiseKit = makePromiseKit();
// Don't trigger Node.js's UnhandledPromiseRejectionWarning
exitObjPromiseKit.promise.catch(_ => {});
const instanceAdmin = instanceToInstanceAdmin.get(instance);
const seatHandle = makeHandle('SeatHandle');

const { userSeat, notifier, zoeSeatAdmin } = makeZoeSeatAdminKit(
Expand Down
4 changes: 1 addition & 3 deletions packages/zoe/test/unitTests/zcf/test-zcf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,9 +1218,7 @@ test(`zcf.shutdown - zcfSeat exits`, async t => {
t.truthy(await E(userSeat).hasExited());
});

// TODO: Any new offers should throw after zcf.shutdown() is called
// https://github.com/Agoric/agoric-sdk/issues/1756
test.failing(`zcf.shutdown - no further offers accepted`, async t => {
test(`zcf.shutdown - no further offers accepted`, async t => {
const { zoe, zcf } = await setupZCFTest({});
const invitation = await zcf.makeInvitation(() => {}, 'seat');
zcf.shutdown();
Expand Down