Skip to content

Commit

Permalink
refactor: clean-ups prompted by failing regression tests
Browse files Browse the repository at this point in the history 
A test cued me that between giveWanted and
tryReclaimingWithdrawnPayments, it's crucial that we save state as we
go, so if there's a problem later, the earlier withdrawals will be
reclaimed.
  • Loading branch information
@Chris-Hibbert
Chris-Hibbert committed Nov 8, 2023
1 parent 9d2d54b commit fb03e29
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 36 deletions.
4 changes: 3 additions & 1 deletion packages/inter-protocol/src/price/roundsManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,10 @@ export const prepareRoundsManagerKit = baggage =>
);
}

if (status.lastReportedRound >= roundId)
if (status.lastReportedRound >= roundId) {
return 'cannot report on previous rounds';
}

if (
roundId !== reportingRoundId &&
roundId !== add(reportingRoundId, 1) &&
Expand Down
7 changes: 2 additions & 5 deletions packages/inter-protocol/test/smartWallet/test-psm-integration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,6 @@ test('want stable (insufficient funds)', async t => {
'Withdrawal of {"brand":"[Alleged: AUSD brand]","value":"[20000n]"} failed because the purse only contained {"brand":"[Alleged: AUSD brand]","value":"[10000n]"}';
const status = computedState.offerStatuses.get('insufficientFunds');
t.is(status?.error, `Error: ${msg}`);
/** @type {[PromiseRejectedResult]} */
// @ts-expect-error cast
const result = status.result;
t.is(result[0].status, 'rejected');
t.is(result[0].reason.message, msg);
});

test('govern offerFilter', async t => {
Expand Down Expand Up @@ -384,6 +379,8 @@ test('deposit multiple payments to unknown brand', async t => {
}
});

// related to recovering dropped Payments

// XXX belongs in smart-wallet package, but needs lots of set-up that's handy here.
test('recover when some withdrawals succeed and others fail', async t => {
const { fromEntries } = Object;
Expand Down
20 changes: 10 additions & 10 deletions packages/smart-wallet/src/offerWatcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
SeatShape,
} from '@agoric/zoe/src/typeGuards.js';
import { AmountShape } from '@agoric/store/test/borrow-guards.js';
import { objectMap } from '@agoric/internal';
import { objectMap, deeplyFulfilledObject } from '@agoric/internal';

/** Value for "result" field when the result can't be published */
export const UNPUBLISHED_RESULT = 'UNPUBLISHED';
Expand Down Expand Up @@ -59,7 +59,9 @@ const offerWatcherGuard = harden({
exitOpenSeats: M.call(M.any()).returns(),
}),
paymentWatcher: M.interface('paymentWatcher', {
onFulfilled: M.call(PaymentPKeywordRecordShape, SeatShape).returns(),
onFulfilled: M.call(PaymentPKeywordRecordShape, SeatShape).returns(
M.promise(),
),
onRejected: M.call(M.any(), SeatShape).returns(),
}),
resultWatcher: M.interface('resultWatcher', {
Expand Down Expand Up @@ -158,18 +160,16 @@ export const makeOfferWatcherMaker = baggage => {
},

paymentWatcher: {
onFulfilled(payouts) {
async onFulfilled(payouts) {
const { state, facets } = this;

// This will block until all payouts succeed, but user will be updated
// as each payout will trigger its corresponding purse notifier.
objectMap(payouts, paymentRef =>
E.when(paymentRef, payment =>
state.deposit.receive(payment).then(amountsOrDeferred => {
facets.helper.updateStatus({ payouts: amountsOrDeferred });
}),
),
// since each payout will trigger its corresponding purse notifier.
const amountPKeywordRecord = objectMap(payouts, paymentRef =>
E.when(paymentRef, payment => state.deposit.receive(payment)),
);
const amounts = await deeplyFulfilledObject(amountPKeywordRecord);
facets.helper.updateStatus({ payouts: amounts });
},
onRejected(reason, seat) {
const { facets } = this;
Expand Down
90 changes: 71 additions & 19 deletions packages/smart-wallet/src/smartWallet.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
AmountKeywordRecordShape,
PaymentPKeywordRecordShape,
} from '@agoric/zoe/src/typeGuards.js';

import { makeInvitationsHelper } from './invitations.js';
import { shape } from './typeGuards.js';
import { objectMapStoragePath } from './utils.js';
Expand Down Expand Up @@ -175,6 +176,7 @@ const trace = makeTracer('SmrtWlt');
* currentRecorderKit: import('@agoric/zoe/src/contractSupport/recorder.js').RecorderKit<CurrentWalletRecord>,
* liveOffers: MapStore<OfferId, import('./offerWatcher.js').OfferStatus>,
* liveOfferSeats: MapStore<OfferId, UserSeat<unknown>>,
* liveOfferPayments: MapStore<OfferId, MapStore<Brand, Payment>>,
* }>} ImmutableState
*
* @typedef {BrandDescriptor & { purse: Purse }} PurseRecord
Expand Down Expand Up @@ -231,7 +233,7 @@ const getBrandToPurses = (walletPurses, key) => {
return brandToPurses;
};

const REPAIRED_UNWATCHED_SEATS = 'true';
const REPAIRED_UNWATCHED_SEATS = 'repairedUnwatchedSeats';

/**
* @param {import('@agoric/vat-data').Baggage} baggage
Expand Down Expand Up @@ -328,6 +330,9 @@ export const prepareSmartWallet = (baggage, shared) => {
liveOfferSeats: makeScalarBigMapStore('live offer seats', {
durable: true,
}),
liveOfferPayments: makeScalarBigMapStore('live offer payments', {
durable: true,
}),
};

return {
Expand Down Expand Up @@ -357,14 +362,15 @@ export const prepareSmartWallet = (baggage, shared) => {
).returns(M.promise()),
purseForBrand: M.call(BrandShape).returns(M.promise()),
}),

deposit: M.interface('depositFacetI', {
receive: M.callWhen(M.await(M.eref(PaymentShape))).returns(AmountShape),
}),
payments: M.interface('payments support', {
withdrawGive: M.call(AmountKeywordRecordShape).returns(
PaymentPKeywordRecordShape,
),
depositPayouts: M.call(PaymentPKeywordRecordShape).returns(M.promise()),
tryReclaimingWithdrawnPayments: M.call(M.string()).returns(M.promise()),
}),
offers: M.interface('offers facet', {
executeOffer: M.call(shape.OfferSpec).returns(M.promise()),
Expand Down Expand Up @@ -405,13 +411,15 @@ export const prepareSmartWallet = (baggage, shared) => {
const {
liveOffers,
liveOfferSeats,
liveOfferPayments,
offerToInvitationMakers,
offerToPublicSubscriberPaths,
offerToUsedInvitation,
} = this.state;
const used =
liveOffers.has(id) ||
liveOfferSeats.has(id) ||
liveOfferPayments.has(id) ||
offerToInvitationMakers.has(id) ||
offerToPublicSubscriberPaths.has(id) ||
offerToUsedInvitation.has(id);
Expand Down Expand Up @@ -554,6 +562,7 @@ export const prepareSmartWallet = (baggage, shared) => {
if (baggage.has(REPAIRED_UNWATCHED_SEATS)) {
return;
}
baggage.init(REPAIRED_UNWATCHED_SEATS, true);

const invitationFromSpec = makeInvitationsHelper(
zoe,
Expand Down Expand Up @@ -594,6 +603,10 @@ export const prepareSmartWallet = (baggage, shared) => {
state.liveOfferSeats.delete(offerStatus.id);
}

if (state.liveOfferPayments.has(offerStatus.id)) {
state.liveOfferPayments.delete(offerStatus.id);
}

if (state.liveOffers.has(offerStatus.id)) {
state.liveOffers.delete(offerStatus.id);
facets.helper.publishCurrentState();
Expand Down Expand Up @@ -694,28 +707,58 @@ export const prepareSmartWallet = (baggage, shared) => {
payments: {
/**
* @param {AmountKeywordRecord} give
* @param {OfferId} offerId
* @returns {PaymentPKeywordRecord}
*/
withdrawGive(give) {
const { facets } = this;
withdrawGive(give, offerId) {
const { state, facets } = this;

/** @type {MapStore<Brand, Payment>} */
const brandPaymentRecord = makeScalarBigMapStore('paymentToBrand', {
durable: true,
});
state.liveOfferPayments.init(offerId, brandPaymentRecord);

// Add each payment to liveOfferPayments as it is withdrawn. If
// there's an error partway through, we can recover the withdrawals.
return objectMap(give, amount => {
/** @type {Promise<Purse>} */
const purseP = facets.helper.purseForBrand(amount.brand);
return E(purseP).withdraw(amount);
const paymentP = E(purseP).withdraw(amount);
void E.when(
paymentP,
payment => brandPaymentRecord.init(amount.brand, payment),
e => {
// recovery will be handled by tryReclaimingWithdrawnPayments()
console.log(`Payment withdrawal failed.`, offerId, e);
},
);
return paymentP;
});
},

/**
* @param {PaymentPKeywordRecord} payouts
* @returns {Promise<AmountKeywordRecord>} amounts for deferred deposits will be empty
*/
async depositPayouts(payouts) {
const { facets } = this;
/** Record<string, Promise<Amount>> */
const amountPKeywordRecord = objectMap(payouts, paymentRef =>
E.when(paymentRef, payment => facets.deposit.receive(payment)),
);
return deeplyFulfilledObject(amountPKeywordRecord);
async tryReclaimingWithdrawnPayments(offerId) {
const { state, facets } = this;
const { liveOfferPayments } = state;

if (liveOfferPayments.has(offerId)) {
const brandPaymentRecord = liveOfferPayments.get(offerId);
if (!brandPaymentRecord) {
return Promise.resolve(undefined);
}
// Use allSettled to ensure we attempt all the deposits, regardless of
// individual rejections.
return Promise.allSettled(
Array.from(brandPaymentRecord.entries()).map(async ([b, p]) => {
// Wait for the withdrawal to complete. This protects against a
// race when updating paymentToPurse.
const purseP = facets.helper.purseForBrand(b);

// Now send it back to the purse.
return E(purseP).deposit(p);
}),
);
}
},
},

Expand All @@ -735,6 +778,8 @@ export const prepareSmartWallet = (baggage, shared) => {

facets.helper.assertUniqueOfferId(String(offerSpec.id));

await null;

let seatRef;
let watcher;
try {
Expand All @@ -761,7 +806,7 @@ export const prepareSmartWallet = (baggage, shared) => {
const [paymentKeywordRecord, invitationAmount] = await Promise.all([
proposal?.give &&
deeplyFulfilledObject(
facets.payments.withdrawGive(proposal.give),
facets.payments.withdrawGive(proposal.give, offerSpec.id),
),
E(invitationIssuer).getAmountOf(invitation),
]);
Expand Down Expand Up @@ -795,7 +840,6 @@ export const prepareSmartWallet = (baggage, shared) => {
} catch (err) {
console.error('OFFER ERROR:', err);
// Notify the user
debugger;
if (watcher) {
watcher.helper.updateStatus({ error: err.toString() });
} else {
Expand All @@ -808,8 +852,16 @@ export const prepareSmartWallet = (baggage, shared) => {
);
}

if (offerSpec?.proposal?.give) {
facets.payments
.tryReclaimingWithdrawnPayments(offerSpec.id)
.catch(e =>
console.error('recovery failed reclaiming payments', e),
);
}

if (seatRef) {
E.when(E(seatRef).hasExited(), hasExited => {
void E.when(E(seatRef).hasExited(), hasExited => {
if (!hasExited) {
void E(seatRef).tryExit();
}
Expand Down
2 changes: 1 addition & 1 deletion packages/smart-wallet/src/walletFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export const makeAssetRegistry = assetPublisher => {
* }} WalletReviver
*/

// NB: even though all the wallets share this contract, they
// NB: even though all the wallets share this contract,
// 1. they should not rely on that; they may be partitioned later
// 2. they should never be able to detect behaviors from another wallet
/**
Expand Down

0 comments on commit fb03e29

Please sign in to comment.