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

Why is makeZoeInstanceStorageManager()'s instance parameter typed M.or(InstanceHandleShape, BundleShape)? #8645

Open
Chris-Hibbert opened this issue Dec 11, 2023 · 2 comments · May be fixed by #8699
Assignees
Labels
hygiene Tidying up around the house research Zoe package: Zoe

Comments

@Chris-Hibbert
Copy link
Contributor

Chris-Hibbert commented Dec 11, 2023

What is the Problem Being Solved?

In #8642, I noticed that the instance parameter to makeZoeInstanceStorageManager() is typed M.or(InstanceHandleShape, BundleShape). The instance parameter is stored in instanceRecordStorage, so the type seems wrong.

Description of the Design

installBundle also takes M.or(InstanceHandleShape, BundleShape). Is it related?

While I'm looking, also investigate this line in startInstance. ZoeStorageManager has a startInstanceAccess facet with these two methods. What gets passed to makeStartInstance()?

@param {Pick<ZoeStorageManager, 'makeZoeInstanceStorageManager' | 'unwrapInstallation'>} startInstanceAccess

Security Considerations

Not a security issue

Scaling Considerations

N/A

Test Plan

No plan to test anything.

Upgrade Considerations

Changing types impacts upgrade.

@Chris-Hibbert
Copy link
Contributor Author

in #8642, I commented

#4974 and #4975 make me doubt my instincts. I wouldn't change anything here now.

There seem to be places that insist that parameter must be an InstanceHandle, and others that insist that it must be a Bundle. The PRs I just referenced seem to expect the argument to installBundle() to be allowed to be any of multiple types, but I don't see how Zoe can be storing all of those under the name instance in the instanceRecord, which is how this parameter is being used.

Chris-Hibbert added a commit that referenced this issue Dec 28, 2023
closes: #8645

Two typeGuards in Zoe allowed both where they should have had one or
the other.
@Chris-Hibbert Chris-Hibbert linked a pull request Dec 28, 2023 that will close this issue
1 task
@Chris-Hibbert
Copy link
Contributor Author

makeZoeInstanceStorageManager() stores the instance parameter in the instance field of the instanceRecord, which is typed as Instance. The only place where makeZoeInstanceStorageManager() is called the parameter has just been initialized with makeInstanceHandle();. So both source and consumer insist it must be an InstanceHandle.

zoeStorageManager.installBundle() has a guard

    installBundle: M.call(M.or(InstanceHandleShape, BundleShape))
      .optional(M.string())
      .returns(M.promise())
export const BundleShape = M.and(
  M.splitRecord({ moduleFormat: M.any() }),
  M.recordOf(M.string(), M.string({ stringLengthLimit: Infinity })),
);

installBundle immediately passes its parameters to installationStorage.installBundle(). Its guard is

    installBundle: M.call(
      M.or(
        InstanceHandleShape,
        M.recordOf(M.string(), M.string({ stringLengthLimit: Infinity })),
      ),
    )
      .optional(M.string())
      .returns(M.promise())

the type declaration for zoeStorageManager's installBundle is

/**
 * @callback InstallBundle
 *
 * Create an installation by safely evaluating the code and
 * registering it with Zoe. Returns an installation.
 *
 * @param {Bundle | SourceBundle} bundle
 * @param {string} [bundleLabel]
 * @returns {Promise<Installation>}
 */

The type of installationStorage's version is undeclared.

AFAICT, that TypeGuard was introduced by #5997, and the implementation then matched what it does now. I'm going to drop InstanceHandleShape from the definition.

Chris-Hibbert added a commit that referenced this issue Dec 29, 2023
closes: #8645

Two typeGuards in Zoe allowed both where they should have had one or
the other.
Chris-Hibbert added a commit that referenced this issue May 30, 2024
closes: #8645

Two typeGuards in Zoe allowed both where they should have had one or
the other.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hygiene Tidying up around the house research Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant