fix!(smart-wallet): omit redundant chainStorage balances updates #6807

dckc · 2023-01-17T18:34:53Z

Description

For balances such as BLD, IST, USDC_axl that are already published in x/bank, stop publishing them in x/vstorage.

TODO:

ensure client work is represented by issues:
- stop reading denom purse balances from chainstorage #6788
- PSM dapp, wallet poll RPC nodes rapidly #6870

Security Considerations

Perhaps reduces a risk of inconsistency between what clients see via x/vstorage and what's actually stored in x/bank.

Documentation Considerations

Presuming our x/vstorage paths and their contents were documented (which is only partly the case), those docs need updating.

Testing Considerations

I updated unit tests such as packages/inter-protocol/test/smartWallet/test-psm-integration.js.

I ran some of the live-blockchain smoketests, but I would appreciate if other would do more of that, or help me understand exactly which ones I should run and what I should expect them to do.

turadg

Looking good. I see the Draft and todos so not reviewing fully.

Is this really a request to merge into https://github.com/Agoric/agoric-sdk/tree/6652-wallet-perf ? Might want to base off master.

If there's a lot more work to do, the CLI fixes are good to merge as is.

packages/inter-protocol/test/smartWallet/contexts.js

packages/smart-wallet/src/smartWallet.js

dckc · 2023-02-08T22:14:04Z

@turadg @michaelfig @samsiegart PTAL.

@turadg I ran some of the live-blockchain smoketests, but I would appreciate if you would do more of that, or help me understand exactly which ones I should run and what I should expect them to do.

turadg

live-blockchain smoketests

All the -smoketest.sh scripts should work. I run them by copy pasting commands into a terminal. If they're not, maybe I'm unwittingly modifying them on the fly. I can pair tomorrow.

I think we also need more coverage of the Ava integration tests, like the payment queue bug you discovered.

packages/inter-protocol/test/smartWallet/test-psm-integration.js

turadg · 2023-02-09T00:28:46Z

packages/smart-wallet/src/smartWallet.js

- * `balance` update supports forward-compatibility for more than one purse per
- * brand. An additional key will be needed to disambiguate. For now the brand in
- * the amount suffices.


why lose this part?

Because this PR gets rid of balance updates, no? hm... maybe not for zoe invitations...

balance updates are still here, as they should be for Invitations

agoric-sdk/packages/smart-wallet/src/smartWallet.js

Lines 540 to 541 in fac5c32

helper.watchPurse(invitationPurse);

},

Please restore the design note

oops... forgot gain. sorry

packages/smart-wallet/src/smartWallet.js

turadg · 2023-02-09T00:38:24Z

packages/smart-wallet/src/smartWallet.js

          }

          // When there is no purse, queue the payment
          if (queues.has(brand)) {
            queues.get(brand).push(payment);
          } else {
-            queues.init(brand, harden([payment]));
+            queues.init(brand, harden([payment])); // @@ this can't be right


oh yeah we must not have coverage of this. You can't push onto a hardened array.

Good catch! Care to add the failing test 😬 ?

8b6dd84 and

smart wallet fails on 2nd payment from unknown brand #6961

please replace @@ with a link to the ticket

packages/smart-wallet/src/utils.js

samsiegart · 2023-02-09T07:16:44Z

web-components changes LGTM. I'm unsure if anything in wallet-app needs to change but I'll have to make a pass on that soon anyway so LGTM, leaving approval for @turadg and @michaelfig

turadg · 2023-02-10T00:27:06Z

packages/inter-protocol/test/smartWallet/test-psm-integration.js

@@ -369,6 +360,71 @@ test('deposit unknown brand', async t => {
  t.deepEqual(result, { brand: rial.brand, value: 0n });
 });

+test.failing('deposit > 1 payment to unknown brand', async t => {


worth a link to the ticket #6961

turadg · 2023-02-10T00:29:20Z

packages/smart-wallet/src/smartWallet.js

- * `balance` update supports forward-compatibility for more than one purse per
- * brand. An additional key will be needed to disambiguate. For now the brand in
- * the amount suffices.


balance updates are still here, as they should be for Invitations

agoric-sdk/packages/smart-wallet/src/smartWallet.js

Lines 540 to 541 in fac5c32

helper.watchPurse(invitationPurse);

},

Please restore the design note

turadg · 2023-02-10T00:33:44Z

packages/smart-wallet/src/smartWallet.js

@@ -101,12 +95,10 @@ const mapToRecord = map => Object.fromEntries(map.entries());
 *   walletStorageNode: StorageNode,
 * }} UniqueParams
 *
+ * @typedef {Pick<MapStore<Brand, BrandDescriptor>, 'has' | 'get' | 'values'>} ReadOnlyMapStore


ReadOnlyMapStore would be generic on the key/value types. Please pick a name that's specific to the Brand/BrandDescriptor mapping or this use case. BrandDescriptorRegistry?

turadg · 2023-02-10T00:36:28Z

packages/smart-wallet/src/smartWallet.js

@@ -150,7 +141,7 @@ export const prepareSmartWallet = (baggage, shared) => {
      invitationDisplayInfo: DisplayInfoShape,
      publicMarshaller: M.remotable('Marshaller'),
      zoe: M.eref(M.remotable('ZoeService')),
-      registry: M.remotable('AssetRegistry'),
+      registry: M.remotable('AssetRegistry'), // XXX too strict


Since this needn't be far, let's drop the Far from const registry = Far in WalletFactory and give this a shape. That could be ReadonlyMapStoreShape.

The registry can't go thru shape validation of any kind as long as it's not passable. And function properties are only allowed on Far objects.

This seems to work:

const { registry: _, ...passableShared } = shared; mustMatch( harden(passableShared), ...

turadg · 2023-02-10T00:37:23Z

packages/smart-wallet/src/smartWallet.js

          }

          // When there is no purse, queue the payment
          if (queues.has(brand)) {
            queues.get(brand).push(payment);
          } else {
-            queues.init(brand, harden([payment]));
+            queues.init(brand, harden([payment])); // @@ this can't be right


please replace @@ with a link to the ticket

turadg · 2023-02-10T00:38:29Z

packages/smart-wallet/src/smartWallet.js

-                const purse = E(bank).getPurse(desc.brand);
-                await facets.helper.addBrand(desc, purse);
-                return purse;
+                throw assert.error(`cannot find/make purse for ${brand}`);


Is throw necessary for never?

Suggested change

throw assert.error(`cannot find/make purse for ${brand}`);

throw Fail`cannot find/make purse for ${brand}`;

assert.error is not never; it constructs an error. But Fail is fine

yeah I was asking why the throw. because without it assert.fail is an option.

Something about "arrow function needs return" IIRC

turadg

Well done.

dckc requested review from turadg and michaelfig January 17, 2023 18:34

turadg reviewed Jan 17, 2023

View reviewed changes

packages/inter-protocol/test/smartWallet/contexts.js Outdated Show resolved Hide resolved

packages/inter-protocol/test/smartWallet/contexts.js Outdated Show resolved Hide resolved

packages/smart-wallet/src/smartWallet.js Outdated Show resolved Hide resolved

dckc mentioned this pull request Jan 20, 2023

polish smart-wallet test scripts #6817

Merged

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

dckc force-pushed the dc-wallet-balances-dry branch from 95d9729 to eaf72d5 Compare January 20, 2023 03:09

Base automatically changed from 6652-wallet-perf to master January 20, 2023 18:19

This was referenced Jan 23, 2023

feat(vats): share Invitation issuer/brand in agoricNames #6835

Merged

Read vbank assets from x/bank not wallet.current Agoric/wallet-app#49

Closed

PSM dapp, wallet poll RPC nodes rapidly #6870

Closed

dckc force-pushed the dc-wallet-balances-dry branch 2 times, most recently from 33814ad to 8e04f4e Compare February 7, 2023 23:50

This was referenced Feb 8, 2023

stop reading denom purse balances from chainstorage #6788

Closed

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

Closed

dckc self-assigned this Feb 8, 2023

dckc requested a review from samsiegart February 8, 2023 22:12

dckc marked this pull request as ready for review February 8, 2023 22:13

turadg reviewed Feb 9, 2023

View reviewed changes

dckc added this to the Vaults Functional Testing milestone Feb 9, 2023

dckc force-pushed the dc-wallet-balances-dry branch from b63db15 to ef986b1 Compare February 9, 2023 20:59

dckc requested a review from turadg February 10, 2023 00:23

turadg reviewed Feb 10, 2023

View reviewed changes

dckc requested a review from turadg February 10, 2023 04:17

turadg approved these changes Feb 10, 2023

View reviewed changes

dckc force-pushed the dc-wallet-balances-dry branch from 1c2114f to f77e3a5 Compare February 10, 2023 06:22

dckc added the automerge:rebase Automatically rebase updates, then merge label Feb 10, 2023

dckc added 2 commits February 10, 2023 06:45

fix!(smart-wallet): omit redundant chainStorage balances updates

282fdc0

fix!(web-components): token balances from vstorage is obsolete

5ee9a06

dckc added 3 commits February 10, 2023 06:45

fix(agoric-cli): get vbank brand info from agoricNames

5d3ab2f

test(smart-wallet): deposit unknown brand more than once fails

1839eaa

test: smart wallet recovers when only some withdrawals succeed

20fd7a2

dckc force-pushed the dc-wallet-balances-dry branch from f77e3a5 to 20fd7a2 Compare February 10, 2023 06:45

mergify bot merged commit 6ca3660 into master Feb 10, 2023

mergify bot deleted the dc-wallet-balances-dry branch February 10, 2023 07:52

This was referenced Feb 12, 2023

6701 ec oracles list #6963

Closed

vaults chain doesn't support pismo wallet #6993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!(smart-wallet): omit redundant chainStorage balances updates #6807

fix!(smart-wallet): omit redundant chainStorage balances updates #6807

dckc commented Jan 17, 2023 •

edited

Loading

turadg left a comment

dckc commented Feb 8, 2023

turadg left a comment

turadg Feb 9, 2023

dckc Feb 9, 2023

turadg Feb 10, 2023

dckc Feb 10, 2023

turadg Feb 9, 2023

dckc Feb 9, 2023 •

edited

Loading

turadg Feb 10, 2023

samsiegart commented Feb 9, 2023

turadg Feb 10, 2023

turadg Feb 10, 2023

turadg Feb 10, 2023

turadg Feb 10, 2023

dckc Feb 10, 2023

turadg Feb 10, 2023

turadg Feb 10, 2023

dckc Feb 10, 2023

turadg Feb 10, 2023

dckc Feb 10, 2023

turadg left a comment

	throw assert.error(`cannot find/make purse for ${brand}`);
	throw Fail`cannot find/make purse for ${brand}`;

fix!(smart-wallet): omit redundant chainStorage balances updates #6807

fix!(smart-wallet): omit redundant chainStorage balances updates #6807

Conversation

dckc commented Jan 17, 2023 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

turadg left a comment

Choose a reason for hiding this comment

dckc commented Feb 8, 2023

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

dckc Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart commented Feb 9, 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

turadg left a comment

Choose a reason for hiding this comment

dckc commented Jan 17, 2023 •

edited

Loading

dckc Feb 9, 2023 •

edited

Loading