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

smart wallet report rejections of durable promises #8998

Merged
merged 4 commits into from
Feb 27, 2024
Merged
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
55 changes: 46 additions & 9 deletions packages/smart-wallet/src/offerWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ const offerWatcherGuard = harden({
.optional(M.record())
.returns(),
publishResult: M.call(M.any()).returns(),
handleError: M.call(M.error()).returns(),
}),
paymentWatcher: M.interface('paymentWatcher', {
onFulfilled: M.call(PaymentPKeywordRecordShape, SeatShape).returns(
Expand Down Expand Up @@ -131,6 +132,9 @@ export const prepareOfferWatcher = baggage => {
}),
{
helper: {
/**
* @param {Record<string, unknown>} offerStatusUpdates
*/
updateStatus(offerStatusUpdates) {
const { state } = this;
state.status = harden({ ...state.status, ...offerStatusUpdates });
Expand All @@ -153,6 +157,7 @@ export const prepareOfferWatcher = baggage => {
);
},

/** @param {unknown} result */
publishResult(result) {
const { state, facets } = this;

Expand All @@ -169,13 +174,15 @@ export const prepareOfferWatcher = baggage => {
facets.helper.updateStatus({ result });
break;
case 'copyRecord':
// @ts-expect-error narrowed by passStyle
if ('invitationMakers' in result) {
// save for continuing invitation offer

void facets.helper.onNewContinuingOffer(
String(state.status.id),
state.invitationAmount,
result.invitationMakers,
// @ts-expect-error narrowed by passStyle
result.publicSubscribers,
);
}
Expand All @@ -186,6 +193,22 @@ export const prepareOfferWatcher = baggage => {
facets.helper.updateStatus({ result: UNPUBLISHED_RESULT });
}
},
/**
* Called when the offer result promise rejects. The other two watchers
* are waiting for particular values out of Zoe but they settle at the same time
* and don't need their own error handling.
Comment on lines +197 to +199
Copy link
Member

@gibson042 gibson042 Feb 27, 2024

Choose a reason for hiding this comment

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

Suggested change
* Called when the offer result promise rejects. The other two watchers
* are waiting for particular values out of Zoe but they settle at the same time
* and don't need their own error handling.
* Called when the offer result promise rejects. The payment and
* numWantsSatisfied promises settle at the same time and don't need
* their own error handling.

* @param {Error} err
*/
handleError(err) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment this will likely be called multiple times (aka it needs to handle parallels duplicate calls)

Copy link
Member

Choose a reason for hiding this comment

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

I don't get "parallel". Did you mean just "handle duplicate calls?"

Copy link
Member

Choose a reason for hiding this comment

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

There will be duplicate call, and since the async operation this initiates is not awaited, they will overlap

const { facets } = this;
facets.helper.updateStatus({ error: err.toString() });
const { seatRef } = this.state;
void E.when(E(seatRef).hasExited(), hasExited => {
if (!hasExited) {
void E(seatRef).tryExit();
}
});
},
},

/** @type {OutcomeWatchers['paymentWatcher']} */
Expand All @@ -202,13 +225,17 @@ export const prepareOfferWatcher = baggage => {
facets.helper.updateStatus({ payouts: amounts });
},
/**
* @param {Error} err
* If promise disconnected, watch again. Or if there's an Error, handle it.
*
* @param {Error | import('@agoric/internal/src/upgrade-api.js').UpgradeDisconnection} reason
* @param {UserSeat} seat
*/
onRejected(err, seat) {
onRejected(reason, seat) {
const { facets } = this;
if (isUpgradeDisconnection(err)) {
if (isUpgradeDisconnection(reason)) {
void watchForPayout(facets, seat);
} else {
facets.helper.handleError(reason);
}
},
},
Expand All @@ -220,13 +247,17 @@ export const prepareOfferWatcher = baggage => {
facets.helper.publishResult(result);
},
/**
* @param {Error} err
* If promise disconnected, watch again. Or if there's an Error, handle it.
*
* @param {Error | import('@agoric/internal/src/upgrade-api.js').UpgradeDisconnection} reason
* @param {UserSeat} seat
*/
onRejected(err, seat) {
onRejected(reason, seat) {
const { facets } = this;
if (isUpgradeDisconnection(err)) {
if (isUpgradeDisconnection(reason)) {
void watchForOfferResult(facets, seat);
} else {
facets.helper.handleError(reason);
}
},
},
Expand All @@ -239,12 +270,18 @@ export const prepareOfferWatcher = baggage => {
facets.helper.updateStatus({ numWantsSatisfied: numSatisfied });
},
/**
* @param {Error} err
* If promise disconnected, watch again.
*
* Errors are handled by the paymentWatcher because numWantsSatisfied()
* and getPayouts() settle the same (they await the same promise and
* then synchronously return a local value).
*
* @param {Error | import('@agoric/internal/src/upgrade-api.js').UpgradeDisconnection} reason
* @param {UserSeat} seat
*/
onRejected(err, seat) {
onRejected(reason, seat) {
const { facets } = this;
if (isUpgradeDisconnection(err)) {
if (isUpgradeDisconnection(reason)) {
void watchForNumWants(facets, seat);
}
},
Expand Down
41 changes: 25 additions & 16 deletions packages/smart-wallet/src/smartWallet.js
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ export const prepareSmartWallet = (baggage, shared) => {
}),
};

// TODO move to top level so its type can be exported
/**
* Make the durable object to return, but taking some parameters that are awaited by a wrapping function.
* This is necessary because the class kit construction helpers, `initState` and `finish` run synchronously
Expand Down Expand Up @@ -853,6 +854,10 @@ export const prepareSmartWallet = (baggage, shared) => {

payments: {
/**
* Withdraw the offered amount from the appropriate purse of this wallet.
*
* Save its amount in liveOfferPayments in case we need to reclaim the payment.
*
* @param {AmountKeywordRecord} give
* @param {OfferId} offerId
* @returns {PaymentPKeywordRecord}
Expand All @@ -868,8 +873,8 @@ export const prepareSmartWallet = (baggage, shared) => {
.getLiveOfferPayments()
.init(offerId, brandPaymentRecord);

// Add each payment to liveOfferPayments as it is withdrawn. If
// there's an error partway through, we can recover the withdrawals.
// Add each payment amount to brandPaymentRecord as it is withdrawn. If
// there's an error later, we can use it to redeposit the correct amount.
return objectMap(give, amount => {
/** @type {Promise<Purse>} */
const purseP = facets.helper.purseForBrand(amount.brand);
Expand All @@ -890,19 +895,27 @@ export const prepareSmartWallet = (baggage, shared) => {
});
},

/**
* Find the live payments for the offer and deposit them back in the appropriate purses.
*
* @param {OfferId} offerId
* @returns {Promise<void>}
*/
async tryReclaimingWithdrawnPayments(offerId) {
const { facets } = this;

await null;

const liveOfferPayments = facets.helper.getLiveOfferPayments();
if (liveOfferPayments.has(offerId)) {
const brandPaymentRecord = liveOfferPayments.get(offerId);
if (!brandPaymentRecord) {
return Promise.resolve(undefined);
return;
}
// Use allSettled to ensure we attempt all the deposits, regardless of
// individual rejections.
return Promise.allSettled(
Array.from(brandPaymentRecord.entries()).map(async ([b, p]) => {
await Promise.allSettled(
Array.from(brandPaymentRecord.entries()).map(([b, p]) => {
// Wait for the withdrawal to complete. This protects against a
// race when updating paymentToPurse.
const purseP = facets.helper.purseForBrand(b);
Expand Down Expand Up @@ -990,21 +1003,23 @@ export const prepareSmartWallet = (baggage, shared) => {
// await so that any errors are caught and handled below
await watchOfferOutcomes(watcher, seatRef);
} catch (err) {
facets.helper.logWalletError('OFFER ERROR:', err);
// This block only runs if the block above fails during one vat incarnation.
facets.helper.logWalletError('IMMEDIATE OFFER ERROR:', err);

// Notify the user
// Update status to observers
if (err.upgradeMessage === 'vat upgraded') {
Copy link
Member

Choose a reason for hiding this comment

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

@Chris-Hibbert I just checked again, and this is the only non-test check manually using upgradeMessage rather than the isUpgradeDisconnection. I'm satisfied enough at our analysis that this is harmless to request it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@turadg, since you're still making changes, would you mind updating this?

Suggested change
if (err.upgradeMessage === 'vat upgraded') {
if (isUpgradeDisconnection(err)) {

Copy link
Member Author

@turadg turadg Feb 27, 2024

Choose a reason for hiding this comment

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

oh yeah, I meant to but I don't think it's worth spinning CI at this point

// The offer watchers will reconnect. Don't reclaim or exit
return;
} else if (watcher) {
watcher.helper.updateStatus({ error: err.toString() });
// The watcher's onRejected will updateStatus()
} else {
facets.helper.updateStatus({
error: err.toString(),
...offerSpec,
});
}

// Backstop recovery, in case something very basic fails.
if (offerSpec?.proposal?.give) {
facets.payments
.tryReclaimingWithdrawnPayments(offerSpec.id)
Comment on lines 1023 to 1025
Copy link
Member

Choose a reason for hiding this comment

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

So we assume this case only matters for immediate rejections from the seat?

I suppose that tryReclaimingWithdrawnPayments is not very idempotent, nor parallel safe.

Copy link
Member

Choose a reason for hiding this comment

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

"parallel"?

Noting that at #8488 I see

along the way payments.tryReclaimingWithdrawnPayments() went away. I'm pretty sure it was redundant with ERTP recoverySets.

Expand All @@ -1016,14 +1031,8 @@ export const prepareSmartWallet = (baggage, shared) => {
);
}

if (seatRef) {
void E.when(E(seatRef).hasExited(), hasExited => {
if (!hasExited) {
void E(seatRef).tryExit();
}
});
}

// XXX tests rely on throwing immediate errors, not covering the
// error handling in the event the failure is after an upgrade
throw err;
}
},
Expand Down
Loading