From 5397e0cf76f29074e77227f61576e784e5016d08 Mon Sep 17 00:00:00 2001
From: Turadg Aleahmad <turadg@agoric.com>
Date: Mon, 26 Feb 2024 15:50:17 -0800
Subject: [PATCH] fix: publish 'error' message for failure after upgrade

---
 packages/smart-wallet/src/offerWatcher.js | 34 +++++++++++++++++++++++
 packages/smart-wallet/src/smartWallet.js  | 19 ++++++-------
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/packages/smart-wallet/src/offerWatcher.js b/packages/smart-wallet/src/offerWatcher.js
index 97309cd6b7d..18d04909e32 100644
--- a/packages/smart-wallet/src/offerWatcher.js
+++ b/packages/smart-wallet/src/offerWatcher.js
@@ -87,6 +87,7 @@ const offerWatcherGuard = harden({
       .optional(M.record())
       .returns(),
     publishResult: M.call(M.any()).returns(),
+    handleError: M.call(M.error()).returns(),
   }),
   paymentWatcher: M.interface('paymentWatcher', {
     onFulfilled: M.call(PaymentPKeywordRecordShape, SeatShape).returns(
@@ -131,6 +132,9 @@ export const prepareOfferWatcher = baggage => {
     }),
     {
       helper: {
+        /**
+         * @param {Record<string, unknown>} offerStatusUpdates
+         */
         updateStatus(offerStatusUpdates) {
           const { state } = this;
           state.status = harden({ ...state.status, ...offerStatusUpdates });
@@ -189,6 +193,22 @@ export const prepareOfferWatcher = baggage => {
               facets.helper.updateStatus({ result: UNPUBLISHED_RESULT });
           }
         },
+        /**
+         * Called when the offer result promise rejects. The other two watchers
+         * are waiting for particular values out of Zoe but they settle at the same time
+         * and don't need their own error handling.
+         * @param {Error} err
+         */
+        handleError(err) {
+          const { facets } = this;
+          facets.helper.updateStatus({ error: err.toString() });
+          const { seatRef } = this.state;
+          void E.when(E(seatRef).hasExited(), hasExited => {
+            if (!hasExited) {
+              void E(seatRef).tryExit();
+            }
+          });
+        },
       },
 
       /** @type {OutcomeWatchers['paymentWatcher']} */
@@ -205,6 +225,8 @@ export const prepareOfferWatcher = baggage => {
           facets.helper.updateStatus({ payouts: amounts });
         },
         /**
+         * If promise disconnected, watch again. Or if there's an Error, handle it.
+         *
          * @param {Error} err
          * @param {UserSeat} seat
          */
@@ -212,6 +234,8 @@ export const prepareOfferWatcher = baggage => {
           const { facets } = this;
           if (isUpgradeDisconnection(err)) {
             void watchForPayout(facets, seat);
+          } else {
+            facets.helper.handleError(err);
           }
         },
       },
@@ -223,6 +247,8 @@ export const prepareOfferWatcher = baggage => {
           facets.helper.publishResult(result);
         },
         /**
+         * If promise disconnected, watch again. Or if there's an Error, handle it.
+         *
          * @param {Error} err
          * @param {UserSeat} seat
          */
@@ -230,6 +256,8 @@ export const prepareOfferWatcher = baggage => {
           const { facets } = this;
           if (isUpgradeDisconnection(err)) {
             void watchForOfferResult(facets, seat);
+          } else {
+            facets.helper.handleError(err);
           }
         },
       },
@@ -242,6 +270,12 @@ export const prepareOfferWatcher = baggage => {
           facets.helper.updateStatus({ numWantsSatisfied: numSatisfied });
         },
         /**
+         * If promise disconnected, watch again.
+         *
+         * Errors are handled by the paymentWatcher because numWantsSatisfied()
+         * and getPayouts() settle the same (they await the same promise and
+         * then synchronously return a local value).
+         *
          * @param {Error} err
          * @param {UserSeat} seat
          */
diff --git a/packages/smart-wallet/src/smartWallet.js b/packages/smart-wallet/src/smartWallet.js
index 4242fd2f060..baac91211ce 100644
--- a/packages/smart-wallet/src/smartWallet.js
+++ b/packages/smart-wallet/src/smartWallet.js
@@ -456,6 +456,7 @@ export const prepareSmartWallet = (baggage, shared) => {
     }),
   };
 
+  // TODO move to top level so its type can be exported
   /**
    * Make the durable object to return, but taking some parameters that are awaited by a wrapping function.
    * This is necessary because the class kit construction helpers, `initState` and `finish` run synchronously
@@ -990,14 +991,15 @@ export const prepareSmartWallet = (baggage, shared) => {
             // await so that any errors are caught and handled below
             await watchOfferOutcomes(watcher, seatRef);
           } catch (err) {
-            facets.helper.logWalletError('OFFER ERROR:', err);
+            // This block only runs if the block above fails during one vat incarnation.
+            facets.helper.logWalletError('IMMEDIATE OFFER ERROR:', err);
 
-            // Notify the user
+            // Update status to observers
             if (err.upgradeMessage === 'vat upgraded') {
               // The offer watchers will reconnect. Don't reclaim or exit
               return;
             } else if (watcher) {
-              watcher.helper.updateStatus({ error: err.toString() });
+              // The watcher's onRejected will updateStatus()
             } else {
               facets.helper.updateStatus({
                 error: err.toString(),
@@ -1005,6 +1007,7 @@ export const prepareSmartWallet = (baggage, shared) => {
               });
             }
 
+            // Backstop recovery, in case something very basic fails.
             if (offerSpec?.proposal?.give) {
               facets.payments
                 .tryReclaimingWithdrawnPayments(offerSpec.id)
@@ -1016,14 +1019,8 @@ export const prepareSmartWallet = (baggage, shared) => {
                 );
             }
 
-            if (seatRef) {
-              void E.when(E(seatRef).hasExited(), hasExited => {
-                if (!hasExited) {
-                  void E(seatRef).tryExit();
-                }
-              });
-            }
-
+            // XXX tests rely on throwing immediate errors, not covering the
+            // error handling in the event the failure is after an upgrade
             throw err;
           }
         },