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

fix: split marshal from its encoder #1259

Merged
merged 5 commits into from
Aug 25, 2022
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 21, 2022

NOTE: LOW PRIORITY. NOTHING IS BLOCKED ON THIS.

The encodePassable.js module in agoric-sdk has two interesting differences from the current marshal logic.

  • It is a binary rather than JSON encoding, we may or may not have performance advantages. (We won't actually know until we measure.)
  • It is parameterized at a lower level that marshal, providing more flexibility in the reuse of encoding logic.

If it is such a great lower level of abstraction for encapsulating most encoding concerns, could we use this abstraction boundary to split marshal up into separately understandable pieces, separating concerns? This PR does approximately that, with the difference that the original encodePassable encodes directly to string, where the PR's encodeToJSON encodes to a raw JSON tree, leaving it to the caller to stringify it. Leaving aside that difference, splitting marshal in this way would make it easier to experiment with a marshal that use the encodePassable binary encoding.

This separation would make it easier to address issues like
* Agoric/agoric-sdk#4334
* Agoric/agoric-sdk#4310
* Agoric/agoric-sdk#4311
* #997
* Agoric/agoric-sdk#2780

and is a prerequisite for #1260 , which is staged on this one.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I like this separation! My biggest concerns are around maintaining the distinction between "JSON" (string) and "JSON-representable" (data structure) and naming/commenting accordingly (related: #1248). And we also need to resolve the issue around iface not being guaranteed to exist for every encoded remotable.

packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToJSON.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-split-marshal-vs-encode branch 4 times, most recently from 12b36ca to b68fd00 Compare August 24, 2022 04:07
@erights erights requested a review from gibson042 August 24, 2022 04:18
@erights
Copy link
Contributor Author

erights commented Aug 24, 2022

@gibson042 thanks for the great suggestions! I think I dealt with all of them. PTAL.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM! Remaining comments are all minor, and mostly fixing typos.

packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
packages/marshal/src/encodeToCapData.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-split-marshal-vs-encode branch from ab51707 to 1447b28 Compare August 25, 2022 17:45
@erights erights merged commit 1e3b7fc into master Aug 25, 2022
@erights erights deleted the markm-split-marshal-vs-encode branch August 25, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants