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

dckc · 2022-09-01T04:04:13Z

closes: #6096

Description

When an attempt to trade IST for anchor/reference token fails due to lack of anchor funds in the PSM, seat stagings were left in an inconsistent state, preventing further service. This fix clears the seat stagings on failure.

@erights @Chris-Hibbert this also includes a couple zoe tweaks that @dtribble helped me with.

Security Considerations

availability is good :)

Testing Considerations

added a reasonably thorough unit test

turadg · 2022-09-01T14:50:57Z

packages/inter-protocol/src/psm/psm.js

+      stage.clear();
+      anchorPool.clear();
+      feePool.clear();
+      // NOTE someday, reallocate should guarantee that this case cannot happen


ticket? we probably will want to reference it often and it can serve to teach why and when to clear.

Dunno. That comment pre-dates this PR.

Ok. Since it's written in this contract twice and relevant to other contracts, I'm requesting a ticket to document the footgun and that it be referenced from the places like this that say "someday…"

note that only one of the comments is valid (the one before the mintGains). The assert got copied to provide a better diagnostic, but the comment needs to be deleted.

added #6116 and cited it. I don't understand that issue very well, so I'd appreciate it if someone else would clarify the title / description. @michaelfig helped me a little.

turadg · 2022-09-01T14:56:40Z

packages/inter-protocol/src/psm/psm.js

+      stage.clear();
+      anchorPool.clear();
+      feePool.clear();


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 }], ]);

I think it needs an onFail: clause to invoke burnLosses

postponed to #6116

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

turadg · 2022-09-01T14:59:08Z

packages/inter-protocol/test/psm/test-psm.js

+    /**
+     * @param {Amount<'nat'>} giveAnchor
+     * @param {Amount<'nat'>} [wantStable]
+     * @param {*} [out]


yay for typing. this param would benefit from more docs:

```suggestion * @param {{seat: UserSeat, offerResult: *}} [out] will be filled

But I find the need for this suspicious. This function is returning payouts to abstract away seat and offerResult… but here we are needing to punch through. Will contract users need to also? If so we should just change the return type to hide less: {seat, offerResult, payouts}.

Or would it be sufficient to have {numWantsSatisfied, payouts}?

Yes, I was too lazy to change the other call sites. Returning just the seat might be most straightforward.

I refactored the driver to have additional methods that expose the seat.

turadg · 2022-09-01T15:05:29Z

packages/inter-protocol/test/psm/test-psm.js

+  let ix = 0;
+  for (const [kind, give, want, result] of trades) {
+    switch (kind) {


this data driven test calls for Ava macros. See #5900

Note that the items have to be used within one test. The whole point is to not make a new PSM between them. Do macros work that way?

usually things that need be shared between tests are configured with before and beforeEach. but I won't block on this.

it needs to be shared between these trades but no other tests

then another file would work well. this test function is doing a lot of work to replicate some of the before and macro semantics.

Let's punt on this for today.

turadg · 2022-09-01T15:06:05Z

packages/zoe/src/contractFacet/zcfSeat.js

@@ -335,7 +337,7 @@ export const createSeatManager = (
        );
        return amountKeywordRecord;
      },
-      clear: ignoreContext(clear),
+      clear: ({ self }) => clear(self),


just parroting the linter here: gotta remove the ignoreContext import

though, why delegate here at all? we can delete the const clear above.

Suggested change

clear: ({ self }) => clear(self),

clear: ({ self }) => {

if (zcfSeatToStagedAllocations.has(self)) {

zcfSeatToStagedAllocations.delete(self);

}

};

I think MarkM hinted at the right choice here. Please inline the method, so it'll be more apparent that the parameter handling is parallel to the sibling methods.

turadg · 2022-09-01T15:09:05Z

packages/zoe/src/contractFacet/zcfSeat.js

@@ -114,7 +114,9 @@ export const createSeatManager = (
  };

  const clear = zcfSeat => {
-    zcfSeatToStagedAllocations.delete(zcfSeat);
+    if (zcfSeatToStagedAllocations.has(zcfSeat)) {


this is a change to the throw behavior of clear. was the earlier behavior documented or tested?

I'll go google it since our API docs aren't on the APIs ;-) https://docs.agoric.com/zoe/api/zoe-contract-facet.html#zcfseat-clear Nope.

Not only are there no tests for clear, there appear to be no tests for zcfSeat.

[not suggesting this PR add anything.]

Chris-Hibbert

I think there's more discussion going on, so I'm going to submit my current comments and wait to see how it settles before reviewing again.

Chris-Hibbert · 2022-09-01T16:46:27Z

packages/inter-protocol/test/psm/test-psm.js

+  ['give', 200, 190, false],
+  ['want', 101, 100, 1],
+  ['give', 50, 50, false],
+  ['give', 51, 50, 1],


Even though I've been working with JS for a couple years now, seeing false and 1 as two possible values of a flag strikes me as wrong.

Chris-Hibbert · 2022-09-01T16:48:49Z

packages/inter-protocol/src/psm/psm.js

@@ -160,11 +160,24 @@ export const start = async (zcf, privateArgs, baggage) => {
      AmountMath.isGTE(maxAnchor, wanted),


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

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

Chris-Hibbert · 2022-09-01T16:51:36Z

packages/inter-protocol/src/psm/psm.js

+      stage.clear();
+      anchorPool.clear();
+      feePool.clear();


I think it needs an onFail: clause to invoke burnLosses

Chris-Hibbert · 2022-09-01T18:14:40Z

packages/zoe/src/contractFacet/zcfSeat.js

@@ -335,7 +337,7 @@ export const createSeatManager = (
        );
        return amountKeywordRecord;
      },
-      clear: ignoreContext(clear),
+      clear: ({ self }) => clear(self),


I think MarkM hinted at the right choice here. Please inline the method, so it'll be more apparent that the parameter handling is parallel to the sibling methods.

It led to strange failures, at least in a test.

…ilures

turadg

LGTM given #6116 (which I've added to PSM audit release)

Chris-Hibbert · 2022-09-02T21:15:02Z

packages/inter-protocol/test/psm/test-psm.js

+      E(publicFacet).makeWantStableInvitation(),
+      harden({
+        give: { In: giveAnchor },
+        ...(wantStable ? { want: { Out: wantStable } } : {}),


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

Chris-Hibbert · 2022-09-02T21:20:31Z

packages/inter-protocol/test/psm/test-psm.js

+  ['give', 200, 190, false],
+  ['want', 101, 100, true, 1],
+  ['give', 50, 50, false],
+  ['give', 51, 50, true, 1],


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?

dckc requested review from erights and Chris-Hibbert September 1, 2022 04:04

dckc requested a review from turadg as a code owner September 1, 2022 04:04

turadg reviewed Sep 1, 2022

View reviewed changes

Chris-Hibbert requested changes Sep 1, 2022

View reviewed changes

dckc added 4 commits September 2, 2022 15:24

feat(zoe): zcfSeat.clear() handles no-allocations case

0982aa8

fix(zoe): zcf.clear() - don't ignoreContext

dde1dfe

It led to strange failures, at least in a test.

test(inter-protocol): test that PSM service continues after failures

88e898d

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

7f0f8fd

dckc force-pushed the 6096-psm-clear-seats branch from 48c101b to 7f0f8fd Compare September 2, 2022 20:25

dckc mentioned this pull request Sep 2, 2022

reallocate should guarantee that stage failure cannot happen #6116

Closed

dckc added 3 commits September 2, 2022 15:54

fixup! fix(zoe): zcf.clear() - don't ignoreContext

5f0fb94

fixup! fix(inter-protocol): ensure PSM availability after failure

c10d7a8

fixup! test(inter-protocol): test that PSM service continues after fa…

bb39064

…ilures

dckc requested a review from Chris-Hibbert September 2, 2022 20:56

turadg approved these changes Sep 2, 2022

View reviewed changes

dckc added automerge:no-update (expert!) Automatically merge without updates bypass:integration Prevent integration tests from running on PR labels Sep 2, 2022

dckc changed the title ~~fix(inter-protocol): ensure PSM availability after failure~~ fix(inter-protocol): ensure PSM availability after staging failure Sep 2, 2022

Chris-Hibbert approved these changes Sep 2, 2022

View reviewed changes

mergify bot merged commit f338d2e into master Sep 2, 2022

mergify bot deleted the 6096-psm-clear-seats branch September 2, 2022 21:21

erights mentioned this pull request Nov 18, 2022

fix: use atomicTransfers rather than stagings. #6577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dckc commented Sep 1, 2022

turadg Sep 1, 2022

dckc Sep 1, 2022

turadg Sep 1, 2022

dtribble Sep 1, 2022

dckc Sep 2, 2022

turadg Sep 1, 2022

Chris-Hibbert Sep 1, 2022

dckc Sep 2, 2022

erights Nov 18, 2022

turadg Sep 1, 2022

turadg Sep 1, 2022

dckc Sep 1, 2022

dckc Sep 2, 2022

turadg Sep 1, 2022

dckc Sep 1, 2022

turadg Sep 1, 2022

dckc Sep 1, 2022

turadg Sep 1, 2022

dckc Sep 2, 2022

turadg Sep 1, 2022

turadg Sep 1, 2022 •

edited

Loading

Chris-Hibbert Sep 1, 2022

turadg Sep 1, 2022

Chris-Hibbert Sep 1, 2022

Chris-Hibbert left a comment

Chris-Hibbert Sep 1, 2022

Chris-Hibbert Sep 1, 2022

dckc Sep 2, 2022

Chris-Hibbert Sep 1, 2022

Chris-Hibbert Sep 1, 2022

turadg left a comment

Chris-Hibbert Sep 2, 2022

Chris-Hibbert Sep 2, 2022

-      clear: ({ self }) => clear(self),
+      clear: ({ self }) => {
+          if (zcfSeatToStagedAllocations.has(self)) {
+            zcfSeatToStagedAllocations.delete(self);
+          }
+      };

		@@ -160,11 +160,24 @@ export const start = async (zcf, privateArgs, baggage) => {
		AmountMath.isGTE(maxAnchor, wanted),

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

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

Conversation

dckc commented Sep 1, 2022

Description

Security Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Sep 1, 2022 •

edited

Loading