Skip to content

Commit

Permalink
fix potential flake caused by checking IST balances (#10783)
Browse files Browse the repository at this point in the history
closes: #10774

## Description

Due to the execution fee charged in batches is difficult to guess/reproduce, we introduced the method `tryISTBalances` instead of a regular balance check done using `t.is`. This PR is an effort to migrate existing IST balance checks using `t.is` (or alike) to use `tryISTBalances`. I believe this will prevent potential flakes in our `a3p-integration` tests in the future.

### Security Considerations

We are only changing the way we compare numbers in our tests. No security concern here.

### Scaling Considerations

By avoiding flakes we'll improve the scaling in the long run.

### Documentation Considerations

None.

### Testing Considerations

Integration tests should be forced in CI and all tests should pass.

### Upgrade Considerations

We are improving the way we test our upgrades. No other concern here as well.
  • Loading branch information
mergify[bot] authored Dec 31, 2024
2 parents cb515a8 + 7852991 commit 0a07188
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 8 deletions.
12 changes: 8 additions & 4 deletions a3p-integration/proposals/n:upgrade-next/priceFeedUpdate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
} from '@agoric/synthetic-chain';
import {
getPriceFeedRoundId,
tryISTBalances,
verifyPushedPrice,
} from './test-lib/price-feed.js';

Expand Down Expand Up @@ -79,16 +80,19 @@ const createNewBid = async t => {
};

const openMarginalVault = async t => {
let user1IST = await getISTBalance(USER1ADDR);
const istBalanceBeforeVaultOpen = await getISTBalance(USER1ADDR, 'uist', 1);
await bankSend(USER1ADDR, `20000000${ATOM_DENOM}`);
const currentVaults = await agops.vaults('list', '--from', USER1ADDR);

t.log('opening a vault');
// @ts-expect-error bad typedef
await openVault(USER1ADDR, 5, 10);
user1IST += 5;
const istBalanceAfterVaultOpen = await getISTBalance(USER1ADDR);
t.is(istBalanceAfterVaultOpen, user1IST);
const istBalanceAfterVaultOpen = await getISTBalance(USER1ADDR, 'uist', 1);
await tryISTBalances(
t,
istBalanceAfterVaultOpen,
istBalanceBeforeVaultOpen + 5_000_000, // in uist
);

const activeVaultsAfter = await agops.vaults('list', '--from', USER1ADDR);
t.log(currentVaults, activeVaultsAfter);
Expand Down
29 changes: 29 additions & 0 deletions a3p-integration/proposals/n:upgrade-next/test-lib/price-feed.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,32 @@ export const getPriceFeedRoundId = async brand => {
console.log(latestRoundPath, latestRound);
return Number(latestRound.roundId);
};

/**
* Copy from https://github.com/Agoric/agoric-sdk/blob/745f2a82cc94e246f98dd1bd69cb679b608a7170/a3p-integration/proposals/p%3Aupgrade-19/test-lib/psm-lib.js#L277
*
* Checking IST balances can be tricky because of the execution fee mentioned in
* https://github.com/Agoric/agoric-sdk/issues/6525. So we first check for
* equality, but if that fails we recheck against an assumption that a fee of
* the default "minFeeDebit" has been charged.
*
* @param {import('ava').ExecutionContext} t
* @param {number} actualBalance
* @param {number} expectedBalance
*/
export const tryISTBalances = async (t, actualBalance, expectedBalance) => {
const firstTry = await t.try(tt => {
tt.is(actualBalance, expectedBalance);
});
if (firstTry.passed) {
firstTry.commit();
return;
}

firstTry.discard();
t.log('tryISTBalances assuming no batched IST fee', ...firstTry.errors);
// See golang/cosmos/x/swingset/types/default-params.go
// and `ChargeBeans` in golang/cosmos/x/swingset/keeper/keeper.go.
const minFeeDebit = 200_000;
t.is(actualBalance + minFeeDebit, expectedBalance);
};
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
} from '@agoric/synthetic-chain';
import { makeVstorageKit, retryUntilCondition } from '@agoric/client-utils';
import { readFile, writeFile } from 'node:fs/promises';
import { psmSwap, snapshotAgoricNames } from './psm-lib.js';
import { psmSwap, snapshotAgoricNames, tryISTBalances } from './psm-lib.js';

/** @type {(x: number) => number} */
const scale6 = x => x * 1_000_000;

/**
* @param {string} fileName base file name without .tjs extension
Expand Down Expand Up @@ -89,6 +92,13 @@ export const mintPayment = async (t, address, assetList, value) => {
}
};

/**
*
* @param {import('ava').ExecutionContext} t
* @param {string} address
* @param {Array<{ label: string, denom: string, mintHolderVat: string}>} assetList
* @param {number} want in IST
*/
export const swap = async (t, address, assetList, want) => {
for (const asset of assetList) {
const { label, denom } = asset;
Expand All @@ -100,7 +110,7 @@ export const swap = async (t, address, assetList, want) => {

const pair = `IST.${label}`;

const istBalanceBefore = await getISTBalance(address, 'uist');
const istBalanceBefore = await getISTBalance(address, 'uist', 1); // we want uist not IST
const anchorBalanceBefore = await getISTBalance(address, denom);

const psmSwapIo = {
Expand All @@ -116,10 +126,14 @@ export const swap = async (t, address, assetList, want) => {
psmSwapIo,
);

const istBalanceAfter = await getISTBalance(address, 'uist');
const istBalanceAfter = await getISTBalance(address, 'uist', 1); // we want uist not IST
const anchorBalanceAfter = await getISTBalance(address, denom);

t.is(istBalanceAfter, istBalanceBefore + want);
await tryISTBalances(
t,
istBalanceAfter,
istBalanceBefore + scale6(want), // scale "want" to uist
);
t.is(anchorBalanceAfter, anchorBalanceBefore - want);
}
};
Expand Down

0 comments on commit 0a07188

Please sign in to comment.