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(inter-protocol): ensure PSM availability after staging failure #6102

Merged
merged 7 commits into from
Sep 2, 2022
Merged

fix(inter-protocol): ensure PSM availability after staging failure #6102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0982aa8
feat(zoe): zcfSeat.clear() handles no-allocations case
dckc Sep 1, 2022
dde1dfe
fix(zoe): zcf.clear() - don't ignoreContext
dckc Sep 1, 2022
88e898d
test(inter-protocol): test that PSM service continues after failures
dckc Sep 1, 2022
7f0f8fd
fix(inter-protocol): ensure PSM availability after failure
dckc Sep 1, 2022
5f0fb94
fixup! fix(zoe): zcf.clear() - don't ignoreContext
dckc Sep 2, 2022
c10d7a8
fixup! fix(inter-protocol): ensure PSM availability after failure
dckc Sep 2, 2022
bb39064
fixup! test(inter-protocol): test that PSM service continues after fa…
dckc Sep 2, 2022
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
35 changes: 25 additions & 10 deletions packages/inter-protocol/src/psm/psm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,16 +155,28 @@ export const start = async (zcf, privateArgs, baggage) => {
const fee = ceilMultiplyBy(given, params.getGiveStableFee());
const afterFee = AmountMath.subtract(given, fee);
const maxAnchor = floorMultiplyBy(afterFee, anchorPerStable);
// TODO this prevents the reallocate from failing. Can this be tested otherwise?
assert(
AmountMath.isGTE(maxAnchor, wanted),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the TODO on line 158 interact with the current problem at all? Can it be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

we certainly have relevant tests; I'm getting rid of that TODO in favor of #6116

X`wanted ${wanted} is more then ${given} minus fees ${fee}`,
);
stageTransfer(seat, stage, { In: afterFee }, { Stable: afterFee });
stageTransfer(seat, feePool, { In: fee }, { Stable: fee });
stageTransfer(anchorPool, seat, { Anchor: maxAnchor }, { Out: maxAnchor });
zcf.reallocate(seat, anchorPool, stage, feePool);
stableMint.burnLosses({ Stable: afterFee }, stage);
try {
stageTransfer(seat, stage, { In: afterFee }, { Stable: afterFee });
stageTransfer(seat, feePool, { In: fee }, { Stable: fee });
stageTransfer(
anchorPool,
seat,
{ Anchor: maxAnchor },
{ Out: maxAnchor },
);
zcf.reallocate(seat, anchorPool, stage, feePool);
stableMint.burnLosses({ Stable: afterFee }, stage);
} catch (e) {
stage.clear();
anchorPool.clear();
feePool.clear();
// TODO(#6116) someday, reallocate should guarantee that this case cannot happen
throw e;
}
totalAnchorProvided = AmountMath.add(totalAnchorProvided, maxAnchor);
};

Expand All @@ -183,13 +195,16 @@ export const start = async (zcf, privateArgs, baggage) => {
X`wanted ${wanted} is more then ${given} minus fees ${fee}`,
);
stableMint.mintGains({ Stable: asStable }, stage);
stageTransfer(seat, anchorPool, { In: given }, { Anchor: given });
stageTransfer(stage, seat, { Stable: afterFee }, { Out: afterFee });
stageTransfer(stage, feePool, { Stable: fee });
try {
stageTransfer(seat, anchorPool, { In: given }, { Anchor: given });
stageTransfer(stage, seat, { Stable: afterFee }, { Out: afterFee });
stageTransfer(stage, feePool, { Stable: fee });
zcf.reallocate(seat, anchorPool, stage, feePool);
} catch (e) {
// NOTE someday, reallocate should guarantee that this case cannot happen
stage.clear();
anchorPool.clear();
feePool.clear();
Comment on lines +204 to +206
Copy link
Member

Choose a reason for hiding this comment

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

could we codify this pattern now with a contract helper? something like,

atomicReallocate(zcf, [
  [seat, anchorPool, { In: given }, { Anchor: given }],
  [stage, seat, { Stable: afterFee }, { Out: afterFee }],
  [stage, feePool, { Stable: fee }],
]);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs an onFail: clause to invoke burnLosses

Copy link
Member Author

Choose a reason for hiding this comment

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

postponed to #6116

Copy link
Member

Choose a reason for hiding this comment

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

Progress on #6116 at #6577 , where stageMgr.commit() replaces zcf.reallocate(...) and does guarantee that all staging are cleared, whether the commit succeeded or failed.

@dtribble 's suggested stageMgr.transfer operation is much like @turadg 's rows of atomicReallocate above. The contrast between the list and the reified stageMgr is approx the contrast between #6577 and #4327

// TODO(#6116) someday, reallocate should guarantee that this case cannot happen
stableMint.burnLosses({ Stable: asStable }, stage);
throw e;
}
Expand Down
171 changes: 154 additions & 17 deletions packages/inter-protocol/test/psm/test-psm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import {
makeRatio,
natSafeMath as NatMath,
} from '@agoric/zoe/src/contractSupport/index.js';
import centralSupplyBundle from '@agoric/vats/bundles/bundle-centralSupply.js';
import { E } from '@endo/eventual-send';
import { NonNullish } from '@agoric/assert';
import path from 'path';
import { eventLoopIteration } from '@agoric/zoe/tools/eventLoopIteration.js';
import { makeTracer } from '../../src/makeTracer.js';
import {
makeMockChainStorageRoot,
mintRunPayment,
setUpZoeForTest,
subscriptionKey,
withAmountUtils,
Expand Down Expand Up @@ -90,6 +92,8 @@ const makeTestContext = async () => {

const committeeInstall = await E(zoe).install(committeeBundle);
const psmInstall = await E(zoe).install(psmBundle);
const centralSupply = await E(zoe).install(centralSupplyBundle);

const mintLimit = AmountMath.make(anchor.brand, MINT_LIMIT);

const marshaller = makeBoard().getReadonlyMarshaller();
Expand Down Expand Up @@ -119,7 +123,7 @@ const makeTestContext = async () => {
initialPoserInvitation,
stable: { issuer: stableIssuer, brand: stableBrand },
anchor,
installs: { committeeInstall, psmInstall },
installs: { committeeInstall, psmInstall, centralSupply },
mintLimit,
marshaller,
terms: {
Expand Down Expand Up @@ -179,6 +183,42 @@ async function makePsmDriver(t, customTerms) {
}),
);

/**
* @param {Amount<'nat'>} giveAnchor
* @param {Amount<'nat'>} [wantStable]
*/
const swapAnchorForStableSeat = async (giveAnchor, wantStable) => {
const seat = E(zoe).offer(
E(publicFacet).makeWantStableInvitation(),
harden({
give: { In: giveAnchor },
...(wantStable ? { want: { Out: wantStable } } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this. It's a definite improvement on my approach of making the proposal, and then adding the want later.

}),
// @ts-expect-error known defined
harden({ In: anchor.mint.mintPayment(giveAnchor) }),
);
await eventLoopIteration();
return seat;
};

/**
* @param {Amount<'nat'>} giveRun
* @param {Payment<'nat'>} runPayment
* @param {Amount<'nat'>} [wantAnchor]
*/
const swapStableForAnchorSeat = async (giveRun, runPayment, wantAnchor) => {
const seat = E(zoe).offer(
E(publicFacet).makeGiveStableInvitation(),
harden({
give: { In: giveRun },
...(wantAnchor ? { want: { Out: wantAnchor } } : {}),
}),
harden({ In: runPayment }),
);
await eventLoopIteration();
return seat;
};

return {
mockChainStorage,
publicFacet,
Expand Down Expand Up @@ -209,31 +249,26 @@ async function makePsmDriver(t, customTerms) {
return feePayoutAmount;
},

/** @param {Amount<'nat'>} giveAnchor */
async swapAnchorForStable(giveAnchor) {
const seat = E(zoe).offer(
E(publicFacet).makeWantStableInvitation(),
harden({ give: { In: giveAnchor } }),
// @ts-expect-error known defined
harden({ In: anchor.mint.mintPayment(giveAnchor) }),
);
await eventLoopIteration();
/**
* @param {Amount<'nat'>} giveAnchor
* @param {Amount<'nat'>} [wantStable]
*/
async swapAnchorForStable(giveAnchor, wantStable) {
const seat = swapAnchorForStableSeat(giveAnchor, wantStable);
return E(seat).getPayouts();
},
swapAnchorForStableSeat,

/**
* @param {Amount<'nat'>} giveRun
* @param {Payment<'nat'>} runPayment
* @param {Amount<'nat'>} [wantAnchor]
*/
async swapStableForAnchor(giveRun, runPayment) {
const seat = E(zoe).offer(
E(publicFacet).makeGiveStableInvitation(),
harden({ give: { In: giveRun } }),
harden({ In: runPayment }),
);
await eventLoopIteration();
async swapStableForAnchor(giveRun, runPayment, wantAnchor) {
const seat = swapStableForAnchorSeat(giveRun, runPayment, wantAnchor);
return E(seat).getPayouts();
},
swapStableForAnchorSeat,
};
}

Expand Down Expand Up @@ -305,6 +340,108 @@ test('limit', async t => {
// t.throwsAsync(() => await E(seat1).getOfferResult());
});

/** @type {[kind: 'want' | 'give', give: number, want: number, ok: boolean, wants?: number][]} */
const trades = [
['give', 200, 190, false],
['want', 101, 100, true, 1],
['give', 50, 50, false],
['give', 51, 50, true, 1],
Comment on lines +345 to +348
Copy link
Contributor

Choose a reason for hiding this comment

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

I get what this is doing now, but only because of side conversations about ava macros. It would be nice if it said that the first line was intended to fail because the PSM doesn't have any anchor until after a want completes. Does the third fail because it doesn't include fees?

];

test('mix of trades: failures do not prevent later service', async t => {
const {
terms,
stable,
anchor,
feeMintAccess,
zoe,
installs: { centralSupply },
} = t.context;
const driver = await makePsmDriver(t);

const ist100 = await mintRunPayment(500n * 1_000_000n, {
centralSupply,
feeMintAccess,
zoe,
});

assert(anchor.issuer);
const anchorPurse = await E(anchor.issuer).makeEmptyPurse();
const stablePurse = await E(stable.issuer).makeEmptyPurse();
await E(stablePurse).deposit(ist100);

const scale6 = x => BigInt(Math.round(x * 1_000_000));

const wantStable = async (ix, give, want, ok, wants) => {
t.log('wantStable', ix, give, want, ok, wants);
const giveAnchor = AmountMath.make(anchor.brand, scale6(give));
const wantAmt = AmountMath.make(stable.brand, scale6(want));
const seat = await driver.swapAnchorForStableSeat(giveAnchor, wantAmt);
if (!ok) {
await t.throwsAsync(E(seat).getOfferResult());
return;
}
await E(seat).getOfferResult();
t.is(await E(seat).numWantsSatisfied(), wants);
if (wants === 0) {
return;
}
const runPayouts = await E(seat).getPayouts();
const expectedRun = minusAnchorFee(giveAnchor, terms.anchorPerStable);
const actualRun = await E(stablePurse).deposit(await runPayouts.Out);
t.deepEqual(actualRun, expectedRun);
};

const giveStable = async (ix, give, want, ok, wants) => {
t.log('giveStable', ix, give, want, ok, wants);
const giveRun = AmountMath.make(stable.brand, scale6(give));
const runPayment = await E(stablePurse).withdraw(giveRun);
const wantAmt = AmountMath.make(anchor.brand, scale6(want));
const seat = await driver.swapStableForAnchorSeat(
giveRun,
runPayment,
wantAmt,
);
const anchorPayouts = await E(seat).getPayouts();
if (!ok) {
await t.throwsAsync(E(seat).getOfferResult());
return;
}
await E(seat).getOfferResult();

t.is(await E(seat).numWantsSatisfied(), wants);
if (wants === 0) {
return;
}
const actualAnchor = await E(anchorPurse).deposit(await anchorPayouts.Out);
const expectedAnchor = AmountMath.make(
anchor.brand,
minusStableFee(giveRun).value,
);
t.deepEqual(actualAnchor, expectedAnchor);
};

let ix = 0;
for (const [kind, give, want, ok, wants] of trades) {
switch (kind) {
case 'give':
// eslint-disable-next-line no-await-in-loop
await giveStable(ix, give, want, ok, wants);
break;
case 'want':
// eslint-disable-next-line no-await-in-loop
await wantStable(ix, give, want, ok, wants);
break;
default:
assert.fail(kind);
}
if (kind === 'give') {
// eslint-disable-next-line no-await-in-loop
}
ix += 1;
}
});

test('anchor is 2x stable', async t => {
const { stable, anchor } = t.context;
const anchorPerStable = makeRatio(200n, anchor.brand, 100n, stable.brand);
Expand Down
11 changes: 5 additions & 6 deletions packages/zoe/src/contractFacet/zcfSeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
provideDurableMapStore,
makeScalarBigMapStore,
vivifyKind,
ignoreContext,
makeScalarBigWeakMapStore,
} from '@agoric/vat-data';
import { E } from '@endo/eventual-send';
Expand Down Expand Up @@ -113,10 +112,6 @@ export const createSeatManager = (
);
};

const clear = zcfSeat => {
zcfSeatToStagedAllocations.delete(zcfSeat);
};

const setStagedAllocation = (zcfSeat, newStagedAllocation) => {
if (zcfSeatToStagedAllocations.has(zcfSeat)) {
zcfSeatToStagedAllocations.set(zcfSeat, newStagedAllocation);
Expand Down Expand Up @@ -335,7 +330,11 @@ export const createSeatManager = (
);
return amountKeywordRecord;
},
clear: ignoreContext(clear),
clear: ({ self }) => {
if (zcfSeatToStagedAllocations.has(self)) {
zcfSeatToStagedAllocations.delete(self);
}
},
hasStagedAllocation: ({ self }) => hasStagedAllocation(self),
},
);
Expand Down