6678 assetRegistry through walletFactory upgrade #7111

turadg · 2023-03-04T00:51:31Z

closes: #6878

Description

6878 issue said,

It will have to hold the issuers from before upgrade. This can be done using ZCF saveIssuer and let the ZCF be responsible for its durability. The MapStore to BrandDescription need not be durable since it's part of shared made in prepare.

This doesn't do that but it adds a test to verify that it isn't necessary. A Bank's getAssetSubscription comes from a SubscriptionKit which uses a pinnedHistoryTopic which is prefix lossy.

agoric-sdk/packages/notifier/src/subscriber.js

Lines 57 to 60 in 71a1db7

    
           // The publish kit subscriber is prefix-lossy, so making *this* subscriber completely 
        
           // lossless from initialisation requires pinning the former's history. 
        
           const pinnedHistoryTopic = makePinnedHistoryTopic(subscriber);

So each time an observeIteration is started with it, all the values will be consumed. Therefore the contract doesn't need to keep a history of them. On upgrade it reconstructs its non-durable maps using the stream.

To test this I tried to check use vstorage to confirm the offers execute, but the "secondOffer" one wasn't showing up. I don't know why and didn't want to spend too much time down that rabbit hole. Instead I made executeOffer throw its exceptions after trying to recover from it. (I think we had a handler to ensure there was always cleanup but it also swallowed the error unnecessarily.)

This uncovered a bug in the runUtils with calls after a rejection. This fixes that as well.

Security Considerations

--

Scaling Considerations

If there are many kinds of assets, walletFactory upgrade will triggeer iterating over all of them. I expect it to be order 10 until this contract is upgraded.

Documentation Considerations

--

Testing Considerations

New test

datadog-full-agoric · 2023-03-06T22:49:44Z

Datadog Report

Branch report: 6678-walletFactory-assetRegistry
Commit report: 8d27301

✅ agoric-sdk: 0 Failed, 0 New Flaky, 3659 Passed, 57 Skipped, 14m 36.23s Wall Time

dckc

Looks pretty much good to go,

but I need to be sure the bridge protocol doesn't do something drastic like crashing the kernel when a bridge call throws / rejects.

dckc · 2023-03-06T23:17:15Z

packages/smart-wallet/src/offers.js

-      await tryBody().catch(err => handleError(err));
+      await tryBody().catch(err => {
+        handleError(err);
+        // propagate to caller


If the caller is a bridge handler, I'm not sure throwing is allowed. Is there something higher up that deals with that?

PTAL 71314fe

dckc · 2023-03-06T23:17:44Z

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

-      want: {},
-    },
-  });
+  await t.throwsAsync(


dckc · 2023-03-06T23:18:47Z

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

+      },
+      proposal: {},
+    }),
+    { message: '"nameKey" not found: "not-present"' },


nameKey here should be instance, I think. (orthogonal to this PR)

I think it comes from NameHub https://github.com/Agoric/agoric-sdk/blob/master/packages/vats/src/nameHub.js#L23

dckc · 2023-03-06T23:19:48Z

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

+      proposal: {},
+    }),
+    {
+      message: 'target has no method "makeGiveMintedInvitation ", has []',


the has [] is likely to change when #6570 is fixed.

dckc · 2023-03-06T23:20:33Z

packages/smart-wallet/src/walletFactory.js

+ * Make a registry for use by the wallet instances.
+ *
+ * This doesn't need to persist durably because the `assetPublisher` has a
+ * "pinned" topic and call togetAssetSubscription gets a fresh stream of all the


Suggested change

* "pinned" topic and call togetAssetSubscription gets a fresh stream of all the

* "pinned" topic and call to getAssetSubscription gets a fresh stream of all the

dckc · 2023-03-06T23:23:45Z

...rt-wallet/test/swingsetTests/upgradeWalletFactory/bootstrap-walletFactory-service-upgrade.js

-
-      ({ bank } = makeFakeBankKit([]));
+      );
+      zoePK.resolve(zoeService);


dckc · 2023-03-06T23:26:00Z

...rt-wallet/test/swingsetTests/upgradeWalletFactory/bootstrap-walletFactory-service-upgrade.js


      return true;
    },

+    testOfferV1: async () => {
+      // If this doesn't throw, the offer succeeded.
+      // I tried to also check vstorage but it doesn't always update in time.


yeah... lack of a return promise from setValue makes coordinating that sort of thing nearly impossible.

dckc · 2023-03-06T23:28:56Z

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

@@ -72,8 +71,11 @@ test('bridge handler', async t => {
    ),
    blockTime: 0,
    blockHeight: 0,
+  };
+  assert(t.context.sendToBridge);
+  await t.throwsAsync(t.context.sendToBridge(validMsg), {


again, I'd like to be sure the bridge protocol doesn't do something drastic like crashing the kernel when a bridge call throws / rejects.

ok... found it. Nothing drastic. Just an unhandled rejection.

agoric-sdk/packages/vats/src/bridge.js

Line 40 in 748d52c

void scopedManagers.get(srcID).fromBridge(obj);

Chris-Hibbert

The upgrade test changes look fine to me. The rest of it flew right past me.

@dckc do you know what it's about?

Chris-Hibbert · 2023-03-06T23:20:49Z

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

-      want: {},
-    },
-  });
+  await t.throwsAsync(


Why are some of the t.throwsAsync() calls missing a message to match?

I started adding them after a bit. The errors messages are tested by the vstorage check.

If we commit to this throing from executeOffer, I can take out the redundant vstorage checks and have a test focused on vstorage writing out when there's an error. The different errors needn't all be checked in vstorage.

dckc

@dckc do you know what it's about?

yes. @turadg landed a large part of walletFactory upgrade in an earlier PR. This remaining part takes care of a notifier (by deciding that it doesn't need to be durable).

dckc · 2023-03-07T00:37:07Z

packages/smart-wallet/src/smartWallet.js

         */
        handleBridgeAction(actionCapData, canSpend = false) {
          const { publicMarshaller } = shared;

          const { offers } = this.facets;

-          return E.when(
+          E.when(


should be void E.when(?

mhofman · 2023-03-07T00:38:18Z

packages/smart-wallet/src/walletFactory.js

@@ -171,7 +175,7 @@ export const prepare = async (zcf, privateArgs, baggage) => {
        const wallet = walletsByAddress.get(obj.owner); // or throw

        console.log('walletFactory:', { wallet, actionCapData });
-        return E(wallet).handleBridgeAction(actionCapData, canSpend);
+        E(wallet).handleBridgeAction(actionCapData, canSpend);


Surprised this doesn't need a void prefix, E(foo).bar() will always return a promise.

Doesn't affect runtime and doesn't affect CI unless AGORIC_ESLINT_TYPES is enabled. But it's not #5788

You're right it would be better and I ran locally with that enabled to catch these:

Amending and force pushing now.

dckc · 2023-03-07T00:38:39Z

packages/smart-wallet/src/walletFactory.js

@@ -171,7 +175,7 @@ export const prepare = async (zcf, privateArgs, baggage) => {
        const wallet = walletsByAddress.get(obj.owner); // or throw

        console.log('walletFactory:', { wallet, actionCapData });
-        return E(wallet).handleBridgeAction(actionCapData, canSpend);
+        E(wallet).handleBridgeAction(actionCapData, canSpend);


add void?

mhofman · 2023-03-07T00:39:29Z

packages/smart-wallet/src/smartWallet.js

- *  { updated: 'balance'; currentAmount: Amount }
+ * @typedef {{ updated: 'offerStatus', status: import('./offers.js').OfferStatus }
+ *   | { updated: 'balance'; currentAmount: Amount }
+ *   | { updated: 'bridgeStatus'; status: { error: string } }


This seem like an abstraction leak to mention "bridge"

how so? the smartWallet contract relies on a Cosmos bridge. we've even considered renaming this contract to have bridge in the name.

is there something else you'd suggest?

I mean that if this is a value that is exposed to the Wallet UI app, does it make sense for it to be aware that the implementation on chain uses something called a "bridge". The way I see it, it should just know it's sending a "wallet spend action", and how that's routed internally is not its concern. This effectively reifies that implementation detail.

Does executeOffer update offerStatus in all cases of rejections? If so, I'd be willing to leave the unhandled rejection for the default case (unknown method).

Deferring to #7004

turadg changed the base branch from master to 6878-walletFactory-upgrade March 4, 2023 00:52

Base automatically changed from 6878-walletFactory-upgrade to master March 4, 2023 01:24

turadg force-pushed the 6678-walletFactory-assetRegistry branch from 8c12623 to 71a1db7 Compare March 6, 2023 22:42

turadg changed the title ~~6678 walletFactory assetRegistry~~ 6678 assetRegistry through walletFactory upgrade Mar 6, 2023

turadg requested review from dckc and Chris-Hibbert March 6, 2023 23:00

turadg force-pushed the 6678-walletFactory-assetRegistry branch from 71a1db7 to e4c984b Compare March 6, 2023 23:15

turadg marked this pull request as ready for review March 6, 2023 23:16

dckc reviewed Mar 6, 2023

View reviewed changes

Chris-Hibbert reviewed Mar 6, 2023

View reviewed changes

dckc approved these changes Mar 7, 2023

View reviewed changes

dckc reviewed Mar 7, 2023

View reviewed changes

mhofman reviewed Mar 7, 2023

View reviewed changes

dckc reviewed Mar 7, 2023

View reviewed changes

mhofman reviewed Mar 7, 2023

View reviewed changes

turadg force-pushed the 6678-walletFactory-assetRegistry branch from 37e0d49 to a5664b7 Compare March 7, 2023 18:53

turadg added 6 commits March 7, 2023 11:08

test: tighten writes threshold

31f13aa

test(bank-utils): addAsset

5d7b587

test(runUtils): fix resolution after error

5abb89a

refactor(wallet): simplify finish()

066934d

feat(wallet): executeOffer throw errors

224dbca

test(walletFactory): assetRegistry through upgrade

770b2bf

turadg force-pushed the 6678-walletFactory-assetRegistry branch from a5664b7 to 770b2bf Compare March 7, 2023 19:16

turadg added the automerge:no-update (expert!) Automatically merge without updates label Mar 7, 2023

mergify bot merged commit 0ac8eb2 into master Mar 7, 2023

mergify bot deleted the 6678-walletFactory-assetRegistry branch March 7, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6678 assetRegistry through walletFactory upgrade #7111

6678 assetRegistry through walletFactory upgrade #7111

turadg commented Mar 4, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 6, 2023 •

edited

Loading

dckc left a comment •

edited

Loading

dckc Mar 6, 2023

turadg Mar 7, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

turadg Mar 7, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

dckc Mar 6, 2023

Chris-Hibbert left a comment

Chris-Hibbert Mar 6, 2023

turadg Mar 7, 2023

dckc left a comment

dckc Mar 7, 2023

mhofman Mar 7, 2023 •

edited

Loading

turadg Mar 7, 2023

dckc Mar 7, 2023

mhofman Mar 7, 2023

turadg Mar 7, 2023

mhofman Mar 7, 2023

mhofman Mar 7, 2023 •

edited

Loading

turadg Mar 7, 2023

	// The publish kit subscriber is prefix-lossy, so making this subscriber completely
	// lossless from initialisation requires pinning the former's history.

	const pinnedHistoryTopic = makePinnedHistoryTopic(subscriber);

	* "pinned" topic and call togetAssetSubscription gets a fresh stream of all the
	* "pinned" topic and call to getAssetSubscription gets a fresh stream of all the

6678 assetRegistry through walletFactory upgrade #7111

6678 assetRegistry through walletFactory upgrade #7111

Conversation

turadg commented Mar 4, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

datadog-full-agoric bot commented Mar 6, 2023 • edited Loading

Datadog Report

dckc left a comment • edited Loading

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

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Mar 7, 2023 • edited Loading

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

mhofman Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Mar 4, 2023 •

edited

Loading

datadog-full-agoric bot commented Mar 6, 2023 •

edited

Loading

dckc left a comment •

edited

Loading

mhofman Mar 7, 2023 •

edited

Loading

mhofman Mar 7, 2023 •

edited

Loading