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

missing Remotable and PassByRef types #1488

Closed
turadg opened this issue Feb 7, 2023 · 9 comments · Fixed by #1933 or #2238
Closed

missing Remotable and PassByRef types #1488

turadg opened this issue Feb 7, 2023 · 9 comments · Fixed by #1933 or #2238
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@turadg
Copy link
Member

turadg commented Feb 7, 2023

The description is correct but its type is aliased to Passable (which is wrong). Moreover since Passable is any, Passable is any.

/**
* @typedef {*} Passable
*
* A Passable value that may be marshalled. It is classified as one of
* PassStyle. A Passable must be hardened.
*
* A Passable has a pass-by-copy superstructure. This includes
* * the atomic pass-by-copy primitives ("undefined" | "null" |
* "boolean" | "number" | "bigint" | "string" | "symbol"),
* * the pass-by-copy containers
* ("copyRecord" | "copyArray" | "tagged") that
* contain other Passables,
* * and the special cases ("error" | "promise").
*
* A Passable's pass-by-copy superstructure ends in
* PassableCap leafs ("remotable" | "promise"). Since a
* Passable is hardened, its structure and classification is stable --- its
* structure and classification cannot change even if some of the objects are
* proxies.
*/
/**
* @callback PassStyleOf
* @param {Passable} passable
* @returns {PassStyle}
*/
/**
* @typedef {Passable} PureData
*
* A Passable is PureData when its pass-by-copy superstructure whose
* nodes are pass-by-copy composites (CopyArray, CopyRecord, Tagged) leaves are
* primitives or empty composites. No remotables, promises, or errors.
*
* This check assures purity *given* that none of these pass-by-copy composites
* can be a Proxy. TODO SECURITY BUG we plan to enforce this, giving these
* pass-by-copy composites much of the same security properties as the
* proposed Records and Tuples (TODO need link).
*
* Given this (currently counter-factual) assumption, a PureData value cannot
* be used as a communications channel,
* and can therefore be safely shared with subgraphs that should not be able
* to communicate with each other.
*/
/**
* @typedef {Passable} Remotable
* Might be an object explicitly declared to be `Remotable` using the
* `Far` or `Remotable` functions, or a remote presence of a Remotable.
*/
/**
* @typedef {Promise | Remotable} PassableCap
* The authority-bearing leaves of a Passable's pass-by-copy superstructure.
*/

https://docs.agoric.com/guides/js-programming/far.html

Remotable = Far object or local presence for a Far object (can be done with a tag b/c only Far() and unserialize return it)

PassByCopy needs a def.

PureData is deeply PassByCopy. (currently Passable/any)

PassByRef = Remotable | Promise<Remotable> | Promise<PassByCopy> (in snippet as PassableCap)

Passable = PassByRef | PassByCopy (but probably not feasible to fully define)

@gibson042 points out adjustments may be needed for error handling

@turadg turadg added bug Something isn't working documentation Improvements or additions to documentation labels Feb 7, 2023
@turadg turadg self-assigned this Feb 7, 2023
@erights
Copy link
Contributor

erights commented Feb 8, 2023

PassByCopy !== PureData

PassByCopy is shallow. PureData is deeply PassByCopy. harden([Far('x',{})]) is a PassByCopy CopyArray, but is not PureData because it contains a Remotable.

@erights
Copy link
Contributor

erights commented Feb 8, 2023

@gibson042 points out adjustments may be needed for error handling

Points out where? Or what?

@erights
Copy link
Contributor

erights commented Feb 8, 2023

Remotable = Far object or local presence for a Far object (can be done with a tag b/c only Far() and serialize return it)

Did you mean "unserialize"?

@erights
Copy link
Contributor

erights commented Feb 8, 2023

PassByRef = Remotable | Promise | Promise (in snippet as PassableCap)

What does the third disjunct mean?

@turadg
Copy link
Member Author

turadg commented Feb 8, 2023

For context, this issue body was from a huddle (@gibson042, @dckc and me)

PassByCopy !== PureData

Thanks. I've updated the body.

Points out where? Or what?

It was merely a verbal statement like "adjustments may be needed for error handling" (paraphrase)

Did you mean "unserialize"?

Yes, corrected.

What does the third disjunct mean?

Markdown interpreted the <> as HTML tag. I've now wrapped in backticks.

@kriskowal
Copy link
Member

What’s the status of this change?

@kriskowal kriskowal added the kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 label Jan 8, 2024
@turadg
Copy link
Member Author

turadg commented Jan 8, 2024

What’s the status of this change?

This is an issue so no changes have yet been made.

I'd still like to see this problem solved. The any cropped up for me again in #1928 just now.

I'll see if I can make some progress today

@mhofman
Copy link
Contributor

mhofman commented Jan 8, 2024

Wondering if this is linked, and possibly was worked around by the manual type generation step before publish

@aj-agoric
Copy link

Decided this is high priority in the endo project meeting cc @turadg

@aj-agoric aj-agoric removed the kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 label Jan 31, 2024
turadg added a commit that referenced this issue May 2, 2024
closes: #1488

## Description

Reattempting:
-  #1933

Again:
- Makes `Passable` a generic type instead of `any`.
- Defines overloads for `passStyleOf` to return the actual style.
- Defines the `Key` in terms of `Passable`
- Makes a ton of fixes and suppressions in places that relied on the
previous `any`'s

### Security Considerations

n/a

### Scaling Considerations

n/a

### Documentation Considerations

Better types are better documentation.

### Testing Considerations

- [ ] Agoric/agoric-sdk#8774

### Upgrade Considerations

These changes may cause type errors downstream. I don't think we should
call those breaking change à la semver because they don't affect the
runtime.
turadg added a commit that referenced this issue May 21, 2024
refs: #1488 

## Description

Simplify build by using NPM lifecycle hooks. Adds a CI test for the
package graph sensitivity that the extant method was checking.

Part of making it work was adding "typescript" to `devDependencies` of
packages that need it. The root `build:types` was running with the root
`tsc` but when building in each package separately they wouldn't get the
root's typescript version and fall back to the global one, which didn't
know the `@import` syntax.


### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

Less to document 

### Testing Considerations

I tried the "publish" commands in CONTRIBUTING.md. 

integration PR in agoric-sdk:
Agoric/agoric-sdk#9385

### Compatibility Considerations

Removes a necessary accommodation on agoric-sdk

### Upgrade Considerations

None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants