Add tests to decode error results #366

jacque006 · 2022-10-27T02:56:15Z

What is this PR doing?

~~Add getOperationResults client function to more easily parse operation results & errors.~~ Done in earlier PR
~~Add typemoq to clients to aid in mocking for unit tests.~~ Removed
Add test coverage for OperationResults.ts logic.
Add nyc test coverage instrumentation to identify gaps in our client tests.

How can these changes be manually tested?

cd ./contracts/clients && yarn test

Does this PR resolve or contribute to any issues?

Resolves #365

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)

TODO

Add coverage for remaining new code branches
Add coverage for decodeError code branches
~~Enforce client test coverage https://www.npmjs.com/package/nyc#coverage-thresholds~~ Will be slightly more involved than originally expected, broke off into Enforce client test coverage #436

contracts/clients/package.json

blakecduncan

lgtm 👍

voltrevo · 2022-11-01T00:06:30Z

contracts/clients/src/OperationResults.ts

+  if (!transactionReceipt.events || !transactionReceipt.events.length) {
+    throw new Error(
+      `no events found in transaction ${transactionReceipt.transactionHash}`,
+    );


What's the idea behind throwing an error instead of just returning an empty array in these cases? Isn't that a valid result?

It was mainly to indicate to a user they may have passed in the wrong transaction. But I could likely do a better check on the method called and throw an error on that instead and return empty from here.

Checking the VG contract, it looks like you can technically submit a completely empty bundle, but I'm sure it would revert when it attempts to validate the empty sig.

Thinking through this some more, we likely can't assume someone only will call VG.processBundle as they may go through a BLSExpander function instead. I will remove this check. I hope someone doesn't get too frustrated if they pass the wrong txn receipt in...

voltrevo · 2022-11-01T00:20:03Z

contracts/clients/test/OpertionResults.test.ts

+
+      expect(() => getOperationResults(txnReceiptMock.object)).to.throw(
+        `no events found in transaction ${hash}`,
+      );


I'm concerned that TypeMoq is unnecessarily complex for our use case.

Why not just write:

const txnReceiptMoq = { transactionHash: hash, events: undefined, } as ContractReceipt;

I was trying to avoid as casts when possible as the underlying object technically fails proper typing. typemoq does add a bit of complexity but it will fully stub out the object, and could be useful in the future for testing more complex OO cases.

I will try without typemoq.

but it will fully stub out the object

That doesn't seem possible... on this line:

const txnReceiptMock = TypeMoq.Mock.ofType<ContractReceipt>()

ContractReceipt only exists in the type space, so the actual object produced here must not have any information about ContractReceipt.

The stub defaults to undefined return values. There also is a behavior where you can make it throw if you don't define something. But I think at this point is is overkill so I just removed.

voltrevo · 2022-11-01T00:21:44Z

contracts/clients/test/OpertionResults.test.ts

+
+      expect(() => getOperationResults(txnReceiptMock.object)).to.throw(
+        `no WalletOperationProcessed events found in transaction ${hash}`,
+      );


Similarly:

const txnReceiptMock = { transactionHash: hash, events: [ { event: "Other" }, ], } as ContractReceipt;

voltrevo · 2022-11-01T00:26:03Z

Also OpertionResults.test.ts -> OperationResults.test.ts (note the 'a')

jacque006 · 2022-11-10T22:17:28Z

Should this be updated with #391 ? Should we wait until contract-updates hits main?

voltrevo · 2022-11-11T04:40:00Z

@jacque006 I'd say it's easier to wait until contract-updates is merged in.

I'm a bit confused about how this branch adds OperationResults.ts but that file also exists in contract-updates. Is there some backstory here?

jacque006 · 2022-11-14T18:00:16Z

@voltrevo I just noticed that as well, forgot to update here. I cherry-picked the core code because I needed the action error decoding for testing the contracts, so I brought over the func.

We can use this branch/PR after contract-updates merge to address your comments and add tests.

jacque006 · 2022-11-23T16:37:24Z

Wait on #377 before updating

jacque006 · 2022-12-15T23:19:44Z

https://github.com/web3well/bls-wallet/actions/runs/3708751514/jobs/6286645375

JohnGuilding

Lgtm, left a few questions and minor comments. Also wondering whether this logic needed a test? https://github.com/web3well/bls-wallet/blob/e1e985a82cb36983e47ff8a7dfdf4d39cb2a2357/contracts/clients/src/OperationResults.ts#L175-L178

contracts/clients/test/OperationResults.test.ts

contracts/clients/src/OperationResults.ts

contracts/clients/test/OperationResults.test.ts

…ion errors. Change existing contract test case to use getOperationResults. Add typemoq to allow typed mocking. Bump clients patch version.

Remove Typemoq Fix encoding error for test Remove number of events check in getOperationResults Remove other event test case

Add html output to nyc test coverage to more easily find gaps.

Remove unnecessary unknown casts. Split test error encoding functions into one for strings and one for abi encoded messages. Remove magic numbers/slicing to extract error action data and properly generate instead.

.github/workflows/clients.yml

jacque006 · 2023-01-04T20:51:44Z

Ready for re-review

JohnGuilding

Looks good!

JohnGuilding · 2023-01-05T11:02:46Z

Lgtm, left a few questions and minor comments. Also wondering whether this logic needed a test?

https://github.com/web3well/bls-wallet/blob/e1e985a82cb36983e47ff8a7dfdf4d39cb2a2357/contracts/clients/src/OperationResults.ts#L175-L178

Just to clarify @jacque006, this doesn't need a test?

jacque006 · 2023-01-05T18:04:15Z

Just to clarify @jacque006, this doesn't need a test?

Missed this, kind of. This 'invariant' is actually run when the file is loaded into the JS runtime: https://github.com/web3well/bls-wallet/blob/9f4064eef9de499b4d959b6462180b5bd885d5f3/contracts/clients/src/OperationResults.ts#L6-L13
And you can see here those 3 calls being run in coverage file:

This means that if those asserts failed, it would probably just crash the test suite/code loading this in, which is what I believe @voltrevo wanted.

The one thing we are not testing here is what happens when the assert fails. Normally in coverage detection a branch statement like:

if (somethingIsWrong) {
    throw new Error("you done goofed");
}

Would require you to run the function this is in twice, testing the true/false paths for this statement to get full branch coverage. However, since that selector check is an assert, the branching actually happens in https://github.com/web3well/bls-wallet/blob/main/contracts/clients/src/helpers/assert.ts . So from a testing coverage standpoint you would test the two paths in assert and things are fine as is.

We could export calculateAndCheckSelector and directly test it to make sure it throws when there is the mismatch, but I don't think there is a ton of value in that ATM. So going to leave as is.

jacque006 · 2023-01-05T18:07:45Z

Going to merge w/o @voltrevo 's approval, believe all their comments are addressed. Can follow up with additional work if other issues need to be addressed.

github-actions bot added clients contracts Smart contract related documentation Improvements or additions to documentation labels Oct 27, 2022

jacque006 force-pushed the feature/decode-errors-from-action-results branch from 95f5598 to f7d4d1a Compare October 27, 2022 03:01

jacque006 changed the title ~~Feature/decode errors from action results~~ Decode error results from operations in transaction receipt Oct 27, 2022

jacque006 force-pushed the feature/decode-errors-from-action-results branch from f7d4d1a to 26eec4c Compare October 27, 2022 03:12

jacque006 marked this pull request as ready for review October 27, 2022 03:14

jacque006 mentioned this pull request Oct 27, 2022

Audit fixes 1-4 and BLSWalletWrapper changes #350

Merged

2 tasks

jacque006 commented Oct 27, 2022

View reviewed changes

contracts/clients/package.json Outdated Show resolved Hide resolved

blakecduncan approved these changes Oct 28, 2022

View reviewed changes

voltrevo suggested changes Nov 1, 2022

View reviewed changes

jacque006 mentioned this pull request Nov 10, 2022

Bundle error logging #377

Merged

2 tasks

github-actions bot removed documentation Improvements or additions to documentation contracts Smart contract related labels Nov 30, 2022

jacque006 changed the title ~~Decode error results from operations in transaction receipt~~ Add tests to decode error results Dec 1, 2022

jacque006 force-pushed the feature/decode-errors-from-action-results branch 3 times, most recently from 8d4e968 to 24e70a2 Compare December 15, 2022 21:47

JohnGuilding reviewed Dec 23, 2022

View reviewed changes

jacque006 added 5 commits January 4, 2023 12:39

Add getOperationResults to clients to make it easier to decode operat…

f66fa37

…ion errors. Change existing contract test case to use getOperationResults. Add typemoq to allow typed mocking. Bump clients patch version.

Address PR feedback

9c552dc

Remove Typemoq Fix encoding error for test Remove number of events check in getOperationResults Remove other event test case

Add nyc test coverage tool

1b95731

Finish test coverage of OperationResults

ee5f044

Add html output to nyc test coverage to more easily find gaps.

Address additional PR feedback

2f4d91e

Remove unnecessary unknown casts. Split test error encoding functions into one for strings and one for abi encoded messages. Remove magic numbers/slicing to extract error action data and properly generate instead.

jacque006 force-pushed the feature/decode-errors-from-action-results branch from bcaa37a to 2f4d91e Compare January 4, 2023 20:20

This was referenced Jan 4, 2023

Enforce client test coverage #436

Closed

Bug - Intermittent recovery test failure - should recover before bls key update #434

Closed

Split client unit & intergration test CI runs

9f4064e

github-actions bot added the automation CI/CD related label Jan 4, 2023

jacque006 commented Jan 4, 2023

View reviewed changes

.github/workflows/clients.yml Show resolved Hide resolved

JohnGuilding approved these changes Jan 5, 2023

View reviewed changes

jacque006 merged commit 852b676 into main Jan 5, 2023

jacque006 deleted the feature/decode-errors-from-action-results branch January 5, 2023 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests to decode error results #366

Add tests to decode error results #366

jacque006 commented Oct 27, 2022 •

edited

Loading

blakecduncan left a comment

voltrevo Nov 1, 2022

jacque006 Nov 7, 2022

jacque006 Dec 1, 2022 •

edited

Loading

voltrevo Nov 1, 2022

jacque006 Nov 7, 2022

voltrevo Nov 10, 2022 •

edited

Loading

jacque006 Dec 1, 2022

voltrevo Nov 1, 2022

jacque006 Dec 1, 2022

voltrevo commented Nov 1, 2022

jacque006 commented Nov 10, 2022

voltrevo commented Nov 11, 2022 •

edited

Loading

jacque006 commented Nov 14, 2022

jacque006 commented Nov 23, 2022

jacque006 commented Dec 15, 2022

JohnGuilding left a comment

jacque006 commented Jan 4, 2023

JohnGuilding left a comment

JohnGuilding commented Jan 5, 2023

jacque006 commented Jan 5, 2023 •

edited

Loading

jacque006 commented Jan 5, 2023

Add tests to decode error results #366

Add tests to decode error results #366

Conversation

jacque006 commented Oct 27, 2022 • edited Loading

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

TODO

blakecduncan left a comment

Choose a reason for hiding this comment

voltrevo Nov 1, 2022

Choose a reason for hiding this comment

jacque006 Nov 7, 2022

Choose a reason for hiding this comment

jacque006 Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

voltrevo Nov 1, 2022

Choose a reason for hiding this comment

jacque006 Nov 7, 2022

Choose a reason for hiding this comment

voltrevo Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

jacque006 Dec 1, 2022

Choose a reason for hiding this comment

voltrevo Nov 1, 2022

Choose a reason for hiding this comment

jacque006 Dec 1, 2022

Choose a reason for hiding this comment

voltrevo commented Nov 1, 2022

jacque006 commented Nov 10, 2022

voltrevo commented Nov 11, 2022 • edited Loading

jacque006 commented Nov 14, 2022

jacque006 commented Nov 23, 2022

jacque006 commented Dec 15, 2022

JohnGuilding left a comment

Choose a reason for hiding this comment

jacque006 commented Jan 4, 2023

JohnGuilding left a comment

Choose a reason for hiding this comment

JohnGuilding commented Jan 5, 2023

jacque006 commented Jan 5, 2023 • edited Loading

jacque006 commented Jan 5, 2023

jacque006 commented Oct 27, 2022 •

edited

Loading

jacque006 Dec 1, 2022 •

edited

Loading

voltrevo Nov 10, 2022 •

edited

Loading

voltrevo commented Nov 11, 2022 •

edited

Loading

jacque006 commented Jan 5, 2023 •

edited

Loading