handle floating promises in run-protocol #5502

turadg · 2022-06-03T16:57:11Z

Description

#5495 revealed where promises weren't being handled.

This resolves all the warnings within run-protocol. Classes of changes separated by commit.

Security Considerations

--

Documentation Considerations

--

Testing Considerations

Note that some tests changed due only to more awaits within tests: a6275e7 (no changes required for changes to promise handling within application code)

Chris-Hibbert · 2022-06-06T21:28:28Z

packages/run-protocol/src/vaultFactory/vaultManager.js

+            .increaseLiquidationShortfall(accounting.shortfall)
+            .catch(reason =>
+              console.error(
+                'liquidateAndRemove fialed to increaseLiquidationShortfall',


Chris-Hibbert · 2022-06-06T21:35:58Z

packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js

@@ -391,7 +391,7 @@ test('amm doubleSwap', async t => {

  const metricsSub = await E(amm.ammPublicFacet).getMetrics();
  const m = await subscriptionTracker(t, metricsSub);
-  m.assertInitial({ XYK: [] });
+  await m.assertInitial({ XYK: [] });


void here would say "don't wait, and ignore it if a broken promise isn't handled". await here says "don't proceed until we have the results from the call".

I think not having a marker means that it doesn't wait, and the test will fail if a broken promise isn't handled. Is that closer to what we want in a test? Or am I misunderstanding how the alternatives work?

I think the empty case does a worse job of telling us where the issue was, but are the others better at that?

I think not having a marker means that it doesn't wait, and the test will fail if a broken promise isn't handled. Is that closer to what we want in a test? Or am I misunderstanding how the alternatives work?

I think there's some misunderstanding. void does not change the handling of the promise; it serves to tell the linter that the non-handling is intentional.

This code works exactly the same without void,

test.only('unhandled', async t => { const makeRejector = async () => { assert.fail('problem'); }; void makeRejector(); t.pass(); });

With this Ava output,

Unhandled rejection in test/amm/vpool-xyk-amm/test-xyk-amm-swap.js Error: problem at makeRejector (packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1174:12) at packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1176:3 at async Promise.all (index 0) › makeRejector (packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1174:12) › packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1176:3 ─ 1 test passed [16:12:26] 1 unhandled rejection

I contend this code (as above) is better,

test.only('unhandled', async t => { const makeRejector = async () => { assert.fail('problem'); }; await makeRejector(); t.pass(); });

because it handles the rejection and pretty-prints the error:

Rejected promise returned by test. Reason: Error { message: 'problem', } › makeRejector (packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1174:12) › packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js:1176:9 ─ 1 test failed [16:13:22]

Chris-Hibbert · 2022-06-06T21:41:06Z

packages/run-protocol/test/vaultFactory/test-liquidator.js

+    tick: async (ticks = 1) => {
      for (let i = 0; i < ticks; i += 1) {
-        timer.tick();
+        // eslint-disable-next-line no-await-in-loop
+        await timer.tick();


This seems like a case where tick() doesn't need to be async, as long as all the callers await the results. Alternatively, this could await the last or a Promise.all() for the group of them.

This seems like a case where tick() doesn't need to be async, as long as all the callers await the results.

The callers can't await the result of tick: (line 443) unless it's async (as here) or returns a Promise. So yeah, this could instead be:

tick: (ticks = 1) => Promise.all(Array(ticks).map(() => timer.tick());

Do you think that'd be better?

Chris-Hibbert · 2022-06-06T21:44:10Z

packages/run-protocol/test/vaultFactory/test-vault-interest.js

@@ -1,3 +1,4 @@
+/* eslint-disable no-await-in-loop */


Let's not do this unless we're adding an issue to fix it.

I've been thinking the other direction, to disable the rule in tests. The rule was created because,

performing an await as part of each operation is an indication that the program is not taking full advantage of the parallelization benefits of async/await

but that's less pertinent to test code. In test code clarity of what's happening is more important than small perf wins. Also we often use loops like this to do data-driven coding that isn't necessarily parallelizable. E.g. in the await on 115.

So I don't think there's anything to fix about allowing an await in that loop. If pressed we could refactor like so,

async function checkInterest(i, charge) { await testJig.advanceRecordingPeriod(); interest += charge; t.is( vault.getCurrentDebt().value, startingDebt + interest, `interest charge ${i} should have been ${charge}`, ); t.is(vault.getNormalizedDebt().value, startingDebt); }; await checkInterest(1, 3n); await checkInterest(2, 4n); await checkInterest(3, 4n); await checkInterest(4, 4n);

but makes room for errors in the iteration number.

I'm not the one that turned on no-await-in-loop, so I don't feel like I can approve that choice. An issue to discuss whether the rule is superfluous in tests would satisfy me.

Good idea! #5527

Chris-Hibbert · 2022-06-06T21:46:59Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

@@ -1,3 +1,4 @@
+/* eslint-disable no-await-in-loop */


Let's find a different solution to multiple ticks than calling await on every one.

Ok, would tickN on ManualTimer be sufficient?

Chris-Hibbert · 2022-06-06T21:54:16Z

packages/run-protocol/test/vaultFactory/test-vaultFactory.js

  await manualTimer.tick();
  await waitForPromisesToSettle();

-  shortfallBalance += 42n;
+  shortfallBalance += 46n;


I want to understand what changed here. Changing the number of ticks shouldn't change what the shortfall balance is on the next state change.

I'd like to as well but am balancing that against other priorities. Are you requesting that I figure out what happened and report here before this can be approved?

I do consider that a blocking issue. I can probably make time to investigate in the next few days if you want to assign the PR to me.

Ok, I'll assign to you and pull it back if I find time.

To get these improvements in sooner I made the tests pass without changing numbers and left comments on what changes to tick awaiting result in number changing with FIXME test result relies on awaiting each tick: aa16d77

Chris-Hibbert

Thanks for the changes. This looks good.

mhofman · 2022-06-08T18:00:44Z

I'm not sure why mergify landed this PR before integration tests ran successfully, but this PR is somehow ~~both~~ failing getting started integrations tests

turadg requested a review from Chris-Hibbert June 3, 2022 16:57

turadg requested a review from dtribble as a code owner June 3, 2022 16:57

Chris-Hibbert reviewed Jun 6, 2022

View reviewed changes

turadg force-pushed the ta/handle-floating-promises-in-runp branch from 6ad610c to d80ac48 Compare June 7, 2022 14:21

turadg assigned Chris-Hibbert Jun 7, 2022

turadg added 6 commits June 8, 2022 07:33

feat(zoe): tickN for ManualTimer

7750c86

test: handle floating promises

787f308

test: revisions due to promise handling

aa16d77

test: refactor eventLoopIteration() helper

8f0e277

chore: clean up Ava types

19fa2c8

refactor(run-protocol): handle floating promises

46bfcb5

turadg force-pushed the ta/handle-floating-promises-in-runp branch from d80ac48 to 46bfcb5 Compare June 8, 2022 15:39

turadg unassigned Chris-Hibbert Jun 8, 2022

turadg requested a review from Chris-Hibbert June 8, 2022 15:41

Chris-Hibbert approved these changes Jun 8, 2022

View reviewed changes

turadg added the automerge:no-update (expert!) Automatically merge without updates label Jun 8, 2022

mergify bot merged commit f18e90d into master Jun 8, 2022

mergify bot deleted the ta/handle-floating-promises-in-runp branch June 8, 2022 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle floating promises in run-protocol #5502

handle floating promises in run-protocol #5502

turadg commented Jun 3, 2022 •

edited

Loading

Chris-Hibbert Jun 6, 2022

Chris-Hibbert Jun 6, 2022

turadg Jun 6, 2022 •

edited

Loading

Chris-Hibbert Jun 6, 2022

turadg Jun 6, 2022

Chris-Hibbert Jun 7, 2022

Chris-Hibbert Jun 6, 2022

turadg Jun 6, 2022 •

edited

Loading

Chris-Hibbert Jun 7, 2022

turadg Jun 7, 2022 •

edited

Loading

Chris-Hibbert Jun 6, 2022

turadg Jun 6, 2022

Chris-Hibbert Jun 7, 2022

Chris-Hibbert Jun 6, 2022

turadg Jun 6, 2022

Chris-Hibbert Jun 7, 2022

turadg Jun 7, 2022

turadg Jun 8, 2022

Chris-Hibbert left a comment

mhofman commented Jun 8, 2022 •

edited

Loading

handle floating promises in run-protocol #5502

handle floating promises in run-protocol #5502

Conversation

turadg commented Jun 3, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jun 6, 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

Choose a reason for hiding this comment

turadg Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jun 7, 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

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

Chris-Hibbert left a comment

Choose a reason for hiding this comment

mhofman commented Jun 8, 2022 • edited Loading

turadg commented Jun 3, 2022 •

edited

Loading

turadg Jun 6, 2022 •

edited

Loading

turadg Jun 6, 2022 •

edited

Loading

turadg Jun 7, 2022 •

edited

Loading

mhofman commented Jun 8, 2022 •

edited

Loading