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(deploy-script-support): Write out bundle file names in machine r… #8559

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

kriskowal
Copy link
Member

…eadable file

Description

During the Agoric 2023 Hackathon, @FUDCo and I found it useful to write out the file names that the proposal writer generated, so that a subsequent script could publish these to the local dev chain automatically.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@kriskowal kriskowal marked this pull request as ready for review November 22, 2023 01:09
@kriskowal kriskowal requested a review from michaelfig November 22, 2023 01:33
@kriskowal kriskowal force-pushed the kriskowal-and-fudco-hackathon branch from ef3e73d to aa510bd Compare November 22, 2023 02:04
@@ -162,6 +164,8 @@ const overrideManifest = ${stringify(overrideManifest, true)};
log(`creating ${proposalJsFile}`);
await writeFile(proposalJsFile, trimmed);

await writeFile(`${filePrefix}-bundles.json`, `${JSON.stringify(files)}\n`);
Copy link
Member

Choose a reason for hiding this comment

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

having the filenames of the permits and scripts is really handy too.

I have written a parser that grabs this stuff from the agoric run output a few times.
And agoric-3-proposals uses this structure all over:

buildAssets: {
    'upgrade-walletFactory-proposal': {
      evals: [{ permit: 'upgrade-walletFactory-permit.json', script: 'upgrade-walletFactory.js' }],
      bundles: [
        // entry: upgrade-walletFactory-proposal.js
        'b1-e229e4bb6c8720016d92116e3dccaebec20a43699d5547a1c815f8710985ba897e825cbe4cd5b80c1d9d674f086bcaf3981b82a0d5546a095542c14174d5f942.json',
        // entry: src/walletFactory.js
        'b1-fa06290e58e5df0b5e8e26ebf7926176770bee5d32f42bcaa62bb77737955a8d9da2922760e644e26643b36ec3118c3c0d546f2af4faf717fdb6ae1fb36773d0.json',
      ],
    },
  },

https://github.com/Agoric/agoric-3-proposals/blob/3ed7ca88c6dbfb323a4dbf02afb1a8bf787fc18a/proposals/49%3Asmart-wallet-nft/upgrade-wf.test.js#L51-L61

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you recommending that we generalize this to produce the inner structure of upgrade-walletFactory-proposal? Could be done. We didn’t think to do that just because those names are self-evident and consistent.

We tried running bundle-source ourselves for the proposal and the contract, but didn’t converge on the same hashes for the proposal for reasons I haven’t isolated, so it was necessary to ask the proposal builder to report what it had made. Ideally, writing out the bundle hashes wouldn’t be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Self evident how? How does a consumer compute the list of permits, script pairs?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the particular case of our variant of dapp-game-places, we were able to contrive a hand-written reload.sh in which the names of the permits and core eval script were hard-coded https://github.com/agoric-labs/dapp-mud/blob/main/contract/scripts/reload.sh#L8-L18. I agree that reload.sh could be generalized to be fully data-driven if writeCoreProposal wrote out a tree like your example.

Copy link
Member

@michaelfig michaelfig Nov 23, 2023

Choose a reason for hiding this comment

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

I don't think a tree needs to be written, since there may be multiple writeCoreProposal invocations in the same proposal builder and it would be good to have them remain independent. I would appreciate outputting upgrade-walletFactory-proposal.json with something like:

{
  "name": "upgrade-walletFactory",
  "main": "./upgrade-walletFactory.js",
  "permit": "./upgrade-walletFactory-permit.json",
  "bundleDependencies": {
    "b1-abcdef...": "~/.agoric/cache/b1-abcdef....json",
    "b1-ghiffd...": "~/.agoric/cache/b1-ghiffd....json"
  }
}

No problem if there isn't any JS code yet that can act on this file format, just that all the information is there to allow it to be built in the future.

Copy link
Member

Choose a reason for hiding this comment

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

My uses typically want to know which bundles go with which evals. But I can probably get by without it.

Copy link
Member Author

@kriskowal kriskowal Dec 2, 2023

Choose a reason for hiding this comment

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

It might help me to understand what variety of tools you each expect to produce and consume files with this schema.

Producers:

  • writeCoreProposal
  • some other proposal builders?

Consumers:

  • reload.sh (which submits all bundles then submits all evals) and presumably would be sufficient for development of contracts beyond just dapp-mud.
  • some other proposal submission workflows?

Copy link
Member

Choose a reason for hiding this comment

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

I think the first time I wrote parseProposalParts was for ava tests that simulate running core-eval proposal such as add-STARS.js. That one seems to be in packages/boot/tools/supports.ts

It then made its way over to packages/deployment/upgrade-test/upgrade-test-scripts/commonUpgradeHelpers.js, again, testing. But this time integrated with a running chain node, not in ava.

And now we've moved much of the work from upgrade-test-scripts to the agoric-3-proposals repo, so we have upgrade-test-scripts/lib/commonUpgradeHelpers.js there too.

The callers typically parse the output of agoric run, which may call writeCoreProposal() more than once. Then in preparation for running one of the resulting proposals, they install the relevant bundles. They don't always run all of the proposals that agoric run spits out. Case in point: add-collateral-core.js spits out a proposal to add collateral and one to start a PSM. But sometimes you only want to do the PSM part; for example: How do I add a PSM for a new stable token? #8450. And conversely, when adding stATOM, only the collateral part was proposed to the BLD stakers. In these last two examples, the consumers aren't necessarily machines. But it's handy to be able to spit out kread-committee-info.json and start-kread-info.json; I did consume those from tests at one point. Note that the bundles and evals are split between 2 files in that case.

Copy link
Member

Choose a reason for hiding this comment

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

@dckc's use cases are the only ones I'm aware of. My proposal was not to scratch my own itch, but rather to give a hint as to what's available to be written, and how it may potentially be consumed.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are clarifying. Making evals an array obfuscated that writeCoreProposal only ever produces a single script and permit, but may entrain multiple bundles. I’ve simplified:

{
  "name": "start-game1",
  "script": "start-game1.js",
  "permit": "start-game1-permit.json",
  "bundles": [
    {
      "entrypoint": "../src/gameAssetContract.js",
      "bundleID": "b1-fd6615953261cedd2f8479d1c0320c3772de43a170fe1f35adc536d65a83bcb8b1b7598d6f1884f20e26a2c1b51a4b4e039cbbf5b9cb8dd0051949ce3839d9f1",
      "fileName": "/Users/kris/.agoric/cache/b1-fd6615953261cedd2f8479d1c0320c3772de43a170fe1f35adc536d65a83bcb8b1b7598d6f1884f20e26a2c1b51a4b4e039cbbf5b9cb8dd0051949ce3839d9f1.json"
    },
    {
      "entrypoint": "../src/start-game1-proposal.js",
      "bundleID": "b1-badf957765b2bcbed96badf587f61130a44a36c956e50bf73d94e54641d306a9e6bd8ea362315334fd0b9e8c01a05a045cfa8989a7c9c8d050ce5ffd7393e1c1",
      "fileName": "/Users/kris/.agoric/cache/b1-badf957765b2bcbed96badf587f61130a44a36c956e50bf73d94e54641d306a9e6bd8ea362315334fd0b9e8c01a05a045cfa8989a7c9c8d050ce5ffd7393e1c1.json"
    }
  ]
}

This should make it straight-forward to aggregate some or all of the writing proposals based on their names.

const args = cacheToArgs.get(cache) || ['--to', cache];
const args = cacheToArgs.get(cache) || ['--cache-js', cache];
Copy link
Member

Choose a reason for hiding this comment

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

What does this change do? is it compatible?

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 does not change behavior. When I added --cache-json, I also back-filled --cache-js to replace the less-specific --to. The usage states bundle-source [--cache-json | --cache-js] <etc>, so this just brings usage into alignment.

@kriskowal kriskowal marked this pull request as draft November 22, 2023 05:10
@kriskowal kriskowal force-pushed the kriskowal-and-fudco-hackathon branch 3 times, most recently from d2cd757 to bf77a48 Compare December 4, 2023 23:48
@kriskowal kriskowal force-pushed the kriskowal-and-fudco-hackathon branch from bf77a48 to 68235ec Compare December 4, 2023 23:53
@kriskowal kriskowal marked this pull request as ready for review December 4, 2023 23:53
@kriskowal kriskowal requested a review from dckc December 4, 2023 23:53
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Looks plausible.

I'm reluctant to approve without a test. I defer to MFig.

@kriskowal
Copy link
Member Author

Looks plausible.

I'm reluctant to approve without a test. I defer to MFig.

To that end, I’d like to find a way to automate the manual test. @FUDCo and I created a reload.sh script that would create a proposal and submit it to our local (Docker) chain. I suspect this might fit in an existing test.

#!/bin/bash
set -xueo pipefail

yarn docker:make mint4k

yarn build:proposal

jq -r '.bundles[] | .fileName' start-game1-plan.json | while read -r BUNDLE; do
	agd tx swingset install-bundle "@$BUNDLE" --from user1 --chain-id agoriclocal --compress --keyring-backend=test --gas auto --gas-adjustment 1.2 --yes
	sleep 10
done

SCRIPT=$(jq -r .script start-game1-plan.json)
PERMIT=$(jq -r .permit start-game1-plan.json)
PROPOSAL=$(agd query gov proposals --output json | jq -c '.proposals | length | .+1')

agd tx gov submit-proposal swingset-core-eval "$PERMIT" "$SCRIPT" \
  --title="Enable <something>" --description="Evaluate $SCRIPT" --deposit=10000000ubld \
  --gas=auto --gas-adjustment=1.2 --from user1 --chain-id agoriclocal --keyring-backend=test \
  --yes

sleep 2

yarn docker:make PROPOSAL="$PROPOSAL" vote

@turadg
Copy link
Member

turadg commented Dec 13, 2023

Once #8635 lands, it could test the happy path. I'd love to replace this hack:

const { buildScript } = proposal;
// XXX use a temp file
execSync(`agoric run ${buildScript} | tee run-report.txt`);
const proposalPath = `proposals/${proposal.proposalIdentifier}:${proposal.proposalName}`;
const submissionPath = `${proposalPath}/submission`;
execSync(`mkdir -p ${submissionPath}`);
// XXX super brittle
execSync(`mv *.js *-permit.json ${submissionPath}`);
execSync(
`cat run-report.txt | grep install-bundle | sed "s/agd tx swingset install-bundle @//" |xargs -I _ cp _ ${submissionPath}`,
);
execSync('rm run-report.txt');
}

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

This is straightforward, given the existing context of writeCoreProposal.js. Feel free to land without a test.

:shipit:

@michaelfig
Copy link
Member

Once #8635 lands, it could test the happy path. I'd love to replace this hack:

FWIW, a slightly less hacky way to do things in the meantime is:

   const { buildScript } = proposal;
   const proposalPath = `proposals/${proposal.proposalIdentifier}:${proposal.proposalName}`; 
   const submissionPath = `${proposalPath}/submission`;
   const relativeBuildScript = path.relative(submissionPath, buildScript);

   execSync(`mkdir -p ${submissionPath}`); 
   // Generate files only in submission path.
   execSync(`agoric run ${relativeBuildScript}`, { cwd: submissionPath, env: { ...process.env, HOME: '.' } }); 
   // Move bundles from submission subdir to submission path.
   execSync('mv ${submissionPath}/.agoric/cache/* ${submissionPath}');
 } 

@dckc
Copy link
Member

dckc commented Dec 15, 2023

@FUDCo and I created a reload.sh script that would create a proposal and submit it to our local (Docker) chain.

For which thanks, btw. It was really handy in reducing yarn start:contract to one command implemented with a start-contract make target and a couple small shell scripts:

start-contract: start-game1.js start-game1-permit.json install-bundles
	scripts/propose-start-contract.sh

install-bundles: bundles/bundle-list
	./scripts/install-bundles.sh

build-proposal: bundles/bundle-list

bundles/bundle-list start-game1.js start-game1-permit.json:
	./scripts/build-proposal.sh

This PR should enable us to get rid of the copy of parseProposals.mjs used in build-proposal.sh.

To that end, I’d like to find a way to automate the manual test.

The yarn start:contract above should be in ci in due course.

@0xpatrickdev
Copy link
Member

Maybe out of scope for the current PR, but doing some work today it would've been handy to have access to the generated bundle sizes.

Vite prints things nicely for the ui build and throws a warning when they are too large.

@kriskowal kriskowal added the automerge:rebase Automatically rebase updates, then merge label Jan 3, 2024
@mergify mergify bot merged commit 8ecb5d7 into master Jan 4, 2024
76 checks passed
@mergify mergify bot deleted the kriskowal-and-fudco-hackathon branch January 4, 2024 00:25
@kriskowal
Copy link
Member Author

Maybe out of scope for the current PR, but doing some work today it would've been handy to have access to the generated bundle sizes.

Vite prints things nicely for the ui build and throws a warning when they are too large.

This is a good idea and would be a small follow-up.

mhofman pushed a commit that referenced this pull request Jan 13, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
mhofman pushed a commit that referenced this pull request Jan 15, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
mhofman pushed a commit that referenced this pull request Jan 15, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
mhofman pushed a commit that referenced this pull request Jan 15, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
mhofman pushed a commit that referenced this pull request Jan 19, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
mhofman pushed a commit that referenced this pull request Jan 19, 2024
feat(deploy-script-support): Write out bundle file names in machine r…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants