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

handle floating promises in run-protocol #5502

Merged
merged 6 commits into from
Jun 8, 2022
Merged

handle floating promises in run-protocol #5502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7750c86
feat(zoe): tickN for ManualTimer
turadg Jun 7, 2022
787f308
test: handle floating promises
turadg Jun 3, 2022
aa16d77
test: revisions due to promise handling
turadg Jun 3, 2022
8f0e277
test: refactor eventLoopIteration() helper
turadg Jun 8, 2022
19fa2c8
chore: clean up Ava types
turadg Jun 8, 2022
46bfcb5
refactor(run-protocol): handle floating promises
turadg Jun 3, 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
2 changes: 1 addition & 1 deletion packages/governance/test/unitTests/test-committee.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ test('committee-open question:mixed', async t => {
.getOutcome()
.catch(e => t.deepEqual(e, 'No quorum'));

timer.tick();
await timer.tick();

const questions = await publicFacet.getOpenQuestions();
t.deepEqual(questions.length, 2);
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/scripts/build-bundles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ process.env.INTERCHAIN_ISSUER_BOARD_ID =
'arbitrary value to prevent build error';

const dirname = url.fileURLToPath(new URL('.', import.meta.url));
extractProposalBundles(
await extractProposalBundles(
[
['.', defaultProposalBuilder],
['.', collateralProposalBuilder],
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/src/proposals/demoIssuers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ export const fundAMM = async ({
give: { Secondary: secondaryAmount, Central: centralAmount },
});

E(zoe).offer(
await E(zoe).offer(
E(ammPublicFacet).addPoolInvitation(),
proposal,
harden({
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/src/proposals/econ-behaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ export const grantVaultFactoryControl = async (
{ consume: { client, priceAuthorityAdmin, vaultFactoryCreator } },
{ options: { vaultFactoryControllerAddress } = {} } = {},
) => {
E(client).assignBundle([
await E(client).assignBundle([
addr => ({
vaultFactoryCreatorFacet:
typeof vaultFactoryControllerAddress === 'string' &&
Expand Down
6 changes: 3 additions & 3 deletions packages/run-protocol/src/runStake/runStakeKit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ const helperBehavior = {

helper.assertVaultHoldsNoRun();

helper.updateUiState();
void helper.updateUiState();
clientSeat.exit();

return 'We have adjusted your balances; thank you for your business.';
Expand Down Expand Up @@ -357,7 +357,7 @@ const helperBehavior = {
manager.burnDebt(currentDebt, vaultSeat);
state.open = false;
helper.updateDebtSnapshot(AmountMath.makeEmpty(debtBrand));
helper.updateUiState();
void helper.updateUiState();
helper.assertVaultHoldsNoRun();
seat.exit();

Expand Down Expand Up @@ -417,7 +417,7 @@ const behavior = {
*/
const finish = ({ facets }) => {
const { helper } = facets;
helper.updateUiState();
void helper.updateUiState();
};

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/src/runStake/runStakeManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ const finish = ({ state, facets }) => {

const periodNotifier = E(timerService).makeNotifier(0n, recordingPeriod);

observeNotifier(periodNotifier, {
void observeNotifier(periodNotifier, {
updateState: updateTime =>
helper
.chargeAllVaults(updateTime)
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/src/vaultFactory/vaultDirector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ const getLiquidationConfig = directorParamManager => ({
*/
const watchGovernance = (govParams, vaultManager, oldInstall, oldTerms) => {
const subscription = govParams.getSubscription();
observeIteration(subscription, {
void observeIteration(subscription, {
updateState(_paramUpdate) {
const { install, terms } = getLiquidationConfig(govParams);
if (install === oldInstall && keyEQ(terms, oldTerms)) {
Expand Down
15 changes: 10 additions & 5 deletions packages/run-protocol/src/vaultFactory/vaultManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ const helperBehavior = {

facets.helper.assetNotify();
trace('chargeAllVaults complete');
facets.helper.reschedulePriceCheck();
return facets.helper.reschedulePriceCheck();
},

/** @param {MethodContext} context */
Expand Down Expand Up @@ -454,9 +454,14 @@ const helperBehavior = {
facets.helper.updateMetrics();

if (!AmountMath.isEmpty(accounting.shortfall)) {
E(factoryPowers.getShortfallReporter()).increaseLiquidationShortfall(
accounting.shortfall,
);
E(factoryPowers.getShortfallReporter())
.increaseLiquidationShortfall(accounting.shortfall)
.catch(reason =>
console.error(
'liquidateAndRemove failed to increaseLiquidationShortfall',
reason,
),
);
}
})
.catch(e => {
Expand Down Expand Up @@ -693,7 +698,7 @@ const finish = ({ state, facets: { helper } }) => {
// push initial state of metrics
helper.updateMetrics();

observeNotifier(state.periodNotifier, {
void observeNotifier(state.periodNotifier, {
updateState: updateTime =>
helper
.chargeAllVaults(updateTime, state.poolIncrementSeat)
Expand Down
4 changes: 2 additions & 2 deletions packages/run-protocol/src/vpool-xyk-amm/priceAuthority.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ export const makePriceAuthority = (
// for each active trigger.
const priceObserver = Far('priceObserver', {
updateState: () => {
fireTriggers(createQuote);
void fireTriggers(createQuote);
},
});

observeNotifier(notifier, priceObserver);
void observeNotifier(notifier, priceObserver);

return priceAuthority;
};
2 changes: 1 addition & 1 deletion packages/run-protocol/test/amm/vpool-xyk-amm/test-amm-governance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test.before(async t => {
test('start Economic Committee', async t => {
const space = await setupAMMBootstrap();
const { consume } = space;
startEconomicCommittee(space, {
await startEconomicCommittee(space, {
options: {
econCommitteeOptions: {
committeeName: 'The Cabal',
Expand Down
6 changes: 2 additions & 4 deletions packages/run-protocol/test/amm/vpool-xyk-amm/test-liquidity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { setup } from '@agoric/zoe/test/unitTests/setupBasicMints.js';
import { setupAmmServices } from './setup.js';
import { unsafeMakeBundleCache } from '../../bundleTool.js';
import { subscriptionTracker } from '../../metrics.js';
import { waitForPromisesToSettle } from '../../supports.js';

/** @typedef {Record<string, any> & {
* bundleCache: Awaited<ReturnType<typeof unsafeMakeBundleCache>>,
Expand Down Expand Up @@ -368,7 +367,6 @@ test('MinInitialPoolLiquidity to reserve', async t => {
`Added Moola and Central Liquidity`,
);

await waitForPromisesToSettle();
const reserveAllocations = await E(reserveCreatorFacet).getAllocations();
t.deepEqual(reserveAllocations, {
RmoolaLiquidity: AmountMath.make(liquidityBrand, 1000n),
Expand Down Expand Up @@ -476,7 +474,7 @@ test('add wrong liquidity', async t => {
message: /liquidity brand must be \[object Alleged: MoolaLiquidity brand]/,
});

E(addLiquiditySeatBreaking).getPayouts();
await E(addLiquiditySeatBreaking).getPayouts();

t.deepEqual(
await E(amm.ammPublicFacet).getPoolAllocation(moolaR.brand),
Expand Down Expand Up @@ -507,7 +505,7 @@ test('add wrong liquidity', async t => {
'Added liquidity.',
);

E(addLiquiditySeatCorrect).getPayouts();
await E(addLiquiditySeatCorrect).getPayouts();

const poolLiquidity2 = {
centralAmount: { value: 1500000000n + 15000n },
Expand Down
2 changes: 1 addition & 1 deletion packages/run-protocol/test/amm/vpool-xyk-amm/test-xyk-amm-swap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] });
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@turadg turadg Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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]


const aliceAddLiquidityInvitation = E(
amm.ammPublicFacet,
Expand Down
29 changes: 8 additions & 21 deletions packages/run-protocol/test/reserve/test-reserve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,16 @@
// @ts-check
/* global setImmediate */

// eslint-disable-next-line import/no-extraneous-dependencies
import { test } from '@agoric/zoe/tools/prepare-test-env-ava.js';

import { E } from '@endo/eventual-send';
import { makeIssuerKit, AmountMath } from '@agoric/ertp';
import buildManualTimer from '@agoric/zoe/tools/manualTimer.js';
import { makePromiseKit } from '@endo/promise-kit';

import { setupReserveServices } from './setup.js';
import { unsafeMakeBundleCache } from '../bundleTool.js';
import { subscriptionTracker } from '../metrics.js';

// Some notifier updates aren't propogating sufficiently quickly for the tests.
// This invocation (thanks to Warner) waits for all promises that can fire to
// have all their callbacks run
const waitForPromisesToSettle = async () => {
const pk = makePromiseKit();
setImmediate(pk.resolve);
return pk.promise;
};
import { eventLoopIteration } from '../supports.js';

const addLiquidPool = async (
runPayment,
Expand Down Expand Up @@ -217,9 +207,9 @@ test('governance add Liquidity to the AMM', async t => {
await E(voterFacet).castBallotFor(details.questionHandle, [
details.positions[0],
]);
timer.tick();
timer.tick();
await waitForPromisesToSettle();
await timer.tick();
await timer.tick();
await eventLoopIteration();

t.deepEqual(
await E(reserve.reserveCreatorFacet).getAllocations(),
Expand Down Expand Up @@ -297,9 +287,8 @@ test('request more collateral than available', async t => {
await E(voterFacet).castBallotFor(details.questionHandle, [
details.positions[0],
]);
timer.tick();
timer.tick();
await waitForPromisesToSettle();
await timer.tick();
await timer.tick();

await outcomeOfUpdate
.then(() => t.fail('expecting failure'))
Expand Down Expand Up @@ -454,10 +443,8 @@ test('reserve burn IST', async t => {
await E(voterFacet).castBallotFor(details.questionHandle, [
details.positions[0],
]);
timer.tick();
await waitForPromisesToSettle();
timer.tick();
await waitForPromisesToSettle();
await timer.tick();
await timer.tick();

runningShortfall = 0n;

Expand Down
Loading