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

Core-eval governance support #5042

Merged
merged 9 commits into from
Apr 8, 2022
Merged

Core-eval governance support #5042

merged 9 commits into from
Apr 8, 2022

Conversation

michaelfig
Copy link
Member

Description

New features and tests to help make core-eval governance more reproducible.

Security Considerations

Documentation Considerations

Testing Considerations

@michaelfig michaelfig added the cosmic-swingset package: cosmic-swingset label Apr 8, 2022
@michaelfig michaelfig requested a review from dckc April 8, 2022 00:42
@michaelfig michaelfig self-assigned this Apr 8, 2022
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.

presuming you have tested that this is useful in your oracle work, looks good.

my comments are not critical

}

if (board) {
const bundlerMakerId = await E(board).getId(bundlerMaker);
Copy link
Member

Choose a reason for hiding this comment

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

this await is not top level. worth a comment to say that this is not production code? either here or at the top of the file or both?

import url from 'url';

export const makeGetBundlerMaker =
({ board, zoe, scratch }, { lookup, bundleSource }) =>
Copy link
Member

Choose a reason for hiding this comment

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

lack of types obscures the fact that board is optional. hm.

@@ -0,0 +1,38 @@
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

Add a note that this should go away after we have bundlecaps, with a pointer to and from a relevant issue?

Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

The Go file LGTM.

@michaelfig michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Apr 8, 2022
@mergify mergify bot merged commit 4bbcc69 into master Apr 8, 2022
@mergify mergify bot deleted the mfig-core-gov branch April 8, 2022 06:22
stdout.write(trimmed);
};

if (isEntrypoint(import.meta.url)) {
Copy link
Member

Choose a reason for hiding this comment

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

interesting approach. isEntrypoint is powerful, but I suppose so was require.main.

A Stackoverflow post has a succinct version:

if (import.meta.url === `file://${process.argv[1]}`) {
  // module was not imported but called directly
}

but that's less robust; it's followed by one that uses url.pathToFileURL as in isEntrypoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates cosmic-swingset package: cosmic-swingset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants