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: Start migrating encodePassable and rankOrder from agoric-sdk to endo #1260

Merged
merged 5 commits into from
Nov 7, 2022

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 21, 2022

For our passable data types, mostly, we have a clean abstraction layering between the "marshal" layer (here in the endo repository) and the "store" layer (currently in the agoric-sdk repository). However, two modules (rankOrder and encodePassable) currently at the store layer really belong at the marshal layer.

The encodePassable API and marshal's encodeToCapData and encodeToSmallcaps APIs have converged to the degree that they should. They can be understood together. Before smallcaps, I originally wanted to do this in the hope that encodePassable would become an alternate to capData for general use, but @warner 's criticisms below are likely fatal. The remaining purpose of encodePassable is specialized uses for which a sort-order-preserving string encoding is useful. Nevertheless, because of the conceptual similarity and layering, it is clearer for these to live here.

See also Agoric/agoric-sdk#4435 which we should make progress on as part of a larger project to explain these two abstraction levels and how they relate.

See also Agoric/agoric-sdk#6260 for keeping these two in sync until the situation simplifies.

@erights erights self-assigned this Aug 21, 2022
@erights erights force-pushed the markm-split-marshal-vs-encode branch from 3467d9b to 7c298e2 Compare August 21, 2022 05:09
@erights erights marked this pull request as draft August 22, 2022 02:34
@erights erights requested a review from kriskowal August 22, 2022 02:34
@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:35
@erights erights force-pushed the markm-split-marshal-vs-encode branch from ab51707 to 1447b28 Compare August 25, 2022 17:45
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I’m excited to see this move up and generally excited about separating the JSON/binary representation layers. I also see a lot of similarity to Syrup in this binary encoding. Similar but not the same, of course.

I can give this a closer read when it gets higher in priority.

packages/marshal/src/encodePassable.js Outdated Show resolved Hide resolved
Base automatically changed from markm-split-marshal-vs-encode to master August 25, 2022 23:09
@erights erights force-pushed the markm-binary-marshal branch 2 times, most recently from 429f29b to 9a37743 Compare August 27, 2022 22:06
@erights erights changed the base branch from master to markm-dean-smaller-json September 19, 2022 02:01
@erights erights marked this pull request as ready for review September 19, 2022 03:54
@erights erights changed the title Start migrating encodePassable and rankOrder from agoric-sdk to endo fix: Start migrating encodePassable and rankOrder from agoric-sdk to endo Sep 19, 2022
@erights erights mentioned this pull request Sep 21, 2022
@erights erights force-pushed the markm-dean-smaller-json branch 2 times, most recently from f0e5e87 to ae8d7e6 Compare September 23, 2022 02:39
Base automatically changed from markm-dean-smaller-json to master September 23, 2022 07:35
@erights erights force-pushed the markm-binary-marshal branch 2 times, most recently from 98bb329 to e2d0a4e Compare September 26, 2022 20:37
@erights erights force-pushed the markm-binary-marshal branch 2 times, most recently from f08817f to d0403e4 Compare September 29, 2022 03:10
@erights erights changed the base branch from master to 1303-markm-unserialize-without-assign September 29, 2022 03:11
@erights erights force-pushed the 1303-markm-unserialize-without-assign branch from d63cd69 to 84a183a Compare October 1, 2022 03:31
Base automatically changed from 1303-markm-unserialize-without-assign to master October 1, 2022 03:36
@erights erights force-pushed the markm-binary-marshal branch 3 times, most recently from 91bd78f to 013fc1d Compare October 3, 2022 17:37
@erights erights requested a review from mhofman October 11, 2022 23:53
@erights erights force-pushed the markm-binary-marshal branch 2 times, most recently from 607e292 to 499971c Compare October 20, 2022 19:38
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've made it through encodePassable.js, and will come back later for rankOrder.js.

packages/marshal/src/encodePassable.js Outdated Show resolved Hide resolved
packages/marshal/src/encodePassable.js Show resolved Hide resolved
@gibson042 gibson042 self-requested a review October 21, 2022 20:22
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 feel most strongly about PassStyleRankAndCover being out-of-place in rankOrder.js (at least while it hard-codes prefix sigils from encodePassable.js), but everything appears like it will function correctly. I support merging now or fixing first, whichever is least disruptive to the relationship between endo and agoric-sdk.

packages/marshal/src/encodePassable.js Outdated Show resolved Hide resolved
packages/marshal/src/encodePassable.js Outdated Show resolved Hide resolved
packages/marshal/src/encodePassable.js Outdated Show resolved Hide resolved
packages/marshal/src/rankOrder.js Outdated Show resolved Hide resolved
packages/marshal/src/rankOrder.js Show resolved Hide resolved
packages/marshal/src/rankOrder.js Outdated Show resolved Hide resolved
packages/marshal/src/rankOrder.js Outdated Show resolved Hide resolved
packages/marshal/src/rankOrder.js Outdated Show resolved Hide resolved
packages/marshal/src/rankOrder.js Outdated Show resolved Hide resolved
packages/marshal/test/test-encodePassable.js Show resolved Hide resolved
@erights erights force-pushed the markm-binary-marshal branch 2 times, most recently from f4a2682 to 132ff75 Compare November 5, 2022 06:13
@erights erights merged commit a025103 into master Nov 7, 2022
@erights erights deleted the markm-binary-marshal branch November 7, 2022 06:14
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.

4 participants