feat: integrate JSON.stringify/parse in board marshallers #6646

dckc · 2022-12-07T16:58:04Z

based on feedback, plan is to add support for this in endo, starting with

marshal method names are misleading endojs/endo#1248
- clearer marshal method names endojs/endo#1402

Description

I think we're seeing the same "long time to serialize" problem ...

That reminded me of the two round trips in each wallet state update - one for serialize and one for setValue:

by changing E(marshaller).serialize(value).then(x => E(storageNode).setValue(JSON.stringify(x)) to E(storageNode).setValue(E(marshaller).serializeAndStringify(x)) we can save a round trip for each:

gist with before and after .svg diagrams

Drawbacks

we were using serialize from the marshal API. serializeAndStringify means a distinct interface/type threaded through all the places that use storage nodes.
- It's a little awkward, but saving a round trip is worth it, yes?

Security Considerations

setValue() takes a promise now. Using pattern guards in makeChainStorageRoot would let us use callAfter as well as the usual security benefits

Documentation Considerations

?

Testing Considerations

serializeAndStringify unit test in test-lib-board.js

dckc · 2022-12-07T19:52:51Z

@warner @michaelfig @gibson042 @turadg

I'd like to get some feedback on this in draft form.

gibson042

I like the approach, but I hate the naming (although that's nothing new; this is just further fallout from failing to address endojs/endo#1248 ). But yes, I agree that saving the unnecessary round trips seems worthwhile.

gibson042 · 2022-12-07T20:38:39Z

packages/vats/src/lib-board.js

+    withStringify({
+      ...makeMarshal(val => {


I've we're going to adopt this, why not have makeMarsal (if not an even deeper layer) provide the composite method?

I'd like to get some feedback on this in draft form.

I echo @gibson042's desire for clearer names and to consider putting this into Endo instead.

IMO it's not too late to advance endojs/endo#1248 . The new method serializeAndStringify could be more clear as toCapDataAndStringify or stringifyAsCapData. I'd suggest serializeAsCapData but that term is ambiguous now (ergo issue 1248).

OK, cool... so pushing it down into endo is pretty clearly The Right Thing. (I wasn't aware of 1248 and I didn't know how receptive others would be to the suggestion.)

I agree it is still not too late to rename the marshal methods. I approved endojs/endo#1402 last year and my LGTM still stands. There is no unresolved "Request changes".

I know from painful experience that renaming without breaking things can be painful but worth it. endojs/endo#1402 is the right first step -- it just introduces the new names while deprecating the old.

I am open to having makeMarshal provide the composite methods. This makes more sense to me, but I'll of course withhold judgement until I see what it looks like. But please do endojs/endo#1402 first, unless there's an impediment I'm not seeing.

dckc · 2023-01-23T18:34:58Z

@erights @warner this is the one that is pending endojs/endo#1248

turadg · 2023-01-23T18:47:04Z

pending endojs/endo#1248

I expect we can resolve naming considerations (including 1248) after landing this as a feature.

erights · 2023-01-23T19:19:54Z

Where is the definition of serializeAndStringify? I couldn't find it.

dckc · 2023-01-23T20:00:45Z

Where is the definition of serializeAndStringify? I couldn't find it.

https://github.com/Agoric/agoric-sdk/pull/6646/files#diff-298ac87f534f4fb60e42f478a8a866d0eb2f4e4853f6c7cb12c7f99c92594b2eR64

p.s. that didn't work as expected. But it's one line:

    serializeAndStringify: v => JSON.stringify(m.serialize(v)),
  });

dckc · 2023-04-14T17:12:43Z

What next?

I updated this branch. Since the board became upgradeable, the rebase was so messy I just re-did the changes.

Now where are we on naming? I see toCapData landed in endo (endojs/endo#1402) but it seems to be part of the pile of things waiting for endo/agoric-sdk sync (#7090).

I see

I expect we can resolve naming considerations (including 1248) after landing this as a feature.

Is that still the case? We can land this as serialzeAndStringify?

mhofman · 2023-04-14T17:18:19Z

Is that still the case? We can land this as serialzeAndStringify?

I still believe the name is not right. CapData is an encoding, not a serialization. I would rather encodeAndStringify or toCapDataAndStringify.

Edit: to clarify, the output of toCapData is something that cannot be stored or transmitted as-is by our current abstractions, and thus does not qualify the definition of serialization. JSON.stringify is however a serialization.

In computing, serialization (or serialisation) is the process of translating a data structure or object state into a format that can be stored (e.g. files in secondary storage devices, data buffers in primary storage devices) or transmitted (e.g. data streams over computer networks) and reconstructed later (possibly in a different computer environment).[1] When the resulting series of bits is reread according to the serialization format, it can be used to create a semantically identical clone of the original object.

dckc · 2023-04-14T17:23:48Z

@mhofman suggested integrating JSON.parse as well as JSON.stringify for similar reasons.

turadg · 2023-04-14T17:47:56Z

packages/vats/src/lib-board.js

+        parseAndDecode(str) {
+          const data = JSON.parse(str);
+          const readonly = makeReadonlyMarshaller(this.state);
+          return readonly.unserialize(data);


this should be fromCapData. the Endo sync is imminent and will save some churn to wait for it: https://github.com/Agoric/agoric-sdk/pull/7273/files#diff-298ac87f534f4fb60e42f478a8a866d0eb2f4e4853f6c7cb12c7f99c92594b2eR22

though I suppose this could land first and that PR could update the call.

will save some churn to wait for it

ok

gibson042

I agree with #6646 (comment) regarding naming.

gibson042 · 2023-04-14T18:08:10Z

packages/internal/src/lib-chainStorage.js

+    // @ts-expect-error M.callWhen
+    return E(storageNode).setValue(serializedP);


Shouldn't this include generalization of StorageNode setValue to include awaiting its argument?

Suggested change

// @ts-expect-error M.callWhen

return E(storageNode).setValue(serializedP);

return E(storageNode).setValue(serializedP);

IIUC, M.callWhen ensures the promise is resolved before calling the setValue method.

Right, but I don't see the expected update to introduce that M.callWhen.

update? isn't it already there?

agoric-sdk/packages/internal/src/lib-chainStorage.js

Line 39 in 2b027a4

setValue: M.callWhen(M.string()).returns(),

Hmm, then what's the TS error?

The TS error is because the caller passes a Promise<string> and the callee is declared to accept string. tsc isn't aware of M.callWhen fixing things up in the middle. Hence @tsc-expect-error M.callWhen.

I wonder if it wouldn't make more sense to deal with that at the source in lib-chainStorage.js by fibbing like /** @type {(value: ERef<string>) => Promise<void>} */, but if not then maybe provide more context here?

Suggested change

// @ts-expect-error M.callWhen

return E(storageNode).setValue(serializedP);

// @ts-expect-error M.callWhen auto-awaits serializedP for setValue(value: string)

return E(storageNode).setValue(serializedP);

But I get the shorthand now so these are completely non-blocking.

fibbing means the setValue implementation won't be typechecked correctly. But I suppose it would make sense to put 1 @ts-expect-error or the like there rather than 1 for every caller.

Reviewing @endo/patterns, it also looks like ChainStorageNodeI isn't quite right... I think it needs the following change:

const ChainStorageNodeI = M.interface('StorageNode', { - setValue: M.callWhen(M.string()).returns(), + setValue: M.callWhen(M.await(M.string())).returns(), getPath: M.call().returns(M.string()), getStoreKey: M.callWhen().returns(M.record()), makeChildNode: M.call(M.string()) .optional(M.splitRecord({}, { sequence: M.boolean() }, {})) .returns(M.remotable('StorageNode')), });

gibson042 · 2023-04-14T18:13:22Z

packages/vats/src/lib-board.js

@@ -326,7 +327,6 @@ export const prepareBoardKit = baggage => {
      /**
       * @param {string} id
       * @throws if id is not in the mapping
-       * @returns {Marshaller}


Why is this being removed?

Because Marshaller doesn't have the 2 new methods.

Suggested change

* @returns {Marshaller}

* @returns {Marshaller & {

serializeAndStringify: (value: unknown) => string,

parseAndDecode: (value: string) => any,

}}

or better yet,

// TODO Move into endo. /** * @typedef {Marshaller & { * serializeAndStringify: (value: unknown) => string, * parseAndDecode: (value: string) => any, * }} ExtendedMarshaller */

Suggested change

* @returns {Marshaller}

* @returns {ExtendedMarshaller}

We typically leave return types inferred, no?

But yes, this type is needed in lots of other places, so I have started on an ExtendedMarshaller type. Might as well use it here, I suppose.

We typically leave return types inferred, no?

I don't know about typically, but my own practice is that when typing a function at all, I usually state the return type I expect explicitly, in order to catch surprises earlier.

dckc · 2023-05-08T04:21:15Z

Cleared the milestone at @turadg 's suggestion.

dckc force-pushed the dc-save-rt branch from 7e74fbf to 82b92b1 Compare December 7, 2022 16:58

dckc requested review from gibson042, michaelfig, turadg and warner December 7, 2022 19:52

gibson042 reviewed Dec 7, 2022

View reviewed changes

ivanlei requested a review from erights January 23, 2023 18:34

This was referenced Jan 27, 2023

6765 publish vaultManager's price feed #6862

Merged

clearer marshal method names endojs/endo#1402

Merged

dckc added this to the Vaults EVP milestone Feb 9, 2023

dckc mentioned this pull request Feb 15, 2023

6689 fix watchPurse race #6999

Merged

dckc self-assigned this Feb 15, 2023

dckc mentioned this pull request Feb 16, 2023

feat(board)!: migrate to smallCaps for boardMarshaller, vstorage, smartWallet #7013

Closed

5 tasks

dckc force-pushed the dc-save-rt branch from 82b92b1 to a59f8cc Compare April 14, 2023 17:05

dckc changed the title ~~serializeAndStringify to save a round-trip (WIP)~~ serializeAndStringify to save a round-trip Apr 14, 2023

dckc marked this pull request as ready for review April 14, 2023 17:08

dckc changed the title ~~serializeAndStringify to save a round-trip~~ feat: integrate JSON.stringify/parse in board marshallers Apr 14, 2023

turadg reviewed Apr 14, 2023

View reviewed changes

gibson042 reviewed Apr 14, 2023

View reviewed changes

erights mentioned this pull request Apr 19, 2023

feat(marshal): cheap capData stringify and parse endojs/endo#1558

Closed

dckc added 2 commits May 6, 2023 18:07

feat: combined serialize + stringify on board marshaller

ac85e43

feat: combined JSON.parse and unserialize on board marshaller

391c091

dckc added 2 commits May 6, 2023 18:18

fixup! feat: combined serialize + stringify on board marshaller

d01b447

fixup! feat: combined serialize + stringify on board marshaller

241e85b

dckc force-pushed the dc-save-rt branch from bd8ff72 to 241e85b Compare May 6, 2023 23:18

dckc marked this pull request as draft May 6, 2023 23:19

dckc removed this from the Vaults EVP milestone May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate JSON.stringify/parse in board marshallers #6646

feat: integrate JSON.stringify/parse in board marshallers #6646

dckc commented Dec 7, 2022 •

edited

Loading

dckc commented Dec 7, 2022

gibson042 left a comment

gibson042 Dec 7, 2022

turadg Dec 7, 2022

dckc Dec 7, 2022

erights Jan 27, 2023 •

edited

Loading

dckc commented Jan 23, 2023

turadg commented Jan 23, 2023

erights commented Jan 23, 2023

dckc commented Jan 23, 2023 •

edited

Loading

dckc commented Apr 14, 2023

mhofman commented Apr 14, 2023 •

edited

Loading

dckc commented Apr 14, 2023

turadg Apr 14, 2023 •

edited

Loading

dckc Apr 14, 2023

gibson042 left a comment

gibson042 Apr 14, 2023

dckc Apr 14, 2023

gibson042 Apr 14, 2023

dckc Apr 14, 2023

gibson042 Apr 14, 2023

dckc Apr 14, 2023

gibson042 Apr 14, 2023

dckc Apr 14, 2023

gibson042 Apr 15, 2023

gibson042 Apr 14, 2023

dckc Apr 14, 2023

gibson042 Apr 14, 2023

dckc Apr 14, 2023

erights Apr 14, 2023

dckc commented May 8, 2023

		// @ts-expect-error M.callWhen
		return E(storageNode).setValue(serializedP);

-       * @returns {Marshaller}
+       * @returns {Marshaller & {
+           serializeAndStringify: (value: unknown) => string,
+           parseAndDecode: (value: string) => any,
+         }}

feat: integrate JSON.stringify/parse in board marshallers #6646

Are you sure you want to change the base?

feat: integrate JSON.stringify/parse in board marshallers #6646

Conversation

dckc commented Dec 7, 2022 • edited Loading

Description

Drawbacks

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Dec 7, 2022

gibson042 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

dckc commented Jan 23, 2023

turadg commented Jan 23, 2023

erights commented Jan 23, 2023

dckc commented Jan 23, 2023 • edited Loading

dckc commented Apr 14, 2023

mhofman commented Apr 14, 2023 • edited Loading

dckc commented Apr 14, 2023

turadg Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented May 8, 2023

dckc commented Dec 7, 2022 •

edited

Loading

erights Jan 27, 2023 •

edited

Loading

dckc commented Jan 23, 2023 •

edited

Loading

mhofman commented Apr 14, 2023 •

edited

Loading

turadg Apr 14, 2023 •

edited

Loading