Update bundle receipt to match ethers transaction response #480

JohnGuilding · 2023-01-23T15:26:16Z

What is this PR doing?

Updates the bundle receipt type returned from the aggregator to closer match an ethers response type.
Update test in BundleServiceSubmitting.test.ts to ensure receiptFromBundle works and returns the bundle hash.
Update types in the client module (Aggregator.ts)
Update the provider _getTransactionReceipt method to return the updated type.
Update provider tests.
Skip signer test that was causing issues.

How can these changes be manually tested?

Hit lookupReceipt endpoint and observe updated bundle receipt
Send a local transaction with Quill to ensure the changes have not broken how Quill retrieves transaction receipts.

Does this PR resolve or contribute to any issues?

#435

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)

JohnGuilding · 2023-01-23T15:50:36Z

contracts/test-integration/BlsProvider.test.ts

    );

    // Assert
-    // TODO: bls-wallet #412 Update values returned in bundle receipt to more closely match ethers transaction response
+    const expectedBlockNumber = await blsProvider.getBlockNumber();
    expect(transactionReceipt).to.be.an("object").that.deep.includes({


These assertions are more verbose but feel like the tradeoff is worth it unless anyone has suggestions on making them more concise.

If someone edits the return value in the BundleService, either by removing a property or changing property types, the test will fail. Note the test does not fail if someone adds an invalid property to the bundle receipt see comment

JohnGuilding · 2023-01-23T15:52:40Z

contracts/clients/src/Aggregator.ts

+/**
+ * The bundle receipt returned from a BLS Wallet Aggregator instance. It is a combination of an ethers {@link ContractReceipt} and a {@link BlsWallet} type.
+ */
+export type BundleReceipt = ContractReceipt & {


Been thinking about validation in this file. I tried adding a property to the return type for receiptFromBundle - invalidProperty: "invalid property”, and it was successfully propagated to the provider. When we’re using our own aggregator, we have confidence the bundleReceipt will have certain properties, and as mentioned in this comment, our integration tests will pick up certain changes to the returned bundleReceipt. But what if a third party developer is using the client module/provider and decides to use their own aggregator. In this case, invalid responses could reach client module code.

My question is, should we validate incoming responses so that we don’t allow invalid data into the client module? Maybe a future issue?

I think that is probably a good future issue. Just my opinion but I think validating the contract receipt right now might be a little scope creep. This is probably something we can handle if people start using other aggregators that don't follow our pattern here.

JohnGuilding · 2023-01-23T15:55:30Z

contracts/clients/src/Aggregator.ts

+/**
+ * The BLS Wallet specific values in a {@link BundleReceipt}.
+ */
+export type BlsWallet = {


Can't decide if I like this name, any suggestions welcome.

Few random ideas:
BlsWalletReceipt
BlsTransactionReceipt
BlsBundleReceipt
BundleTransactionsReceipt
Bundles
BlsBundles

Yeah I agree, I think it would be good to have the word receipt in the name. Maybe something like BlsBundleReceipt sounds good 👍

BlsBundleReceipt sounds good to me, will update

blakecduncan

Nice, this looks good!

I think this is the process that is typically used for client updates. But the only additional thing I would do is push up an experimental version of the client library. That way we can update the client version in the aggregator and the extension and make sure everything is working together. Then I think in a future PR if everything is working as expected we can create a new official release.

blakecduncan · 2023-01-23T16:30:35Z

contracts/test-integration/BlsSignerContractInteraction.test.ts

@@ -217,17 +217,17 @@ describe("Signer contract interaction tests", function () {
    });

    // TODO: Investigate why safeMint() fails with a BLS wallet address. Note - it passes in isolation
-    it("safeMint() call fails with BLS wallet address", async function () {
+    it.skip("safeMint() call fails with BLS wallet address", async function () {


Do we still want this skipped?

It seems to be passing in isolation but then failing when run with other tests. Thought it would be better to skip it in case it causes issues like some other integration tests have, then implement a fix for it. I can add a follow up issue to look into it so it's tracked.

Let me know if you have any additional thoughts

Follow up #484

Nope, that sounds good! I just wanted to make sure it wasn't an accidental add.

blakecduncan · 2023-01-23T16:32:17Z

contracts/clients/src/Aggregator.ts

+/**
+ * The bundle receipt returned from a BLS Wallet Aggregator instance. It is a combination of an ethers {@link ContractReceipt} and a {@link BlsWallet} type.
+ */
+export type BundleReceipt = ContractReceipt & {


Will this updated type break anything in the extension?

Tested the extension locally and is able to get the transaction receipt successfully. The transaction hashes lined up with what was logged in the aggregator logs.

Thinking about it, I might check it works on testnet as well as locally - then I can check the transaction hash works on etherscan

Just tested successfully via quill running a local aggregator pointing at arbitrum goerli.

The extension does not query the hash via bundleReceipt.bundleHash, so it still works (if it did query the hash this way, it would break).

It appears to save the bundle hash here so it doesn't have to query it.

It also uses the returned bundle hash here to get the transaction receipt.

Can't see anywhere else it could break to my knowledge.

… to proxy agg

blakecduncan

thanks for the updates 👍

github-actions bot added aggregator Aggregator backend related clients contracts Smart contract related documentation Improvements or additions to documentation labels Jan 23, 2023

JohnGuilding marked this pull request as ready for review January 23, 2023 15:47

JohnGuilding commented Jan 23, 2023

View reviewed changes

blakecduncan suggested changes Jan 23, 2023

View reviewed changes

JohnGuilding mentioned this pull request Jan 24, 2023

Signer - investigate why safeMint() fails with a BLS wallet address #484

Closed

JohnGuilding added 3 commits January 24, 2023 15:51

Update bundle receipt in aggregator

a92a394

Update bundle receipt in client and provider

2fb5cef

Update todo

6dbee9f

JohnGuilding force-pushed the update-bundle-receipt-to-match-ethers-transaction-response branch from 05f8972 to 6dbee9f Compare January 24, 2023 15:51

Add experimental client module version

e214c6e

github-actions bot added the extension Browser extension related label Jan 24, 2023

Update repo to same ethers version and add experimental client module…

fee9d20

… to proxy agg

github-actions bot added the aggregator-proxy Aggregator proxy related label Jan 24, 2023

blakecduncan approved these changes Jan 25, 2023

View reviewed changes

JohnGuilding added 3 commits January 26, 2023 09:50

Rename BlsWallet type tp BlsBundleReceipt

4f9ddac

Update experimental release

d39c16b

Remove extra level of nesting in BundleReceipt

fe9eacd

JohnGuilding merged commit 016c1e5 into main Jan 26, 2023

JohnGuilding deleted the update-bundle-receipt-to-match-ethers-transaction-response branch January 26, 2023 13:14

jacque006 mentioned this pull request Jun 29, 2023

Update values returned in bundle receipt to more closely match ethers transaction response #435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update bundle receipt to match ethers transaction response #480

Update bundle receipt to match ethers transaction response #480

JohnGuilding commented Jan 23, 2023 •

edited

Loading

JohnGuilding Jan 23, 2023 •

edited

Loading

JohnGuilding Jan 23, 2023 •

edited

Loading

blakecduncan Jan 25, 2023

JohnGuilding Jan 23, 2023

blakecduncan Jan 25, 2023

JohnGuilding Jan 25, 2023

blakecduncan left a comment

blakecduncan Jan 23, 2023

JohnGuilding Jan 24, 2023

JohnGuilding Jan 24, 2023

blakecduncan Jan 25, 2023

blakecduncan Jan 23, 2023

JohnGuilding Jan 24, 2023

JohnGuilding Jan 25, 2023

blakecduncan left a comment

Update bundle receipt to match ethers transaction response #480

Update bundle receipt to match ethers transaction response #480

Conversation

JohnGuilding commented Jan 23, 2023 • 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

JohnGuilding Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

JohnGuilding Jan 23, 2023 • edited Loading

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

blakecduncan 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

blakecduncan left a comment

Choose a reason for hiding this comment

JohnGuilding commented Jan 23, 2023 •

edited

Loading

JohnGuilding Jan 23, 2023 •

edited

Loading

JohnGuilding Jan 23, 2023 •

edited

Loading