feat(wallet/api): marshal contexts for wallet, board #5883

dckc · 2022-08-03T00:58:40Z

refs: #4398
obsoletes #5874

Description

When serializing wallet state for chainStorage, there's a tension between

keeping purses etc. closely held
recognizing identity of brands also referenced in the state of contracts such as the AMM

makeMarshal is parameterized by the type of slots. Here we use a disjoint union of

board ids for widely shared objects
kind:seq ids for closely held objects; for example purse:123

@samsiegart note that unlike #5874 , this PR doesn't include any changes to lib-wallet.js; I hope you can layer those changes on top of this as a form of review.

This PR includes makeLoggingPresence, which is some advanced work on smart wallet middleware. While the design is still in progress, test coverage of this version is good enough that I'd like to land it.

Security Considerations

As @warner noted, there's a risk that chainStorage data from the AMM could try to refer to a purse using a slots such as purse:123; such a reference must fail.

Documentation Considerations

not much; these are internal APIs.

Testing Considerations

Test coverage is 100%.

agoric-sdk/packages/wallet/api$ yarn c8 ava test/test-wallet-marshal.js test/test-middleware.js
yarn run v1.22.19
$ .../.bin/c8 ava test/test-wallet-marshal.js test/test-middleware.js

  ✔ middleware › makeImportContext in wallet UI can unserialize messages
  ✔ middleware › makeLoggingPresence logs calls on purse/payment actions
  ✔ wallet-marshal › makeImportContext preserves identity across AMM and wallet
  ✔ wallet-marshal › ensureBoardId allows re-registration; initBoardId does not
  ✔ wallet-marshal › makeExportContext.serialize handles unregistered identites
  ─

  5 tests passed
---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------------|---------|----------|---------|---------|-------------------
All files            |     100 |      100 |     100 |     100 |                   
 marshal-contexts.js |     100 |      100 |     100 |     100 |                   
---------------------|---------|----------|---------|---------|-------------------
Done in 0.71s.

dckc · 2022-08-08T22:09:18Z

good progress:

agoric-sdk/packages/wallet/api$ yarn c8 ava test/test-wallet-marshal.js test/test-middleware.js
yarn run v1.22.19
$ /home/connolly/projects/agoric-sdk/node_modules/.bin/c8 ava test/test-wallet-marshal.js test/test-middleware.js

  ✔ middleware › makeImportContext in wallet UI can unserialize messages
  ✔ middleware › makeLoggingPresence logs calls on purse/payment actions
  ✔ wallet-marshal › makeImportContext preserves identity across AMM and wallet
  ✔ wallet-marshal › ensureBoardId allows re-registration; initBoardId does not
  ✔ wallet-marshal › makeExportContext.serialize handles unregistered identites
  ─

  5 tests passed
---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------------|---------|----------|---------|---------|-------------------
All files            |   99.61 |    97.29 |     100 |   99.61 |                   
 marshal-contexts.js |   99.61 |    97.29 |     100 |   99.61 | 206               
---------------------|---------|----------|---------|---------|-------------------
Done in 0.79s.

samsiegart · 2022-08-09T01:27:56Z

So you'd like me to do something like b516b66 to integrate this? Can we make a checklist of things to do, like purses, payments, issuers, brands, etc

samsiegart · 2022-08-09T01:31:17Z

packages/wallet/api/test/test-middleware.js

+    ['applyFunction', purse.actions, [1, 'thing']],
+  ]);
+
+  const ctx = makeExportContext();


I'm confused about this test case, because ctx I thought would be inside lib-wallet, while the logging presence would be used outside lib-wallet

Good point; I guess I'll think this over some more.

I'd like to come back to this in a later PR.

packages/wallet/api/src/marshal-contexts.js

turadg

Looking good.

I don't know how to assess this:

there's a risk that chainStorage data from the AMM could try to refer to a purse using a slots such as purse:123; such a reference must fail.

Can a test be added for that?

turadg · 2022-08-09T21:01:20Z

packages/wallet/api/src/marshal-contexts.js

+ * @template T
+ * @typedef {`${string & keyof T}:${Digits}`} WalletSlot<T>


bravo on the string template type. non-blocking suggestion to help document how this works,

Suggested change

* @template T

* @typedef {`${string & keyof T}:${Digits}`} WalletSlot<T>

* @template {Record<string, IdTable<*,*>} T

* @typedef {`${keyof T}:${Digits}`} WalletSlot<T>

I integrated most of that suggestion. Constraining T that way helps not only documentation but also got rid of one or two @ts-expect-errors.

But getting rid of string & doesn't seem to fly.

turadg · 2022-08-09T21:02:38Z

packages/wallet/api/src/marshal-contexts.js

+ * @returns {WalletSlot<T>}
+ */
+const makeWalletSlot = (_tables, kind, id) => {
+  const digits = /** @type {Digits} */ (`${id}`);


consider asserting id >= 1 for the digits

turadg · 2022-08-09T21:03:06Z

packages/wallet/api/src/marshal-contexts.js

+ */
+
+/**
+ * @template T


for documentation purposes,

Suggested change

* @template T

* @template {Record<string, IdTable<*,*>} T

Thanks; I'll give that a try.

Earlier I explored similar things without luck and punted.

turadg · 2022-08-09T21:03:44Z

packages/wallet/api/src/marshal-contexts.js

+ */
+const findKey = (record, predicate) => {
+  const key = Object.keys(record).find(predicate);
+  // @ts-expect-error clearly keys(r).find() is keyof T


well, keyof T | undefined. the jsdoc is right but this comment is loose

turadg · 2022-08-09T21:07:21Z

packages/wallet/api/src/marshal-contexts.js

+/**
+ * @template T
+ * @param {T} _tables
+ * @param {string & keyof T} kind


I think the above would also simplify this,

Suggested change

* @param {string & keyof T} kind

* @param {keyof T} kind

turadg · 2022-08-09T21:07:37Z

packages/wallet/api/src/marshal-contexts.js

+ * @template T
+ * @param {T} record
+ * @param {(value: string, index: number, obj: string[]) => boolean} predicate
+ * @returns {string & keyof T | undefined}


ditto on above suggestions about typing the template

turadg · 2022-08-09T21:08:52Z

packages/wallet/api/src/marshal-contexts.js

+};
+
+/**
+ * Since KindSlots always include a colon and BoardIds never do,


turadg · 2022-08-09T21:13:01Z

packages/wallet/api/test/test-wallet-marshal.js

+  t.deepEqual(cap2, {
+    body: JSON.stringify({


when these don't match, it'll be a string diff. consider comparing the object with JSON.parse(cap2).

If not comparing objects then it should be t.is

The test is supposed to be at the CapData level, where body is a string. Writing the expected body using JSON.stringify() makes it easier to read. I suppose I could add a comment...

Meanwhile, your comment prompts me to think about this a bit more carefully... In the API, slots are compared by identity, so t.deepEqual() is only correct because we happen to be using primitives (strings). In nearby designs, the slots are { kind: 'purse', id: 123} objects where it's really easy to have things that pass t.deepEqual() but not t.is().

turadg · 2022-08-09T21:14:38Z

packages/wallet/api/test/test-middleware.js

+  });
+});
+
+//   {


dckc · 2022-08-10T01:51:46Z

I don't know how to assess this:

there's a risk that chainStorage data from the AMM could try to refer to a purse using a slots such as purse:123; such a reference must fail.

Can a test be added for that?

It's in there:

test('makeImportContext preserves identity across AMM and wallet', t => {
...
  t.throws(
    () => context.fromBoard.unserialize(walletCapData),
    { message: /bad board slot/ },
    'AMM cannot refer to purses',
  );

p.s. "AMM cannot forge references to purses" would be a better phrasing.

turadg

Approving now. (No automerge label so you'll be able to try the TS tweak before it merges.)

- perhaps one bug fix around `purse:1` vs `1`?

turadg mentioned this pull request Aug 3, 2022

make smart wallet's cache available off-chain #5868

Open

dckc mentioned this pull request Aug 4, 2022

feat(wallet/api): marshal contexts for wallet, board #5874

Closed

dckc added 5 commits August 8, 2022 09:17

feat(wallet/api): marshal contexts for wallet, board

2dc7624

test(wallet/api): makeLoggingPresence seems to work

06ff6ad

fix(wallet/api): importContext has .serialize too

f6fb536

feat(wallet/api): makeLoggingPresence

a4fcde3

feat(wallet/api): handle unregistered identites in serialization

8721eb0

dckc force-pushed the dc-smart-middle branch from 7344776 to 8721eb0 Compare August 8, 2022 15:57

dckc added 7 commits August 8, 2022 11:37

chore: prune unused private brand, issuer maps

0d10ecb

refactor: fromPart -> fromMyWallet

e874903

docs: makeExportContext

7e6bf18

feat: ensureBoardId with test

1f56597

docs: test names etc.

4930560

CHECKPOINT: tests pass. coverage is good.

da084e5

chore(wallet/api): marshal stuff working (IOU words...)

10f1c32

dckc marked this pull request as ready for review August 9, 2022 00:50

dckc requested review from michaelfig, samsiegart and turadg August 9, 2022 00:50

dckc changed the title ~~WIP: smart wallet middleware~~ marshal contexts for smart wallet with board integration Aug 9, 2022

samsiegart reviewed Aug 9, 2022

View reviewed changes

turadg reviewed Aug 9, 2022

View reviewed changes

refactor(wallet/api): JSON.stringify capData body for clarity

45ae56b

dckc requested a review from turadg August 9, 2022 19:11

turadg reviewed Aug 9, 2022

View reviewed changes

turadg approved these changes Aug 10, 2022

View reviewed changes

refactor(wallet/api): type safety for marshal-contexts

e38efe5

- perhaps one bug fix around `purse:1` vs `1`?

dckc added 2 commits August 10, 2022 11:25

docs(wallet/api): refine makeExportContext docstring

90b83a7

fixup! test(wallet/api): makeLoggingPresence seems to work

d8194de

dckc force-pushed the dc-smart-middle branch from dfc14a9 to d8194de Compare August 10, 2022 16:29

dckc added the automerge:squash Automatically squash merge label Aug 10, 2022

turadg changed the title ~~marshal contexts for smart wallet with board integration~~ feat(wallet/api): marshal contexts for wallet, board Aug 10, 2022

Merge branch 'master' into dc-smart-middle

dc6d02b

mergify bot merged commit 088e144 into master Aug 10, 2022

mergify bot deleted the dc-smart-middle branch August 10, 2022 18:37

turadg mentioned this pull request Aug 27, 2022

6044 wallet voting #6077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallet/api): marshal contexts for wallet, board #5883

feat(wallet/api): marshal contexts for wallet, board #5883

dckc commented Aug 3, 2022 •

edited

Loading

dckc commented Aug 8, 2022

samsiegart commented Aug 9, 2022

samsiegart Aug 9, 2022

dckc Aug 10, 2022

dckc Aug 10, 2022

turadg left a comment

turadg Aug 9, 2022

dckc Aug 10, 2022

turadg Aug 9, 2022

turadg Aug 9, 2022

dckc Aug 10, 2022

turadg Aug 9, 2022

turadg Aug 9, 2022

turadg Aug 9, 2022

turadg Aug 9, 2022

turadg Aug 9, 2022

dckc Aug 10, 2022

turadg Aug 9, 2022

dckc commented Aug 10, 2022 •

edited

Loading

turadg left a comment

		* @template T
		* @typedef {`${string & keyof T}:${Digits}`} WalletSlot<T>

+                });
+              });
+              //   {

feat(wallet/api): marshal contexts for wallet, board #5883

feat(wallet/api): marshal contexts for wallet, board #5883

Conversation

dckc commented Aug 3, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Aug 8, 2022

samsiegart commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg 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

dckc commented Aug 10, 2022 • edited Loading

turadg left a comment

Choose a reason for hiding this comment

dckc commented Aug 3, 2022 •

edited

Loading

dckc commented Aug 10, 2022 •

edited

Loading