Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

413 improve private key management in bls provider and signer #548

Merged
merged 4 commits into from
Mar 22, 2023
Merged

413 improve private key management in bls provider and signer #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3bbd39
Remove reference to signer from provider
JohnGuilding Mar 16, 2023
32c6b13
Cleanup test code & add contract interaction gas estimate tests
JohnGuilding Mar 16, 2023
3c0f36f
re-add accidentally deleted tests
JohnGuilding Mar 16, 2023
5423f65
Add comment explaining throwaway BlsWalletWrapper
JohnGuilding Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 146 additions & 51 deletions contracts/clients/src/BlsProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { ethers, BigNumber } from "ethers";
import { Deferrable } from "ethers/lib/utils";

import { ActionData, Bundle } from "./signer/types";
import { ActionData, Bundle, PublicKey } from "./signer/types";
import Aggregator, { BundleReceipt } from "./Aggregator";
import BlsSigner, {
TransactionBatchResponse,
Expand All @@ -14,14 +14,19 @@ import BlsWalletWrapper from "./BlsWalletWrapper";
import {
AggregatorUtilities__factory,
BLSWallet__factory,
VerificationGateway__factory,
} from "../typechain-types";
import addSafetyPremiumToFee from "./helpers/addSafetyDivisorToFee";

export type PublicKeyLinkedToActions = {
publicKey: PublicKey;
actions: Array<ActionData>;
};

export default class BlsProvider extends ethers.providers.JsonRpcProvider {
readonly aggregator: Aggregator;
readonly verificationGatewayAddress: string;
readonly aggregatorUtilitiesAddress: string;
signer!: BlsSigner;

constructor(
aggregatorUrl: string,
Expand All @@ -39,34 +44,43 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider {
override async estimateGas(
transaction: Deferrable<ethers.providers.TransactionRequest>,
): Promise<BigNumber> {
if (!transaction.to) {
const resolvedTransaction = await ethers.utils.resolveProperties(
transaction,
);

if (!resolvedTransaction.to) {
throw new TypeError("Transaction.to should be defined");
}

// 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");
if (!resolvedTransaction.from) {
throw new TypeError("Transaction.from should be defined");
}

const action: ActionData = {
ethValue: transaction.value?.toString() ?? "0",
contractAddress: transaction.to.toString(),
encodedFunction: transaction.data?.toString() ?? "0x",
ethValue: resolvedTransaction.value?.toString() ?? "0",
contractAddress: resolvedTransaction.to.toString(),
encodedFunction: resolvedTransaction.data?.toString() ?? "0x",
};

const nonce = await BlsWalletWrapper.Nonce(
this.signer.wallet.PublicKey(),
this.verificationGatewayAddress,
this,
const nonce = await this.getTransactionCount(
resolvedTransaction.from.toString(),
);

const actionWithFeePaymentAction =
this._addFeePaymentActionForFeeEstimation([action]);

// TODO: (merge-ok) bls-wallet #560 Estimate fee without requiring a signed bundle
// There is no way to estimate the cost of a bundle without signing a bundle. The
// alternative would be to use a signer instance in this method which is undesirable,
// as this would result in tight coupling between a provider and a signer.
const throwawayPrivateKey = await BlsWalletWrapper.getRandomBlsPrivateKey();
Copy link
Collaborator Author

@JohnGuilding JohnGuilding Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions on being able to estimate fees in the provider without having reference to a signer. This was my hacky solution. It does result in slightly different gas estimates each time, but still close enough to send the transaction successfully.

See this comment for examples of differences in tests without mocking a consistent private key value.

Not a fan of creating a throwaway BlsWalletWrapper, but feels slightly better than having a signer tightly coupled to a provider.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now this is fine. I would document why your are doing this in code.

We should have an estimation endpoint/method for v2 that does not require a signed bundle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment and tracked via a follow up issue for V2 #560

const throwawayBlsWalletWrapper = await BlsWalletWrapper.connect(
throwawayPrivateKey,
this.verificationGatewayAddress,
this,
);

const feeEstimate = await this.aggregator.estimateFee(
this.signer.wallet.sign({
throwawayBlsWalletWrapper.sign({
nonce,
actions: [...actionWithFeePaymentAction],
}),
Expand All @@ -79,13 +93,6 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider {
override async sendTransaction(
signedTransaction: string | Promise<string>,
): Promise<ethers.providers.TransactionResponse> {
// 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 resolvedTransaction = await signedTransaction;
const bundle: Bundle = JSON.parse(resolvedTransaction);

Expand All @@ -107,23 +114,16 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider {
encodedFunction: bundle.operations[0].actions[0].encodedFunction,
};

return this.signer.constructTransactionResponse(
return await this._constructTransactionResponse(
actionData,
bundle.senderPublicKeys[0],
result.hash,
this.signer.wallet.address,
);
}

async sendTransactionBatch(
signedTransactionBatch: string,
): Promise<TransactionBatchResponse> {
// 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);
Expand All @@ -132,33 +132,28 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider {
throw new Error(JSON.stringify(result.failures));
}

const actionData: Array<ActionData> = bundle.operations
.map((operation) => operation.actions)
.flat();
const publicKeysLinkedToActions: Array<PublicKeyLinkedToActions> =
bundle.senderPublicKeys.map((publicKey, i) => {
const operation = bundle.operations[i];
const actions = operation.actions;

return this.signer.constructTransactionBatchResponse(
actionData,
return {
publicKey,
actions,
};
});

return await this._constructTransactionBatchResponse(
publicKeysLinkedToActions,
result.hash,
this.signer.wallet.address,
);
}

override getSigner(
privateKey: string,
addressOrIndex?: string | number,
): BlsSigner {
if (this.signer) {
return this.signer;
}

const signer = new BlsSigner(
_constructorGuard,
this,
privateKey,
addressOrIndex,
);
this.signer = signer;
return signer;
return new BlsSigner(_constructorGuard, this, privateKey, addressOrIndex);
}

override getUncheckedSigner(
Expand Down Expand Up @@ -295,4 +290,104 @@ export default class BlsProvider extends ethers.providers.JsonRpcProvider {
},
];
}

async _constructTransactionResponse(
action: ActionData,
publicKey: PublicKey,
hash: string,
nonce?: BigNumber,
): Promise<ethers.providers.TransactionResponse> {
const chainId = await this.send("eth_chainId", []);

if (!nonce) {
nonce = await BlsWalletWrapper.Nonce(
publicKey,
this.verificationGatewayAddress,
this,
);
}

const verificationGateway = VerificationGateway__factory.connect(
this.verificationGatewayAddress,
this,
);
const from = await BlsWalletWrapper.AddressFromPublicKey(
publicKey,
verificationGateway,
);

return {
hash,
to: action.contractAddress,
from,
nonce: nonce.toNumber(),
gasLimit: BigNumber.from("0x0"),
data: action.encodedFunction.toString(),
value: BigNumber.from(action.ethValue),
chainId: parseInt(chainId, 16),
type: 2,
confirmations: 1,
wait: (confirmations?: number) => {
return this.waitForTransaction(hash, confirmations);
},
};
}

async _constructTransactionBatchResponse(
publicKeysLinkedToActions: Array<PublicKeyLinkedToActions>,
hash: string,
nonce?: BigNumber,
): Promise<TransactionBatchResponse> {
const chainId = await this.send("eth_chainId", []);
const verificationGateway = VerificationGateway__factory.connect(
this.verificationGatewayAddress,
this,
);

const transactions: Array<ethers.providers.TransactionResponse> = [];

for (const publicKeyLinkedToActions of publicKeysLinkedToActions) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible in the provider to get a bundle with multiple public keys and operations, this logic ensures all of the transactions in transactions: Array<ethers.providers.TransactionResponse> = [] are linked to the correct nonce and from address.

e.g. if I just mapped across all of the actions like I was before, then there would be no way of telling which public key was linked to which group of actions.

const from = await BlsWalletWrapper.AddressFromPublicKey(
publicKeyLinkedToActions.publicKey,
verificationGateway,
);

if (!nonce) {
nonce = await BlsWalletWrapper.Nonce(
publicKeyLinkedToActions.publicKey,
this.verificationGatewayAddress,
this,
);
}

for (const action of publicKeyLinkedToActions.actions) {
if (action.contractAddress === this.aggregatorUtilitiesAddress) {
break;
}

transactions.push({
hash,
to: action.contractAddress,
from,
nonce: nonce!.toNumber(),
gasLimit: BigNumber.from("0x0"),
data: action.encodedFunction.toString(),
value: BigNumber.from(action.ethValue),
chainId: parseInt(chainId, 16),
type: 2,
confirmations: 1,
wait: (confirmations?: number) => {
return this.waitForTransaction(hash, confirmations);
},
});
}
}

return {
transactions,
awaitBatchReceipt: (confirmations?: number) => {
return this.waitForTransaction(hash, confirmations);
},
};
}
}
Loading