7004 wallet errors #7135

turadg · 2023-03-08T21:53:39Z

closes: #7004

Description

Users need to be able find out about errors in their wallet actions. The offer handler already publishes errors to vstorage. This makes it so that errors that happen earlier, in reading the walletAction, also get published, under a new updated: walletAction status.

It leaves out publishing errors before that because there's no way yet to associate it with the account owner or transaction. And once there is, feeding it back will be done by the Cosmos bridge instead of the contract.

This also cleans up error handling within the offer executor. This required test changes, mostly simplifications to use throwsAsync instead of checking vstorage after. We have sufficient tests for offer failures writing to vstorage so we don't have to test it everywhere.

Security Considerations

Error's message is logged to vstorage. The error construction should redact as needed.

Scaling Considerations

Writes walletAction errors to vstorage.

Documentation Considerations

--

Testing Considerations

CI

turadg · 2023-03-08T21:56:57Z

packages/vats/test/bootstrapTests/test-vaults-integration.js

@@ -182,15 +202,17 @@ test('close vault', async t => {
    {
      offerId: 'close-well',
      collateralBrandKey,
-      giveMinted: 5.0,


this was a bug in the test. it wasn't detected because I had mistakenly thought numWantsSatisified: 1 meant it succeeded. There were no wants so they were trivially satisfied.

That it looked like it succeeded and it didn't underlines the value of the promise rejecting.

datadog-full-agoric · 2023-03-08T22:04:01Z

Datadog Report

Branch report: 7004-wallet-errors
Commit report: 94718b4

✅ agoric-sdk: 0 Failed, 0 New Flaky, 3663 Passed, 55 Skipped, 14m 38.85s Wall Time

dckc · 2023-03-09T02:39:42Z

feeding it back will be done by the Cosmos bridge instead of the contract

I hope we have an issue to represent that plan.

This PR would still be forward progress without it, though...

samsiegart · 2023-03-09T02:51:25Z

Are there any UI changes that need to accompany this?

dckc

I need to understand the compatibility impact on the wallet UI better.

With this change, if the wallet UI doesn't change, will the wallet UI fail to notice some of the errors that it currently notices?

dckc · 2023-03-09T02:45:21Z

packages/smart-wallet/src/payments.js

@@ -1,3 +1,4 @@
+import { Fail } from '@agoric/assert';


Where did we get on that idiom? Ah yes... #5672 is still open.

dckc · 2023-03-09T02:57:42Z

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

-      error:
-        'Error: In "pushPrice" method of (OracleKit oracle): arg 0: unitPrice: number 1 - Must be a bigint',
-      // trivially satisfied because the Want is empty
-      numWantsSatisfied: 1,
+      message:
+        'In "pushPrice" method of (OracleKit oracle): arg 0: unitPrice: number 1 - Must be a bigint',


Does the wallet UI need a corresponding update to error handling?

That would make this a BREAKING CHANGE, yes?

That would be a breaking change, but the wallet UI isn't affected. The same errors are written to vstorage.

dckc · 2023-03-09T02:58:44Z

packages/smart-wallet/src/smartWallet.js

+                err,
+              );
+              updatePublishKit.publisher.publish({
+                updated: 'walletAction',


impacts wallet UI?

Wallet UI can begin to observe these messages. It shouldn't break because it should never have assumed that new updates would never be added.

dckc · 2023-03-09T02:59:01Z

packages/smart-wallet/src/walletFactory.js

+       * Designed to be called by the bridgeManager vat.
+       *
+       * If this errors before calling handleBridgeAction(), the failure will not be observable. The
+       * promise does rejects, but as of now bridge manager drops instead of handling it. Eventually


Suggested change

* promise does rejects, but as of now bridge manager drops instead of handling it. Eventually

* promise does reject, but as of now bridge manager drops instead of handling it. Eventually

dckc · 2023-03-09T02:59:19Z

packages/smart-wallet/src/walletFactory.js

+       * associate it with. (We could have a shared `lastError` node but it would be so noisy as to
+       * not provide much info to the end user.


Suggested change

* associate it with. (We could have a shared `lastError` node but it would be so noisy as to

* not provide much info to the end user.

* associate it with. (We could have a shared `lastError` node but it would be so noisy as to

* not provide much info to the end user.)

dckc · 2023-03-09T03:03:09Z

packages/vats/test/bootstrapTests/test-vaults-integration.js

+const likePayouts = (collateral, minted) => ({
+  Collateral: {
+    value: {
+      digits: String(collateral * 1_000_000),


I don't think this will survive the move to smallcaps (#6822).

Not critical. We'll notice when it's time.

turadg · 2023-03-09T17:01:14Z

With this change, if the wallet UI doesn't change, will the wallet UI fail to notice some of the errors that it currently notices?

No, this doesn't change any of the what has been published to vstorage. The only change to vstorage is that it adds a new update event for walletAction messages. The wallet UI can begin to read and display those. A good idea but not a breaking change.

The changes to the tests for existing messages is that now the function call carries the rejection message so tests running in JS (which can get the promise) don't need to resort to reading over RPC.

dckc

ok, not a breaking change then.

turadg · 2023-03-09T21:55:00Z

feeding it back will be done by the Cosmos bridge instead of the contract

I hope we have an issue to represent that plan.

#7148

turadg requested review from dckc and samsiegart March 8, 2023 21:53

turadg commented Mar 8, 2023

View reviewed changes

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

dckc reviewed Mar 9, 2023

View reviewed changes

dckc approved these changes Mar 9, 2023

View reviewed changes

turadg added 3 commits March 9, 2023 09:24

refactor: assertion by || Fail

7667c30

feat(wallet): record bridge errors to vstorage

f8581e9

feat(wallet)!: reject executeOffer on failure

308caab

turadg force-pushed the 7004-wallet-errors branch from 5d89556 to 308caab Compare March 9, 2023 17:25

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

mergify bot merged commit 95c7d1a into master Mar 9, 2023

mergify bot deleted the 7004-wallet-errors branch March 9, 2023 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7004 wallet errors #7135

7004 wallet errors #7135

turadg commented Mar 8, 2023 •

edited

Loading

turadg Mar 8, 2023 •

edited

Loading

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

edited

Loading

dckc commented Mar 9, 2023

samsiegart commented Mar 9, 2023

dckc left a comment

dckc Mar 9, 2023

dckc Mar 9, 2023

turadg Mar 9, 2023

dckc Mar 9, 2023

turadg Mar 9, 2023

dckc Mar 9, 2023

dckc Mar 9, 2023

dckc Mar 9, 2023

turadg commented Mar 9, 2023

dckc left a comment

turadg commented Mar 9, 2023

	* promise does rejects, but as of now bridge manager drops instead of handling it. Eventually
	* promise does reject, but as of now bridge manager drops instead of handling it. Eventually

		* associate it with. (We could have a shared `lastError` node but it would be so noisy as to
		* not provide much info to the end user.

7004 wallet errors #7135

7004 wallet errors #7135

Conversation

turadg commented Mar 8, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

turadg Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

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

Datadog Report

dckc commented Mar 9, 2023

samsiegart commented Mar 9, 2023

dckc 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

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 commented Mar 9, 2023

dckc left a comment

Choose a reason for hiding this comment

turadg commented Mar 9, 2023

turadg commented Mar 8, 2023 •

edited

Loading

turadg Mar 8, 2023 •

edited

Loading

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

edited

Loading