Add multi-action send transaction method to bls signer #540

JohnGuilding · 2023-02-21T15:00:30Z

What is this PR doing?

Adds multi-action transactions to bls provider and bls signer.

New methods were added as to not break the ethers provider base-class function signatures:

async sendTransactionBatch(transactionBatch: TransactionBatch): Promise<TransactionBatchResponse> {}

async constructTransactionBatchResponse(actions: Array<ActionData>, hash: string, from: string, nonce?: BigNumber): Promise<TransactionBatchResponse> {}

async signTransactionBatch(transactionBatch: TransactionBatch): Promise<string> {}

async sendTransactionBatch(signedTransactionBatch: string): Promise<TransactionBatchResponse> {}

multi-action standards

TLDR ended up going for the wallet_batchTransactions standard. It was the simplest option and we can easily build upon it as future requirements come up.
TLDR: Gut feeling is EIP 5792 is the option to go for. But after reading the discussion on eth magicians, it feels like there is some pushback which makes me think it won't get adopted in it's current state. e.g. disagreements about standardisation of bundle identifiers and disagreement about level of 'atomicity'.

New types:

export type TransactionBatch = {
  transactions: Array<Transaction>;
  batchOptions?: BatchOptions;
};

These were based on this draft batch transaction standard - wallet_batchTransactions JSON-RPC method. The idea was to ensure our code abides by a spec if one exists. There is no formal standard and it was a choice between the wallet_batchTransactions draft and EIP 5792 (draft). While the draft EIP is further along in it's journey towards becoming a standard (by this, I mean it's an actual draft EIP with discussion, as opposed to an informal draft), it's had some pushback in it's eth magicians thread. I've gone for the wallet_batchTransactions option for simplicity.

I've also created a new response type, this isn't based on any of the above standards, but it was easy to implement.

interface TransactionBatchResponse {
  transactions: Array<ethers.providers.TransactionResponse>;
  awaitBatchReceipt: (
    confirmations?: number,
  ) => Promise<ethers.providers.TransactionReceipt>;
}

Further notes and other changes

Preliminary decision with signer.sendTransactionBatch was to only add transactions to a single operation. This means that all transactions in the batch will be atomic. Another option may be to give the developer a choice on whether to add transactions to different operations, and thus permit non-atomic transaction flows. Non-atomic tx flows are out of scope for this work.
provider.sendTransactionBatch operates on an already signed bundle, so could theoretically receive multiple operations. I've added some code to support this in case of that scenario:

    const actionData: Array<ActionData> = bundle.operations
      .map((operation) => operation.actions)
      .flat();

Abstracted common fee estimate code into helper functions:
- Provider. _addFeePaymentActionForFeeEstimation
- Provider. _addFeePaymentActionWithSafeFee
- addSafetyDivisorToFee

How can these changes be manually tested?

run integration tests in workflow or locally

Does this PR resolve or contribute to any issues?

Resolves #375

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-02-24T13:52:17Z

contracts/clients/src/BlsSigner.ts

+ */
+export type TransactionBatch = {
+  transactions: Array<Transaction>;
+  batchOptions?: BatchOptions;


Not really utilising this optional property. Tbh slightly confused by what it's purpose is from the draft standard. My interpretation is that these values help smart contract wallets deal with figuring out batch transaction options from multiple transactions, but not sure if that's correct

I think using this spec we need to accept them, but some of them we will be unable to pass forward to the agg. gas can be used with latest contract changes in contract-updates.

Done. Added a helper function (_validateBatchOptions) that validates the batch options if they are passed as an argument. It checks the chain id is correct and ensures the nonce is the correct type.

jacque006

Overall, think this is a great start on batch txn handling for provider (nice tests btw). Also sets us up well to add additional batch specs. Let's sync offline on how to handle some of the batch options.

To start, I think we can assume that all actions (batched txns) are atomic. We can add more logic later to split actions across multiple operations later.

jacque006 · 2023-02-26T23:21:39Z

contracts/clients/src/BlsSigner.ts

+export type Transaction = {
+  to?: string;
+  value?: BigNumberish;
+  gas?: BigNumberish;
+  maxPriorityFeePerGas?: BigNumberish;
+  maxFeePerGas?: BigNumberish;
+  data?: BytesLike;
+  nonce?: BigNumberish;
+  chainId?: number;
+  accessList?: Array<AccessListish>;
+};


You should be able to import this from ethers.js

jacque006 · 2023-02-26T23:23:33Z

contracts/clients/src/BlsSigner.ts

+ */
+export type TransactionBatch = {
+  transactions: Array<Transaction>;
+  batchOptions?: BatchOptions;


I think using this spec we need to accept them, but some of them we will be unable to pass forward to the agg. gas can be used with latest contract changes in contract-updates.

jacque006 · 2023-02-26T23:24:59Z

contracts/clients/src/BlsSigner.ts

+    const actions: Array<ActionData> = transactionBatch.transactions.map(
+      (transaction) => {
+        if (!transaction.to) {
+          throw new TypeError(
+            "Transaction.to should be defined for all transactions",


Suggested change

const actions: Array<ActionData> = transactionBatch.transactions.map(

(transaction) => {

if (!transaction.to) {

throw new TypeError(

"Transaction.to should be defined for all transactions",

const actions: Array<ActionData> = transactionBatch.transactions.map(

(transaction, i) => {

if (!transaction.to) {

throw new TypeError(

`Transaction.to missing on txn {i}`,

jacque006 · 2023-02-26T23:25:54Z

contracts/clients/src/BlsSigner.ts

+      (transaction) => {
+        if (!transaction.to) {
+          throw new TypeError(
+            "Transaction.to should be defined for all transactions",


JohnGuilding · 2023-03-03T11:30:52Z

contracts/test-integration/BlsSigner.test.ts

+    // Assert
+    expect(result).to.deep.equal(batchOptions);
+    expect(result.nonce).to.equal(BigNumber.from("39"));
+    expect(result.nonce).to.have.property("add");


expect(result.nonce).to.equal(BigNumber.from("39")); can result in false positives, so these 2 extra assertions test "BigNumber.from" worked.
(e.g. before these extra assertions, the test will still pass if "BigNumber.from" is removed in the method which is incorrect behaviour)

JohnGuilding · 2023-03-03T11:35:53Z

contracts/test-integration/BlsProviderContractInteraction.test.ts

@@ -161,6 +161,9 @@ describe("Provider tests", function () {
        .transfer(recipient, amountToTransfer);
      await tx.wait();

+      // wait 1 second to ensure listener count updates
+      await new Promise((resolve) => setTimeout(resolve, 1000));


When running the whole test suite locally, the expect(blsProvider.listenerCount(filter)).to.equal(0); assertion would periodically fail as the event listener was still subscribed. Seems a bit hacky for testing a function that is supposed to be synchronous, but the timer appears to fix that issue.

jacque006

👍 We we decide to promote this for general usage, we should make sure to document the batch function & spec so people are aware they can use that in addition to the normal provider/signer APIs

github-actions bot added clients contracts Smart contract related labels Feb 21, 2023

JohnGuilding force-pushed the 375-add-multi-action-transactions-to-bls-provider branch from 9bcab53 to 982b417 Compare February 23, 2023 14:40

JohnGuilding commented Feb 24, 2023

View reviewed changes

JohnGuilding marked this pull request as ready for review February 24, 2023 13:53

JohnGuilding requested a review from jacque006 February 24, 2023 13:53

JohnGuilding marked this pull request as draft February 24, 2023 17:32

jacque006 reviewed Feb 26, 2023

View reviewed changes

github-actions bot added the automation CI/CD related label Mar 1, 2023

JohnGuilding force-pushed the 375-add-multi-action-transactions-to-bls-provider branch from 157790b to b257437 Compare March 1, 2023 17:02

github-actions bot removed the automation CI/CD related label Mar 3, 2023

JohnGuilding force-pushed the 375-add-multi-action-transactions-to-bls-provider branch from 0f15fad to edd67d1 Compare March 3, 2023 11:27

JohnGuilding commented Mar 3, 2023

View reviewed changes

JohnGuilding marked this pull request as ready for review March 3, 2023 11:38

JohnGuilding added 12 commits March 14, 2023 14:04

Add multi-action send transaction method to bls signer

0c65c78

Add multi-action send transaction method to bls provider

bcc1637

Move bls signer test to integration tests

c1c518e

Add contract interaction tests for provider batched transactions

3a8446f

Move common fee estimate logic to helper functions

e86db6f

Use mint instead of safeMint to fix failing test

b4e6be0

Unskip batched tx test

3567167

Adding missing transaction batch signing test case

24a8718

Include txn index in errors when "to" has not been defined

94d6bc8

Fix intermittent signature mismatch

cd8065b

Handle batch options

f6770cf

Fix intermittent timing issue with test

ee17357

JohnGuilding force-pushed the 375-add-multi-action-transactions-to-bls-provider branch from edd67d1 to ee17357 Compare March 14, 2023 14:08

jacque006 approved these changes Mar 14, 2023

View reviewed changes

jacque006 merged commit b203072 into main Mar 14, 2023

jacque006 deleted the 375-add-multi-action-transactions-to-bls-provider branch March 14, 2023 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi-action send transaction method to bls signer #540

Add multi-action send transaction method to bls signer #540

JohnGuilding commented Feb 21, 2023 •

edited by jacque006

Loading

JohnGuilding Feb 24, 2023 •

edited

Loading

jacque006 Feb 26, 2023 •

edited

Loading

JohnGuilding Mar 3, 2023

jacque006 left a comment

jacque006 Feb 26, 2023

jacque006 Feb 26, 2023 •

edited

Loading

jacque006 Feb 26, 2023

jacque006 Feb 26, 2023

JohnGuilding Mar 3, 2023 •

edited

Loading

JohnGuilding Mar 3, 2023

jacque006 left a comment

Add multi-action send transaction method to bls signer #540

Add multi-action send transaction method to bls signer #540

Conversation

JohnGuilding commented Feb 21, 2023 • edited by jacque006 Loading

What is this PR doing?

multi-action standards

Further notes and other changes

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

JohnGuilding Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

jacque006 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

JohnGuilding Mar 3, 2023

Choose a reason for hiding this comment

jacque006 left a comment

Choose a reason for hiding this comment

jacque006 Feb 26, 2023

Choose a reason for hiding this comment

jacque006 Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

jacque006 Feb 26, 2023

Choose a reason for hiding this comment

jacque006 Feb 26, 2023

Choose a reason for hiding this comment

JohnGuilding Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

JohnGuilding Mar 3, 2023

Choose a reason for hiding this comment

jacque006 left a comment

Choose a reason for hiding this comment

JohnGuilding commented Feb 21, 2023 •

edited by jacque006

Loading

JohnGuilding Feb 24, 2023 •

edited

Loading

jacque006 Feb 26, 2023 •

edited

Loading

jacque006 Feb 26, 2023 •

edited

Loading

JohnGuilding Mar 3, 2023 •

edited

Loading