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.

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

feat: doctor checks for "set -e" in test scripts #207

Merged
merged 13 commits into from
Dec 10, 2024
3 changes: 2 additions & 1 deletion .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and then fulfill the remaining checklist items before merging.
Closes #

# adopt a passed proposal

Copy link
Member

Choose a reason for hiding this comment

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

is the prettier style set up to stick?

or was this just a 1-time?

Copy link
Member Author

@turadg turadg Dec 9, 2024

Choose a reason for hiding this comment

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

thanks for inquiring. It's one time and I didn't know why CI hadn't errored on it. Looks like --write makes --check only warn because it's updating: https://github.com/Agoric/agoric-3-proposals/actions/runs/12125081638/job/33804228478

I'll fix it

When a proposal passes on agoric-3 Mainnet, it should be included in the history that this synthetic image tracks.

- [ ] before this PR, change any `fromTag` using `latest` (such as [a3p-integration](https://github.com/Agoric/agoric-sdk/blob/master/a3p-integration/package.json)) to use a fixed version (otherwise they will fail when this PR changes latest and they pick it up)
Expand All @@ -19,11 +20,11 @@ When a proposal passes on agoric-3 Mainnet, it should be included in the history
This should not affect other repos but be prepared that it might.

## @agoric/synthetic-chain library

These need to be published to be consumed in other apps. It's not yet on 1.0 so breaking changes are allowed, but please don't make them gratuitously.

- [ ] indicate in the PR description when you expect these to be published and by whom (it's currently a manual process)

## tooling improvements

As long as it passes CI it should be fine.

1 change: 0 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,3 @@ Each proposal to Mainnet (agoric-3) should be contributed to this repo before be
1. Move generally useful tools to the [`@agoric/synthetic-chain` package](https://github.com/Agoric/agoric-3-proposals/tree/main/packages/synthetic-chain).
2. Move tests that should continue to pass afterwards into [proposals/z:acceptance](https://github.com/Agoric/agoric-sdk/tree/master/a3p-integration/proposals/z%3Aacceptance).
3. Remove the passed proposal.

4 changes: 3 additions & 1 deletion packages/synthetic-chain/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
"better-sqlite3": "^9.6.0",
"chalk": "^5.3.0",
"cosmjs-types": "^0.9.0",
"execa": "8.x"
"execa": "8.x",
"glob": "^11.0.0"
},
"devDependencies": {
"@agoric/cosmic-proto": "^0.4.1-dev-c5284e4.0",
"@types/better-sqlite3": "^7.6.11",
"@types/glob": "^8.1.0",
"@types/node": "^18.19.50",
"ava": "^6.1.3",
"tsimp": "^2.0.11",
Expand Down
14 changes: 14 additions & 0 deletions packages/synthetic-chain/src/cli/doctor.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
import fs from 'node:fs';
import path from 'node:path';
import { glob } from 'glob';
import { ProposalInfo } from './proposals.js';
import assert from 'node:assert';
import { execSync } from 'node:child_process';

const checkShellScripts = (proposalPath: string) => {
const shellScripts = glob.sync('*.sh', { cwd: proposalPath });
for (const script of shellScripts) {
const scriptPath = path.join(proposalPath, script);
const content = fs.readFileSync(scriptPath, 'utf-8');
assert(
content.includes('set -e'),
`${script} must include "set -e"; otherwise lines may fail silently. "set -euo pipefail" is recommended.`,
);
}
};

const fixupProposal = (proposal: ProposalInfo) => {
const proposalPath = path.join('proposals', proposal.path);
checkShellScripts(proposalPath);
const packageJson = JSON.parse(
fs.readFileSync(path.join(proposalPath, 'package.json'), 'utf-8'),
);
Expand Down
6 changes: 2 additions & 4 deletions packages/synthetic-chain/src/lib/econHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,8 @@ export const pushPrices = async (price, brandIn, oraclesByBrand, round) => {
};

export const getRoundId = async (price, io = {}) => {
const {
agoric = { follow: agoricAmbient.follow },
prefix = 'published.',
} = io;
const { agoric = { follow: agoricAmbient.follow }, prefix = 'published.' } =
io;
const path = `:${prefix}priceFeed.${price}-USD_price_feed.latestRound`;
const round = await agoric.follow('-lF', path);
return parseInt(round.roundId);
Expand Down
1 change: 0 additions & 1 deletion proposals/16:upgrade-8/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash

set -e

source /usr/src/upgrade-test-scripts/env_setup.sh
Expand Down
1 change: 1 addition & 0 deletions proposals/29:upgrade-9/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash
set -e
Copy link
Member

Choose a reason for hiding this comment

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

docs say...

"set -euo pipefail" is recommended.

you didn't want to risk compat problems?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but I should follow the advice. 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.

I had to revert it to get to green. Something for another time.


source /usr/src/upgrade-test-scripts/env_setup.sh

Expand Down
1 change: 0 additions & 1 deletion proposals/34:upgrade-10/performActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
getUser,
newOfferId,
waitForBlock,

openVault,
adjustVault,
closeVault,
Expand Down
1 change: 0 additions & 1 deletion proposals/34:upgrade-10/post.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
GOV2ADDR,
GOV3ADDR,
USER1ADDR,

agd,
agoric,
calculateWalletState,
Expand Down
2 changes: 2 additions & 0 deletions proposals/34:upgrade-10/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#!/bin/bash
set -e

source /usr/src/upgrade-test-scripts/env_setup.sh

yarn ava post.test.js
2 changes: 2 additions & 0 deletions proposals/43:upgrade-11/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#!/bin/bash
set -e

source /usr/src/upgrade-test-scripts/env_setup.sh

yarn ava post.test.js
Expand Down
1 change: 0 additions & 1 deletion proposals/49:smart-wallet-nft/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash

set -e

source /usr/src/upgrade-test-scripts/env_setup.sh
Expand Down
1 change: 0 additions & 1 deletion proposals/53:kread-start/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash

set -e

# source /usr/src/upgrade-test-scripts/env_setup.sh
Expand Down
3 changes: 3 additions & 0 deletions proposals/55:statom-vaults/test.sh
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#!/bin/bash
set -e

# TODO stATOM-USD price feed instance in agoricNames
# TODO manager in vstorage
# TODO price feed in vstorage - after setting prices
Expand Down
49 changes: 24 additions & 25 deletions proposals/59:usdc-psm/submission/add-usdc.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@

const manifestBundleRef = {
bundleID:
"b1-4c34c89b707bc8ece5a41e97e6a354081f7ae8a40391f1462848348613dd1218dcce574b3e30901a9825a966cb85bda6a92ba9f9ce9ba325e4c475f9a678b930",
'b1-4c34c89b707bc8ece5a41e97e6a354081f7ae8a40391f1462848348613dd1218dcce574b3e30901a9825a966cb85bda6a92ba9f9ce9ba325e4c475f9a678b930',
};
const getManifestCall = harden([
"getManifestForPsm",
'getManifestForPsm',
{
anchorOptions: {
decimalPlaces: 6,
denom:
"ibc/FE98AAD68F02F03565E9FA39A5E627946699B2B07115889ED812D8BA639576A9",
keyword: "USDC",
proposedName: "USDC",
'ibc/FE98AAD68F02F03565E9FA39A5E627946699B2B07115889ED812D8BA639576A9',
keyword: 'USDC',
proposedName: 'USDC',
},
installKeys: {},
},
Expand All @@ -24,12 +24,12 @@ const overrideManifest = {
agoricNamesAdmin: true,
anchorBalancePayments: true,
anchorKits: true,
bankManager: "bank",
bankManager: 'bank',
startUpgradable: true,
},
installation: {
consume: {
mintHolder: "zoe",
mintHolder: 'zoe',
},
},
produce: {
Expand All @@ -44,36 +44,36 @@ const overrideManifest = {
startPSM: {
brand: {
consume: {
IST: "zoe",
IST: 'zoe',
},
},
consume: {
agoricNamesAdmin: true,
anchorBalancePayments: true,
board: true,
chainStorage: true,
chainTimerService: "timer",
chainTimerService: 'timer',
diagnostics: true,
econCharterKit: "econCommitteeCharter",
economicCommitteeCreatorFacet: "economicCommittee",
feeMintAccess: "zoe",
econCharterKit: 'econCommitteeCharter',
economicCommitteeCreatorFacet: 'economicCommittee',
feeMintAccess: 'zoe',
provisionPoolStartResult: true,
psmKit: true,
zoe: "zoe",
zoe: 'zoe',
},
installation: {
consume: {
contractGovernor: "zoe",
psm: "zoe",
contractGovernor: 'zoe',
psm: 'zoe',
},
},
instance: {
consume: {
economicCommittee: "economicCommittee",
economicCommittee: 'economicCommittee',
},
},
produce: {
psmKit: "true",
psmKit: 'true',
},
vatParameters: {
chainStorageEntries: true,
Expand All @@ -93,21 +93,21 @@ const overrideManifest = {
const { entries, fromEntries } = Object;

// deeplyFulfilled is a bit overkill for what we need.
const shallowlyFulfilled = async (obj) => {
const shallowlyFulfilled = async obj => {
if (!obj) {
return obj;
}
const ents = await Promise.all(
entries(obj).map(async ([key, valueP]) => {
const value = await valueP;
return [key, value];
})
}),
);
return fromEntries(ents);
};

/** @param {ChainBootstrapSpace & BootstrapPowers & { evaluateBundleCap: any }} allPowers */
const behavior = async (allPowers) => {
const behavior = async allPowers => {
// NOTE: If updating any of these names extracted from `allPowers`, you must
// change `permits` above to reflect their accessibility.
const {
Expand All @@ -120,13 +120,13 @@ const overrideManifest = {
const [exportedGetManifest, ...manifestArgs] = getManifestCall;

// Get the on-chain installation containing the manifest and behaviors.
console.info("evaluateBundleCap", {
console.info('evaluateBundleCap', {
manifestBundleRef,
exportedGetManifest,
vatAdminSvc,
});
let bcapP;
if ("bundleName" in manifestBundleRef) {
if ('bundleName' in manifestBundleRef) {
bcapP = E(vatAdminSvc).getNamedBundleCap(manifestBundleRef.bundleName);
} else {
bcapP = E(vatAdminSvc).getBundleCap(manifestBundleRef.bundleID);
Expand All @@ -135,7 +135,7 @@ const overrideManifest = {

const manifestNS = await evaluateBundleCap(bundleCap);

console.error("execute", {
console.error('execute', {
exportedGetManifest,
behaviors: Object.keys(manifestNS),
});
Expand All @@ -153,7 +153,7 @@ const overrideManifest = {
behaviors: manifestNS,
manifest: overrideManifest || manifest,
makeConfig: (name, _permit) => {
log("coreProposal:", name);
log('coreProposal:', name);
return { options };
},
});
Expand All @@ -162,4 +162,3 @@ const overrideManifest = {
// Make the behavior the completion value.
return behavior;
})({ manifestBundleRef, getManifestCall, overrideManifest, E });

2 changes: 1 addition & 1 deletion proposals/59:usdc-psm/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/bash

set -e
echo check for USDC in psm pairs
agd query vstorage children published.psm.IST | grep USDC
1 change: 1 addition & 0 deletions proposals/63:upgrade-12/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash
set -e
source /usr/src/upgrade-test-scripts/env_setup.sh

yarn ava post.test.js
1 change: 0 additions & 1 deletion proposals/64:crabble-start/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash

set -e

# source /usr/src/upgrade-test-scripts/env_setup.sh
Expand Down
1 change: 1 addition & 0 deletions proposals/65:upgrade-13/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash
set -e
source /usr/src/upgrade-test-scripts/env_setup.sh

yarn ava post.test.js
2 changes: 1 addition & 1 deletion proposals/71:upgrade-14/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/bin/bash

set -e
# Place here any test that should be executed using the executed proposal.
# The effects of this step are not persisted in further proposal layers.

Expand Down
1 change: 1 addition & 0 deletions proposals/71:upgrade-14/use.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash
set -e

# Place here any actions that should happen after the upgrade has executed. The
# actions are executed in the upgraded chain software and the effects are
Expand Down
8 changes: 4 additions & 4 deletions proposals/74:upgrade-15/prepare.sh
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/bin/bash

# Exit when any command fails
set -uxeo pipefail
set -euxo pipefail

# Place here any actions that should happen before the upgrade is proposed. The
# actions are executed in the previous chain software, and the effects are
Expand All @@ -10,16 +10,16 @@ set -uxeo pipefail

printISTBalance() {
addr=$(agd keys show -a "$1" --keyring-backend=test)
agd query bank balances "$addr" -o json \
| jq -c '.balances[] | select(.denom=="uist")'
agd query bank balances "$addr" -o json |
jq -c '.balances[] | select(.denom=="uist")'

}

echo TEST: Offer with bad invitation
printISTBalance gov1

badInvitationOffer=$(mktemp)
cat > "$badInvitationOffer" << 'EOF'
cat >"$badInvitationOffer" <<'EOF'
{"body":"#{\"method\":\"executeOffer\",\"offer\":{\"id\":\"bad-invitation-15\",\"invitationSpec\":{\"callPipe\":[[\"badMethodName\"]],\"instancePath\":[\"reserve\"],\"source\":\"agoricContract\"},\"proposal\":{\"give\":{\"Collateral\":{\"brand\":\"$0.Alleged: IST brand\",\"value\":\"+15000\"}}}}}","slots":["board0257"]}
EOF

Expand Down
2 changes: 1 addition & 1 deletion proposals/74:upgrade-15/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/bin/bash

set -e
# Place here any test that should be executed using the executed proposal.
# The effects of this step are not persisted in further proposal layers.

Expand Down
2 changes: 1 addition & 1 deletion proposals/75:upgrade-16/prepare.sh
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/bin/bash

# Exit when any command fails
set -uxeo pipefail
set -euxo pipefail

# Place here any actions that should happen before the upgrade is proposed. The
# actions are executed in the previous chain software, and the effects are
Expand Down
2 changes: 1 addition & 1 deletion proposals/75:upgrade-16/test.sh
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#!/bin/bash

set -e
# Place here any test that should be executed using the executed proposal.
# The effects of this step are not persisted in further proposal layers.

Expand Down
1 change: 1 addition & 0 deletions proposals/76:vaults-auctions/eval.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#!/bin/bash
set -e

echo "[$PROPOSAL] Recording the auctioneer instance"
./saveAuctionInstance.js
Expand Down
Loading
Loading