Fix timing issue when re-using proposal in bootstrap tests

[iomekam]

refs: #8270

Description

I added some test to validate that concurrent liquidation of multiple asset works as intended. In doing so, each test needs to re-use the add-STARS proposal to setup a new token for the test to leverage. The test introduced a timing bug where the proposal can be generated simutaneously by multiple running tests. This introduced a race condition where one test could delete the generated files before the other test was able to use them.

To fix this problem, anytime we try to run a proposal, we do so within a unique directory. This allows each test to control the lifespan of the files generated.

Testing Considerations

Updating the way that we create and delete proposals to ensure that they are idempotent and don't affect other test runs

Upgrade Considerations

Touches the bootstrap tests that requires running a proposal to succeed. This change shouldn't affect the upgrade directly.

[Chris-Hibbert]

LGTM.

I get twitchy when all the sections of the template are ignored. This PR updated some tests, and they passed. That's worth mentioning.

The changes here impact at least ZCF's upgrade, and I lump all the bootstrap tests as part of upgrade. Please say something about how this is relevant to upgrade.

[turadg]

A "package script" is a line in "scripts" in package.json. This PR removes those so this function name is no longer accurate.

The location can have the package in it so there's no longer need for a separate pkg param.

Suggested change

	const runPackageScript = async (outputDir, pkg, location, env) => {
	const runProposalBuilder = async (outputDir, builderPath, env) => {

Then the call in liquidation.js would be like,

  const proposal = await buildProposal({
    builderScript: '@agoric/builders/scripts/inter-protocol/add-STARS.js',
  });

You can get a pathname for that with something like this:

agoric-sdk/packages/boot/test/upgrading/test-upgrade-contracts.js

Lines 15 to 17 in d4b32f1

	const importSpec = spec =>
	importMetaResolve(spec, import.meta.url).then(u => new URL(u).pathname);

[turadg]

This is a nice simplification to the code. Note that it changes the CLI of this package and someone may be surprised when it no longer works. Consider a deprecation message.

Suggested change

	"build:add-STARS-proposal": "agoric run scripts/inter-protocol/add-STARS.js",
	"build:add-STARS-proposal": "echo instead: agoric run scripts/inter-protocol/add-STARS.js",

[iomekam]

We can also just leave this here in that case. We just won't have any callers within the tests

[turadg]

I'd rank options this way:

1. counsel the user in what to do instead
2. take it out
3. leave it (last because it doesn't get us closer to not needing these. the next script to be added someone will think they need to include it here)

[turadg]

Some comments, none blocking

[turadg]

not a blocker but we shouldn't need to shell out here. yarn bin agoric is running https://github.com/Agoric/agoric-sdk/blob/master/packages/agoric-cli/src/entrypoint.js#L53 which is just a wrapper around the JS module https://github.com/Agoric/agoric-sdk/blob/master/packages/agoric-cli/src/main.js that could be called here as a function

[turadg]

now that it's one param it could be,

const proposal = await buildProposal('@agoric/builders/scripts/inter-protocol/add-STARS.js');

but there's another named param it still takes env and I'm not sure what that's for. Not a blocker but it would be good to document what it's for or at least have some code that uses it.