From 0c65c78d7e5b1a7472ef00bc5405387772ebd701 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 21 Feb 2023 14:53:27 +0000 Subject: [PATCH 01/12] Add multi-action send transaction method to bls signer --- contracts/clients/src/BlsSigner.ts | 200 ++++++++++++++++++- contracts/test-integration/BlsSigner.test.ts | 165 +++++++++++++++ 2 files changed, 361 insertions(+), 4 deletions(-) diff --git a/contracts/clients/src/BlsSigner.ts b/contracts/clients/src/BlsSigner.ts index 6a8ae051..9c48b598 100644 --- a/contracts/clients/src/BlsSigner.ts +++ b/contracts/clients/src/BlsSigner.ts @@ -1,6 +1,13 @@ /* eslint-disable camelcase */ -import { ethers, BigNumber, Signer, Bytes } from "ethers"; -import { Deferrable, hexlify, isBytes, RLP } from "ethers/lib/utils"; +import { ethers, BigNumber, Signer, Bytes, BigNumberish } from "ethers"; +import { + AccessListish, + BytesLike, + Deferrable, + hexlify, + isBytes, + RLP, +} from "ethers/lib/utils"; import { AggregatorUtilities__factory } from "../typechain-types"; import BlsProvider from "./BlsProvider"; @@ -9,6 +16,62 @@ import { ActionData, bundleToDto } from "./signer"; export const _constructorGuard = {}; +/** + * @param to - 20 Bytes The address the transaction is directed to. Undefined for creation transactions + * @param value - the value sent with this transaction + * @param gas - transaction gas limit + * @param maxPriorityFeePerGas - miner tip aka priority fee + * @param maxFeePerGas - the maximum total fee per gas the sender is willing to pay (includes the network/base fee and miner/priority fee) in wei + * @param data - the hash of the invoked method signature and encoded parameters + * @param nonce - integer of a nonce. This allows overwriting your own pending transactions that use the same nonce + * @param chainId - chain ID that this transaction is valid on + * @param accessList - EIP-2930 access list + */ +export type Transaction = { + to?: string; + value?: BigNumberish; + gas?: BigNumberish; + maxPriorityFeePerGas?: BigNumberish; + maxFeePerGas?: BigNumberish; + data?: BytesLike; + nonce?: BigNumberish; + chainId?: number; + accessList?: Array; +}; + +/** + * @param gas - transaction gas limit + * @param maxPriorityFeePerGas - miner tip aka priority fee + * @param maxFeePerGas - the maximum total fee per gas the sender is willing to pay (includes the network/base fee and miner/priority fee) in wei + * @param nonce - integer of a nonce. This allows overwriting your own pending transactions that use the same nonce + * @param chainId - chain ID that this transaction is valid on + * @param accessList - EIP-2930 access list + */ +export type BatchOptions = { + gas?: BigNumberish; + maxPriorityFeePerGas: BigNumberish; + maxFeePerGas: BigNumberish; + nonce: BigNumberish; + chainId: number; + accessList?: Array; +}; + +/** + * @param transactions - an array of transaction objects + * @param batchOptions - optional batch options taken into account by SC wallets + */ +export type TransactionBatch = { + transactions: Array; + batchOptions?: BatchOptions; +}; + +export interface TransactionBatchResponse { + transactions: Array; + awaitBatch: ( + confirmations?: number, + ) => Promise; +} + export default class BlsSigner extends Signer { override readonly provider: BlsProvider; readonly verificationGatewayAddress!: string; @@ -76,7 +139,6 @@ export default class BlsSigner extends Signer { this.provider, ); - // TODO: bls-wallet #375 Add multi-action transactions to BlsProvider & BlsSigner const action: ActionData = { ethValue: transaction.value?.toString() ?? "0", contractAddress: transaction.to.toString(), @@ -118,6 +180,93 @@ export default class BlsSigner extends Signer { ); } + async sendTransactionBatch( + transactionBatch: TransactionBatch, + ): Promise { + await this.initPromise; + + const actions: Array = transactionBatch.transactions.map( + (transaction) => { + if (!transaction.to) { + throw new TypeError( + "Transaction.to should be defined for all transactions", + ); + } + + return { + ethValue: transaction.value?.toString() ?? "0", + contractAddress: transaction.to!.toString(), + encodedFunction: transaction.data?.toString() ?? "0x", + }; + }, + ); + + const nonce = await BlsWalletWrapper.Nonce( + this.wallet.PublicKey(), + this.verificationGatewayAddress, + this.provider, + ); + + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + this.aggregatorUtilitiesAddress, + this.provider, + ); + + const feeEstimate = await this.provider.aggregator.estimateFee( + this.wallet.sign({ + nonce, + actions: [ + ...actions, + { + ethValue: 1, + // Provide 1 wei with this action so that the fee transfer to + // tx.origin can be included in the gas estimate. + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }), + ); + + // Due to small fluctuations is gas estimation, we add a little safety premium + // to the fee to increase the chance that it actually gets accepted during + // aggregation. + const safetyDivisor = 5; + const feeRequired = BigNumber.from(feeEstimate.feeRequired); + const safetyPremium = feeRequired.div(safetyDivisor); + const safeFee = feeRequired.add(safetyPremium); + + const bundle = this.wallet.sign({ + nonce, + actions: [ + ...actions, + { + ethValue: safeFee, + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }); + const result = await this.provider.aggregator.add(bundle); + + if ("failures" in result) { + throw new Error(JSON.stringify(result.failures)); + } + + return this.constructTransactionBatchResponse( + actions, + result.hash, + this.wallet.address, + nonce, + ); + } + async getAddress(): Promise { await this.initPromise; if (this._address) { @@ -149,7 +298,7 @@ export default class BlsSigner extends Signer { hash, to: action.contractAddress, from, - nonce: BigNumber.from(nonce).toNumber(), + nonce: nonce.toNumber(), gasLimit: BigNumber.from("0x0"), data: action.encodedFunction.toString(), value: BigNumber.from(action.ethValue), @@ -162,6 +311,49 @@ export default class BlsSigner extends Signer { }; } + async constructTransactionBatchResponse( + actions: Array, + hash: string, + from: string, + nonce?: BigNumber, + ): Promise { + await this.initPromise; + const chainId = await this.getChainId(); + if (!nonce) { + nonce = await BlsWalletWrapper.Nonce( + this.wallet.PublicKey(), + this.verificationGatewayAddress, + this.provider, + ); + } + + const transactions: Array = + actions.map((action) => { + return { + hash, + to: action.contractAddress, + from, + nonce: nonce!.toNumber(), + gasLimit: BigNumber.from("0x0"), + data: action.encodedFunction.toString(), + value: BigNumber.from(action.ethValue), + chainId, + type: 2, + confirmations: 1, + wait: (confirmations?: number) => { + return this.provider.waitForTransaction(hash, confirmations); + }, + }; + }); + + return { + transactions, + awaitBatch: (confirmations?: number) => { + return this.provider.waitForTransaction(hash, confirmations); + }, + }; + } + /** * This method passes calls through to the underlying node and allows users to unlock EOA accounts through this provider. * The personal namespace is used to manage keys for ECDSA signing. BLS keys are not supported natively by execution clients. diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 520c62fb..3a47939d 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -160,6 +160,171 @@ describe("BlsSigner", () => { ); }); + it("should send a batch of ETH transfers (empty calls) successfully", async () => { + // Arrange + const expectedAmount = parseEther("1"); + const recipients = []; + const transactionBatch = []; + + for (let i = 0; i < 3; i++) { + recipients.push(ethers.Wallet.createRandom().address); + transactionBatch.push({ + to: recipients[i], + value: expectedAmount, + }); + } + + // Act + const transaction = await blsSigner.sendTransactionBatch({ + transactions: transactionBatch, + }); + await transaction.awaitBatch(); + + // Assert + expect(await blsProvider.getBalance(recipients[0])).to.equal( + expectedAmount, + ); + expect(await blsProvider.getBalance(recipients[1])).to.equal( + expectedAmount, + ); + expect(await blsProvider.getBalance(recipients[2])).to.equal( + expectedAmount, + ); + }); + + it("should throw an error sending a transaction when 'transaction.to' has not been defined", async () => { + // Arrange + const transactionBatch = new Array(3).fill({ + ...{ value: parseEther("1") }, + }); + + // Act + const result = async () => + await blsSigner.sendTransactionBatch({ + transactions: transactionBatch, + }); + + // Assert + await expect(result()).to.be.rejectedWith( + TypeError, + "Transaction.to should be defined", + ); + }); + + it("should throw an error when sending an invalid transaction batch", async () => { + // Arrange + const invalidTransactionBatch = new Array(3).fill({ + ...{ + to: ethers.Wallet.createRandom().address, + value: parseEther("-1"), + }, + }); + + // Act + const result = async () => + await blsSigner.sendTransactionBatch({ + transactions: invalidTransactionBatch, + }); + + // Assert + await expect(result()).to.be.rejectedWith( + Error, + 'invalid BigNumber value (argument="value", value=undefined, code=INVALID_ARGUMENT, version=bignumber/5.7.0)', + ); + }); + + it("should return a transaction batch response when sending a transaction batch", async () => { + // Arrange + const expectedAmount = parseEther("1"); + const recipients = []; + const transactionBatch = []; + const expectedNonce = await BlsWalletWrapper.Nonce( + blsSigner.wallet.PublicKey(), + blsSigner.verificationGatewayAddress, + blsProvider, + ); + + for (let i = 0; i < 3; i++) { + recipients.push(ethers.Wallet.createRandom().address); + transactionBatch.push({ + to: recipients[i], + value: expectedAmount, + }); + } + + // Act + const transactionBatchResponse = await blsSigner.sendTransactionBatch({ + transactions: transactionBatch, + }); + + // Assert + // tx 1 + expect(transactionBatchResponse.transactions[0]) + .to.be.an("object") + .that.includes({ + hash: transactionBatchResponse.transactions[0].hash, + to: recipients[0], + from: blsSigner.wallet.address, + data: "0x", + chainId: 1337, + type: 2, + confirmations: 1, + }); + expect(transactionBatchResponse.transactions[0].nonce).to.equal( + expectedNonce, + ); + expect(transactionBatchResponse.transactions[0].gasLimit).to.equal( + BigNumber.from("0x0"), + ); + expect(transactionBatchResponse.transactions[0].value).to.equal( + BigNumber.from(expectedAmount), + ); + + // tx 2 + expect(transactionBatchResponse.transactions[1]) + .to.be.an("object") + .that.includes({ + hash: transactionBatchResponse.transactions[1].hash, + to: recipients[1], + from: blsSigner.wallet.address, + data: "0x", + chainId: 1337, + type: 2, + confirmations: 1, + }); + expect(transactionBatchResponse.transactions[1].nonce).to.equal( + expectedNonce, + ); + expect(transactionBatchResponse.transactions[1].gasLimit).to.equal( + BigNumber.from("0x0"), + ); + expect(transactionBatchResponse.transactions[1].value).to.equal( + BigNumber.from(expectedAmount), + ); + + // tx 3 + expect(transactionBatchResponse.transactions[2]) + .to.be.an("object") + .that.includes({ + hash: transactionBatchResponse.transactions[2].hash, + to: recipients[2], + from: blsSigner.wallet.address, + data: "0x", + chainId: 1337, + type: 2, + confirmations: 1, + }); + expect(transactionBatchResponse.transactions[2].nonce).to.equal( + expectedNonce, + ); + expect(transactionBatchResponse.transactions[2].gasLimit).to.equal( + BigNumber.from("0x0"), + ); + expect(transactionBatchResponse.transactions[2].value).to.equal( + BigNumber.from(expectedAmount), + ); + }); + it("should throw an error when invalid private key is supplied", async () => { // Arrange const newBlsProvider = new Experimental.BlsProvider( From bcc16373f9d95569c5b13ce2a2e1d73ca316b9cb Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 22 Feb 2023 12:58:07 +0000 Subject: [PATCH 02/12] Add multi-action send transaction method to bls provider --- contracts/clients/src/BlsProvider.ts | 53 +++- contracts/clients/src/BlsSigner.ts | 77 ++++++ contracts/clients/test/BlsProvider.test.ts | 29 ++ .../test-integration/BlsProvider.test.ts | 247 +++++++++++++++++- contracts/test-integration/BlsSigner.test.ts | 136 +++++++++- 5 files changed, 523 insertions(+), 19 deletions(-) diff --git a/contracts/clients/src/BlsProvider.ts b/contracts/clients/src/BlsProvider.ts index 148cfb32..e1d08714 100644 --- a/contracts/clients/src/BlsProvider.ts +++ b/contracts/clients/src/BlsProvider.ts @@ -4,7 +4,11 @@ import { Deferrable } from "ethers/lib/utils"; import { ActionData, Bundle } from "./signer/types"; import Aggregator, { BundleReceipt } from "./Aggregator"; -import BlsSigner, { UncheckedBlsSigner, _constructorGuard } from "./BlsSigner"; +import BlsSigner, { + TransactionBatchResponse, + UncheckedBlsSigner, + _constructorGuard, +} from "./BlsSigner"; import poll from "./helpers/poll"; import BlsWalletWrapper from "./BlsWalletWrapper"; import { @@ -101,20 +105,24 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider { } const resolvedTransaction = await signedTransaction; - const userBundle: Bundle = JSON.parse(resolvedTransaction); + const bundle: Bundle = JSON.parse(resolvedTransaction); - const result = await this.aggregator.add(userBundle); + if (bundle.operations.length > 1) { + throw new Error( + "Can only operate on single operations. Call provider.sendTransactionBatch instead", + ); + } + + const result = await this.aggregator.add(bundle); if ("failures" in result) { throw new Error(JSON.stringify(result.failures)); } - // TODO: bls-wallet #375 Add multi-action transactions to BlsProvider & BlsSigner - // We're assuming the first operation and action constitute the correct values. We will need to refactor this when we add multi-action transactions const actionData: ActionData = { - ethValue: userBundle.operations[0].actions[0].ethValue, - contractAddress: userBundle.operations[0].actions[0].contractAddress, - encodedFunction: userBundle.operations[0].actions[0].encodedFunction, + ethValue: bundle.operations[0].actions[0].ethValue, + contractAddress: bundle.operations[0].actions[0].contractAddress, + encodedFunction: bundle.operations[0].actions[0].encodedFunction, }; return this.signer.constructTransactionResponse( @@ -124,6 +132,35 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider { ); } + async sendTransactionBatch( + signedTransactionBatch: string, + ): Promise { + // TODO: bls-wallet #413 Move references to private key outside of BlsSigner. + // Without doing this, we would have to call `const signer = this.getSigner(privateKey)`. + // We do not want to pass the private key to this method. + if (!this.signer) { + throw new Error("Call provider.getSigner first"); + } + + const bundle: Bundle = JSON.parse(signedTransactionBatch); + + const result = await this.aggregator.add(bundle); + + if ("failures" in result) { + throw new Error(JSON.stringify(result.failures)); + } + + const actionData: Array = bundle.operations + .map((operation) => operation.actions) + .flat(); + + return this.signer.constructTransactionBatchResponse( + actionData, + result.hash, + this.signer.wallet.address, + ); + } + override getSigner( privateKey: string, addressOrIndex?: string | number, diff --git a/contracts/clients/src/BlsSigner.ts b/contracts/clients/src/BlsSigner.ts index 9c48b598..81ac3f51 100644 --- a/contracts/clients/src/BlsSigner.ts +++ b/contracts/clients/src/BlsSigner.ts @@ -416,6 +416,83 @@ export default class BlsSigner extends Signer { return JSON.stringify(bundleToDto(bundle)); } + async signTransactionBatch( + transactionBatch: TransactionBatch, + ): Promise { + await this.initPromise; + + const actions: Array = transactionBatch.transactions.map( + (transaction) => { + if (!transaction.to) { + throw new TypeError( + "Transaction.to should be defined for all transactions", + ); + } + + return { + ethValue: transaction.value?.toString() ?? "0", + contractAddress: transaction.to!.toString(), + encodedFunction: transaction.data?.toString() ?? "0x", + }; + }, + ); + + const nonce = await BlsWalletWrapper.Nonce( + this.wallet.PublicKey(), + this.verificationGatewayAddress, + this.provider, + ); + + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + this.aggregatorUtilitiesAddress, + this.provider, + ); + + const feeEstimate = await this.provider.aggregator.estimateFee( + this.wallet.sign({ + nonce, + actions: [ + ...actions, + { + ethValue: 1, + // Provide 1 wei with this action so that the fee transfer to + // tx.origin can be included in the gas estimate. + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }), + ); + + // Due to small fluctuations is gas estimation, we add a little safety premium + // to the fee to increase the chance that it actually gets accepted during + // aggregation. + const safetyDivisor = 5; + const feeRequired = BigNumber.from(feeEstimate.feeRequired); + const safetyPremium = feeRequired.div(safetyDivisor); + const safeFee = feeRequired.add(safetyPremium); + + const bundle = this.wallet.sign({ + nonce, + actions: [ + ...actions, + { + ethValue: safeFee, + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }); + + return JSON.stringify(bundleToDto(bundle)); + } + /** Sign a message */ // TODO: Come back to this once we support EIP-1271 override async signMessage(message: Bytes | string): Promise { diff --git a/contracts/clients/test/BlsProvider.test.ts b/contracts/clients/test/BlsProvider.test.ts index 5b1cea67..bfe1924f 100644 --- a/contracts/clients/test/BlsProvider.test.ts +++ b/contracts/clients/test/BlsProvider.test.ts @@ -149,6 +149,35 @@ describe("BlsProvider", () => { ); }); + it("should throw an error sending a transaction batch when this.signer is not defined", async () => { + // Arrange + const newBlsProvider = new Experimental.BlsProvider( + aggregatorUrl, + verificationGateway, + aggregatorUtilities, + rpcUrl, + network, + ); + const signedTransaction = await blsSigner.signTransactionBatch({ + transactions: [ + { + value: parseEther("1"), + to: ethers.Wallet.createRandom().address, + }, + ], + }); + + // Act + const result = async () => + await newBlsProvider.sendTransactionBatch(signedTransaction); + + // Assert + await expect(result()).to.be.rejectedWith( + Error, + "Call provider.getSigner first", + ); + }); + it("should return the connection info for the provider", () => { // Arrange const expectedConnection = regularProvider.connection; diff --git a/contracts/test-integration/BlsProvider.test.ts b/contracts/test-integration/BlsProvider.test.ts index 0d0d8123..844231c6 100644 --- a/contracts/test-integration/BlsProvider.test.ts +++ b/contracts/test-integration/BlsProvider.test.ts @@ -4,7 +4,9 @@ import { BigNumber, ethers } from "ethers"; import { formatEther, parseEther } from "ethers/lib/utils"; import { + AggregatorUtilities__factory, BlsWalletWrapper, + bundleToDto, Experimental, MockERC20__factory, NetworkConfig, @@ -99,7 +101,7 @@ describe("BlsProvider", () => { await expect(gasEstimate()).to.not.be.rejected; }); - it("should send ETH (empty call) given a valid bundle successfully", async () => { + it("should send ETH (empty call) given a valid bundle", async () => { // Arrange const recipient = ethers.Wallet.createRandom().address; const expectedBalance = parseEther("1"); @@ -125,6 +127,77 @@ describe("BlsProvider", () => { ).to.equal(expectedBalance); }); + it("should throw an error when sending multiple signed operations to sendTransaction", async () => { + // Arrange + const expectedAmount = parseEther("1"); + const verySafeFee = parseEther("0.1"); + const firstRecipient = ethers.Wallet.createRandom().address; + const secondRecipient = ethers.Wallet.createRandom().address; + + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + aggregatorUtilities, + blsProvider, + ); + + const firstOperation = { + nonce: await blsSigner.wallet.Nonce(), + actions: [ + { + ethValue: expectedAmount, + contractAddress: firstRecipient, + encodedFunction: "0x", + }, + { + ethValue: verySafeFee, + contractAddress: blsProvider.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }; + + const secondOperation = { + nonce: (await blsSigner.wallet.Nonce()).add(1), + actions: [ + { + ethValue: expectedAmount, + contractAddress: secondRecipient, + encodedFunction: "0x", + }, + { + ethValue: verySafeFee, + contractAddress: blsProvider.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }; + + const firstBundle = blsSigner.wallet.sign(firstOperation); + const secondBundle = blsSigner.wallet.sign(secondOperation); + + const aggregatedBundle = blsSigner.wallet.blsWalletSigner.aggregate([ + firstBundle, + secondBundle, + ]); + + // Act + const result = async () => + await blsProvider.sendTransaction( + JSON.stringify(bundleToDto(aggregatedBundle)), + ); + + // Assert + await expect(result()).to.rejectedWith( + Error, + "Can only operate on single operations. Call provider.sendTransactionBatch instead", + ); + }); + it("should get the account nonce when the signer constructs the transaction response", async () => { // Arrange const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); @@ -147,6 +220,7 @@ describe("BlsProvider", () => { // Once when calling "signer.signTransaction", once when calling "blsProvider.estimateGas", and once when calling "blsSigner.constructTransactionResponse". // This unit test is concerned with the latter being called. expect(spy).to.have.been.called.exactly(3); + chai.spy.restore(spy); }); it("should throw an error when sending a modified signed transaction", async () => { @@ -173,19 +247,188 @@ describe("BlsProvider", () => { ); }); - it("should throw an error when sending an invalid signed transaction", async () => { + it("should throw an error when sending invalid signed transactions", async () => { // Arrange const invalidTransaction = "Invalid signed transaction"; // Act const result = async () => await blsProvider.sendTransaction(invalidTransaction); + const batchResult = async () => + await blsProvider.sendTransaction(invalidTransaction); // Assert await expect(result()).to.be.rejectedWith( Error, "Unexpected token I in JSON at position 0", ); + await expect(batchResult()).to.be.rejectedWith( + Error, + "Unexpected token I in JSON at position 0", + ); + }); + + it("should send a batch of ETH transfers (empty calls) given a valid bundle", async () => { + // Arrange + const expectedAmount = parseEther("1"); + const recipients = []; + const unsignedTransactionBatch = []; + + for (let i = 0; i < 3; i++) { + recipients.push(ethers.Wallet.createRandom().address); + unsignedTransactionBatch.push({ + to: recipients[i], + value: expectedAmount, + }); + } + + const signedTransactionBatch = await blsSigner.signTransactionBatch({ + transactions: unsignedTransactionBatch, + }); + + // Act + const result = await blsProvider.sendTransactionBatch( + signedTransactionBatch, + ); + await result.awaitBatch(); + + // Assert + expect(await blsProvider.getBalance(recipients[0])).to.equal( + expectedAmount, + ); + expect(await blsProvider.getBalance(recipients[1])).to.equal( + expectedAmount, + ); + expect(await blsProvider.getBalance(recipients[2])).to.equal( + expectedAmount, + ); + }); + + it("should send a batch of ETH transfers (empty calls) given two aggregated bundles", async () => { + // Arrange + const expectedAmount = parseEther("1"); + const verySafeFee = parseEther("0.1"); + const firstRecipient = ethers.Wallet.createRandom().address; + const secondRecipient = ethers.Wallet.createRandom().address; + + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + aggregatorUtilities, + blsProvider, + ); + + const firstOperation = { + nonce: await blsSigner.wallet.Nonce(), + actions: [ + { + ethValue: expectedAmount, + contractAddress: firstRecipient, + encodedFunction: "0x", + }, + { + ethValue: verySafeFee, + contractAddress: blsProvider.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }; + + const secondOperation = { + nonce: (await blsSigner.wallet.Nonce()).add(1), + actions: [ + { + ethValue: expectedAmount, + contractAddress: secondRecipient, + encodedFunction: "0x", + }, + { + ethValue: verySafeFee, + contractAddress: blsProvider.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }; + + const firstBundle = blsSigner.wallet.sign(firstOperation); + const secondBundle = blsSigner.wallet.sign(secondOperation); + + const aggregatedBundle = blsSigner.wallet.blsWalletSigner.aggregate([ + firstBundle, + secondBundle, + ]); + + // Act + const result = await blsProvider.sendTransactionBatch( + JSON.stringify(bundleToDto(aggregatedBundle)), + ); + await result.awaitBatch(); + + // Assert + expect(await blsProvider.getBalance(firstRecipient)).to.equal( + expectedAmount, + ); + expect(await blsProvider.getBalance(secondRecipient)).to.equal( + expectedAmount, + ); + }); + + it("should get the account nonce when the signer constructs the transaction batch response", async () => { + // Arrange + const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); + const recipient = ethers.Wallet.createRandom().address; + const expectedBalance = parseEther("1"); + + const unsignedTransaction = { + value: expectedBalance.toString(), + to: recipient, + data: "0x", + }; + const signedTransaction = await blsSigner.signTransactionBatch({ + transactions: [unsignedTransaction], + }); + + // Act + await blsProvider.sendTransactionBatch(signedTransaction); + + // Assert + // Once when calling "signer.signTransaction", and once when calling "blsSigner.constructTransactionResponse". + // This unit test is concerned with the latter being called. + expect(spy).to.have.been.called.exactly(2); + chai.spy.restore(spy); + }); + + it("should throw an error when sending a modified signed transaction", async () => { + // Arrange + const address = await blsSigner.getAddress(); + + const signedTransaction = await blsSigner.signTransactionBatch({ + transactions: [ + { + value: parseEther("1"), + to: ethers.Wallet.createRandom().address, + data: "0x", + }, + ], + }); + + const userBundle = JSON.parse(signedTransaction); + userBundle.operations[0].actions[0].ethValue = parseEther("2"); + const invalidBundle = JSON.stringify(userBundle); + + // Act + const result = async () => + await blsProvider.sendTransactionBatch(invalidBundle); + + // Assert + await expect(result()).to.be.rejectedWith( + Error, + `[{"type":"invalid-signature","description":"invalid signature for wallet address ${address}"}]`, + ); }); it("should throw an error when the transaction receipt cannot be found", async () => { diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 3a47939d..ffc47bb8 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -18,6 +18,7 @@ import { // eslint-disable-next-line camelcase MockERC20__factory, AggregatorUtilities__factory, + Operation, } from "../clients/src"; import getNetworkConfig from "../shared/helpers/getNetworkConfig"; @@ -404,13 +405,14 @@ describe("BlsSigner", () => { to: recipient, data: "0x", }; - const action: ActionData = { + + // get expected signature + const expectedAction: ActionData = { ethValue: parseEther("1"), contractAddress: recipient, encodedFunction: "0x", }; - // get expected signature const wallet = await BlsWalletWrapper.connect( privateKey, verificationGateway, @@ -418,25 +420,25 @@ describe("BlsSigner", () => { ); const walletAddress = wallet.address; - const nonce = await BlsWalletWrapper.Nonce( + const expectedNonce = await BlsWalletWrapper.Nonce( wallet.PublicKey(), verificationGateway, blsSigner, ); - const feeEstimate = await blsProvider.estimateGas(transaction); + const expectedFeeEstimate = await blsProvider.estimateGas(transaction); const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( blsProvider.aggregatorUtilitiesAddress, blsProvider, ); - const operation = { - nonce, + const expectedOperation = { + nonce: expectedNonce, actions: [ - action, + expectedAction, { - ethValue: feeEstimate, + ethValue: expectedFeeEstimate, contractAddress: blsProvider.aggregatorUtilitiesAddress, encodedFunction: aggregatorUtilitiesContract.interface.encodeFunctionData( @@ -447,7 +449,7 @@ describe("BlsSigner", () => { }; const expectedBundle = wallet.blsWalletSigner.sign( - operation, + expectedOperation, walletAddress, ); @@ -479,6 +481,120 @@ describe("BlsSigner", () => { ); }); + it("should sign a transaction batch to create a bundleDto and serialize the result", async () => { + // Arrange + const expectedAmount = parseEther("1"); + + const recipients = []; + const transactions = []; + const expectedActions = []; + for (let i = 0; i < 3; i++) { + recipients.push(ethers.Wallet.createRandom().address); + transactions.push({ + to: recipients[i], + value: expectedAmount, + data: "0x", + }); + expectedActions.push({ + contractAddress: recipients[i], + ethValue: expectedAmount, + encodedFunction: "0x", + }); + } + + // get expected signature + const wallet = await BlsWalletWrapper.connect( + privateKey, + verificationGateway, + blsProvider, + ); + const walletAddress = wallet.address; + + const expectedNonce = await BlsWalletWrapper.Nonce( + wallet.PublicKey(), + verificationGateway, + blsSigner, + ); + + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + aggregatorUtilities, + blsProvider, + ); + + const expectedFeeEstimate = await blsProvider.aggregator.estimateFee( + blsSigner.wallet.sign({ + nonce: expectedNonce, + actions: [ + ...expectedActions, + { + ethValue: 1, + contractAddress: aggregatorUtilities, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }), + ); + + const safetyDivisor = 5; + const feeRequired = BigNumber.from(expectedFeeEstimate.feeRequired); + const safetyPremium = feeRequired.div(safetyDivisor); + const safeFee = feeRequired.add(safetyPremium); + + const expectedOperation: Operation = { + nonce: expectedNonce, + actions: [ + ...expectedActions, + { + ethValue: safeFee, + contractAddress: blsProvider.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ], + }; + + const expectedBundle = wallet.blsWalletSigner.sign( + expectedOperation, + walletAddress, + ); + + // Act + const signedTransaction = await blsSigner.signTransactionBatch({ + transactions, + }); + + // Assert + const bundleDto = JSON.parse(signedTransaction); + expect(bundleDto.signature).to.deep.equal(expectedBundle.signature); + }); + + it("should throw an error when signing an invalid transaction batch", async () => { + // Arrange + const invalidEthValue = parseEther("-1"); + + const unsignedTransaction = { + value: invalidEthValue, + to: ethers.Wallet.createRandom().address, + }; + + // Act + const result = async () => + await blsSigner.signTransactionBatch({ + transactions: [unsignedTransaction], + }); + + // Assert + await expect(result()).to.be.rejectedWith( + Error, + 'invalid BigNumber value (argument="value", value=undefined, code=INVALID_ARGUMENT, version=bignumber/5.7.0)', + ); + }); + it("should check transaction", async () => { // Arrange const recipient = ethers.Wallet.createRandom().address; @@ -785,6 +901,7 @@ describe("BlsSigner", () => { data: testERC20.interface.encodeFunctionData("totalSupply"), from: blsSigner.wallet.address, // Assert that 'from' has been added to the provider call }); + chai.spy.restore(spy); }); it("should estimate gas without throwing an error, with the signer account address being used as the from field.", async () => { @@ -808,6 +925,7 @@ describe("BlsSigner", () => { value: parseEther("1"), from: blsSigner.wallet.address, // Assert that 'from' has been added to the provider call }); + chai.spy.restore(spy); }); // ENS is not supported by hardhat so we are checking the correct error behaviour in this scenario From c1c518e9229906b5fac1ae5094492b1dcd260e25 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 22 Feb 2023 14:45:04 +0000 Subject: [PATCH 03/12] Move bls signer test to integration tests --- contracts/clients/test/BlsProvider.test.ts | 29 -------------------- contracts/test-integration/BlsSigner.test.ts | 29 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/contracts/clients/test/BlsProvider.test.ts b/contracts/clients/test/BlsProvider.test.ts index bfe1924f..5b1cea67 100644 --- a/contracts/clients/test/BlsProvider.test.ts +++ b/contracts/clients/test/BlsProvider.test.ts @@ -149,35 +149,6 @@ describe("BlsProvider", () => { ); }); - it("should throw an error sending a transaction batch when this.signer is not defined", async () => { - // Arrange - const newBlsProvider = new Experimental.BlsProvider( - aggregatorUrl, - verificationGateway, - aggregatorUtilities, - rpcUrl, - network, - ); - const signedTransaction = await blsSigner.signTransactionBatch({ - transactions: [ - { - value: parseEther("1"), - to: ethers.Wallet.createRandom().address, - }, - ], - }); - - // Act - const result = async () => - await newBlsProvider.sendTransactionBatch(signedTransaction); - - // Assert - await expect(result()).to.be.rejectedWith( - Error, - "Call provider.getSigner first", - ); - }); - it("should return the connection info for the provider", () => { // Arrange const expectedConnection = regularProvider.connection; diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index ffc47bb8..e4383050 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -326,6 +326,35 @@ describe("BlsSigner", () => { ); }); + it("should throw an error sending a transaction batch when this.signer is not defined", async () => { + // Arrange + const newBlsProvider = new Experimental.BlsProvider( + aggregatorUrl, + verificationGateway, + aggregatorUtilities, + rpcUrl, + network, + ); + const signedTransaction = await blsSigner.signTransactionBatch({ + transactions: [ + { + value: parseEther("1"), + to: ethers.Wallet.createRandom().address, + }, + ], + }); + + // Act + const result = async () => + await newBlsProvider.sendTransactionBatch(signedTransaction); + + // Assert + await expect(result()).to.be.rejectedWith( + Error, + "Call provider.getSigner first", + ); + }); + it("should throw an error when invalid private key is supplied", async () => { // Arrange const newBlsProvider = new Experimental.BlsProvider( From 3a8446fda170d2db714438d43f6c6a3bca9d63bf Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 23 Feb 2023 11:41:43 +0000 Subject: [PATCH 04/12] Add contract interaction tests for provider batched transactions --- contracts/contracts/mock/MockERC20Spender.sol | 15 +++ contracts/test-integration/BlsSigner.test.ts | 3 +- .../BlsSignerContractInteraction.test.ts | 92 +++++++++++++++++++ 3 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 contracts/contracts/mock/MockERC20Spender.sol diff --git a/contracts/contracts/mock/MockERC20Spender.sol b/contracts/contracts/mock/MockERC20Spender.sol new file mode 100644 index 00000000..fd4b2d66 --- /dev/null +++ b/contracts/contracts/mock/MockERC20Spender.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/token/ERC721/IERC721.sol"; + +contract MockTokenSpender { + function TransferERC20ToSelf(address _token, uint256 _amount) public { + IERC20(_token).transferFrom(msg.sender, address(this), _amount); + } + + function TransferERC721ToSelf(address _token, uint256 _tokenId) public { + IERC721(_token).transferFrom(msg.sender, address(this), _tokenId); + } +} diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index e4383050..3b9e3d8d 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -510,7 +510,8 @@ describe("BlsSigner", () => { ); }); - it("should sign a transaction batch to create a bundleDto and serialize the result", async () => { + // TODO: passing in isolation but failing when run with whole file + it.skip("should sign a transaction batch to create a bundleDto and serialize the result", async () => { // Arrange const expectedAmount = parseEther("1"); diff --git a/contracts/test-integration/BlsSignerContractInteraction.test.ts b/contracts/test-integration/BlsSignerContractInteraction.test.ts index 7c1d7b5d..e8cbd7d1 100644 --- a/contracts/test-integration/BlsSignerContractInteraction.test.ts +++ b/contracts/test-integration/BlsSignerContractInteraction.test.ts @@ -184,6 +184,50 @@ describe("Signer contract interaction tests", function () { expect(newBalance.sub(initialBalance)).to.equal(erc20ToTransfer); }); + it("approve() and transferFrom() calls batched together", async () => { + // Arrange + const MockTokenSpender = await ethers.getContractFactory( + "MockTokenSpender", + ); + const mockTokenSpender = await MockTokenSpender.connect( + fundedWallet, + ).deploy(); + await mockTokenSpender.deployed(); + + const spender = mockTokenSpender.address; + const initialBalance = await mockERC20.balanceOf(spender); + const erc20ToTransfer = utils.parseUnits("33.6"); + + const transactionBatch = { + transactions: [ + { + to: mockERC20.address, + value: 0, + data: mockERC20.interface.encodeFunctionData("approve", [ + spender, + erc20ToTransfer, + ]), + }, + { + to: spender, + value: 0, + data: mockTokenSpender.interface.encodeFunctionData( + "TransferERC20ToSelf", + [mockERC20.address, erc20ToTransfer], + ), + }, + ], + }; + + // Act + const result = await blsSigners[0].sendTransactionBatch(transactionBatch); + await result.awaitBatch(); + + // Assert + const newBalance = await mockERC20.balanceOf(spender); + expect(newBalance.sub(initialBalance)).to.equal(erc20ToTransfer); + }); + it("contract factory connects and reconnects to new signer", async () => { // Truncated as not required for test const mockERC20Bytecode = "0x60806040523480"; @@ -360,5 +404,53 @@ describe("Signer contract interaction tests", function () { // Check signer[1]'s address is now an approved address for the token expect(await mockERC721.getApproved(tokenId)).to.equal(spender); }); + + it("approve() and transferFrom() calls batched together", async () => { + // Arrange + const MockTokenSpender = await ethers.getContractFactory( + "MockTokenSpender", + ); + const mockTokenSpender = await MockTokenSpender.connect( + fundedWallet, + ).deploy(); + await mockTokenSpender.deployed(); + + const owner = await blsSigners[3].getAddress(); + const spender = mockTokenSpender.address; + const tokenId = 7; + + const mint = await mockERC721 + .connect(blsSigners[0]) + .safeMint(owner, tokenId); + await mint.wait(); + + const transactionBatch = { + transactions: [ + { + to: mockERC721.address, + value: 0, + data: mockERC721.interface.encodeFunctionData("approve", [ + spender, + tokenId, + ]), + }, + { + to: mockTokenSpender.address, + value: 0, + data: mockTokenSpender.interface.encodeFunctionData( + "TransferERC721ToSelf", + [mockERC721.address, tokenId], + ), + }, + ], + }; + + // Act + const result = await blsSigners[3].sendTransactionBatch(transactionBatch); + await result.awaitBatch(); + + // Assert + expect(await mockERC721.ownerOf(tokenId)).to.equal(spender); + }); }); }); From e86db6f72e071696b6b2356164514dcc4e183a5c Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 23 Feb 2023 13:09:49 +0000 Subject: [PATCH 05/12] Move common fee estimate logic to helper functions --- contracts/clients/src/BlsProvider.ts | 73 +++++++--- contracts/clients/src/BlsSigner.ts | 133 +++++------------- .../src/helpers/addSafetyDivisorToFee.ts | 18 +++ .../test-integration/BlsProvider.test.ts | 91 +++++------- contracts/test-integration/BlsSigner.test.ts | 63 +++------ 5 files changed, 150 insertions(+), 228 deletions(-) create mode 100644 contracts/clients/src/helpers/addSafetyDivisorToFee.ts diff --git a/contracts/clients/src/BlsProvider.ts b/contracts/clients/src/BlsProvider.ts index e1d08714..2456f95e 100644 --- a/contracts/clients/src/BlsProvider.ts +++ b/contracts/clients/src/BlsProvider.ts @@ -15,6 +15,7 @@ import { AggregatorUtilities__factory, BLSWallet__factory, } from "../typechain-types"; +import addSafetyPremiumToFee from "./helpers/addSafetyDivisorToFee"; export default class BlsProvider extends ethers.providers.JsonRpcProvider { readonly aggregator: Aggregator; @@ -61,37 +62,18 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider { this, ); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - this.aggregatorUtilitiesAddress, - this, - ); + const actionsWithFeePaymentAction = + this._addFeePaymentActionForFeeEstimation([action]); const feeEstimate = await this.aggregator.estimateFee( this.signer.wallet.sign({ nonce, - actions: [ - action, - { - ethValue: 1, - // Provide 1 wei with this action so that the fee transfer to - // tx.origin can be included in the gas estimate. - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithFeePaymentAction], }), ); - // Due to small fluctuations is gas estimation, we add a little safety premium - // to the fee to increase the chance that it actually gets accepted during - // aggregation. - const safetyDivisor = 5; const feeRequired = BigNumber.from(feeEstimate.feeRequired); - const safetyPremium = feeRequired.div(safetyDivisor); - return feeRequired.add(safetyPremium); + return addSafetyPremiumToFee(feeRequired); } override async sendTransaction( @@ -268,4 +250,49 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider { status: bundleReceipt.status, }; } + + _addFeePaymentActionForFeeEstimation( + actions: Array, + ): Array { + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + this.aggregatorUtilitiesAddress, + this, + ); + + return [ + ...actions, + { + // Provide 1 wei with this action so that the fee transfer to + // tx.origin can be included in the gas estimate. + ethValue: 1, + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ]; + } + + _addFeePaymentActionWithSafeFee( + actions: Array, + fee: BigNumber, + ): Array { + const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( + this.aggregatorUtilitiesAddress, + this, + ); + + return [ + ...actions, + { + ethValue: fee, + contractAddress: this.aggregatorUtilitiesAddress, + encodedFunction: + aggregatorUtilitiesContract.interface.encodeFunctionData( + "sendEthToTxOrigin", + ), + }, + ]; + } } diff --git a/contracts/clients/src/BlsSigner.ts b/contracts/clients/src/BlsSigner.ts index 81ac3f51..6268e574 100644 --- a/contracts/clients/src/BlsSigner.ts +++ b/contracts/clients/src/BlsSigner.ts @@ -8,10 +8,10 @@ import { isBytes, RLP, } from "ethers/lib/utils"; -import { AggregatorUtilities__factory } from "../typechain-types"; import BlsProvider from "./BlsProvider"; import BlsWalletWrapper from "./BlsWalletWrapper"; +import addSafetyPremiumToFee from "./helpers/addSafetyDivisorToFee"; import { ActionData, bundleToDto } from "./signer"; export const _constructorGuard = {}; @@ -146,25 +146,14 @@ export default class BlsSigner extends Signer { }; const feeEstimate = await this.provider.estimateGas(transaction); - - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - this.aggregatorUtilitiesAddress, - this.provider, + const actionsWithSafeFee = this.provider._addFeePaymentActionWithSafeFee( + [action], + feeEstimate, ); const bundle = this.wallet.sign({ nonce, - actions: [ - action, - { - ethValue: feeEstimate, - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }); const result = await this.provider.aggregator.add(bundle); @@ -207,51 +196,28 @@ export default class BlsSigner extends Signer { this.provider, ); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - this.aggregatorUtilitiesAddress, - this.provider, - ); + const actionsWithFeePaymentAction = + this.provider._addFeePaymentActionForFeeEstimation(actions); const feeEstimate = await this.provider.aggregator.estimateFee( this.wallet.sign({ nonce, - actions: [ - ...actions, - { - ethValue: 1, - // Provide 1 wei with this action so that the fee transfer to - // tx.origin can be included in the gas estimate. - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithFeePaymentAction], }), ); - // Due to small fluctuations is gas estimation, we add a little safety premium - // to the fee to increase the chance that it actually gets accepted during - // aggregation. - const safetyDivisor = 5; - const feeRequired = BigNumber.from(feeEstimate.feeRequired); - const safetyPremium = feeRequired.div(safetyDivisor); - const safeFee = feeRequired.add(safetyPremium); + const safeFee = addSafetyPremiumToFee( + BigNumber.from(feeEstimate.feeRequired), + ); + + const actionsWithSafeFee = this.provider._addFeePaymentActionWithSafeFee( + actions, + safeFee, + ); const bundle = this.wallet.sign({ nonce, - actions: [ - ...actions, - { - ethValue: safeFee, - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }); const result = await this.provider.aggregator.add(bundle); @@ -393,24 +359,14 @@ export default class BlsSigner extends Signer { const feeEstimate = await this.provider.estimateGas(transaction); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - this.aggregatorUtilitiesAddress, - this.provider, + const actionsWithSafeFee = this.provider._addFeePaymentActionWithSafeFee( + [action], + feeEstimate, ); const bundle = this.wallet.sign({ nonce, - actions: [ - action, - { - ethValue: feeEstimate, - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }); return JSON.stringify(bundleToDto(bundle)); @@ -443,51 +399,28 @@ export default class BlsSigner extends Signer { this.provider, ); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - this.aggregatorUtilitiesAddress, - this.provider, - ); + const actionsWithFeePaymentAction = + this.provider._addFeePaymentActionForFeeEstimation(actions); const feeEstimate = await this.provider.aggregator.estimateFee( this.wallet.sign({ nonce, - actions: [ - ...actions, - { - ethValue: 1, - // Provide 1 wei with this action so that the fee transfer to - // tx.origin can be included in the gas estimate. - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithFeePaymentAction], }), ); - // Due to small fluctuations is gas estimation, we add a little safety premium - // to the fee to increase the chance that it actually gets accepted during - // aggregation. - const safetyDivisor = 5; - const feeRequired = BigNumber.from(feeEstimate.feeRequired); - const safetyPremium = feeRequired.div(safetyDivisor); - const safeFee = feeRequired.add(safetyPremium); + const safeFee = addSafetyPremiumToFee( + BigNumber.from(feeEstimate.feeRequired), + ); + + const actionsWithSafeFee = this.provider._addFeePaymentActionWithSafeFee( + actions, + safeFee, + ); const bundle = this.wallet.sign({ nonce, - actions: [ - ...actions, - { - ethValue: safeFee, - contractAddress: this.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }); return JSON.stringify(bundleToDto(bundle)); diff --git a/contracts/clients/src/helpers/addSafetyDivisorToFee.ts b/contracts/clients/src/helpers/addSafetyDivisorToFee.ts new file mode 100644 index 00000000..d9659354 --- /dev/null +++ b/contracts/clients/src/helpers/addSafetyDivisorToFee.ts @@ -0,0 +1,18 @@ +import { BigNumber } from "ethers"; + +/** + * Used to add a small safety premium to estimated fees. This protects + * against small fluctuations is gas estimation, and thus increases + * the chance that bundles get accepted during aggregation. + * + * @param feeEstimate fee required for bundle + * @param safetyDivisor optional safety divisor. Default is 5 + * @returns fee estimate with added safety premium + */ +export default function addSafetyPremiumToFee( + feeEstimate: BigNumber, + safetyDivisor: number = 5, +): BigNumber { + const safetyPremium = feeEstimate.div(safetyDivisor); + return feeEstimate.add(safetyPremium); +} diff --git a/contracts/test-integration/BlsProvider.test.ts b/contracts/test-integration/BlsProvider.test.ts index 844231c6..4416e51a 100644 --- a/contracts/test-integration/BlsProvider.test.ts +++ b/contracts/test-integration/BlsProvider.test.ts @@ -4,7 +4,6 @@ import { BigNumber, ethers } from "ethers"; import { formatEther, parseEther } from "ethers/lib/utils"; import { - AggregatorUtilities__factory, BlsWalletWrapper, bundleToDto, Experimental, @@ -134,47 +133,34 @@ describe("BlsProvider", () => { const firstRecipient = ethers.Wallet.createRandom().address; const secondRecipient = ethers.Wallet.createRandom().address; - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - aggregatorUtilities, - blsProvider, - ); - - const firstOperation = { - nonce: await blsSigner.wallet.Nonce(), - actions: [ + const firstActionWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + [ { ethValue: expectedAmount, contractAddress: firstRecipient, encodedFunction: "0x", }, - { - ethValue: verySafeFee, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, ], - }; - - const secondOperation = { - nonce: (await blsSigner.wallet.Nonce()).add(1), - actions: [ + verySafeFee, + ); + const secondActionWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + [ { ethValue: expectedAmount, contractAddress: secondRecipient, encodedFunction: "0x", }, - { - ethValue: verySafeFee, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, ], + verySafeFee, + ); + + const firstOperation = { + nonce: await blsSigner.wallet.Nonce(), + actions: [...firstActionWithSafeFee], + }; + const secondOperation = { + nonce: (await blsSigner.wallet.Nonce()).add(1), + actions: [...secondActionWithSafeFee], }; const firstBundle = blsSigner.wallet.sign(firstOperation); @@ -311,47 +297,34 @@ describe("BlsProvider", () => { const firstRecipient = ethers.Wallet.createRandom().address; const secondRecipient = ethers.Wallet.createRandom().address; - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - aggregatorUtilities, - blsProvider, - ); - - const firstOperation = { - nonce: await blsSigner.wallet.Nonce(), - actions: [ + const firstActionWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + [ { ethValue: expectedAmount, contractAddress: firstRecipient, encodedFunction: "0x", }, - { - ethValue: verySafeFee, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, ], - }; - - const secondOperation = { - nonce: (await blsSigner.wallet.Nonce()).add(1), - actions: [ + verySafeFee, + ); + const secondActionWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + [ { ethValue: expectedAmount, contractAddress: secondRecipient, encodedFunction: "0x", }, - { - ethValue: verySafeFee, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, ], + verySafeFee, + ); + + const firstOperation = { + nonce: await blsSigner.wallet.Nonce(), + actions: [...firstActionWithSafeFee], + }; + const secondOperation = { + nonce: (await blsSigner.wallet.Nonce()).add(1), + actions: [...secondActionWithSafeFee], }; const firstBundle = blsSigner.wallet.sign(firstOperation); diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 3b9e3d8d..532a6771 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -15,12 +15,11 @@ import { ActionData, BlsWalletWrapper, NetworkConfig, - // eslint-disable-next-line camelcase MockERC20__factory, - AggregatorUtilities__factory, Operation, } from "../clients/src"; import getNetworkConfig from "../shared/helpers/getNetworkConfig"; +import addSafetyPremiumToFee from "../clients/src/helpers/addSafetyDivisorToFee"; let networkConfig: NetworkConfig; @@ -457,24 +456,14 @@ describe("BlsSigner", () => { const expectedFeeEstimate = await blsProvider.estimateGas(transaction); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - blsProvider.aggregatorUtilitiesAddress, - blsProvider, + const actionsWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + [expectedAction], + expectedFeeEstimate, ); const expectedOperation = { nonce: expectedNonce, - actions: [ - expectedAction, - { - ethValue: expectedFeeEstimate, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }; const expectedBundle = wallet.blsWalletSigner.sign( @@ -546,46 +535,28 @@ describe("BlsSigner", () => { blsSigner, ); - const aggregatorUtilitiesContract = AggregatorUtilities__factory.connect( - aggregatorUtilities, - blsProvider, - ); + const actionsWithFeePaymentAction = + blsProvider._addFeePaymentActionForFeeEstimation(expectedActions); const expectedFeeEstimate = await blsProvider.aggregator.estimateFee( blsSigner.wallet.sign({ nonce: expectedNonce, - actions: [ - ...expectedActions, - { - ethValue: 1, - contractAddress: aggregatorUtilities, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithFeePaymentAction], }), ); - const safetyDivisor = 5; - const feeRequired = BigNumber.from(expectedFeeEstimate.feeRequired); - const safetyPremium = feeRequired.div(safetyDivisor); - const safeFee = feeRequired.add(safetyPremium); + const safeFee = addSafetyPremiumToFee( + BigNumber.from(expectedFeeEstimate.feeRequired), + ); + + const actionsWithSafeFee = blsProvider._addFeePaymentActionWithSafeFee( + expectedActions, + safeFee, + ); const expectedOperation: Operation = { nonce: expectedNonce, - actions: [ - ...expectedActions, - { - ethValue: safeFee, - contractAddress: blsProvider.aggregatorUtilitiesAddress, - encodedFunction: - aggregatorUtilitiesContract.interface.encodeFunctionData( - "sendEthToTxOrigin", - ), - }, - ], + actions: [...actionsWithSafeFee], }; const expectedBundle = wallet.blsWalletSigner.sign( From b4e6be0f98e442284fb9ebe82fb0e7d842db33a4 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Thu, 23 Feb 2023 14:22:25 +0000 Subject: [PATCH 06/12] Use mint instead of safeMint to fix failing test --- .../test-integration/BlsSignerContractInteraction.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/test-integration/BlsSignerContractInteraction.test.ts b/contracts/test-integration/BlsSignerContractInteraction.test.ts index e8cbd7d1..7d70ead4 100644 --- a/contracts/test-integration/BlsSignerContractInteraction.test.ts +++ b/contracts/test-integration/BlsSignerContractInteraction.test.ts @@ -419,9 +419,7 @@ describe("Signer contract interaction tests", function () { const spender = mockTokenSpender.address; const tokenId = 7; - const mint = await mockERC721 - .connect(blsSigners[0]) - .safeMint(owner, tokenId); + const mint = await mockERC721.connect(blsSigners[0]).mint(owner, tokenId); await mint.wait(); const transactionBatch = { From 3567167ccbc66d8e4ccb0dbd4dc122ad207d53d0 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 24 Feb 2023 11:43:52 +0000 Subject: [PATCH 07/12] Unskip batched tx test --- contracts/test-integration/BlsSigner.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 532a6771..4942018d 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -499,8 +499,7 @@ describe("BlsSigner", () => { ); }); - // TODO: passing in isolation but failing when run with whole file - it.skip("should sign a transaction batch to create a bundleDto and serialize the result", async () => { + it("should sign a transaction batch to create a bundleDto and serialize the result", async () => { // Arrange const expectedAmount = parseEther("1"); From 24a871814ef0789009d2e66e86b80d70aae62a5b Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 24 Feb 2023 12:35:31 +0000 Subject: [PATCH 08/12] Adding missing transaction batch signing test case --- contracts/clients/src/BlsProvider.ts | 4 ++-- contracts/test-integration/BlsSigner.test.ts | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/contracts/clients/src/BlsProvider.ts b/contracts/clients/src/BlsProvider.ts index 2456f95e..ca1eb36b 100644 --- a/contracts/clients/src/BlsProvider.ts +++ b/contracts/clients/src/BlsProvider.ts @@ -62,13 +62,13 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider { this, ); - const actionsWithFeePaymentAction = + const actionWithFeePaymentAction = this._addFeePaymentActionForFeeEstimation([action]); const feeEstimate = await this.aggregator.estimateFee( this.signer.wallet.sign({ nonce, - actions: [...actionsWithFeePaymentAction], + actions: [...actionWithFeePaymentAction], }), ); diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 4942018d..cccff1e8 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -192,22 +192,30 @@ describe("BlsSigner", () => { ); }); - it("should throw an error sending a transaction when 'transaction.to' has not been defined", async () => { + it("should throw an error sending & signing a transaction batch when 'transaction.to' has not been defined", async () => { // Arrange const transactionBatch = new Array(3).fill({ ...{ value: parseEther("1") }, }); // Act - const result = async () => + const sendResult = async () => await blsSigner.sendTransactionBatch({ transactions: transactionBatch, }); + const signResult = async () => + await blsSigner.signTransactionBatch({ + transactions: transactionBatch, + }); // Assert - await expect(result()).to.be.rejectedWith( + await expect(sendResult()).to.be.rejectedWith( TypeError, - "Transaction.to should be defined", + "Transaction.to should be defined for all transactions", + ); + await expect(signResult()).to.be.rejectedWith( + TypeError, + "Transaction.to should be defined for all transactions", ); }); From 94d6bc8280ccb5dc4d8c26154a754eb7a44559c6 Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 28 Feb 2023 15:30:31 +0000 Subject: [PATCH 09/12] Include txn index in errors when "to" has not been defined --- contracts/clients/src/BlsSigner.ts | 38 +++----------------- contracts/test-integration/BlsSigner.test.ts | 4 +-- 2 files changed, 7 insertions(+), 35 deletions(-) diff --git a/contracts/clients/src/BlsSigner.ts b/contracts/clients/src/BlsSigner.ts index 6268e574..5a24eb1f 100644 --- a/contracts/clients/src/BlsSigner.ts +++ b/contracts/clients/src/BlsSigner.ts @@ -2,7 +2,6 @@ import { ethers, BigNumber, Signer, Bytes, BigNumberish } from "ethers"; import { AccessListish, - BytesLike, Deferrable, hexlify, isBytes, @@ -16,29 +15,6 @@ import { ActionData, bundleToDto } from "./signer"; export const _constructorGuard = {}; -/** - * @param to - 20 Bytes The address the transaction is directed to. Undefined for creation transactions - * @param value - the value sent with this transaction - * @param gas - transaction gas limit - * @param maxPriorityFeePerGas - miner tip aka priority fee - * @param maxFeePerGas - the maximum total fee per gas the sender is willing to pay (includes the network/base fee and miner/priority fee) in wei - * @param data - the hash of the invoked method signature and encoded parameters - * @param nonce - integer of a nonce. This allows overwriting your own pending transactions that use the same nonce - * @param chainId - chain ID that this transaction is valid on - * @param accessList - EIP-2930 access list - */ -export type Transaction = { - to?: string; - value?: BigNumberish; - gas?: BigNumberish; - maxPriorityFeePerGas?: BigNumberish; - maxFeePerGas?: BigNumberish; - data?: BytesLike; - nonce?: BigNumberish; - chainId?: number; - accessList?: Array; -}; - /** * @param gas - transaction gas limit * @param maxPriorityFeePerGas - miner tip aka priority fee @@ -61,7 +37,7 @@ export type BatchOptions = { * @param batchOptions - optional batch options taken into account by SC wallets */ export type TransactionBatch = { - transactions: Array; + transactions: Array; batchOptions?: BatchOptions; }; @@ -175,11 +151,9 @@ export default class BlsSigner extends Signer { await this.initPromise; const actions: Array = transactionBatch.transactions.map( - (transaction) => { + (transaction, i) => { if (!transaction.to) { - throw new TypeError( - "Transaction.to should be defined for all transactions", - ); + throw new TypeError(`Transaction.to is missing on transaction ${i}`); } return { @@ -378,11 +352,9 @@ export default class BlsSigner extends Signer { await this.initPromise; const actions: Array = transactionBatch.transactions.map( - (transaction) => { + (transaction, i) => { if (!transaction.to) { - throw new TypeError( - "Transaction.to should be defined for all transactions", - ); + throw new TypeError(`Transaction.to is missing on transaction ${i}`); } return { diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index cccff1e8..7553d217 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -211,11 +211,11 @@ describe("BlsSigner", () => { // Assert await expect(sendResult()).to.be.rejectedWith( TypeError, - "Transaction.to should be defined for all transactions", + "Transaction.to is missing on transaction 0", ); await expect(signResult()).to.be.rejectedWith( TypeError, - "Transaction.to should be defined for all transactions", + "Transaction.to is missing on transaction 0", ); }); From cd8065be16b01ae477bd0ba0f45c795ff9cd3dbd Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Tue, 28 Feb 2023 16:12:30 +0000 Subject: [PATCH 10/12] Fix intermittent signature mismatch --- contracts/test-integration/BlsSigner.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index 7553d217..a20b8be8 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -479,12 +479,18 @@ describe("BlsSigner", () => { walletAddress, ); + const expectedBundleSignatureHexStrings = expectedBundle.signature.map( + (keyElement) => BigNumber.from(keyElement).toHexString(), + ); + // Act const signedTransaction = await blsSigner.signTransaction(transaction); // Assert const bundleDto = JSON.parse(signedTransaction); - expect(bundleDto.signature).to.deep.equal(expectedBundle.signature); + expect(bundleDto.signature).to.deep.equal( + expectedBundleSignatureHexStrings, + ); }); it("should throw an error when signing an invalid transaction", async () => { @@ -507,7 +513,7 @@ describe("BlsSigner", () => { ); }); - it("should sign a transaction batch to create a bundleDto and serialize the result", async () => { + it.only("should sign a transaction batch to create a bundleDto and serialize the result", async () => { // Arrange const expectedAmount = parseEther("1"); From f6770cf2d4c1a063d66a04f5412afad34d8e4a8d Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Wed, 1 Mar 2023 13:03:27 +0000 Subject: [PATCH 11/12] Handle batch options --- contracts/clients/src/BlsSigner.ts | 79 ++++++--- .../test-integration/BlsProvider.test.ts | 4 +- contracts/test-integration/BlsSigner.test.ts | 159 +++++++++++++++++- .../BlsSignerContractInteraction.test.ts | 4 +- 4 files changed, 217 insertions(+), 29 deletions(-) diff --git a/contracts/clients/src/BlsSigner.ts b/contracts/clients/src/BlsSigner.ts index 5a24eb1f..7f39e4c7 100644 --- a/contracts/clients/src/BlsSigner.ts +++ b/contracts/clients/src/BlsSigner.ts @@ -16,12 +16,12 @@ import { ActionData, bundleToDto } from "./signer"; export const _constructorGuard = {}; /** - * @param gas - transaction gas limit - * @param maxPriorityFeePerGas - miner tip aka priority fee - * @param maxFeePerGas - the maximum total fee per gas the sender is willing to pay (includes the network/base fee and miner/priority fee) in wei - * @param nonce - integer of a nonce. This allows overwriting your own pending transactions that use the same nonce - * @param chainId - chain ID that this transaction is valid on - * @param accessList - EIP-2930 access list + * @property gas - (THIS PROPERTY IS NOT USED BY BLS WALLET) transaction gas limit + * @property maxPriorityFeePerGas - (THIS PROPERTY IS NOT USED BY BLS WALLET) miner tip aka priority fee + * @property maxFeePerGas - (THIS PROPERTY IS NOT USED BY BLS WALLET) the maximum total fee per gas the sender is willing to pay (includes the network/base fee and miner/priority fee) in wei + * @property nonce - integer of a nonce. This allows overwriting your own pending transactions that use the same nonce + * @property chainId - chain ID that this transaction is valid on + * @property accessList - (THIS PROPERTY IS NOT USED BY BLS WALLET) EIP-2930 access list */ export type BatchOptions = { gas?: BigNumberish; @@ -29,12 +29,12 @@ export type BatchOptions = { maxFeePerGas: BigNumberish; nonce: BigNumberish; chainId: number; - accessList?: Array; + accessList?: AccessListish; }; /** - * @param transactions - an array of transaction objects - * @param batchOptions - optional batch options taken into account by SC wallets + * @property transactions - an array of transaction objects + * @property batchOptions - optional batch options taken into account by smart contract wallets */ export type TransactionBatch = { transactions: Array; @@ -43,7 +43,7 @@ export type TransactionBatch = { export interface TransactionBatchResponse { transactions: Array; - awaitBatch: ( + awaitBatchReceipt: ( confirmations?: number, ) => Promise; } @@ -150,6 +150,21 @@ export default class BlsSigner extends Signer { ): Promise { await this.initPromise; + let nonce: BigNumber; + if (transactionBatch.batchOptions) { + const validatedBatchOptions = await this._validateBatchOptions( + transactionBatch.batchOptions, + ); + + nonce = validatedBatchOptions.nonce as BigNumber; + } else { + nonce = await BlsWalletWrapper.Nonce( + this.wallet.PublicKey(), + this.verificationGatewayAddress, + this.provider, + ); + } + const actions: Array = transactionBatch.transactions.map( (transaction, i) => { if (!transaction.to) { @@ -164,12 +179,6 @@ export default class BlsSigner extends Signer { }, ); - const nonce = await BlsWalletWrapper.Nonce( - this.wallet.PublicKey(), - this.verificationGatewayAddress, - this.provider, - ); - const actionsWithFeePaymentAction = this.provider._addFeePaymentActionForFeeEstimation(actions); @@ -288,7 +297,7 @@ export default class BlsSigner extends Signer { return { transactions, - awaitBatch: (confirmations?: number) => { + awaitBatchReceipt: (confirmations?: number) => { return this.provider.waitForTransaction(hash, confirmations); }, }; @@ -351,6 +360,21 @@ export default class BlsSigner extends Signer { ): Promise { await this.initPromise; + let nonce: BigNumber; + if (transactionBatch.batchOptions) { + const validatedBatchOptions = await this._validateBatchOptions( + transactionBatch.batchOptions, + ); + + nonce = validatedBatchOptions.nonce as BigNumber; + } else { + nonce = await BlsWalletWrapper.Nonce( + this.wallet.PublicKey(), + this.verificationGatewayAddress, + this.provider, + ); + } + const actions: Array = transactionBatch.transactions.map( (transaction, i) => { if (!transaction.to) { @@ -365,12 +389,6 @@ export default class BlsSigner extends Signer { }, ); - const nonce = await BlsWalletWrapper.Nonce( - this.wallet.PublicKey(), - this.verificationGatewayAddress, - this.provider, - ); - const actionsWithFeePaymentAction = this.provider._addFeePaymentActionForFeeEstimation(actions); @@ -445,6 +463,21 @@ export default class BlsSigner extends Signer { async _legacySignMessage(message: Bytes | string): Promise { throw new Error("_legacySignMessage() is not implemented"); } + + async _validateBatchOptions( + batchOptions: BatchOptions, + ): Promise { + const expectedChainId = await this.getChainId(); + + if (batchOptions.chainId !== expectedChainId) { + throw new Error( + `Supplied chain ID ${batchOptions.chainId} does not match the expected chain ID ${expectedChainId}`, + ); + } + + batchOptions.nonce = BigNumber.from(batchOptions.nonce); + return batchOptions; + } } export class UncheckedBlsSigner extends BlsSigner { diff --git a/contracts/test-integration/BlsProvider.test.ts b/contracts/test-integration/BlsProvider.test.ts index 4416e51a..fa61cb32 100644 --- a/contracts/test-integration/BlsProvider.test.ts +++ b/contracts/test-integration/BlsProvider.test.ts @@ -276,7 +276,7 @@ describe("BlsProvider", () => { const result = await blsProvider.sendTransactionBatch( signedTransactionBatch, ); - await result.awaitBatch(); + await result.awaitBatchReceipt(); // Assert expect(await blsProvider.getBalance(recipients[0])).to.equal( @@ -339,7 +339,7 @@ describe("BlsProvider", () => { const result = await blsProvider.sendTransactionBatch( JSON.stringify(bundleToDto(aggregatedBundle)), ); - await result.awaitBatch(); + await result.awaitBatchReceipt(); // Assert expect(await blsProvider.getBalance(firstRecipient)).to.equal( diff --git a/contracts/test-integration/BlsSigner.test.ts b/contracts/test-integration/BlsSigner.test.ts index a20b8be8..d90f9a34 100644 --- a/contracts/test-integration/BlsSigner.test.ts +++ b/contracts/test-integration/BlsSigner.test.ts @@ -178,7 +178,7 @@ describe("BlsSigner", () => { const transaction = await blsSigner.sendTransactionBatch({ transactions: transactionBatch, }); - await transaction.awaitBatch(); + await transaction.awaitBatchReceipt(); // Assert expect(await blsProvider.getBalance(recipients[0])).to.equal( @@ -192,6 +192,67 @@ describe("BlsSigner", () => { ); }); + it("should not retrieve nonce when sending a transaction batch with batch options", async () => { + // Arrange + const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); + + const recipient = ethers.Wallet.createRandom().address; + const expectedAmount = parseEther("1"); + + const transactionBatch = { + transactions: [ + { + to: recipient, + value: expectedAmount, + }, + ], + batchOptions: { + gas: ethers.utils.parseUnits("40000"), + maxPriorityFeePerGas: ethers.utils.parseUnits("0.5", "gwei"), + maxFeePerGas: ethers.utils.parseUnits("23", "gwei"), + nonce: 0, + chainId: 1337, + accessList: [], + }, + }; + + // Act + const transaction = await blsSigner.sendTransactionBatch(transactionBatch); + await transaction.awaitBatchReceipt(); + + // Assert + expect(await blsProvider.getBalance(recipient)).to.equal(expectedAmount); + + // Nonce is supplied with batch options so spy should not be called + expect(spy).to.have.been.called.exactly(0); + chai.spy.restore(spy); + }); + + it("should retrieve nonce when sending a transaction batch without batch options", async () => { + // Arrange + const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); + const recipient = ethers.Wallet.createRandom().address; + const expectedAmount = parseEther("1"); + + // Act + const transaction = await blsSigner.sendTransactionBatch({ + transactions: [ + { + to: recipient, + value: expectedAmount, + }, + ], + }); + await transaction.awaitBatchReceipt(); + + // Assert + expect(await blsProvider.getBalance(recipient)).to.equal(expectedAmount); + + // Nonce is not supplied with batch options so spy should be called once in sendTransactionBatch + expect(spy).to.have.been.called.exactly(1); + chai.spy.restore(spy); + }); + it("should throw an error sending & signing a transaction batch when 'transaction.to' has not been defined", async () => { // Arrange const transactionBatch = new Array(3).fill({ @@ -362,6 +423,50 @@ describe("BlsSigner", () => { ); }); + it("should validate batch options", async () => { + // Arrange + const batchOptions = { + gas: ethers.utils.parseUnits("40000"), + maxPriorityFeePerGas: ethers.utils.parseUnits("0.5", "gwei"), + maxFeePerGas: ethers.utils.parseUnits("23", "gwei"), + nonce: 39, + chainId: 1337, + accessList: [], + }; + + // Act + const result = await blsSigner._validateBatchOptions(batchOptions); + + // 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.not.be.a("number"); + }); + + it("should throw error for wrong chain id when validating batch options", async () => { + // Arrange + const invalidChainId = 123; + const batchOptions = { + gas: BigNumber.from("40000"), + maxPriorityFeePerGas: ethers.utils.parseUnits("0.5", "gwei"), + maxFeePerGas: ethers.utils.parseUnits("23", "gwei"), + nonce: 1, + chainId: invalidChainId, + accessList: [], + }; + + // Act + const result = async () => + await blsSigner._validateBatchOptions(batchOptions); + + // Assert + expect(result()).to.be.rejectedWith( + Error, + `Supplied chain ID ${invalidChainId} does not match the expected chain ID 1337`, + ); + }); + it("should throw an error when invalid private key is supplied", async () => { // Arrange const newBlsProvider = new Experimental.BlsProvider( @@ -513,7 +618,7 @@ describe("BlsSigner", () => { ); }); - it.only("should sign a transaction batch to create a bundleDto and serialize the result", async () => { + it("should sign a transaction batch to create a bundleDto and serialize the result", async () => { // Arrange const expectedAmount = parseEther("1"); @@ -609,6 +714,56 @@ describe("BlsSigner", () => { ); }); + it("should not retrieve nonce when signing a transaction batch with batch options", async () => { + // Arrange + const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); + + const transactionBatch = { + transactions: [ + { + to: ethers.Wallet.createRandom().address, + value: parseEther("1"), + }, + ], + batchOptions: { + gas: ethers.utils.parseUnits("40000"), + maxPriorityFeePerGas: ethers.utils.parseUnits("0.5", "gwei"), + maxFeePerGas: ethers.utils.parseUnits("23", "gwei"), + nonce: 0, + chainId: 1337, + accessList: [], + }, + }; + + // Act + await blsSigner.signTransactionBatch(transactionBatch); + + // Assert + // Nonce is supplied with batch options so spy should not be called + expect(spy).to.have.been.called.exactly(0); + chai.spy.restore(spy); + }); + + it("should retrieve nonce when signing a transaction batch without batch options", async () => { + // Arrange + const spy = chai.spy.on(BlsWalletWrapper, "Nonce"); + + // Act + await blsSigner.signTransactionBatch({ + transactions: [ + { + to: ethers.Wallet.createRandom().address, + value: parseEther("1"), + }, + ], + }); + + // Assert + // Nonce is not supplied with batch options so spy should be called once in sendTransactionBatch + expect(spy).to.have.been.called.exactly(1); + chai.spy.restore(spy); + }); + it("should check transaction", async () => { // Arrange const recipient = ethers.Wallet.createRandom().address; diff --git a/contracts/test-integration/BlsSignerContractInteraction.test.ts b/contracts/test-integration/BlsSignerContractInteraction.test.ts index 7d70ead4..3d163dcb 100644 --- a/contracts/test-integration/BlsSignerContractInteraction.test.ts +++ b/contracts/test-integration/BlsSignerContractInteraction.test.ts @@ -221,7 +221,7 @@ describe("Signer contract interaction tests", function () { // Act const result = await blsSigners[0].sendTransactionBatch(transactionBatch); - await result.awaitBatch(); + await result.awaitBatchReceipt(); // Assert const newBalance = await mockERC20.balanceOf(spender); @@ -445,7 +445,7 @@ describe("Signer contract interaction tests", function () { // Act const result = await blsSigners[3].sendTransactionBatch(transactionBatch); - await result.awaitBatch(); + await result.awaitBatchReceipt(); // Assert expect(await mockERC721.ownerOf(tokenId)).to.equal(spender); From ee17357e03bf17c9bd18c41e532556916358c48e Mon Sep 17 00:00:00 2001 From: JohnGuilding Date: Fri, 3 Mar 2023 11:25:55 +0000 Subject: [PATCH 12/12] Fix intermittent timing issue with test --- .../test-integration/BlsProviderContractInteraction.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/test-integration/BlsProviderContractInteraction.test.ts b/contracts/test-integration/BlsProviderContractInteraction.test.ts index 5da25318..7ff62886 100644 --- a/contracts/test-integration/BlsProviderContractInteraction.test.ts +++ b/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)); + // Assert expect((await erc20.balanceOf(recipient)).sub(balanceBefore)).to.equal( amountToTransfer,