Bundle error logging #377

blakecduncan · 2022-11-04T12:13:49Z

https://loom.com/share/2a2039a17af04e23838561a08b0c6aaf

What is this PR doing?

Adding a submit error column to the DB
Updating the bundle receipt API call to return a 202 if the bundle exists in the DB
New response object for bundle receipt API call (note: this will break demos. It's a simple fix but I want to make sure it is okay with everyone)

Additional work

We would need to update examples and the extension because right now, they expect the /bundleReceipt API call to return a 404 when a bundle is pending.

Future improvements

Add a status column to the bundles table so that we don't delete bundles when the are successful or fail (we are currently storing the successful bundles in memory)

How can these changes be manually tested?

You can run the zeroAddressError script to trigger a bundle error and log out a message

Does this PR resolve or contribute to any issues?

Resolves #301

Checklist

I have manually tested these changes
Post a link to the PR in the group chat

Guidelines

If your PR is not ready, mark it as a draft
The resolve conversation button is for reviewers, not authors
- (But add a 'done' comment or similar)

blakecduncan · 2022-11-10T15:03:25Z

aggregator/manualTests/zeroAddressError.ts

@@ -0,0 +1,50 @@
+#!/usr/bin/env -S deno run --allow-net --allow-env --allow-read --allow-write --unstable


This is just a script to trigger an error in a bundle for testing

aggregator/src/app/AggregationStrategy.ts

@@ -235,11 +235,16 @@ export default class AggregationStrategy {
      let success: boolean;

      if (bundleResult.success) {
-        const [operationResults] = bundleResult.returnValue;
+        const [operationStatuses, results] = bundleResult.returnValue;


aggregator/src/app/EthereumService.ts

+          errorArgBytesString,
+        )[0]; // decoded bytes is a string of the action index that errored.
+
+        console.error(errorString)


aggregator/src/app/AggregationStrategy.ts

+        // TODO: is it possible to decode this?
+        // Here is an example when I try to send more ETH than an account has:results =
+        // results = [ "0x4e487b710000000000000000000000000000000000000000000000000000000000000011" ]


aggregator/src/app/EthereumService.ts

This reverts commit 44fec67.

blakecduncan · 2022-11-22T11:17:41Z

aggregator/src/app/AggregationStrategy.ts

@@ -444,3 +461,89 @@ export default class AggregationStrategy {
    return left;
  }
 }
+
+// The below code is temporary till the functions are exposed in the client module


After this is merged and a new version is published, I will delete the below code and use the function from the client module

#407

@blakecduncan You can do that now #407 . You can use https://github.com/web3well/bls-wallet/blob/main/docs/local_development.md#aggregator-1 as a guide, but feel free to ping me if you need help.

blakecduncan · 2022-11-22T11:19:43Z

aggregator/src/app/AggregationStrategy.ts


-      return { success, fee };
+      return { success, fee, errorReason };


Returning the error reason here so that we can link it to a failed row later when we check the index of the failed bundle

blakecduncan · 2022-11-22T11:20:36Z

aggregator/src/app/BundleRouter.ts

@@ -24,6 +24,16 @@ export default function BundleRouter(bundleService: BundleService) {
  router.get(
    "bundleReceipt/:hash",
    async (ctx) => {
+      // check pending


Return 202 error message if the bundle is in the DB. Also, pass a submit error in the body

jacque006

Would be good to have an integration test case(s) for the HTTP 202 path that ensures the bundle lifecycle & error decoding works as expected.

aggregator/src/app/AggregationStrategy.ts

aggregator/src/app/BundleRouter.ts

blakecduncan · 2022-11-23T14:54:45Z

Would be good to have an integration test case(s) for the HTTP 202 path that ensures the bundle lifecycle & error decoding works as expected.

Yeah good call, I'll write some tests for this 👍

…ent types

voltrevo · 2022-12-01T00:55:51Z

Idk how I'm missing these lint errors

😄. How's your IDE set-up? Are the linting errors showing? Is autofix-on-save working?

blakecduncan · 2022-12-01T12:45:40Z

aggregator/src/app/AggregationStrategy.ts

@@ -176,6 +178,10 @@ export default class AggregationStrategy {

    if (firstFailureIndex !== nil) {
      const failedRow = includedRows[firstFailureIndex];
+      const errorReason = fees[firstFailureIndex].errorReason;
+      if (errorReason) {
+        failedRow.submitError = errorReason.message || '';


@voltrevo Yeah that makes sense I'll switch it. I was doing this because I didn't make the column nullable in the DB, so it was setting it by default to an empty string. I updated that per your comment below and was able to make this more consistent with the undefined/nil

aggregator/src/app/AggregationStrategy.ts

@@ -207,6 +213,7 @@ export default class AggregationStrategy {
  async #measureFees(bundles: Bundle[]): Promise<{
    success: boolean;
    fee: BigNumber;
+    errorReason: OperationResultError | null;


aggregator/src/app/AggregationStrategy.ts

-      const fee = after.returnValue[0].sub(before.returnValue[0]);
+      const [operationStatuses, results] = bundleResult.returnValue;
+
+      let errorReason: OperationResultError | null = null;


aggregator/src/app/AggregationStrategy.ts

+            return;
+          }
+        });
+        errorReason = error[0] ?? null;


voltrevo · 2022-12-01T01:10:11Z

aggregator/src/app/BundleRouter.ts

@@ -24,14 +24,29 @@ export default function BundleRouter(bundleService: BundleService) {
  router.get(
    "bundleReceipt/:hash",
    async (ctx) => {
+      const pendingBundle = await bundleService.lookupBundle(ctx.params.hash!);
+      if (pendingBundle) {
+        ctx.response.status = 202;


I don't think 202 is appropriate here. It's the bundle that's been accepted, not the receipt. Also 'accepted' is supposed to refer to the current request, not the resource you're asking about. More info.

(Possibly 202 is appropriate for the original POST to /bundle, but I'm not actually a fan of worrying about these HTTP nuances.)

On a practical note, if we keep 404 here we can still also provide the submitError, and this way it wouldn't be a breaking change. Wdyt?

Yeah, I get what you're saying. I do like the idea of not doing a breaking change, as there are a good amount of things that have to be updated at this point.

However, I think the /bundleReceipt API call is often used as a status check for a bundle. So my question would be, what should we expect to be returned from this API call? With the approach you recommended, we would keep a 200 status as is (i.e. returns the bundle receipt) and pass the submit error in a 404 response body. That would be fine by me - I think the only thing we aren't communicating explicitly is the bundle status. For example, we could communicate a pending or a failed state (ex: even if it failed once, we may still retry it). Maybe we don't care about that though or it should be a different API endpoint.

@voltrevo What do you think? I'd be happy to hop on a call tomorrow when I get online if you are still around. Also, @jacque006 not sure if you have any thoughts?

I'm not sure we're on the same page here. Definitely sounds like a call is in order.

With the approach you recommended, we would keep a 200 status as is (i.e. returns the bundle receipt) and pass the submit error in a 404 response body.

Did you mean to say 'keep a 200 status'? It seems wrong, and also contradicts the second half of this sentence. At least my reading of it.

To rephrase my suggestion, I think if the bundle is pending, then /bundleReceipt should 404, since there isn't a bundle receipt, but we can also put the submitError in the body of this response. Meaning: "No, there's no receipt (404), and here's why (submitError)".

Ahh my response was poorly worded. We are on the same page. When I said keep the 200 status, I really meant get rid of my logic to add the 202. You worded it better than I did - that is what I meant to say.

Yep, we can discuss offline. In general, I have seen services go one of two ways:

Use a variety/specific HTTP codes to communicate the state of a resource (200/201/202/403/404/418)

Use a limited number of status codes (200/403/404) and use a status field or similar to communicate the state of the resource (pending/failed/succeeded). Some Google Cloud APIs go this route

I think either is fine as long as we are consistent with that pattern on our endpoints and document the possible responses in code. Hopefully most consumers use our clients which can abstract a lot of that away.

aggregator/test/AggregationStrategy.test.ts

+  const bundleRow: BundleRow = {
+    id: 0,
+    hash: "0x0",
+    submitError: "",


aggregator/test/BundleTable.test.ts

@@ -49,6 +49,7 @@ const sampleRows: BundleRow[] = [
      ],
      signature: ["0x00", "0x00"],
    },
+    submitError: "",


contracts/clients/src/Aggregator.ts

+
+export type BundleReceiptResponse = {
+  status: string;
+  submitError: string | null;


contracts/clients/src/Aggregator.ts

+export type BundleReceiptResponse = {
+  status: string;
+  submitError: string | null;
+  receipt: BundleReceipt | null;


voltrevo · 2022-12-01T01:30:09Z

extension/source/background/AggregatorController.ts

+      const bundleReceiptResponse = await aggregator.lookupReceipt(hash);
+      // If receipt doesn't exist, set to undefined to be consistent with
+      // the return type
+      const bundleReceipt = bundleReceiptResponse?.receipt || undefined;


|| undefined is unnecessary (obj?.field produces undefined when field is not there, even when obj is null).

Also prefer ?? over ||.

Yeah I was getting a typescript error the BundleReceipt | undefined does not equal BundleReceipt | undefined | null. I think with the changes you recommended above I can fix this.

voltrevo · 2022-12-02T01:51:24Z

aggregator/src/app/BundleTable.ts

  eligibleAfter: { type: DataType.VarChar },
  nextEligibilityDelay: { type: DataType.VarChar },
 };

 function fromRawRow(rawRow: RawRow): Row {
  const parseResult = parseBundleDto(JSON.parse(rawRow.bundle));
-
+  console.log('blake..', parseResult);


debug log left here

# Conflicts: # contracts/clients/src/Aggregator.ts

jacque006

👍 from me. once @voltrevo is satisfied

blakecduncan added 2 commits November 3, 2022 16:56

Log bundle error and add script to test

dca0eb3

Spacing

dc4b421

blakecduncan marked this pull request as draft November 4, 2022 12:13

github-actions bot added the aggregator Aggregator backend related label Nov 4, 2022

Code clean up

63f230c

blakecduncan commented Nov 10, 2022

View reviewed changes

aggregator/src/app/EthereumService.ts Outdated

errorArgBytesString,

)[0]; // decoded bytes is a string of the action index that errored.

console.error(errorString)

This comment was marked as outdated.

Sign in to view

Fix test failure

e613af7

jacque006 reviewed Nov 10, 2022

View reviewed changes

aggregator/src/app/EthereumService.ts Outdated Show resolved Hide resolved

kautukkundan mentioned this pull request Nov 16, 2022

Bls provider/signer send transaction #402

Merged

2 tasks

blakecduncan added 4 commits November 17, 2022 13:36

Merge branch 'main' into bundleErrorLogging

ebbe072

Add 202 bundle receipt status and add submitError to db

ec3c05d

Fix lint and test errors

c163638

Export decodeError from the client module

44fec67

github-actions bot added the clients label Nov 22, 2022

Revert "Export decodeError from the client module"

bd0ff0d

This reverts commit 44fec67.

github-actions bot removed the clients label Nov 22, 2022

Some more clean up

0855462

blakecduncan commented Nov 22, 2022

View reviewed changes

Standardise the response body

b62d170

blakecduncan marked this pull request as ready for review November 22, 2022 16:48

blakecduncan requested review from jacque006, voltrevo, JohnGuilding and kautukkundan November 22, 2022 16:48

blakecduncan self-assigned this Nov 22, 2022

jacque006 reviewed Nov 22, 2022

View reviewed changes

PR comments

51ee33f

blakecduncan mentioned this pull request Nov 23, 2022

Add status column to bundles table in the aggregator DB #408

Closed

blakecduncan mentioned this pull request Nov 23, 2022

Update demo applications and Quill to accept new response from bundleReceipt api call #409

Closed

jacque006 mentioned this pull request Nov 23, 2022

Add tests to decode error results #366

Merged

5 tasks

blakecduncan added 3 commits November 28, 2022 12:58

Add in unknown error reason

5286429

Add aggregationStrategy test to check error message

d476556

Update bls-wallet-clients to experimental version and update some cli…

69a1a19

…ent types

github-actions bot added aggregator-proxy Aggregator proxy related clients labels Nov 29, 2022

blakecduncan added 2 commits November 30, 2022 10:00

Merge branch 'main' into bundleErrorLogging

035891a

Fix BundleReceipt type and update the extension

56f33ec

github-actions bot added the extension Browser extension related label Nov 30, 2022

blakecduncan added 3 commits November 30, 2022 13:16

Fixing type issue in the extension

916ba72

Idk how I'm missing these lint errors

e243212

Fix aggregator proxy build

270abf9

voltrevo suggested changes Dec 1, 2022

View reviewed changes

blakecduncan added 2 commits December 1, 2022 12:21

Cleaning up empty string vs null inconsistency

8f182bc

Update client bundle receipt response type

efa2e06

voltrevo reviewed Dec 2, 2022

View reviewed changes

blakecduncan added 2 commits December 2, 2022 10:58

Removing 202 error code

231bf59

Merge branch 'main' into bundleErrorLogging

ab1209a

# Conflicts: # contracts/clients/src/Aggregator.ts

blakecduncan requested review from voltrevo and jacque006 December 2, 2022 17:02

jacque006 approved these changes Dec 2, 2022

View reviewed changes

voltrevo approved these changes Dec 5, 2022

View reviewed changes

voltrevo merged commit 1cf7159 into main Dec 5, 2022

voltrevo deleted the bundleErrorLogging branch December 5, 2022 02:05

		@@ -0,0 +1,50 @@
		#!/usr/bin/env -S deno run --allow-net --allow-env --allow-read --allow-write --unstable


		return { success, fee };
		return { success, fee, errorReason };

Bundle error logging #377

Bundle error logging #377

Uh oh!

Conversation

Uh oh!

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!