zoe1 proposal #18

turadg · 2023-11-08T00:22:09Z

Proposal to upgrade Zoe on the chain with version from Agoric/agoric-sdk#8453

90 was not enough https://github.com/Agoric/agoric-3-proposals/actions/runs/6805556962/job/18505303429?pr=18

turadg · 2023-11-09T14:22:34Z

proposals/b:zoe1/core-eval-support.js

+// TODO: factor out ambient authority from these
+// or at least allow caller to supply authority.


not my words! :)

future work #21

turadg · 2023-11-09T14:23:25Z

proposals/b:zoe1/performActions.ts

+// XXX vestige of Ava
+const context = await makeTestContext();


to be cleaned up in #21

turadg · 2023-11-09T18:36:20Z

converting to draft while I,

1. try to solve the missing refs

2023-11-09T15:04:04.1698311Z #60 [execute-upgrade-11 4/5] WORKDIR /usr/src/upgrade-test-scripts
2023-11-09T15:04:04.1699914Z #60 ERROR: failed to load ref: tdx9hly5mm4ihe08v1v26wam8: not found
2023-11-09T15:04:04.1701034Z 
2023-11-09T15:04:04.1701921Z #61 [execute-upgrade-11 3/5] COPY --link --from=prepare-upgrade-11 /root/.agoric /root/.agoric
2023-11-09T15:04:04.1703639Z #61 ERROR: failed to load ref: 9puhtidm1jttk6rwrapechdfo: not found

2. add a test of restarting vat-admin

The SDK PR tests head against head. It shows that the bug with promise watchers is gone but can't test the "repair" because it doesn't have the earlier state to repair.

So this test needs to:

Make an open offer (e.g. bid)
Core-eval to restart vat-admin vat
Make sure the offer stays open (demonstrating repair)

dckc

Looks pretty good.

I got a different hash on one of the bundles; I almost requested a change because of that, but I suppose until the proposal goes on chain, a certain amount of iteration is likely anyway, so I'll leave it to your discretion.

I went into detail about the flip side of cognitive overhead around ambient authority. It's largely orthogonal to this PR, but I hope it moves the overall conversation forward.

dckc · 2023-11-09T16:43:42Z

proposals/b:zoe1/README.md

+A3P=/opt/agoric/agoric-3-proposals
+cd packages/builders
+# build the proposal
+agoric run scripts/vats/upgrade-zoe.js | tee run-report.txt


I'm not able to reproduce these bundles.

I get 1 bundle that matches but one that does not:

b1-b75c88c4a783b0a0fb2886fea6cf2fa0ced579d25f73cbe5f7d72e4dfdf2d2c06c014875726dc5fbaf2d586b2d4f975bb3a73ac3faaeb789e5b20d556b7a9623.json

b1-a3dc21c56535b4bebdf8c8413650eec2b37a6d9f5669b27f8c571601a6f641ffae2b925dd7e3d67c6953a1f8122c8ba814305e034c9087056816a6f05dd41069.json

dckc · 2023-11-09T16:50:11Z

proposals/b:zoe1/.yarnrc.yml

@@ -0,0 +1 @@
+nodeLinker: node-modules


These yarn files are new to me, so I'm reading docs. I can't comment on the .gz file directly...

Why are we committing .yarn/install-state.gz?

.yarn/install-state.gz is an optimization file that you shouldn't ever have to commit
-- https://yarnpkg.com/getting-started/qa

Thanks for catching that. The .gitignore does ignore it, but the pattern assumes .yarn dir will only be at root. https://github.com/yarnpkg/berry/blob/master/.gitignore

I could change the ignore but WDYT of having one yarn.lock for the repo and making all the other packages workspaces? I've shied away because under the existing structure the root package would have to be in /usr/src of the Docker image, which is icky. I could refactor so that proposals is under upgrade-test-scripts, next to lib, and then upgrade-test-scripts has the one root. Though it would still require changing the gitignore so I"ll probably just do that :)

one yarn.lock for the repo sounds attractive.

I am, as usual, reticent to move things around.

I guess don't feel strongly one way or the other. This repo is still young.

proposals/b:zoe1/package.json

dckc · 2023-11-09T16:54:28Z

proposals/b:zoe1/performActions.ts

+};
+
+/**
+ * URLs of assets, including bundle hashes (to be) agreed by BLD stakers


stale comment

dckc · 2023-11-09T16:58:26Z

proposals/b:zoe1/performActions.ts

+
+// TODO move into core-eval-support
+const readSubmissions = async () => {
+  const files = await fsp.readdir('submission');


Here I need to make a mental note that fsp is accessed ambiently, so that when I see calls to readSubmissions, I know there's an extra implicit argument. This is cognitive overhead.

so that when I see calls to readSubmissions, I know there's an extra implicit argument

Why do you need to know that?

because normally, return values of top-level functions only depend on the args. In particular, a 0-argument function can only return a constant (again, modulo allocation).

This is normal local reasoning. The alternative is a burden to know the implementation details of every function in the system.

dckc · 2023-11-09T17:16:08Z

proposals/b:zoe1/performActions.ts

+// XXX vestige of Ava
+const context = await makeTestContext();
+
+const step = async (name: string, fn: Function) => {


nice: interesting low-cost way to reuse the test(...) idiom.

dckc · 2023-11-09T17:28:41Z

proposals/b:zoe1/submission/upgrade-zoe-permit.json

@@ -0,0 +1,18 @@
+{
+  "consume": {
+    "vatAdminSvc": "vatAdminSvc",


I think a few times I have given the impression that vatAdminSvc is horribly powerful. I just double-checked, and it's not. BundleCap lookup and vat creation is all VatAdminSvc provides. Vat creation is expensive, but it doesn't give the caller access to any existing vats, nor contract privateArgs, etc.

dckc · 2023-11-09T17:34:02Z

proposals/b:zoe1/submission/upgrade-zoe-permit.json

+{
+  "consume": {
+    "vatAdminSvc": "vatAdminSvc",
+    "vatStore": "vatStore",


vatStore, on the other hand, is excess authority. Much like in Agoric/agoric-sdk#8378, what this proposal needs is access to the zoe adminFacet, but vatStore is access to the adminFacet of all vats, plus the ability to update the store - overwrite or delete entries, for example.

dckc · 2023-11-09T17:34:50Z

proposals/b:zoe1/submission/upgrade-zoe-permit.json

+  "consume": {
+    "vatAdminSvc": "vatAdminSvc",
+    "vatStore": "vatStore",
+    "agoricNamesAdmin": "makeCoreProposalBehavior",


I think agoricNamesAdmin is just plain not needed. But agoric run isn't smart enough to know that. IOU another issue?

dckc · 2023-11-09T18:42:58Z

proposals/b:zoe1/submission/upgrade-zoe-permit.json

+    "vatAdminSvc": "vatAdminSvc",
+    "vatStore": "vatStore",
+    "agoricNamesAdmin": "makeCoreProposalBehavior",
+    "zoe": "makeCoreProposalBehavior"


I think makeCoreProposalBehavior uses zoe for E(zoe).install() but we're not installing anything in this proposal, so again, this is excess authority. But zoe is designed to be shared widely (modulo Agoric/agoric-sdk#3725, Agoric/agoric-sdk#4395), so not much harm.

turadg · 2023-11-09T23:48:18Z

It's passing so I suspec the missing-refs problem was solved by omitting the slogfile.

I still have to improve the test coverage but I think this is good to land for now.

turadg mentioned this pull request Nov 8, 2023

feat: Zoe use watchPromise() to wait for contract finish Agoric/agoric-sdk#8453

Merged

turadg force-pushed the 8387-zoe1 branch 2 times, most recently from 3984a5d to 989c3bf Compare November 9, 2023 00:12

turadg added 3 commits November 9, 2023 06:16

proposal b:zoe1

7923014

build: log time each build target takes

07c0087

ci: bump timeout 90 to 120

40b7f24

90 was not enough https://github.com/Agoric/agoric-3-proposals/actions/runs/6805556962/job/18505303429?pr=18

turadg force-pushed the 8387-zoe1 branch from 989c3bf to 40b7f24 Compare November 9, 2023 14:17

turadg marked this pull request as ready for review November 9, 2023 14:17

turadg commented Nov 9, 2023

View reviewed changes

turadg requested review from dckc and Chris-Hibbert November 9, 2023 14:24

turadg marked this pull request as draft November 9, 2023 18:34

dckc approved these changes Nov 9, 2023

View reviewed changes

turadg added 2 commits November 9, 2023 14:46

build: omit SLOGFILE (~1GB)

c723ae7

chore: gitignore yarn install-state

8b98509

turadg marked this pull request as ready for review November 9, 2023 23:47

turadg merged commit 4ba8231 into main Nov 9, 2023
1 check passed

turadg deleted the 8387-zoe1 branch November 9, 2023 23:48

turadg mentioned this pull request Nov 9, 2023

Test that Zoe support seats and offers through upgrade. Agoric/agoric-sdk#8502

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zoe1 proposal #18

zoe1 proposal #18

turadg commented Nov 8, 2023 •

edited

Loading

turadg Nov 9, 2023

turadg Nov 9, 2023

turadg commented Nov 9, 2023

dckc left a comment

dckc Nov 9, 2023

dckc Nov 9, 2023

turadg Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023

dckc Nov 9, 2023

dckc Nov 9, 2023

turadg Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023

dckc Nov 9, 2023

dckc Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023

dckc Nov 9, 2023 •

edited

Loading

turadg commented Nov 9, 2023

		// TODO: factor out ambient authority from these
		// or at least allow caller to supply authority.

		// XXX vestige of Ava
		const context = await makeTestContext();

zoe1 proposal #18

zoe1 proposal #18

Conversation

turadg commented Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Nov 9, 2023

1. try to solve the missing refs

2. add a test of restarting vat-admin

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 Nov 9, 2023 • 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 Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

dckc Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

turadg commented Nov 9, 2023

turadg commented Nov 8, 2023 •

edited

Loading

turadg Nov 9, 2023 •

edited

Loading

turadg Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023 •

edited

Loading

dckc Nov 9, 2023 •

edited

Loading