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

feat(smartWallet): defer deposits until purse available #6172

Merged
merged 4 commits into from
Sep 10, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
8 changes: 5 additions & 3 deletions packages/smart-wallet/src/offers.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const UNPUBLISHED_RESULT = 'UNPUBLISHED';
/**
* @param {object} opts
* @param {ERef<ZoeService>} opts.zoe
* @param {{ receive: (payment: *) => Promise<Amount> }} opts.depositFacet
* @param {object} opts.powers
* @param {import('./types').Cell<number>} opts.powers.lastOfferId
* @param {(spec: import('./invitations').InvitationSpec) => ERef<Invitation>} opts.powers.invitationFromSpec
Expand All @@ -37,6 +38,7 @@ export const UNPUBLISHED_RESULT = 'UNPUBLISHED';
*/
export const makeOfferExecutor = ({
zoe,
depositFacet,
powers,
onStatusChange,
onNewContinuingOffer,
Expand All @@ -52,7 +54,7 @@ export const makeOfferExecutor = ({
* @throws if any parts of the offer can be determined synchronously to be invalid
*/
async executeOffer(offerSpec) {
const paymentsManager = makePaymentsHelper(purseForBrand);
const paymentsManager = makePaymentsHelper(purseForBrand, depositFacet);

/** @type {OfferStatus} */
let status = {
Expand Down Expand Up @@ -144,8 +146,8 @@ export const makeOfferExecutor = ({
E.when(
E(seatRef).getPayouts(),
payouts =>
paymentsManager.depositPayouts(payouts).then(amounts => {
updateStatus({ payouts: amounts });
paymentsManager.depositPayouts(payouts).then(amountsOrDeferred => {
updateStatus({ payouts: amountsOrDeferred });
}),
handleError,
);
Expand Down
15 changes: 5 additions & 10 deletions packages/smart-wallet/src/payments.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import { E } from '@endo/far';
* Used in an offer execution to manage payments state safely.
*
* @param {(brand: Brand) => import('./types').RemotePurse} purseForBrand
* @param {{ receive: (payment: *) => Promise<Amount> }} depositFacet
*/
export const makePaymentsHelper = purseForBrand => {
export const makePaymentsHelper = (purseForBrand, depositFacet) => {
/** @type {PaymentPKeywordRecord | null} */
let keywordPaymentPromises = null;

Expand Down Expand Up @@ -74,20 +75,14 @@ export const makePaymentsHelper = purseForBrand => {
);
},

// TODO(PS0?) when there's not a purse for a brand, hold the payout and wait for a purse to deposit it into
// Cheaper alternative: before offer validate we have issuers for all the 'wants' so the results can be put into purses.
/**
*
* @param {PaymentPKeywordRecord} payouts
* @returns {Promise<AmountKeywordRecord>}
* @returns {Promise<AmountKeywordRecord>} amounts for deferred deposits will be empty
*/
async depositPayouts(payouts) {
/** @type {PaymentKeywordRecord} */
// @ts-expect-error ???
const paymentKeywordRecord = await deeplyFulfilledObject(payouts);
/** Record<string, Promise<Amount>> */
const amountPKeywordRecord = objectMap(paymentKeywordRecord, payment =>
E(purseForBrand(payment.getAllegedBrand())).deposit(payment),
const amountPKeywordRecord = objectMap(payouts, paymentRef =>
E.when(paymentRef, payment => depositFacet.receive(payment)),
Copy link
Member

Choose a reason for hiding this comment

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

👍 good idea to re-use the depositFacet

);
return deeplyFulfilledObject(amountPKeywordRecord);
},
Expand Down
59 changes: 45 additions & 14 deletions packages/smart-wallet/src/smartWallet.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @ts-check
import { AmountShape, PaymentShape } from '@agoric/ertp';
import { AmountMath, AmountShape, PaymentShape } from '@agoric/ertp';
import { isNat } from '@agoric/nat';
import {
makeStoredPublishKit,
Expand Down Expand Up @@ -75,6 +75,7 @@ const { details: X, quote: q } = assert;
* @typedef {Parameters<initState>[0] & Parameters<initState>[1]} HeldParams
*
* @typedef {Readonly<HeldParams & {
* paymentQueues: MapStore<Brand, Array<import('@endo/far').FarRef<Payment>>>,
* offerToInvitationMakers: MapStore<number, import('./types').RemoteInvitationMakers>,
* brandDescriptors: MapStore<Brand, BrandDescriptor>,
* brandPurses: MapStore<Brand, RemotePurse>,
Expand Down Expand Up @@ -120,14 +121,19 @@ export const initState = (unique, shared) => {
);

const preciousState = {
// Private purses. This assumes one purse per brand, which will be valid in MN-1 but not always.
brandPurses: makeScalarBigMapStore('brand purses', { durable: true }),
// Payments that couldn't be deposited when received.
// NB: vulnerable to uncapped growth by unpermissioned deposits.
paymentQueues: makeScalarBigMapStore('payments queues', {
durable: true,
}),
// Invitation makers yielded by offer results
offerToInvitationMakers: makeScalarBigMapStore('invitation makers', {
durable: true,
}),
// What purses have reported on construction and by getCurrentAmountNotifier updates.
purseBalances: makeScalarMapStore(),
// Private purses. This assumes one purse per brand, which will be valid in MN-1 but not always.
brandPurses: makeScalarBigMapStore('brand purses', { durable: true }),
};

const nonpreciousState = {
Expand Down Expand Up @@ -202,8 +208,13 @@ const behavior = {
/** @type {(desc: Omit<BrandDescriptor, 'displayInfo'>, purse: RemotePurse) => Promise<void>} */
async addBrand(desc, purseRef) {
/** @type {State} */
const { address, brandDescriptors, brandPurses, updatePublishKit } =
this.state;
const {
address,
brandDescriptors,
brandPurses,
paymentQueues,
updatePublishKit,
} = this.state;
// assert haven't received this issuer before.
const descriptorsHas = brandDescriptors.has(desc.brand);
const pursesHas = brandPurses.has(desc.brand);
Expand Down Expand Up @@ -248,27 +259,46 @@ const behavior = {
});

updatePublishKit.publisher.publish({ updated: 'brand', descriptor });

// deposit queued payments
const payments = paymentQueues.has(desc.brand)
? paymentQueues.get(desc.brand)
: [];
const deposits = payments.map(p =>
// @ts-expect-error deposit does take a FarRef<Payment>
E(purse).deposit(p),
);
Promise.all(deposits).catch(err =>
console.error('ERROR depositing queued payments', err),
);
},
},
/**
* Similar to {DepositFacet} but async because it has to look up the purse.
*/
// TODO(PS0) decide whether to match canonical `DepositFacet'. it would have to take a local Payment.
// TODO(PS0) decide whether to match canonical `DepositFacet'. it would have to take a local Payment
deposit: {
/**
* Put the assets from the payment into the appropriate purse
* Put the assets from the payment into the appropriate purse.
*
* If the purse doesn't exist, we hold the payment until it does.
*
* @param {import('@endo/far').FarRef<Payment>} payment
* @returns {Promise<Amount>}
* @throws if the purse doesn't exist
* NB: the previous smart wallet contract would try again each time there's a new issuer.
* This version does not: 1) for expedience, 2: to avoid resource exhaustion vulnerability.
* @returns {Promise<Amount>} amounts for deferred deposits will be empty
*/
async receive(payment) {
/** @type {State} */
const { brandPurses } = this.state;
const { brandPurses, paymentQueues: queues } = this.state;
const brand = await E(payment).getAllegedBrand();
const purse = brandPurses.get(brand);
Copy link
Member

Choose a reason for hiding this comment

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

This get will throw rather than returning undefined, no?

Copy link
Member Author

@turadg turadg Sep 9, 2022

Choose a reason for hiding this comment

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

yes good catch! I added a unit test for regression and it caught a missing harden too.

if (!purse) {
if (queues.has(brand)) {
queues.get(brand).push(payment);
} else {
queues.init(brand, [payment]);
}
return AmountMath.makeEmpty(brand);
}

// @ts-expect-error deposit does take a FarRef<Payment>
return E(purse).deposit(payment);
Expand All @@ -292,7 +322,7 @@ const behavior = {
* @throws if any parts of the offer can be determined synchronously to be invalid
*/
async executeOffer(offerSpec) {
const { state } = this;
const { facets, state } = this;
const {
zoe,
brandPurses,
Expand All @@ -305,6 +335,7 @@ const behavior = {

const executor = makeOfferExecutor({
zoe,
depositFacet: facets.deposit,
powers: {
invitationFromSpec: makeInvitationsHelper(
zoe,
Expand Down Expand Up @@ -388,7 +419,7 @@ const finish = ({ state, facets }) => {
/** @type {RemotePurse} */ (invitationPurse),
);
// watch the bank for new issuers to make purses out of
observeIteration(E(bank).getAssetSubscription(), {
void observeIteration(E(bank).getAssetSubscription(), {
async updateState(desc) {
/** @type {RemotePurse} */
// @ts-expect-error cast to RemotePurse
Expand Down