add a CoreEval proposal to a3p-integration #8801

turadg · 2024-01-23T22:34:33Z

Description

Trunk should have a CoreEvalProposal to demonstrate how to use them in release branches.

This also serves as a validation of the changes in Agoric/agoric-3-proposals#59 that make it much simpler to write a CoreEvalProposal package

This uses an alpha release of @agoric/synthetic-chain, 0.0.4-3

I tried to get source: buildScript working but ran into Agoric/agoric-3-proposals#87 and decided it's better to defer.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Concern of the https:/Agoric/agoric-3-proposals/ package

Testing Considerations

Punts on an actual test of restart-vats. Basically just testing that it executes.

Upgrade Considerations

n/a

mhofman

For core evals generated in non agoric-3-proposals repos, I would really expect the core eval material to be generated dynamically from the repo and not checked in. As such I don't understand the approach. Let's discuss.

...9205e90964d278e49039aa65db4a1f30dc44561ddab3a486075b086c12e2c60f4ef7898b0469e0298bd4dc1.json

mhofman · 2024-01-23T22:40:18Z

a3p-integration/package.json

@@ -12,5 +12,6 @@
 "@agoric/synthetic-chain": "^0.0.4-2",
 "tsx": "^4.7.0"
 },
+ "packageManager": "[email protected]",


I'm concerned this will force anyone / anything running this to use corepack. Is it even enabled in CI?

I'm concerned this will force anyone / anything running this to use corepack.

Valid concern. I adopted it to make it possible to yarn link to a local checkout of agoric-3-proposals. I'm open to cutting it. But I would argue that we should adopt corepack throughout our development.

Is it even enabled in CI?

prolly not. easy to solve for this job

I adopted it to make it possible to yarn link to a local checkout of agoric-3-proposals.

I see. Technically a link is irrespective of the package manager, as it's just a symlink in node_modules.

Would temporarily overriding the "version" to a local path not work? Arguably this package is not part of the workspace, so my concerns are not high as long as we enable corepack in CI for this test.

I would argue that we should adopt corepack throughout our development.

Yes we should get off the yarn1 train, but that's another topic.

Technically a link is irrespective of the package manager, as it's just a symlink in node_modules.

That's not how Yarn 2+ works. It uses resolutions in package.json. agoric-3-proposals is using 2+ so it can't produce what a3p-integration with Yarn 1 is expecting. Of course I could manually create the symlink but I thought it would be good progress to bump Yarn here.

so my concerns are not high as long as we enable corepack in CI for this test

I'm hearing that as the request and agreement to maintain the Yarn version bump.

for ref:

eventually move to Yarn 2+ (currently 4) #451

mhofman · 2024-01-23T22:45:56Z

For core evals generated in non agoric-3-proposals repos, I would really expect the core eval material to be generated dynamically from the repo and not checked in.

FWIW, I still believe that for agoric-3-proposals, the material should not be checked in either, but pulled as an artifact published from the source which proposed the core eval (or maybe even the chain itself)

turadg · 2024-01-23T22:51:05Z

FWIW, I still believe that for agoric-3-proposals, the material should not be checked in either, but pulled as an artifact published from the source which proposed the core eval (or maybe even the chain itself)

I've seen so much CI flakiness from network dependency. I agree it should be easy to source from chain history and to validate what's checked in matches that, which is Agoric/agoric-3-proposals#67 .

turadg · 2024-01-25T17:16:25Z

Once approved I'll clean up the commit history with a force push. E.g. the two commits that claim o update dependencies don't
7665d9e
f19f24a

dckc

looks like it's getting there...

dckc · 2024-01-25T17:16:13Z

a3p-integration/package.json

+ "build": "./node_modules/.bin/synthetic-chain build",
+ "test": "./node_modules/.bin/synthetic-chain test",
+ "doctor": "./node_modules/.bin/synthetic-chain doctor"


shouldn't plain "synthetic-chain build" work? doesn't yarn arrange for ./node_modules.bin to be in $PATH?

It should but it doesn't due to the .ts extensions. Another option is to use tsx as in Agoric/agoric-3-proposals@e4c4791

OK. Good to know.

dckc · 2024-01-25T17:17:45Z

a3p-integration/package.json

@@ -12,5 +12,6 @@
 "@agoric/synthetic-chain": "^0.0.4-2",
 "tsx": "^4.7.0"
 },
+ "packageManager": "[email protected]",


for ref:

eventually move to Yarn 2+ (currently 4) #451

dckc · 2024-01-25T17:21:12Z

a3p-integration/proposals/b:restart-vats/package.json

+ "agoricProposal": {
+ "type": "/agoric.swingset.CoreEvalProposal",
+ "source": "build",
+ "buildScript": "../packages/builders/scripts/vats/restart-vats.js"


hm. cross-package reference. Did you consider supporting this sort of thing?

Suggested change

"buildScript": "../packages/builders/scripts/vats/restart-vats.js"

"buildScript": "@agoric/builders/scripts/vats/restart-vats.js"

I'm actually confused as to which context this import is resolved in, given the ../packages. I know the upgrade core proposal mechanism implemented by cosmic-swingset supports the @agoric/builders form, and it would be confusing if this type of proposal didn't.

More generally however, I'm wondering if a "resolve in the context of agoric-sdk" approach is correct for core evals. And where does the agoric-sdk even come from, the one inside the image? Consider 3rd party contracts that only depend on agoric-sdk packages through NPM installed packages, no checkout of agoric-sdk. How should their version of a3p-integration in their own repo resolve the build script?

Right now this is just relative to where the synthetic-chain is run, so a3p-integration. I could do some work to make it resolves against NPM packages, but can we call that future work?

How should their version of a3p-integration in their own repo resolve the build script?

Right now it's the relative path. If we went the package resolution route, it would be the package export. But I don't think we should require them to make their build script an export from a package. It's just a file.

normally you want to generate a core eval from your local repo, which may not be agoric-sdk.

Exactly. That's why I think it best to leave this to filesystem path resolution. Another repo might have a3p-integration as its home for this tooling, or not. It'll just be somewhere in the repo. And the build script will be referenced relative to that.

Oh, maybe here's the confusion: this buildScript is executed before any Docker comes into play. It's a convention of the synthetic-chain tooling that it builds the script and puts the output into the proposal directory, so that it's there during the image building.

Oh, maybe here's the confusion: this buildScript is executed before any Docker comes into play. It's a convention of the synthetic-chain tooling that it builds the script and puts the output into the proposal directory, so that it's there during the image building.

Ah that's where I was confused, and yes this is the behavior I expect. In this case I am fine with relative path, they actually make more sense.

I could do some work to make it resolves against NPM packages, but can we call that future work?

Yes.

And I'm content without an issue until motivation is more pressing.

Now running into,

agoric: run: running /home/runner/work/agoric-sdk/agoric-sdk/packages/builders/scripts/vats/restart-vats.js agoric: (Error#1) Error#1: Cannot find package '@agoric/deploy-script-support' imported from /home/runner/work/agoric-sdk/agoric-sdk/packages/builders/scripts/vats/restart-vats.js

So it's not "just a file". I could add @agoric/deploy-script-support to the a3p-integration package.json, but it'll surprise someone again when they depend on something else.

I think a3p-integration needs to be part of the repo workspace, so when it runs these scripts from the repo they can use anything in the repo.

I've got a commit to do that, but it runs into a new problem with https:/Agoric/agoric-3-proposals/blob/main/packages/synthetic-chain/cli.ts#L53-L56

I'll mark this as Draft until I get that working in a 0.0.4-3 or later

dckc · 2024-01-25T17:23:00Z

a3p-integration/proposals/b:restart-vats/submission/restart-vats-permit.json

+ "runModuleBehaviors": "makeCoreProposalBehavior"
+ }
+ }
+}


github flags lack of a newline at end of file here.
Do we have a shop norm?
Locally, I get a lot of spurious diffs on save for this sort of thing.

We don't have .editorconfig set up to specify line endings but I think the norm is to include them.

This is just a file move of the output from agoric run. If we want the file here to have a newline, I think the proper fix is to update upstream. (I don't think it's worth it)

mhofman

I'm confused about the design goal of the core eval support in synthethic-chain. It seems to be geared towards generating core eval material from the agoric-sdk repo inside the container, but I think that is the outlier use case, and normally you want to generate a core eval from your local repo, which may not be agoric-sdk.

I likely misunderstood something, so let's discuss.

mhofman · 2024-01-25T17:37:08Z

a3p-integration/proposals/b:restart-vats/package.json

+ "agoricProposal": {
+ "type": "/agoric.swingset.CoreEvalProposal",
+ "source": "build",
+ "buildScript": "../packages/builders/scripts/vats/restart-vats.js"


I'm actually confused as to which context this import is resolved in, given the ../packages. I know the upgrade core proposal mechanism implemented by cosmic-swingset supports the @agoric/builders form, and it would be confusing if this type of proposal didn't.

More generally however, I'm wondering if a "resolve in the context of agoric-sdk" approach is correct for core evals. And where does the agoric-sdk even come from, the one inside the image? Consider 3rd party contracts that only depend on agoric-sdk packages through NPM installed packages, no checkout of agoric-sdk. How should their version of a3p-integration in their own repo resolve the build script?

dckc

This looks quite helpful.

turadg · 2024-01-26T16:11:51Z

Integration test failing with,

Refreshing submission for b restart-vats
/bin/sh: 1: agoric: not found
node:internal/errors:86[5](https:/Agoric/agoric-sdk/actions/runs/7670307322/job/20906183146?pr=8801#step:6:6)
  const err = new Error(message);
              ^

Error: Command failed: agoric run ../../../../packages/builders/scripts/vats/restart-vats.js

That seems due to the Yarn 4.0.2 update, which can be punted so I'm removing those commits.

(#8801 (comment) was by Mergify. #8801 (comment) has the fix to test)

mhofman · 2024-01-26T16:48:23Z

That seems due to the Yarn 4.0.2 update, which can be punted so I'm removing those commits.

Looks like we still have the same failure with older yarn

turadg · 2024-01-26T16:56:41Z

Looks like we still have the same failure with older yarn

Yep, that's what the next build shows. It's not happening for me locally because I have agoric in my path. I have really wanted to not have JS code shell out to a CLI that's implemented in JS code. I'll see if I can solve short-circuit that quickly. Probably not today.

dckc · 2024-01-26T16:59:32Z

FWIW, I saw no trouble with b:restart-vats in combination with yarn 4 in the release branch context (#8819).

mhofman · 2024-01-26T17:05:11Z

I have really wanted to not have JS code shell out to a CLI that's implemented in JS code.

In the case of 3rd party dapps, the agoric CLI would be NPM installed. I think in all cases it should be fine to assume that there is an agoric-cli in the "NPM PATH environment"

$ ls -la node_modules/.bin/agoric 
lrwxrwxrwx 1 node node 43 Jan 25 19:13 node_modules/.bin/agoric -> ../../packages/agoric-cli/src/entrypoint.js

I'm wondering if the problem is that a3p-integration is outside the workspace, so the environment created by yarn is scoped to a3p-integration and not to the containing workspace. I'm not sure how we should solve that however (I'm not sure the yarn.lock of the workspace should be affected by a3p-integration)

mhofman · 2024-01-29T22:14:28Z

yarn.lock

It looks like we started taking on dependencies onto the published versions of agoric-sdk packages, and different endo versions. I am very worried this will cause issues. I haven't traced the cause, but I'm wondering if we can avoid this somehow.

mhofman · 2024-01-29T22:16:23Z

yarn.lock

@@ -2686,6 +3468,50 @@ aggregate-error@^4.0.0:
 clean-stack "^4.0.0"
 indent-string "^5.0.0"

+agoric@^0.22.0-u13.0:


I think this is what is entraining all the new deps

mhofman · 2024-01-29T22:17:04Z

a3p-integration/package.json

@@ -11,6 +11,7 @@
 },
 "dependencies": {
 "@agoric/synthetic-chain": "^0.0.4-3",
+ "agoric": "^0.22.0-u13.0",


Oh yeah, why are we depending on the published version here?

aye, this was to get the version when it was in a separate workspace. I put into the SDK workspace, which makes this change unnecessary (and harmful)

I had to revert because mixing a3p-integration into the root project ran into too many assumptions made about workspace packages

mhofman · 2024-01-30T14:57:21Z

a3p-integration/proposals/b:restart-vats/README.md

+The `submission` dir was built using:
+`packages/builders/scripts/vats/restart-vats.js`
+
+UNTIL https:/Agoric/agoric-3-proposals/issues/87


I am extremely uncomfortable with this approach in the context of an a3p-integration test. People will not know and forget to regenerate these bundles, which will get out of sync with the code in the repo.

At the very least, I would want CI to verify that the bundles are up to date, but then that begs the question of why CI can't generate the bundles automatically in the first place.

Is there really not a way to support a "spawn this command to generate bundles" mode? What in the local generation requires manual intervention that it can't be scripted?

Sure it's possible. I understood that someone was blocked on the changes already here so I figured that can land after these other changes. But they don't actually depend on these changes so I'll drop this back into draft and make Agoric/agoric-3-proposals#87 be part of its requirements.

There's other work here that's safe to land which I've pulled out into #8839

turadg · 2024-02-09T20:41:52Z

Current hurdle: JSON parsing failure due to extra output from Cosmos.

[Bug]: prevalent stdout/stderr stream confusion cosmos/cosmos-sdk#19368

The fix merged a couple days ago but hasn't reached our master yet.

mhofman · 2024-02-09T22:00:55Z

Current hurdle: JSON parsing failure due to extra output from Cosmos.

[Bug]: prevalent stdout/stderr stream confusion cosmos/cosmos-sdk#19368

The fix merged a couple days ago but hasn't reached our master yet.

There's an incoming fix for that. I have a branch with the fix, but it fails on another problem (the proposal doesn't seem available when queried for deposit and voting): https:/Agoric/agoric-sdk/compare/mhofman/provisioning-upgrade

dckc

I'm a little surprised to see the bundle checked in, but I guess that's the current state of the art?

dckc · 2024-02-13T16:42:48Z

a3p-integration/proposals/b:restart-vats/package.json

+{
+ "agoricProposal": {
+ "type": "/agoric.swingset.CoreEvalProposal",
+ "source": "subdir"


subdir? not submission?

I'm a little surprised to see the bundle checked in, but I guess that's the current state of the art?

unfortunately yes

add a CoreEval proposal to a3p-integration

turadg requested review from mhofman and Chris-Hibbert January 23, 2024 22:34

turadg mentioned this pull request Jan 23, 2024

simplify CoreEval proposals Agoric/agoric-3-proposals#59

Merged

mhofman reviewed Jan 23, 2024

View reviewed changes

turadg mentioned this pull request Jan 25, 2024

write "source": "build" to a different path that is gitignored Agoric/agoric-3-proposals#81

Open

turadg requested a review from mhofman January 25, 2024 17:16

dckc reviewed Jan 25, 2024

View reviewed changes

mhofman reviewed Jan 25, 2024

View reviewed changes

dckc approved these changes Jan 25, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Jan 25, 2024

turadg force-pushed the ta/a3p-coreeval branch from 76d41d4 to 4716b1e Compare January 25, 2024 23:24

dckc mentioned this pull request Jan 26, 2024

port "add a CoreEval to a3p-integration" to release branch (WIP) #8819

Closed

turadg force-pushed the ta/a3p-coreeval branch 2 times, most recently from 9c18377 to eaaca65 Compare January 26, 2024 16:27

turadg removed the automerge:rebase Automatically rebase updates, then merge label Jan 26, 2024

turadg marked this pull request as draft January 26, 2024 17:32

turadg self-assigned this Jan 29, 2024

turadg force-pushed the ta/a3p-coreeval branch from b147046 to 2f63cae Compare January 29, 2024 22:05

mhofman reviewed Jan 29, 2024

View reviewed changes

turadg force-pushed the ta/a3p-coreeval branch from 2f63cae to 808af39 Compare January 29, 2024 22:22

turadg force-pushed the ta/a3p-coreeval branch from f72f70d to 39f3b96 Compare January 30, 2024 14:38

turadg marked this pull request as ready for review January 30, 2024 14:40

turadg requested a review from mhofman January 30, 2024 14:40

turadg added the automerge:rebase Automatically rebase updates, then merge label Jan 30, 2024

mhofman reviewed Jan 30, 2024

View reviewed changes

turadg mentioned this pull request Jan 30, 2024

a3p dx #8839

Merged

turadg marked this pull request as draft January 30, 2024 15:22

turadg removed the automerge:rebase Automatically rebase updates, then merge label Jan 30, 2024

turadg force-pushed the ta/a3p-coreeval branch from 39f3b96 to af6474a Compare February 3, 2024 14:46

turadg mentioned this pull request Feb 8, 2024

remove reliance on agoric run in shell path Agoric/agoric-3-proposals#98

Open

turadg force-pushed the ta/a3p-coreeval branch from af6474a to a253c42 Compare February 9, 2024 20:41

turadg force-pushed the ta/a3p-coreeval branch from a253c42 to 39092ed Compare February 13, 2024 00:38

turadg added 3 commits February 13, 2024 06:04

docs(a3p): UPGRADE NEEDED case

bab2f82

chore(deps): simplify a3p-integration package

022abb1

test(a3p): restart-vats

f864dd3

turadg force-pushed the ta/a3p-coreeval branch from 39092ed to f864dd3 Compare February 13, 2024 14:06

turadg marked this pull request as ready for review February 13, 2024 16:35

turadg requested review from dckc and mhofman February 13, 2024 16:36

dckc approved these changes Feb 13, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Feb 13, 2024

mergify bot merged commit b4094ba into master Feb 13, 2024
83 checks passed

mergify bot deleted the ta/a3p-coreeval branch February 13, 2024 16:46

This was referenced Feb 13, 2024

Revert "test(a3p): restart-vats" #8905

Merged

Re-add a CoreEval proposal to a3p-integration #8907

Merged

mhofman pushed a commit that referenced this pull request Feb 18, 2024

Merge pull request #8801 from Agoric/ta/a3p-coreeval

a573d6f

add a CoreEval proposal to a3p-integration

mhofman pushed a commit that referenced this pull request Feb 18, 2024

Merge pull request #8801 from Agoric/ta/a3p-coreeval

2f681a4

add a CoreEval proposal to a3p-integration

mhofman pushed a commit that referenced this pull request Feb 19, 2024

Merge pull request #8801 from Agoric/ta/a3p-coreeval

9b42e71

add a CoreEval proposal to a3p-integration

	"buildScript": "../packages/builders/scripts/vats/restart-vats.js"
	"buildScript": "@agoric/builders/scripts/vats/restart-vats.js"

add a CoreEval proposal to a3p-integration #8801

add a CoreEval proposal to a3p-integration #8801

Conversation

turadg commented Jan 23, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Jan 23, 2024

turadg commented Jan 23, 2024

turadg commented Jan 25, 2024

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

turadg commented Jan 26, 2024 • edited Loading

mhofman commented Jan 26, 2024 • edited Loading

turadg commented Jan 26, 2024

dckc commented Jan 26, 2024 • edited Loading

mhofman commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Feb 9, 2024 • edited Loading

mhofman commented Feb 9, 2024

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Jan 23, 2024 •

edited

Loading

turadg commented Jan 26, 2024 •

edited

Loading

mhofman commented Jan 26, 2024 •

edited

Loading

dckc commented Jan 26, 2024 •

edited

Loading

turadg Jan 29, 2024 •

edited

Loading

turadg commented Feb 9, 2024 •

edited

Loading