Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: smartWallet watch ERTP purse balances across zoe upgrades #8775

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #8773

Description

@dckc wrote #8557 in order to address #8293. In #8773, we added this to the upgrade-14 release branch and made sure it worked in the a3p environment. This brings the final version back to master.

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

No user discernible changes here.

Testing Considerations

Tested manually in A3P. tests within a3p-integration are promised.

Upgrade Considerations

upgrade the walletFactory and don't lose anything.

@mhofman
Copy link
Member

mhofman commented Jan 19, 2024

Looking at the diff between #8773 and this branch, I see a few minor differences in smartWallet.js that are not types related. While they don't seem to really affect behavior (however one await order change seem suspicious), to simplify future cherry-picks, can we look into unifying these differences?

https://github.com/Agoric/agoric-sdk/compare/8445-reintegrate..8445-upgradeWF-a3p-test#diff-e7c31ef033a5e98c8569c9a9582f4b490cc45e5bbfaa6847622c5575eaf989ce

git diff 8445-reintegrate 8445-upgradeWF-a3p-test -- packages/smart-wallet/src/*.js
diff --git a/packages/smart-wallet/src/invitations.js b/packages/smart-wallet/src/invitations.js
index a5fcc3255..793e85b40 100644
--- a/packages/smart-wallet/src/invitations.js
+++ b/packages/smart-wallet/src/invitations.js
@@ -59,10 +59,10 @@ const MAX_PIPE_LENGTH = 2;
 
 /**
  * @param {ERef<ZoeService>} zoe
- * @param {ERef<import('@agoric/vats').NameHub>} agoricNames
+ * @param {ERef<NameHub>} agoricNames
  * @param {Brand<'set'>} invitationBrand
  * @param {Purse<'set'>} invitationsPurse
- * @param {(fromOfferId: string) => import('./types.js').InvitationMakers} getInvitationContinuation
+ * @param {(fromOfferId: string) => import('./types').RemoteInvitationMakers} getInvitationContinuation
  */
 export const makeInvitationsHelper = (
   zoe,
diff --git a/packages/smart-wallet/src/offerWatcher.js b/packages/smart-wallet/src/offerWatcher.js
index 269830cb7..5e14abe06 100644
--- a/packages/smart-wallet/src/offerWatcher.js
+++ b/packages/smart-wallet/src/offerWatcher.js
@@ -23,7 +23,7 @@ import { UNPUBLISHED_RESULT } from './offers.js';
 
 /**
  * @template {any} T
- * @typedef {import('@agoric/swingset-liveslots').PromiseWatcher<T, [UserSeat]>} OfferPromiseWatcher<T, [UserSeat]
+ * @typedef {{ onFulfilled: (any) => any, onRejected: (err: Error, seat: any) => void }} OfferPromiseWatcher<T, [UserSeat]
  */
 
 /**
@@ -40,6 +40,7 @@ import { UNPUBLISHED_RESULT } from './offers.js';
  */
 const watchForOfferResult = ({ resultWatcher }, seat) => {
   const p = E(seat).getOfferResult();
+  // @ts-expect-error missing declarations?
   watchPromise(p, resultWatcher, seat);
   return p;
 };
@@ -50,6 +51,7 @@ const watchForOfferResult = ({ resultWatcher }, seat) => {
  */
 const watchForNumWants = ({ numWantsWatcher }, seat) => {
   const p = E(seat).numWantsSatisfied();
+  // @ts-expect-error missing declarations?
   watchPromise(p, numWantsWatcher, seat);
   return p;
 };
@@ -60,6 +62,7 @@ const watchForNumWants = ({ numWantsWatcher }, seat) => {
  */
 const watchForPayout = ({ paymentWatcher }, seat) => {
   const p = E(seat).getPayouts();
+  // @ts-expect-error missing declarations?
   watchPromise(p, paymentWatcher, seat);
   return p;
 };
diff --git a/packages/smart-wallet/src/offers.js b/packages/smart-wallet/src/offers.js
index e892bf2b0..a6f799200 100644
--- a/packages/smart-wallet/src/offers.js
+++ b/packages/smart-wallet/src/offers.js
@@ -5,7 +5,7 @@
 /**
  * @typedef {{
  *   id: OfferId,
- *   invitationSpec: import('./invitations.js').InvitationSpec,
+ *   invitationSpec: import('./invitations').InvitationSpec,
  *   proposal: Proposal,
  *   offerArgs?: unknown
  * }} OfferSpec
diff --git a/packages/smart-wallet/src/smartWallet.js b/packages/smart-wallet/src/smartWallet.js
index 0721f21a3..576d69411 100644
--- a/packages/smart-wallet/src/smartWallet.js
+++ b/packages/smart-wallet/src/smartWallet.js
@@ -171,7 +171,7 @@ const trace = makeTracer('SmrtWlt');
  *
  * @typedef {Readonly<UniqueParams & {
  *   paymentQueues: MapStore<Brand, Array<Payment>>,
- *   offerToInvitationMakers: MapStore<string, import('./types.js').InvitationMakers>,
+ *   offerToInvitationMakers: MapStore<string, import('./types').RemoteInvitationMakers>,
  *   offerToPublicSubscriberPaths: MapStore<string, Record<string, string>>,
  *   offerToUsedInvitation: MapStore<string, Amount>,
  *   purseBalances: MapStore<Purse, Amount>,
@@ -273,12 +273,11 @@ export const prepareSmartWallet = (baggage, shared) => {
 
   const makeOfferWatcher = prepareOfferWatcher(baggage);
 
+  const NotifierShape = M.remotable();
   const updateShape = {
     value: AmountShape,
     updateCount: M.bigint(),
   };
-
-  const NotifierShape = M.remotable();
   const amountWatcherGuard = M.interface('paymentWatcher', {
     onFulfilled: M.call(updateShape, NotifierShape).returns(),
     onRejected: M.call(M.any(), NotifierShape).returns(M.promise()),
@@ -535,23 +534,24 @@ export const prepareSmartWallet = (baggage, shared) => {
 
         /** @type {(purse: ERef<Purse>) => Promise<void>} */
         async watchPurse(purseRef) {
-          const { helper } = this.facets;
+          const purse = await purseRef; // promises don't fit in durable storage
 
+          const { helper } = this.facets;
+          // publish purse's balance and changes
           // This would seem to fit the observeNotifier() pattern,
           // but purse notifiers are not necessarily durable.
           // If there is an error due to upgrade, retry watchPurse().
+          const notifier = await E(purse).getCurrentAmountNotifier();
 
-          const purse = await purseRef; // promises don't fit in durable storage
           const handler = makeAmountWatcher(purse, helper);
-
-          // publish purse's balance and changes
-          const notifier = await E(purse).getCurrentAmountNotifier();
           const startP = E(notifier).getUpdateSince(undefined);
+          // @ts-expect-error import watchPromise's type is unknown
           watchPromise(startP, handler, notifier);
         },
 
         watchNextBalance(handler, notifier, updateCount) {
           const nextP = E(notifier).getUpdateSince(updateCount);
+          // @ts-expect-error import watchPromise's type is unknown
           watchPromise(nextP, handler, notifier);
         },
 
@@ -624,10 +624,10 @@ export const prepareSmartWallet = (baggage, shared) => {
          */
         async repairUnwatchedSeats() {
           const { state, facets } = this;
-          const { address, invitationPurse, liveOffers, liveOfferSeats } =
-            state;
-          const { zoe, agoricNames, invitationBrand, invitationIssuer } =
-            shared;
+          const { address, invitationPurse } = state;
+          const { liveOffers, liveOfferSeats } = state;
+          const { zoe, agoricNames } = shared;
+          const { invitationBrand, invitationIssuer } = shared;
 
           const invitationFromSpec = makeInvitationsHelper(
             zoe,
@@ -674,7 +674,7 @@ export const prepareSmartWallet = (baggage, shared) => {
           trace(`Found ${brandToPurses.values()} purse(s) for ${address}`);
           for (const purses of brandToPurses.values()) {
             for (const record of purses) {
-              void helper.watchPurse(record.purse);
+              helper.watchPurse(record.purse);
               trace(`Repaired purse ${record.petname} of ${address}`);
             }
           }
@@ -968,8 +968,8 @@ export const prepareSmartWallet = (baggage, shared) => {
             await watchOfferOutcomes(watcher, seatRef);
           } catch (err) {
             facets.helper.logWalletError('OFFER ERROR:', err);
-
             // Notify the user
+
             if (err.upgradeMessage === 'vat upgraded') {
               // The offer watchers will reconnect. Don't reclaim or exit
               return;
diff --git a/packages/smart-wallet/src/utils.js b/packages/smart-wallet/src/utils.js
index d89155ea8..de4758f5b 100644
--- a/packages/smart-wallet/src/utils.js
+++ b/packages/smart-wallet/src/utils.js
@@ -9,7 +9,7 @@ const trace = makeTracer('WUTIL', false);
 
 /** @param {Brand<'set'>} [invitationBrand] */
 export const makeWalletStateCoalescer = (invitationBrand = undefined) => {
-  /** @type {Map<import('./offers.js').OfferId, import('./offers.js').OfferStatus>} */
+  /** @type {Map<import('./offers').OfferId, import('./offers').OfferStatus>} */
   const offerStatuses = new Map();
   /** @type {Map<Brand, Amount>} */
   const balances = new Map();
@@ -17,11 +17,11 @@ export const makeWalletStateCoalescer = (invitationBrand = undefined) => {
   /**
    * keyed by description; xxx assumes unique
    *
-   * @type {Map<import('./offers.js').OfferId, { acceptedIn: import('./offers.js').OfferId, description: string, instance: Instance }>}
+   * @type {Map<import('./offers').OfferId, { acceptedIn: import('./offers').OfferId, description: string, instance: Instance }>}
    */
   const invitationsReceived = new Map();
 
-  /** @param {import('./smartWallet.js').UpdateRecord | {}} updateRecord newer than previous */
+  /** @param {import('./smartWallet').UpdateRecord | {}} updateRecord newer than previous */
   const update = updateRecord => {
     if (!('updated' in updateRecord)) {
       return;
@@ -93,7 +93,7 @@ export const makeWalletStateCoalescer = (invitationBrand = undefined) => {
  * If this proves to be a problem we can add an option to this or a related
  * utility to reset state from RPC.
  *
- * @param {ERef<Subscriber<import('./smartWallet.js').UpdateRecord>>} updates
+ * @param {ERef<Subscriber<import('./smartWallet').UpdateRecord>>} updates
  * @param {Brand<'set'>} [invitationBrand]
  */
 export const coalesceUpdates = (updates, invitationBrand) => {
@@ -125,7 +125,7 @@ export const assertHasData = async follower => {
 /**
  * Handles the case of falsy argument so the caller can consistently await.
  *
- * @param {import('./types.js').PublicSubscribers | import('@agoric/zoe/src/contractSupport/index.js').TopicsRecord} [subscribers]
+ * @param {import('./types.js').PublicSubscribers | import('@agoric/zoe/src/contractSupport').TopicsRecord} [subscribers]
  * @returns {ERef<Record<string, string>> | null}
  */
 export const objectMapStoragePath = subscribers => {
diff --git a/packages/smart-wallet/src/walletFactory.js b/packages/smart-wallet/src/walletFactory.js
index 58d8aa0b0..8e279a3d2 100644
--- a/packages/smart-wallet/src/walletFactory.js
+++ b/packages/smart-wallet/src/walletFactory.js
@@ -74,7 +74,7 @@ export const makeAssetRegistry = assetPublisher => {
    *   brand: Brand,
    *   displayInfo: DisplayInfo,
    *   issuer: Issuer,
-   *   petname: import('./types.js').Petname
+   *   petname: import('./types').Petname
    * }} BrandDescriptor
    * For use by clients to describe brands to users. Includes `displayInfo` to save a remote call.
    */
@@ -125,12 +125,12 @@ export const makeAssetRegistry = assetPublisher => {
  *
  * @typedef {{
  *   getAssetSubscription: () => ERef<
- *     IterableEachTopic<import('@agoric/vats/src/vat-bank.js').AssetDescriptor>>
+ *     IterableEachTopic<import('@agoric/vats/src/vat-bank').AssetDescriptor>>
  * }} AssetPublisher
  *
  * @typedef {boolean} isRevive
  * @typedef {{
- *   reviveWallet: (address: string) => Promise<import('./smartWallet.js').SmartWallet>,
+ *   reviveWallet: (address: string) => Promise<import('./smartWallet').SmartWallet>,
  *   ackWallet: (address: string) => isRevive,
  * }} WalletReviver
  */
@@ -282,16 +282,16 @@ export const prepare = async (zcf, privateArgs, baggage) => {
     {
       /**
        * @param {string} address
-       * @param {ERef<import('@agoric/vats/src/vat-bank.js').Bank>} bank
-       * @param {ERef<import('@agoric/vats/src/types.js').NameAdmin>} namesByAddressAdmin
-       * @returns {Promise<[wallet: import('./smartWallet.js').SmartWallet, isNew: boolean]>} wallet
+       * @param {ERef<import('@agoric/vats/src/vat-bank').Bank>} bank
+       * @param {ERef<import('@agoric/vats/').NameAdmin>} namesByAddressAdmin
+       * @returns {Promise<[wallet: import('./smartWallet').SmartWallet, isNew: boolean]>} wallet
        *   along with a flag to distinguish between looking up an existing wallet
        *   and creating a new one.
        */
       provideSmartWallet(address, bank, namesByAddressAdmin) {
         let isNew = false;
 
-        /** @type {(address: string) => Promise<import('./smartWallet.js').SmartWallet>} */
+        /** @type {(address: string) => Promise<import('./smartWallet').SmartWallet>} */
         const maker = async _address => {
           const invitationPurse = await E(invitationIssuer).makeEmptyPurse();
           const walletStorageNode = E(storageNode).makeChildNode(address);

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed it; the code looks familiar; I don't see any problems.

I defer to @mhofman to approve.

@Chris-Hibbert
Copy link
Contributor Author

can we look into unifying these differences?

#8790 merges these changes back in the release branch.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am satisfied that #8790 fixes these differences

@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jan 23, 2024
@mergify mergify bot merged commit 99fc025 into master Jan 23, 2024
66 checks passed
@mergify mergify bot deleted the 8445-reintegrate branch January 23, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants