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

feat(cosmic-swingset): Introduce inquisitor.mjs #10807

Merged
merged 16 commits into from
Feb 27, 2025
Merged

Conversation

gibson042
Copy link
Member

refs: #10725

Description

Introduces a tool that loads a swingstore.sqlite file and allows scripted or interactive interrogation of both the database and vats in an ephemeral environment.

Security Considerations

This adds new exports for internal use by the inquisitor and related tools, entailing some layer-crossing (e.g., returning a reference to the SwingSet controller from the chain-oriented launchAndShareInternals).

Scaling Considerations

n/a

Documentation Considerations

The script includes usage information.

Testing Considerations

This tool is for internal use, and does not have unit tests. There's a risk of bit-rot, but the alternative is excessive effort on covering rich environmental functionality that might still be insufficient protection.

Upgrade Considerations

n/a

@gibson042 gibson042 requested a review from a team as a code owner January 7, 2025 03:06
Copy link

cloudflare-workers-and-pages bot commented Jan 8, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: a72158f
Status: ✅  Deploy successful!
Preview URL: https://3c7ede62.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-2025-01-inquisitor.agoric-sdk.pages.dev

View logs

@gibson042 gibson042 force-pushed the gibson-2025-01-inquisitor branch from 773757a to e39c802 Compare February 11, 2025 03:19
* used to replace the results of recursive picks (but not blanket permits)
* @returns {Attenuated<T, P>}
*/
export const attenuate = (specimen, permit, transform = x => x) => {
Copy link
Member

Choose a reason for hiding this comment

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

this is useful. why bucket it in "SES utils"? I see other stuff in here that's not specific to the ses package so not a big deal. Maybe we just rename this module at some point.

Copy link
Member

Choose a reason for hiding this comment

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

regardless, needs unit tests.

Copy link
Member Author

@gibson042 gibson042 Feb 14, 2025

Choose a reason for hiding this comment

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

this is useful. why bucket it in "SES utils"? I see other stuff in here that's not specific to the ses package so not a big deal. Maybe we just rename this module at some point.

I'd love to have it in an even more fundamental file, but it relies on Fail, which frustratingly depends upon SES, making this the lowest point it can go right now. At some point, I plan to introduce a package that propagates SES's redacting assert functionality when available but otherwise creates approximations like q = x => JSON.stringify(x) and Fail = (strings, ...subs) => { throw Error(String.raw(strings, ...subs.map(q))); }, at which point this function and probably a few others can move into js-utils.js.

*/
export const attenuate = (specimen, permit, transform = x => x) => {
// Fast-path for no attenuation.
if (permit === true || typeof permit === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

why allow string? I could see someone mistakenly thinking they can permit one thing by passing its name.

I see below,

serving as a grouping label for convenience and/or diagram generation

Why would you want that at the exclusion of an actual permit? Seems like an independent feature to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@turadg

someone mistakenly thinking they can permit one thing by passing its name.

this is exactly what I thought. Thanks for articulating it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a following a pattern established within agoric-sdk long ago, documented in deploy-script-support and used by e.g. authorityViz.js and core-eval handling. I'd defer to @dckc and/or @Chris-Hibbert for opinions on whether or not that feature should be abandoned, but I suspect we actually do want to keep it.

There's also the practical matter of TypeScript inferring { $propertyName: true } to be of type { $propertyName: boolean }, which means that const permits = { foo: 'pick' }; works but const permits = { foo: true }; does not and instead requires a type cast (and often also Prettier reformatting) to const permits = /** @type {const} */ ({ foo: true });. But for clarity in this PR, I've updated to that last form.

Copy link
Member

Choose a reason for hiding this comment

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

The core-eval extract attenuator is lazy, making a new Proxy(...) at each level. This one seems to be eager, using Object.entries(...). It's quite a different widget. So I don't see much reason to constrain the design of this one by comparison to extract.

Copy link
Member Author

Choose a reason for hiding this comment

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

extract isn't lazy; it eagerly reads from and validates the specimen just like this function. It does return a Proxy at each level, but only to implement property [[Get]] behavior that throws an error for keys outside the permit. And that's actually the reason for attenuate's optional transform parameter—so core-eval handling code can be redefined as

/**
 * @template T
 * @template {Permit<T>} P
 * @param {P} permit
 * @param {T} allPowers
 */
export const extractPowers = (permit, allPowers) => {
  if (typeof permit === 'object' && permit !== null) {
    const {
      // TODO: use these for more than just visualization.
      home: _h,
      ...effectivePermit
    } = permit;
    permit = effectivePermit;
  }
  return attenuate(allPowers, permit, (subPowers, subPermit) => {
    return new Proxy(subPowers, {
      get: (target, propName) => {
        if (typeof propName !== 'symbol') {
          propName in target ||
            Fail`${q(propName)} not permitted, only ${q(keys(subPermit))}`;
        }
        return target[propName];
      },
    });
  });
};

and thereby benefit from the typing improvements implemented here.

Copy link
Member

Choose a reason for hiding this comment

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

interesting.

OTOH, extractPowers is in the bootstrap vat, which is particularly challenging to upgrade

deleteVatTranscripts: transcriptStore.deleteVatTranscripts,
};
const transcriptStore = attenuate(transcriptStoreInternal, {
initTranscript: 'pick',
Copy link
Member

Choose a reason for hiding this comment

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

pick is misleading because it suggests a label like skip would skip but it also permits.

Any reason not to require this to be boolean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Compatibility with

if (template === true || typeof template === 'string') {
, which should be reimplemented on top of this.

Copy link
Contributor

@siarhei-agoric siarhei-agoric left a comment

Choose a reason for hiding this comment

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

LGTM

@gibson042 gibson042 force-pushed the gibson-2025-01-inquisitor branch 3 times, most recently from 3affcaa to fc9e275 Compare February 27, 2025 17:35
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Feb 27, 2025
Copy link
Contributor

mergify bot commented Feb 27, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@gibson042 gibson042 force-pushed the gibson-2025-01-inquisitor branch from fc9e275 to a72158f Compare February 27, 2025 19:47
@mergify mergify bot merged commit d1ef359 into master Feb 27, 2025
84 checks passed
@mergify mergify bot deleted the gibson-2025-01-inquisitor branch February 27, 2025 20:33
gibson042 added a commit that referenced this pull request Feb 28, 2025
gibson042 added a commit that referenced this pull request Feb 28, 2025
gibson042 added a commit that referenced this pull request Feb 28, 2025
mergify bot added a commit that referenced this pull request Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants