8445 add promiseWatcher to smartWallet #8488

Chris-Hibbert · 2023-11-01T00:24:53Z

closes: #8445

Description

Use watchedPromises to monitor expected payouts. This made it necessary to be able to call several functions from more than one location, so a lot of state and behavior that was ephemeral was made durable. I think the code is simpler now.

along the way payments.tryReclaimingWithdrawnPayments() went away. I'm pretty sure it was redundant with ERTP recoverySets. If there was other functionality that we should keep, please let me know.

Security Considerations

offer security, not operational security. Without this fix, wallets wouldn't be able to pay out if the Zoe vat upgraded.

Scaling Considerations

Repairs all the outstanding seats for each smartWallet on upgrade.

Documentation Considerations

not applicable

Testing Considerations

TBD. Not done yet.

Upgrade Considerations

It's all about upgrade.

Chris-Hibbert · 2023-11-09T00:01:56Z

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

-  /** @type {[PromiseRejectedResult]} */
-  // @ts-expect-error cast
-  const result = status.result;
-  t.is(result[0].status, 'rejected');


The previous code just happened to call updateStatus on a Promise.all() that covered this case. We get the same results without the Promise.all(), so there's nothing to log at that point. The 'rejected' status is redundant with the error detected just above.

This is a breaking change and why test-oracle-integration is failing. When this code handles a rejection:

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

Line 182 in daf4294

facets.helper.updateStatus({ error: err.toString() });

it's not making it to this error handler:

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

Lines 843 to 844 in 0e25d18

} catch (err) {

console.error('OFFER ERROR:', err);

And so it's not getting thrown

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

Line 874 in 0e25d18

throw err;

to the executeOffer caller.

It's a smell that there are two error handlers which both do updateStatus({ error: err.toString() });. Let's see if we can refactor this to have one error handler that throws and executeOffer() waits for the offer to complete.

this 'rejected' status came from tryReclaimingWithdrawnPayments(), which failed because the error happened before there was a seat. insufficientFunds was detected while attempting to create the payment to send to Zoe, so Zoe was never called.

The new approach doesn't attempt to collect those payments, so the rejected status is never set, which I think is fine. status.error tells all that needs to be understood.

test-oracle-integration now passes because of your fixes that pass the error through executeOffer.

packages/zoe/src/zoeService/zoeSeat.js

packages/agoric-cli/src/commands/auction.js

packages/boot/test/bootstrapTests/liquidation.ts

packages/boot/test/bootstrapTests/test-vaults-integration.ts

turadg · 2023-11-29T20:33:04Z

packages/boot/test/bootstrapTests/test-vaults-integration.ts

-      'open-vault',
-    ),
+
+  // does not throw, because it's in the result, but it will show up in errors


this is a breaking change to the API. I don't think it's necessary. Let's discuss.

turadg · 2023-11-29T20:33:59Z

packages/boot/test/bootstrapTests/test-walletSurvivesZoeRestart.ts

  LiquidationSetup,
 } from './liquidation.ts';

 const test = anyTest as TestFn<LiquidationTestContext>;

-//#region Product spec
 const setup: LiquidationSetup = {


why is LiquidationSetup in a test about walletSurvivesZoeRestart?

in order to test that the wallets survive, it's using a vaultLiquidation scenario to exercise the vaults.

packages/smart-wallet/src/smartWallet.js

packages/store/package.json

packages/swingset-liveslots/test/virtual-objects/test-virtualObjectManager.js

packages/wallet/api/src/lib-wallet.js

turadg · 2023-12-05T16:03:51Z

packages/builders/scripts/smart-wallet/build-walletFactory-upgrade.js

@@ -6,7 +6,7 @@
 */

 import { makeHelpers } from '@agoric/deploy-script-support';
-import { getManifestForUpgrade } from '@agoric/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js';
+import { getManifestForUpgrade } from '@agoric/vats/src/proposals/upgrade-wallet-factory-proposal.js';


In master I see the file packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js and when I run agoric run scripts/smart-wallet/build-walletFactory-upgrade.js it works.

Please revert moving the file. The proposal is about wallet-factory so belongs in smart-wallet.

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

packages/smart-wallet/src/invitations.js

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

packages/smart-wallet/src/offerWatcher.js

packages/smart-wallet/src/smartWallet.js

Needed by #8488

turadg · 2023-12-15T17:29:20Z

packages/boot/test/bootstrapTests/test-walletSurvivesZoeRestart.ts

-  const buildAndExecuteProposal = async packageSpec => {
+  const buyer = await walletFactoryDriver.provideSmartWallet('agoric1buyer');
+
+  const buildAndExecuteProposal = async (packageSpec: string) => {


Out of scope but we should move this to SwingsetTestKit

take care to pass I/O access explicitly when doing so.

packages/smart-wallet/package.json

packages/smart-wallet/src/proposals/upgrade-wallet-factory2.js

turadg · 2023-12-15T17:35:41Z

packages/smart-wallet/src/smartWallet.js

@@ -218,6 +249,12 @@ export const prepareSmartWallet = (baggage, shared) => {
      invitationDisplayInfo: DisplayInfoShape,
      publicMarshaller: M.remotable('Marshaller'),
      zoe: M.eref(M.remotable('ZoeService')),
+
+      // known only to smartWallets and walletFactory, this allows the


turadg · 2023-12-15T17:36:56Z

packages/smart-wallet/src/smartWallet.js

+      purseForBrand: M.call(BrandShape).returns(M.promise()),
+      logWalletInfo: M.call(M.any()).returns(),
+      logWalletError: M.call(M.any()).returns(),
+      // XXX is there a better guard for a bigMapStore?


Suggested change

// XXX is there a better guard for a bigMapStore?

// XXX is there a tighter shape for a bigMapStore than M.any()?

fwiw I don't think so

packages/smart-wallet/src/walletFactory.js

packages/smart-wallet/test/swingsetTests/upgradeWalletFactory/walletFactory-V2.js

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

turadg

Please change the conventional commit tag from refactor to feat. It's new functionality. E.g.

feat: smartWallet v2 with watchedPromises

* test: support for watchedPromises in fakeVirtualSupport Needed by #8488 * chore: update allocatePromiseID to use nextExportID * chore: distinct ID range for proises

Chris-Hibbert · 2023-12-19T17:26:26Z

@dckc, are you willing/able to approve this PR for merging. I believe I've addressed all the requested changes.

turadg

I left some non-blocking feedback. Should be squashed into one feat commit

packages/vats/package.json

packages/smart-wallet/src/walletFactory.js

turadg · 2024-01-04T22:13:07Z

packages/smart-wallet/src/smartWallet.js

+      purseForBrand: M.call(BrandShape).returns(M.promise()),
+      logWalletInfo: M.call().rest(M.arrayOf(M.any())).returns(),
+      logWalletError: M.call().rest(M.arrayOf(M.any())).returns(),
+      // XXX is there a tighter guard for a bigMapStore than M.any()?


there isn't, but there could be a shape for it. consider,

Suggested change

// XXX is there a tighter guard for a bigMapStore than M.any()?

// TODO define a shape for bigMapStore

even better, make an issue and mention it here

M.remotable() seems like the right shape.
or with a label such as M.remotable('mapStore').

agoric-sdk/packages/swingset-liveslots/src/collectionManager.js

Line 843 in 5a06ded

return Far('mapStore', mapStore);

I assumed we could do better by including the actual properties (methods) of a mapStore. M.remotable() is incrementally better than M.any() but then the shape can't change later to one that is a local object with methods.

Patterns only admit passables, such as a remotable or a copyRecord with properties (whose values are passable), but local objects with methods aren't passable, IIUC.

And even for a local Far() object, whether a pattern matches or not is pass-invariant, so you can't test its methods with patterns any more than you can for a remote presence.

M.remotable('mapStore') is indeed better than M.any(). Thanks

Thanks @dckc and sorry for steering your wrong @Chris-Hibbert . I neglected to think through it, and was yet was hasty in my certainty. I plead guilty by reason of fever.

turadg · 2024-01-04T22:15:30Z

packages/smart-wallet/src/smartWallet.js

+          const { address, invitationPurse } = state;
+          const { liveOffers, liveOfferSeats } = state;
+          const { zoe, agoricNames } = shared;
+          const { invitationBrand, invitationIssuer } = shared;


I can't tell whether it was intentional to separate them. Please make these two lines or explain why it's four.

I hate prettier.

It used to turn 2 lines here or 4 lines into 10 lines. Now it's happy with two lines per declaration. Hurrah!
But with longer names or more names, it will jump from 2 lines to 6, and I'll go back to multiple declarations, or not declaring them all, or declaring some later.

pulled offers.js and payments.js into smartWallet.js as they shared plenty of state that needs to be durable in order to be callable from the watchedPromise. build an upgrade proposal; tested in Agoric/agoric-3-proposals#34

* test: support for watchedPromises in fakeVirtualSupport Needed by #8488 * chore: update allocatePromiseID to use nextExportID * chore: distinct ID range for proises

…ic#8772) * test: support for watchedPromises in fakeVirtualSupport Needed by Agoric#8488 * chore: update allocatePromiseID to use nextExportID * chore: distinct ID range for proises

Chris-Hibbert added the wallet label Nov 1, 2023

Chris-Hibbert self-assigned this Nov 1, 2023

Chris-Hibbert force-pushed the 8445-addWatcher branch 2 times, most recently from 0584ef2 to 2096070 Compare November 1, 2023 16:43

Chris-Hibbert force-pushed the 8445-addWatcher branch from b5093a4 to fb03e29 Compare November 8, 2023 23:52

Chris-Hibbert commented Nov 9, 2023

View reviewed changes

Chris-Hibbert force-pushed the 8445-addWatcher branch from 65d8aef to d5d0bc1 Compare November 11, 2023 00:00

dckc mentioned this pull request Nov 27, 2023

which branch for near-term walletFactory work? master or release-mainnet1B? #8565

Closed

dckc reviewed Nov 27, 2023

View reviewed changes

packages/zoe/src/zoeService/zoeSeat.js Outdated Show resolved Hide resolved

Chris-Hibbert force-pushed the 8445-addWatcher branch 2 times, most recently from 9c8ec51 to 6730e01 Compare November 29, 2023 17:51

turadg reviewed Nov 29, 2023

View reviewed changes

Chris-Hibbert requested a review from warner November 30, 2023 01:33

turadg force-pushed the 8445-addWatcher branch from d57aa5a to cbb2dae Compare November 30, 2023 19:35

Chris-Hibbert force-pushed the 8445-addWatcher branch from 54f15dc to b80246d Compare November 30, 2023 22:05

turadg mentioned this pull request Nov 30, 2023

test: upgrade tests for 8488 smartWallet with promiseWatchers Agoric/agoric-3-proposals#34

Closed

Chris-Hibbert force-pushed the 8445-addWatcher branch 4 times, most recently from cb60b07 to 625f9dd Compare December 2, 2023 00:18

turadg mentioned this pull request Dec 2, 2023

typecheck vats package #8596

Merged

This was referenced Dec 4, 2023

smartWallet exits offers (e.g. bids) on zoe upgrade #8292

Closed

smartWallet stops updating purse balance on upgrade #8293

Closed

Chris-Hibbert mentioned this pull request Dec 4, 2023

Rebase smartWallet (#8488) to release-mainnet1B branch #8609

Closed

Chris-Hibbert force-pushed the 8445-addWatcher branch from 7bc60c4 to 3cf7cc4 Compare December 5, 2023 00:19

Chris-Hibbert requested a review from turadg December 5, 2023 00:20

turadg requested changes Dec 5, 2023

View reviewed changes

Chris-Hibbert added a commit that referenced this pull request Dec 5, 2023

test: support for watchedPromises in fakeVirtualSupport

588d6a1

Needed by #8488

Chris-Hibbert mentioned this pull request Dec 5, 2023

test: support for watchedPromises in fakeVirtualSupport #8618

Merged

Chris-Hibbert force-pushed the 8445-addWatcher branch from 6e79669 to 86b166c Compare December 6, 2023 01:20

turadg mentioned this pull request Dec 7, 2023

8605 a3p integration #8635

Merged

Chris-Hibbert force-pushed the 8445-addWatcher branch from 2fd24cd to ec4cbce Compare December 14, 2023 00:38

Chris-Hibbert added a commit that referenced this pull request Dec 14, 2023

test: support for watchedPromises in fakeVirtualSupport

f73dd00

Needed by #8488

Chris-Hibbert mentioned this pull request Dec 14, 2023

support for watchedPromises in fakeVirtualSupport #8661

Merged

Chris-Hibbert requested a review from turadg December 15, 2023 17:24

Chris-Hibbert added a commit that referenced this pull request Dec 15, 2023

test: support for watchedPromises in fakeVirtualSupport

fb333ee

Needed by #8488

turadg reviewed Dec 15, 2023

View reviewed changes

Chris-Hibbert force-pushed the 8445-addWatcher branch from 19bfad6 to cd1c09a Compare December 15, 2023 23:03

Chris-Hibbert requested review from turadg and dckc December 19, 2023 17:25

Chris-Hibbert force-pushed the 8445-addWatcher branch 3 times, most recently from 3b6db32 to 50b525c Compare January 4, 2024 19:00

turadg approved these changes Jan 4, 2024

View reviewed changes

Chris-Hibbert force-pushed the 8445-addWatcher branch from 50b525c to 37b4e84 Compare January 4, 2024 23:37

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jan 4, 2024

feat: smartWallet verstion 2 with watchedPromises

5ed5107

pulled offers.js and payments.js into smartWallet.js as they shared plenty of state that needs to be durable in order to be callable from the watchedPromise. build an upgrade proposal; tested in Agoric/agoric-3-proposals#34

Chris-Hibbert force-pushed the 8445-addWatcher branch from 37b4e84 to 5ed5107 Compare January 5, 2024 00:55

mergify bot merged commit 376e47d into master Jan 5, 2024
67 checks passed

mergify bot deleted the 8445-addWatcher branch January 5, 2024 01:27

Chris-Hibbert mentioned this pull request Jan 18, 2024

WalletFactory survive upgrades and repair outstanding purses and offers #8773

Merged

Chris-Hibbert mentioned this pull request Feb 26, 2024

refactor smartWallet error handling on executeOffer #8997

Closed

erights mentioned this pull request Feb 27, 2024

smart wallet report rejections of durable promises #8998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8445 add promiseWatcher to smartWallet #8488

8445 add promiseWatcher to smartWallet #8488

Chris-Hibbert commented Nov 1, 2023

Chris-Hibbert Nov 9, 2023

turadg Nov 29, 2023

Chris-Hibbert Dec 5, 2023

turadg Nov 29, 2023

turadg Nov 29, 2023

Chris-Hibbert Nov 30, 2023

turadg Dec 5, 2023

turadg Dec 15, 2023

dckc Dec 15, 2023

turadg Dec 15, 2023

turadg Dec 15, 2023

turadg left a comment

Chris-Hibbert commented Dec 19, 2023

turadg left a comment

turadg Jan 4, 2024

dckc Jan 4, 2024

turadg Jan 4, 2024

dckc Jan 4, 2024 •

edited

Loading

Chris-Hibbert Jan 4, 2024

turadg Jan 5, 2024

turadg Jan 4, 2024

Chris-Hibbert Jan 4, 2024

	// XXX is there a better guard for a bigMapStore?
	// XXX is there a tighter shape for a bigMapStore than M.any()?

	// XXX is there a tighter guard for a bigMapStore than M.any()?
	// TODO define a shape for bigMapStore

8445 add promiseWatcher to smartWallet #8488

8445 add promiseWatcher to smartWallet #8488

Conversation

Chris-Hibbert commented Nov 1, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

turadg left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Dec 19, 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

dckc Jan 4, 2024 • 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

dckc Jan 4, 2024 •

edited

Loading