-
Notifications
You must be signed in to change notification settings - Fork 232
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: Support publishing source bundles to chain and installing hash bundles to Zoe #4975
Conversation
const { packageDescriptorText } = | ||
await readContainingPackageDescriptor( | ||
read, | ||
`file://${moduleFile}`, | ||
url.pathToFileURL(moduleFile), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something tells me "portable" also means powerful / ambient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly at least means “sensitive to platform”. The big difference is the treatment of drive letters as absolute paths instead of concatenating them like c:/users/kris/endo/d:/users/kris/
etc.
packages/publish-bundle/LICENSE
Outdated
@@ -0,0 +1,201 @@ | |||
Apache License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you scaffold this new project? Would you please leave some clues in the commit message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll leave a note. It’s only semi automatic.
64ce117
to
ad30144
Compare
c07e88c
to
05127ce
Compare
packages/agoric-cli/src/deploy.js
Outdated
@@ -1,17 +1,23 @@ | |||
/* global process setTimeout setInterval clearInterval */ | |||
/* eslint-env node */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spread the word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. I kinda like the explicit list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, indeed. I must have been cross-eyed when I wrote this because I thought that the environment might provide a better hint than any
types for all these globals to TypeScript, but this is an eslint pragma so it probably does not.
@@ -62,7 +62,8 @@ const makeZoeKit = ( | |||
shutdownZoeVat, | |||
); | |||
|
|||
const getBundleCapFromID = bundleID => E(vatAdminSvc).getBundleCap(bundleID); | |||
const getBundleCapFromID = bundleID => | |||
E(vatAdminSvc).waitForBundleCap(bundleID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warner This was the most surgical change I could make to ensure that E(zoe).install(hashBundle)
would wait for the chain to receive the bundle separately. This might be giving us a hint that getBundleCap
should behave like waitForBundleCap
, rather than having separate methods, or it might be a hint that getBundleCapFromID
defined here should be renamed waitForBundleCapWithID
or some such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine. I vaguely remember thinking that Zoe's E(zoe).install()
should wait for the bundle to be installed out-of-band, so that you can only get an installation handle for installations that are actually functional, and aren't waiting for a bundle install that might never happen. And since E(zoe).install()
is interacting with a wrapper function (which sometimes comes from the fake vatadmin service), that wrapper function should do the waiting, which means that when it calls the real vatadmin service, it should be using waitForBundleCap
instead of getBundleCap
.
I've assumed that other (non-zoe, non-agoric) uses/users of swingset and vatAdminService
may have legitimate reasons to query for a bundleID and want a negative answer (instead of an unbounded pause), which means keeping getBundleCap
as the form that doesn't wait, and Zoe can use waitForBundleCap
(which does).
And yeah, at some point renaming this zoe thing waitForBundleCapFromID
might make sense.
Incidentally, while investigating this function, I found it is named getBundleCapFromID
here and in installationStorage.js
, but getBundleCapForID
in zoe.js
, which is a bit confusing for code spelunkers.
waitForBundleCap: bundleID => { | ||
if (!idToBundleCap.has(bundleID)) { | ||
idToBundleCap.init(bundleID, fakeBundleCap()); | ||
} | ||
return Promise.resolve(idToBundleCap.get(bundleID)); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warner For the fake, I duplicated getBundleCap
. This might be another hint that I should just change the behavior of the real getBundleCap
.
removeCallback(); | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelfig This would be a good place to wait for an event from the chain telling us whether the INSTALL_BUNDLE action succeeded or failed, probably by laying a trap somewhere keyed on the hash of the bundle we attempted to install. I think I’ll need you looking over my shoulder for that project.
Otherwise, this hangs indefinitely if publishing fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct way to do that is via the WebSocket, as done for swingset deliver
transactions in chain-cosmos-sdk.js
.
p = (async () => { | ||
const bundle = JSON.parse(action.bundle); | ||
harden(bundle); | ||
return validateAndInstallBundle(bundle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this would be a good time to emit an “event” as to whether the installation worked or not. My understanding from @michaelfig is that this could arrive in the Agoric deploy script via its WebSocket connection, so publishBundle
would need a mechanism to connect a listener for a particular bundle hash’s installBundle resolution on chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after succeeding in JS, the event should be emitted from Golang, and tracked in deploy-bundle via a Tendermint RPC WebSocket subscription (not the ag-solo WebSocket).
e27a2a2
to
454afd3
Compare
Commits are individually reviewable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I browsed it; I don't see any problems.
I hope to try it out soonish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read commit by commit. I don't know much about bundling mechanics but the code LGTM.
One request: the new package.json has older deps than the rest of agoric-sdk; update those so yarn.lock doesn't take on new versions unnecessarily.
What would it cost to measure code coverage? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, and as @turadg suggested, please don't remove the SourceBundle
type.
removeCallback(); | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct way to do that is via the WebSocket, as done for swingset deliver
transactions in chain-cosmos-sdk.js
.
p = (async () => { | ||
const bundle = JSON.parse(action.bundle); | ||
harden(bundle); | ||
return validateAndInstallBundle(bundle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after succeeding in JS, the event should be emitted from Golang, and tracked in deploy-bundle via a Tendermint RPC WebSocket subscription (not the ag-solo WebSocket).
29fc204
to
28caec7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Please fix the lint.
bc20c89
to
90c545f
Compare
This stack of changes-in-progress will address:
install
#4974For manual verification, clone
dapp-fungible-faucet
, check outpublish-bundle
, and run these commands in separate sessions: