smart wallet report rejections of durable promises #8998

turadg · 2024-02-26T23:58:24Z

closes: #8997

Description

This moves the error handling into an onRejected callback of the durable promise watcher for the offer result. The other two watchers are for particular values from Zoe after that promise resolves. Those are synchronous and don't need their own error handling.

It doesn't move tryReclaimingWithdrawnPayments because that would require heavier refactoring and in our analysis its a very unlikely scenario. I've commented with why it's there. I also changed its return type to Promise<void>, instead of all the settlements. Since that value was never read, it's not changing behavior and I called it a refactor.

Security Considerations

No change.

Scaling Considerations

No change.

Documentation Considerations

It would help if Zoe documented in its code which methods' promises settle together.

Testing Considerations

Ideally there'd be a new test for this fix but there isn't.

Upgrade Considerations

To be included in u14 upgrade of smartWallet.

mhofman · 2024-02-27T00:10:08Z

packages/smart-wallet/src/smartWallet.js

            if (offerSpec?.proposal?.give) {
              facets.payments
                .tryReclaimingWithdrawnPayments(offerSpec.id)


So we assume this case only matters for immediate rejections from the seat?

I suppose that tryReclaimingWithdrawnPayments is not very idempotent, nor parallel safe.

"parallel"?

Noting that at #8488 I see

along the way payments.tryReclaimingWithdrawnPayments() went away. I'm pretty sure it was redundant with ERTP recoverySets.

mhofman · 2024-02-27T00:12:11Z

packages/smart-wallet/src/offerWatcher.js

@@ -186,6 +193,17 @@ export const prepareOfferWatcher = baggage => {
              facets.helper.updateStatus({ result: UNPUBLISHED_RESULT });
          }
        },
+        /** @param {Error} err */
+        handleError(err) {


Maybe add a comment this will likely be called multiple times (aka it needs to handle parallels duplicate calls)

I don't get "parallel". Did you mean just "handle duplicate calls?"

There will be duplicate call, and since the async operation this initiates is not awaited, they will overlap

mhofman · 2024-02-27T00:14:17Z

there may be unique error messages.

To be honest, while it's technically possible for a vstorage client to recover each of these errors, it's painful as the same field is updated, likely in the same block.

Chris-Hibbert

I don't think there's any value in reporting the errors 4 times. The different promises are all triggered on the same Zoe promise. If they fail independently, something is deeply wrong and having multiple watchers won't help.

I would drop the watcher in the original catch clause, as well as two of the three in offerWatcher. My nominee to keep would be resultWatcher, mostly because it sounds like it's at a high semantic level.

erights · 2024-02-27T00:30:09Z

packages/smart-wallet/src/offerWatcher.js

@@ -186,6 +193,17 @@ export const prepareOfferWatcher = baggage => {
              facets.helper.updateStatus({ result: UNPUBLISHED_RESULT });
          }
        },
+        /** @param {Error} err */
+        handleError(err) {


I don't get "parallel". Did you mean just "handle duplicate calls?"

erights · 2024-02-27T00:35:58Z

packages/smart-wallet/src/offerWatcher.js

@@ -209,6 +227,8 @@ export const prepareOfferWatcher = baggage => {
          const { facets } = this;
          if (isUpgradeDisconnection(err)) {


This line makes clear that the type of err must include UpgradeDisconnection. So probably Error | UpgradeDisconnection. Likewise elsewhere. The guards for all of these are fine though, since they say only M.any().

erights · 2024-02-27T00:46:11Z

packages/smart-wallet/src/smartWallet.js

            if (offerSpec?.proposal?.give) {
              facets.payments
                .tryReclaimingWithdrawnPayments(offerSpec.id)


"parallel"?

Noting that at #8488 I see

along the way payments.tryReclaimingWithdrawnPayments() went away. I'm pretty sure it was redundant with ERTP recoverySets.

erights

@Chris-Hibbert and I walked through this verbally. LGTM!

We talked over the redundant errors being logged to vstorage. I think this is fine for now. The annoyance is minor, and I don't want to make further code changes at the last minute that we do not need.

erights · 2024-02-27T02:27:26Z

packages/smart-wallet/src/smartWallet.js


-            // Notify the user
+            // Update status to observers
            if (err.upgradeMessage === 'vat upgraded') {


@Chris-Hibbert I just checked again, and this is the only non-test check manually using upgradeMessage rather than the isUpgradeDisconnection. I'm satisfied enough at our analysis that this is harmless to request it in this PR.

@turadg, since you're still making changes, would you mind updating this?

Suggested change

if (err.upgradeMessage === 'vat upgraded') {

if (isUpgradeDisconnection(err)) {

oh yeah, I meant to but I don't think it's worth spinning CI at this point

erights

I did a "request changes" as an interlock, to head off any concurrent attempt to merge. But approving now in case I'm hard to reach. In any case, the one remaining fix I'd like to see isn't crucial anyway. But would be nice.

packages/smart-wallet/src/offerWatcher.js

now returns Promise<void> instead of all the records, but they weren't ever used so it won't change behavior

gibson042 · 2024-02-27T18:12:40Z

packages/smart-wallet/src/offerWatcher.js

+         * 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.


Suggested change

* 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.

* Called when the offer result promise rejects. The payment and

* numWantsSatisfied promises settle at the same time and don't need

* their own error handling.

Chris-Hibbert

Sorry, I requested a change while approving, and then everyone else approved. I don't want my request to get lost, so I'm changing my review status again.

And in the meantime, I requested another (very small) change.

Chris-Hibbert

Thanks. LGTM.

mhofman · 2024-02-27T18:43:58Z

Did this merge without new commits being pushed?

…9000)

mhofman reviewed Feb 27, 2024

View reviewed changes

Chris-Hibbert approved these changes Feb 27, 2024

View reviewed changes

erights reviewed Feb 27, 2024

View reviewed changes

erights approved these changes Feb 27, 2024

View reviewed changes

erights requested changes Feb 27, 2024

View reviewed changes

erights approved these changes Feb 27, 2024

View reviewed changes

gibson042 approved these changes Feb 27, 2024

View reviewed changes

turadg requested a review from Chris-Hibbert February 27, 2024 16:26

turadg marked this pull request as ready for review February 27, 2024 16:31

turadg mentioned this pull request Feb 27, 2024

smart wallet report rejections of durable promises (backport #8998) #9000

Merged

turadg added the force:integration Force integration tests to run on PR label Feb 27, 2024

Chris-Hibbert reviewed Feb 27, 2024

View reviewed changes

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

turadg added 4 commits February 27, 2024 09:23

chore(types): publishResult

8b0ec4c

fix: publish 'error' message for failure after upgrade

5397e0c

refactor: tryReclaimingWithdrawnPayments no return

36bb0e4

now returns Promise<void> instead of all the records, but they weren't ever used so it won't change behavior

refactor: onRejected reason

4d4c3f7

turadg force-pushed the 8997-error-handling branch from 2b7c4e5 to 4d4c3f7 Compare February 27, 2024 17:25

gibson042 reviewed Feb 27, 2024

View reviewed changes

Chris-Hibbert requested changes Feb 27, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Feb 27, 2024

Chris-Hibbert approved these changes Feb 27, 2024

View reviewed changes

mergify bot merged commit 6676107 into master Feb 27, 2024
74 checks passed

mergify bot deleted the 8997-error-handling branch February 27, 2024 18:40

mhofman added a commit that referenced this pull request Feb 27, 2024

smart wallet report rejections of durable promises (backport #8998) (#…

f3e5b2f

…9000)

erights mentioned this pull request Feb 29, 2024

refactor(smart-wallet): test UpgradeDisconnection correctly #9012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

smart wallet report rejections of durable promises #8998

smart wallet report rejections of durable promises #8998

turadg commented Feb 26, 2024 •

edited

Loading

mhofman Feb 27, 2024

erights Feb 27, 2024

mhofman Feb 27, 2024

erights Feb 27, 2024

mhofman Feb 27, 2024

mhofman commented Feb 27, 2024

Chris-Hibbert left a comment

erights Feb 27, 2024

erights Feb 27, 2024

erights Feb 27, 2024

erights left a comment

erights Feb 27, 2024

Chris-Hibbert Feb 27, 2024

turadg Feb 27, 2024 •

edited

Loading

erights left a comment

gibson042 Feb 27, 2024 •

edited

Loading

Chris-Hibbert left a comment

Chris-Hibbert left a comment

mhofman commented Feb 27, 2024

		@@ -209,6 +227,8 @@ export const prepareOfferWatcher = baggage => {
		const { facets } = this;
		if (isUpgradeDisconnection(err)) {

	if (err.upgradeMessage === 'vat upgraded') {
	if (isUpgradeDisconnection(err)) {

smart wallet report rejections of durable promises #8998

smart wallet report rejections of durable promises #8998

Conversation

turadg commented Feb 26, 2024 • edited Loading

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

mhofman commented Feb 27, 2024

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

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

gibson042 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

mhofman commented Feb 27, 2024

turadg commented Feb 26, 2024 •

edited

Loading

turadg Feb 27, 2024 •

edited

Loading

gibson042 Feb 27, 2024 •

edited

Loading