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

Jump to bottom

refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx #6966

Closed
wants to merge 5 commits into from
Closed

refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx #6966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8240433
build: zx devDependency for testing
dckc Feb 10, 2023
0ecad4d
build: lock packages with zx
dckc Feb 10, 2023
d4ecfae
refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx
dckc Feb 10, 2023
550bbd5
fixup! refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx
dckc Feb 10, 2023
c3feae2
fixup! refactor(agoric-cli): sh->js port of agops-perf-smoketest with zx
dckc Feb 10, 2023
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
3 changes: 2 additions & 1 deletion packages/agoric-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
},
"devDependencies": {
"ava": "^5.1.0",
"c8": "^7.12.0"
"c8": "^7.12.0",
"zx": "^7.1.1"
},
"dependencies": {
"@agoric/access-token": "^0.4.20",
Expand Down
53 changes: 53 additions & 0 deletions packages/agoric-cli/test/agops-perf-smoketest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#!/usr/bin/env node
Copy link
Member

Choose a reason for hiding this comment

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

Why not #!/usr/bin/env zx? That would save many imports. I suppose you'd prefer those imports for being explicit about ambient authority. What is the risk that you're trying to mitigate by that?

Copy link
Member Author

Choose a reason for hiding this comment

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

OCap discipline isn't just about mitigating risk. It's about testability, auditability, modularity, reusability. In sum: good engineering.

Copy link
Member

Choose a reason for hiding this comment

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

Engineering is all about trade-offs. One not in your list is velocity.

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 find it's good for velocity too, especially if done always from the beginning so you don't have to retrofit later.

// @ts-check

// #region ambient authority
import process from 'process';
import { $ } from 'zx';

const { env, argv: nodeArgv, exit } = process;
const echo = (...args) => console.log(...args);
// #endregion

// nodeArgv = ['node', 'smoketest', 'wallet']
// argv = ['smoketest', 'wallet']
const argv = nodeArgv.slice(1);

if (!env.AGORIC_NET) {
echo('AGORIC_NET env not set');
echo();
echo('e.g. AGORIC_NET=ollinet (or export to save typing it each time)');
echo();
echo(`To test locally, AGORIC_NET=local and have the following running:
# freshen sdk
yarn install && yarn build

# local chain running with wallet provisioned
packages/inter-protocol/scripts/start-local-chain.sh YOUR_ACCOUNT_KEY
`);
exit(1);
}

const WALLET = argv[1];

if (!WALLET) {
echo(`USAGE: ${argv[0]} key`);
echo('You can reference by name: agd keys list');
echo(
'Make sure it has been provisioned by the faucet: https://$AGORIC_NET.faucet.agoric.net/',
);
exit(1);
}

// NB: fee percentages must be at least the governed param values

// NOTE: USDC_axl = ibc/usdt1234
// perf test wantMinted
let OFFER = await $`mktemp -t agops.XXX`;
await $`bin/agops psm swap --wantMinted 0.01 --feePct 0.01 --pair IST.USDC_axl >|${OFFER}`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm gung ho about zx but there's a downside to JS syntax for for these smoketest scripts. I tend to run them interactively by copying lines over to my shell. This makes that harder to do.

For this script, I'm more inclined to keep the shell commands but place them into a Markdown file that zx would run. I want to make the prose primary and the commands incidental to executing the prose.

Even with that we'd get easier refactoring like extracting all the arg setup and diagnostics to a support function.

Copy link
Member Author

Choose a reason for hiding this comment

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

this script isn't a good candidate for that [=lights out].

got it.

I tend to run them interactively by copying lines over to my shell.

I hope to avoid that. Very much not my style.

If we want a CI test integrating agops I think that's worth writing fresh.

This script does represent a product scenario that's worth lots of testing. The extent to which any automated test shares code with the copy-to-my-shell script, I don't have strong feelings.

Copy link
Member

Choose a reason for hiding this comment

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

I hope to avoid that. Very much not my style.

Sounds worth a conversion to find a mutually agreeable design compatible with our workflows.

await $`time bin/agops perf satisfaction --executeOffer ${OFFER} --from ${WALLET} --keyring-backend="test"`;

// perf test giveMinted
OFFER = await $`mktemp -t agops.XXX`;
await $`bin/agops psm swap --giveMinted 0.01 --feePct 0.03 --pair IST.USDC_axl >|${OFFER}`;
await $`time bin/agops perf satisfaction --executeOffer ${OFFER} --from ${WALLET} --keyring-backend="test"`;
Loading