fix(smart-wallet): create purses for new assets lazily #6774

dckc · 2023-01-10T23:15:16Z

Description

only subscribe to new assets once, in walletFactory.js.
make purses for all vbank assets when creating a smart wallet
for assets added to the vbank after a smart wallet is created, create purses lazily
- Change purseForBrand given to makeOfferExecutor to create purses lazily.

Security Considerations

When a user executes an offer, if we don't have a purse for one of the relevant brands, we take the brand they give and "upgrade" it to an issuer, by way of info we got from the vbank, in order to make a purse. Normally we would expect the user to explicitly indicate the issuers that they trust. This isn't so much a change in the security properties of the contract - it trusted the vbank to provide issuers to make purses before - but this now happens in response to the user placing an offer rather than in response to the vbank announcing a new asset.

Documentation Considerations

We should tell users that if a vbank asset is added after they provision their smart wallet, they won't see a purse until they execute an offer that uses it.

Testing Considerations

I'm somewhat surprised at how little code needed to change for this in order to get yarn test in packages/smart-wallet to pass. I wonder whether those tests have good coverage of executing offers.

p.s. tests in inter-protocol provide more coverage.

samsiegart · 2023-01-11T01:14:12Z

Purses appearing only when executing an offer could be a little surprising. In particular, if a user has some BLD and/or IST, they won't see it in their wallet until they make an offer involving those tokens.
Perhaps when a smart-wallet is created, a purse for each asset currently in the vbank should be created?

I like the suggestion at the end. The first part means we're in a bit of a bind. If we can query balances for different denoms outside of vstorage, like on the cosmos account level, we can mesh assets with and without purses seamlessly in the UI maybe?

turadg · 2023-01-11T20:11:40Z

packages/smart-wallet/src/smartWallet.js

@@ -135,7 +134,7 @@ const mapToRecord = map => Object.fromEntries(map.entries());
 * @returns {State}
 */
 export const initState = (unique, shared) => {
-  // Some validation of inputs. "any" erefs because this synchronous call can't check more than that.


why remove this explanation for M.any?

btw, should some of these M.remotable?

why remove this explanation for M.any?

Because I got rid of all the M.any cases. Oops... but then I reverted that change without putting the comment back.

btw, should some of these M.remotable?

Yes.

Refining the typeguards isn't necessary to fix #6652, so arguably it should go in a separate PR. You don't mind if I include it here, do you?

SGTM, makes sense with the anys gone :)

packages/smart-wallet/src/smartWallet.js

turadg · 2023-01-11T20:18:09Z

packages/smart-wallet/src/smartWallet.js

+            // async so we consistently return a promise
+            purseForBrand: async brand => {


alternately, in a way that doesn't need explaining:

Suggested change

// async so we consistently return a promise

purseForBrand: async brand => {

/** @returns {Promise<Brand>} */

purseForBrand: brand => {

then the return brandPurses.get(brand); would have a squiggly and force,

return Promise.resolve(brandPurses.get(brand));

That exposed a conflict with withdrawGive where the purse for a brand is expected to be available synchronously. Ugh.

ah... it's ok to have promises in keywordPaymentPromises...

turadg · 2023-01-11T20:19:33Z

packages/smart-wallet/src/smartWallet.js

@@ -490,35 +481,20 @@ const SmartWalletKit = defineVirtualFarClassKit(
  },
  {
    finish: ({ state, facets }) => {
-      const { invitationBrand, invitationIssuer, invitationPurse, bank } =
-        state;
+      const invitationDisplayInfo = { assetKind: AssetKind.SET }; // XXX


what should this be instead?

maybe a Zoe const?

A Zoe const might work... but I went with the caller passing it in along with the brand and issuer.

turadg · 2023-01-11T20:19:48Z

packages/smart-wallet/src/smartWallet.js

        },
        // @ts-expect-error cast to RemotePurse
        /** @type {RemotePurse} */ (invitationPurse),
      );
-      // watch the bank for new issuers to make purses out of


turadg · 2023-01-11T20:21:26Z

packages/smart-wallet/test/test-addAsset.js

+/** @type {import('ava').TestFn<Awaited<ReturnType<makeDefaultTestContext>>>} */
+const test = anyTest;
+
+const TODO = undefined;


turns out undefined works, so I inlined it as noBridge.

turadg · 2023-01-11T20:23:17Z

packages/smart-wallet/test/test-addAsset.js

+  });
+  t.true(
+    exp.addedWrites <= (base.addedWrites * exp.qty) / base.qty / 2,
+    'actual writes should be less than half of linear growth',


turadg · 2023-01-11T20:24:34Z

packages/smart-wallet/src/walletFactory.js

+      subscribed = true;
+    };
+
+    // XXX this isn't far, is it?


I think it doesn't need to be, because all the wallets are in the same vat. We've talked about separating them. Any reason not to Far it?

Its clients make synchronous calls on it. So they have to be in the same vat.

turadg · 2023-01-11T20:25:42Z

packages/smart-wallet/src/walletFactory.js

      E(board).getReadonlyMarshaller(),
    ]);

+  const makeAssetRegistry = () => {


this has access to all of start scope, making it a little harder to read and audit.

please move this function def to module scope or another file. consider unit testing it.

Hosting it is a good idea... done.

As to unit testing... I'm inclined to let the existing tests suffice. Mocking an anyBank would be tedious, and using the existing vbank code would be more or less another integration test like the others.

dckc · 2023-01-12T20:21:18Z

@arirubinstein this does include a simple test (test-addAsset.js) for the performance issue. I wonder if you consider it adequate.

dckc · 2023-01-12T20:30:51Z

... If we can query balances for different denoms outside of vstorage, like on the cosmos account level, we can mesh assets with and without purses seamlessly in the UI maybe?

Yes, that's straightforward. agd query bank balances ... (or the equivalent JS) gives a list of { denom, amount } balances. The mapping of denom to brand, displayInfo, etc. is now published in agoricNames.vbankAssets.

turadg

LGTM. I assume it won't merge until fixups are rebased and you have some of the other reviews you're seeking

dckc · 2023-01-12T20:49:07Z

packages/vats/src/core/startWalletFactory.js

  const terms = await deeplyFulfilled(
    harden({
      agoricNames,
      board,
+      anyBank: poolBank,


hm. This gives the walletFactory authority to spend from the poolBank. That's not great. Gotta think that over...

dckc · 2023-01-13T15:58:29Z

... I assume it won't merge until fixups are rebased and ...

I intend to squash it all to one commit.

dckc · 2023-01-17T15:57:25Z

packages/smart-wallet/src/smartWallet.js

+              const desc = registry.getRegisteredAsset(brand);
+              /** @type {RemotePurse} */
+              // @ts-expect-error cast to RemotePurse
+              const purse = E(desc.issuer).makeEmptyPurse();


this needs to make a vbank purse a la E(bank).getPurse(desc.brand)

mhofman · 2023-01-19T10:56:24Z

@Mergifyio refresh

mergify · 2023-01-19T10:57:02Z

refresh

✅ Pull request refreshed

mhofman · 2023-01-19T10:59:23Z

@Mergifyio unqueue

mergify · 2023-01-19T10:59:59Z

unqueue

✅ The pull request has been removed from the queue

mhofman · 2023-01-19T11:08:13Z

@dckc the mergify bot snafu might make it hard to land this PR. Please ping me if re-adding a label doesn't work (You might also try the bot command)

failing test case for #6652

- subscribe for new vbank assets once, in walletFactory.js - provide a read-only registry of the assets - no change to RPC protocol: still publish all brand descriptors in all wallet states - refactor: clarify invitationIssuer promise - add invitationDisplayInfo to synchronously-provided SharedParams - refine typeguards, esp. M.any() => M.remotable('...') - hoist makeAssetRegistry() to module scope to reduce in-scope authority feat(smart-wallet): initialize with existing vbank assets - add anyBank to walletFactory terms so we can subscribe to vbank assets without waiting for creation of the 1st wallet - refine customTermsShape from M.not(M.undefined()) to M.eref(M.remotable()) - attenuate poolBank to {getAssetSubscription} - 'want stable' test: don't assume incremental updates include all assets; get the full current state to start with - constrain BrandDescriptor to take settled issuers, since publishing records that include promises doesn't let off-chain callers compare identities - losen addBrand() typeguard to allow M.eref(PurseShape) since we await the purse inside the function - static type for purseForBrand motivates tweak to keywordPaymentPromises - fixup: Promise<NameHub> isn't durable a promise for agoricNames isn't durable. so I need deeplyFulfilledObject after all. but the types don't work out. so back to deeplyFulfilled I guess

dckc requested review from turadg, warner, michaelfig and samsiegart January 10, 2023 23:15

dckc marked this pull request as draft January 11, 2023 20:17

turadg reviewed Jan 11, 2023

View reviewed changes

dckc force-pushed the 6652-wallet-perf branch from 01e3a0a to 4c5b1bc Compare January 12, 2023 01:30

dckc marked this pull request as ready for review January 12, 2023 17:51

dckc requested a review from arirubinstein January 12, 2023 20:20

dckc requested a review from turadg January 12, 2023 20:28

turadg approved these changes Jan 12, 2023

View reviewed changes

dckc marked this pull request as draft January 12, 2023 20:48

dckc commented Jan 12, 2023

View reviewed changes

dckc force-pushed the 6652-wallet-perf branch 2 times, most recently from e688aaf to 53e7d7b Compare January 13, 2023 17:09

dckc marked this pull request as ready for review January 13, 2023 17:10

dckc mentioned this pull request Jan 13, 2023

introducing new vbank assets results in O(n) work to update smart wallets #6652

Closed

1 task

dckc added automerge:squash Automatically squash merge and removed automerge:squash Automatically squash merge labels Jan 17, 2023

dckc marked this pull request as draft January 17, 2023 15:56

dckc commented Jan 17, 2023

View reviewed changes

dckc force-pushed the 6652-wallet-perf branch from 8641317 to d5c5c45 Compare January 17, 2023 16:06

dckc mentioned this pull request Jan 18, 2023

deeplyFulfilled erases type information endojs/endo#1257

Closed

dckc added 3 commits January 19, 2023 20:54

test(smart-wallet): avoid O(wallets) storage writes for a new asset

ed9741f

failing test case for #6652

docs(smart-wallet): typo

de8bb08

dckc force-pushed the 6652-wallet-perf branch from d5c5c45 to e241ba0 Compare January 20, 2023 03:00

dckc added the automerge:no-update (expert!) Automatically merge without updates label Jan 20, 2023

turadg marked this pull request as ready for review January 20, 2023 16:40

Merge branch 'master' into 6652-wallet-perf

b899329

mergify bot merged commit ac1a0c1 into master Jan 20, 2023

mergify bot deleted the 6652-wallet-perf branch January 20, 2023 18:19

turadg mentioned this pull request Jan 20, 2023

6665 publicSubscribers #6789

Merged

turadg mentioned this pull request Feb 14, 2023

Potential leak in v17 (bank) and v18 (smart wallet) #6611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(smart-wallet): create purses for new assets lazily #6774

fix(smart-wallet): create purses for new assets lazily #6774

dckc commented Jan 10, 2023 •

edited

Loading

samsiegart commented Jan 11, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

dckc Jan 12, 2023

turadg Jan 12, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

dckc Jan 12, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

turadg Jan 11, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

turadg Jan 11, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

turadg Jan 11, 2023

dckc Jan 12, 2023

dckc commented Jan 12, 2023 •

edited

Loading

dckc commented Jan 12, 2023

turadg left a comment

dckc Jan 12, 2023

dckc commented Jan 13, 2023

dckc Jan 17, 2023

mhofman commented Jan 19, 2023

mergify bot commented Jan 19, 2023

mhofman commented Jan 19, 2023

mergify bot commented Jan 19, 2023

mhofman commented Jan 19, 2023

		// async so we consistently return a promise
		purseForBrand: async brand => {

fix(smart-wallet): create purses for new assets lazily #6774

fix(smart-wallet): create purses for new assets lazily #6774

Conversation

dckc commented Jan 10, 2023 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

samsiegart commented Jan 11, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Jan 12, 2023 • edited Loading

dckc commented Jan 12, 2023

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Jan 13, 2023

Choose a reason for hiding this comment

mhofman commented Jan 19, 2023

mergify bot commented Jan 19, 2023

✅ Pull request refreshed

mhofman commented Jan 19, 2023

mergify bot commented Jan 19, 2023

✅ The pull request has been removed from the queue

mhofman commented Jan 19, 2023

dckc commented Jan 10, 2023 •

edited

Loading

dckc commented Jan 12, 2023 •

edited

Loading