-
Notifications
You must be signed in to change notification settings - Fork 206
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
clarify CoreEval vs proposal #9468
Conversation
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 good.
I'd like to discuss just a bit before approving.
getBundlerMaker: helpers.getBundlerMaker, | ||
}); | ||
}, | ||
/** @deprecated use writeCoreEval */ | ||
get writeCoreProposal() { |
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.
good to see this is is not a breaking change.
We document writeCoreProposal
externally, and I'm pretty sure it's used by 3rd parties.
https://docs.agoric.com/guides/agoric-cli/#agoric-run ->
#8087 (comment)
sourceSpec: '@agoric/orchestration/src/proposals/start-stakeAtom.js', | ||
sourceSpec: '@agoric/orchestration/src/core/start-stakeAtom.js', |
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.
did you consider renaming start-stakeAtom.js
to stakeAtom.deploy.js
as well?
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.
ooh, I like that. so a .deploy.js
is a module that exports a manifest and its referenced functions for use in a build script? I'm reluctant do to that since the module shape isn't uniform. Every export name changes from one to the next, even if certain exports are supposed to fit a pattern. Maybe if we had a new deploy module format that had a export const meta
for looking things up.
Regardless, I consider all that out of scope for clarifying CoreEval vs Proposal.
thanks for the breakdown by commit at first it looked like a lot of churn, but viewed by commit it's easy to review quickly |
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.
Nice!
* @param {import('./externalTypes.js').ProposalBuilder} proposalBuilder | ||
* @callback WriteCoreEval write to disk the files needed for a CoreEval (permits, code, and the bundles the code loads) | ||
* see CoreEval in {@link '/golang/cosmos/x/swingset/types/swingset.pb.go'} | ||
* @param {string} filePrefix - name on disk |
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.
* @param {string} filePrefix - name on disk | |
* @param {string} filePrefix - name on disk (code will be written to | |
* ${filePrefix}.js, permits to ${filePrefix}-permit.json, and an overall | |
* summary to ${filePrefix}-plan.json) |
* @param {{ | ||
* publishRef: PublishBundleRef, | ||
* install: InstallEntrypoint, | ||
* wrapInstall?: <T extends InstallEntrypoint>(f: T) => T } | ||
* } powers | ||
* @param {...any} args | ||
* @returns {Promise<ProposalResult>} | ||
* @returns {Promise<{sourceSpec: string, getManifestCall: [exportedGetManifest: string, ...manifestArgs: any[]]}>} |
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.
This is a huge improvement, but I still find myself frustrated by the names.
* @returns {Promise<{sourceSpec: string, getManifestCall: [exportedGetManifest: string, ...manifestArgs: any[]]}>} | |
* @returns {Promise<{sourceSpec: string, getManifestCall: [manifestGetterName: string, ...manifestGetterArgs: any[]]}>} |
or possibly
* @returns {Promise<{sourceSpec: string, getManifestCall: [exportedGetManifest: string, ...manifestArgs: any[]]}>} | |
* @returns {Promise<CoreEvalDescriptor>} |
with
/**
* @typedef CoreEvalDescriptor
* @property {string} sourceSpec import specifier for a module
* @property {[manifestGetterName: string, ...manifestGetterArgs: any[]]} getManifestCall
* the name of a function exported by the module and arguments to invoke it
* with in order to get a manifest (a Record that associates functions to be
* invoked and permits defining bootstrap-space powers they will have access
* to, see {@link ../README.md} and {@link runModuleBehaviors})
*/
|
||
const proposalNS = await import(pathResolve(sourceSpec)); | ||
const evalNS = await import(pathResolve(sourceSpec)); |
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.
Is the "NS" (for "[module] namespace") suffix sufficiently clear? If we're renaming anyway, perhaps something like moduleRecord
or imported
or namespace
would be better?
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 read ending NS
as "namespace". We use that convention in 15 files in the repo.
but i don't feel strongly so I'll change it.
const proposal = await deeplyFulfilled( | ||
harden(proposalBuilder({ publishRef, install })), | ||
// Create the eval structure. | ||
const evalParts = await deeplyFulfilled( |
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.
Naming suggestion:
const evalParts = await deeplyFulfilled( | |
const evalDescriptor = await deeplyFulfilled( |
@@ -11,7 +11,7 @@ export const defaultProposalBuilder = async ( | |||
bondDenom = 'uatom', | |||
} = options; | |||
return harden({ | |||
sourceSpec: '@agoric/orchestration/src/proposals/start-stakeAtom.js', | |||
sourceSpec: '@agoric/orchestration/src/core/start-stakeAtom.js', |
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 README currently includes
There are collections of proposals in .../vats/src/proposals, smart-wallet/src/proposals, orchestration/src/proposals, pegasus/src/proposals.
and also references to specific paths. Does it merit an update?
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.
When I went to make this change, seeing this one in the list be different, I realized we're probably better off making the change all at once across packages. I'm going to remove that filename change from this PR.
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.
// If we like this, move it into deploy-script-support. | ||
// Consider also `.permit.json` to distinguish the module shape qualifier ('permit') from the identifier. (Similarly `.plan.json`.) | ||
const prefix = (evalName = '') => { | ||
const proposalPath = import.meta.url; | ||
const left = proposalPath.lastIndexOf('/') + 1; | ||
const right = proposalPath.lastIndexOf('.'); | ||
const filename = proposalPath.substring(left, right); | ||
return `${filename}-${evalName}`; | ||
}; |
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.
These names seems misleading to me, since the behavior is actually to suffix a base component.
// If we like this, move it into deploy-script-support. | |
// Consider also `.permit.json` to distinguish the module shape qualifier ('permit') from the identifier. (Similarly `.plan.json`.) | |
const prefix = (evalName = '') => { | |
const proposalPath = import.meta.url; | |
const left = proposalPath.lastIndexOf('/') + 1; | |
const right = proposalPath.lastIndexOf('.'); | |
const filename = proposalPath.substring(left, right); | |
return `${filename}-${evalName}`; | |
}; | |
// If we like this, move it into deploy-script-support. | |
// Consider also `.permit.json` to distinguish the module shape qualifier ('permit') from the identifier. (Similarly `.plan.json`.) | |
const getBaseName = (keepExtension = false) => { | |
const proposalPath = import.meta.url; | |
const baseName = proposalPath.split('/').at(-1); | |
return keepExtension ? baseName : baseName.replace(/[.][^.]*$/, ''); | |
}; |
with
await writeCoreEval(`${getBaseName()}-gov`, defaultProposalBuilder);
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 idea was "this generates the prefix for writeCoreEval". If we were to move the function out, it would be just,
const getBaseName = (proposalPath, keepExtension = false) => {
const baseName = proposalPath.split('/').at(-1);
return keepExtension ? baseName : baseName.replace(/[.][^.]*$/, '');
};
because we can import import.meta.url
. At that point it's basically Node's path.basename
. I'll drop this commit too and push a separate PR to hash out the design.
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.
b6bc629
to
ef80a22
Compare
Deploying agoric-sdk with Cloudflare Pages
|
ef80a22
to
ed37a0b
Compare
more accurate. What's built is a code/permit pair (and its bundles), not a proposal (which has a title and a list of core evals)
ed37a0b
to
593361d
Compare
Build failed on ,
I suspect because of importing I've reverted that change so this can merge. |
Description
I was recently confused for the nth time on why writeCoreProposal wasn't writing a proposal. Now that we model proposals in
a3p-integration
(something proposed to stakers) I've been running into this confusion more often.CoreEvalProposal has a title and
evals
which isis an array of CoreEval. That's the thing that has permits and code, thatwriteCoreProposal
is writing out.This names what it's creating
CoreEval
. It's not exactly the CoreEval type but it's 1:1 with a CoreEval so I think it's warranted.Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
Improves clarity. I don't know what external docs need to be updated.
Testing Considerations
CI should suffice
Upgrade Considerations
It should be fully backwards compatible as I maintained the module exports. I renamed a module but it shouldn't have been deep imported anywhere.