Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: account abstraction and paymaster syntax #1

Closed
wants to merge 14 commits into from
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@
"ethers": "^6.7.1"
},
"scripts": {
"test": "mocha -r ts-node/register --bail tests/setup.test.ts tests/unit/*.test.ts tests/integration/*.test.ts",
"test": "yarn test:unit && yarn test:integration",
"test:unit": "mocha -r ts-node/register --bail tests/unit/*.test.ts",
"test:integration": "mocha -r ts-node/register --bail tests/setup.test.ts tests/integration/*.test.ts",
"test:aa": "yarn test:unit && yarn test:aa:wallet",
"test:aa:wallet": "mocha -r ts-node/register --bail tests/integration/wallet.test.ts --grep 'AbstractWallet'",
"build": "yarn types && tsc --strict true && ncp ./abi ./build/abi",
"lint": "prettier . --write",
"lint:check": "prettier . --check",
Expand Down
3 changes: 2 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export * as utils from "./utils";
export * as types from "./types";
export { EIP712Signer, Signer, L1Signer } from "./signer";
export { Wallet } from "./wallet";
export { Wallet, AbstractWallet, signingFunction } from "./wallet";
export { BrowserProvider, Provider } from "./provider";
export { ContractFactory, Contract } from "./contract";
export { Paymaster } from "./paymaster";
37 changes: 37 additions & 0 deletions src/paymaster.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { getAddress } from "ethers";
import { ApprovalBasedPaymasterInput, GeneralPaymasterInput } from "./types";
import { encodeData, getPaymasterParams } from "./utils";

export class Paymaster {
type: string;
address: string;
paymasterInput: ApprovalBasedPaymasterInput | GeneralPaymasterInput;

constructor(
type: "ApprovalBased" | "General",
address: string,
tokenAddress: string = "",
minimalAllowance: number = 1,
innerInput: any[] = [],
) {
this.type = type;
this.address = getAddress(address);
const encodedInnerInput = encodeData(innerInput);
this.paymasterInput =
type == "ApprovalBased"
? {
type,
token: getAddress(tokenAddress),
minimalAllowance,
innerInput: encodedInnerInput,
}
: {
type,
innerInput: encodedInnerInput,
};
}

getPaymasterParams() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this class if we simply want to build paymaster params? Shouldn't it be a util instead?

Copy link
Author

Choose a reason for hiding this comment

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

Actually I also notice this method will require adding parameters as those are required for the ApprovalBased paymasters that accept a quantity of tokens to cover the transaction cost.

As for making this a class, this class will also include a method to enrich a transaction with the data for paymasterParams which is useful when developers only want to sponsor some transactions. Something like .addParamsToTransaction(trx) that calls getPaymasterParams to fill in the transaction object. This removes the need for the developer to manipulate the transaction object manually.

Not precious about this either though, so if you think strongly of keeping these as functions then we could go that way but I find this:

let tx = contract.methodWrite(...);
tx = utils.addPaymasterParamsToTransaction(utils.getPaymasterParams(...));

Less appealing than:

const paymaster = new Paymaster(...);
let tx = contract.methodWrite(...);
tx = paymaster.addParamsToTransaction(tx);

return getPaymasterParams(this.address, this.paymasterInput);
}
}
29 changes: 25 additions & 4 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { AbiCoder, BigNumberish, BytesLike, ethers, SignatureLike } from "ethers";
import { IERC20__factory, IL1Bridge__factory } from "../typechain";
import { Provider } from "./provider";
import { EIP712Signer } from "./signer";
import {
Address,
DeploymentInfo,
Expand All @@ -7,11 +10,8 @@ import {
PaymasterParams,
PriorityOpTree,
PriorityQueueType,
TransactionLike,
TransactionLike
} from "./types";
import { Provider } from "./provider";
import { EIP712Signer } from "./signer";
import { IERC20__factory, IL1Bridge__factory } from "../typechain";

export * from "./paymaster-utils";

Expand Down Expand Up @@ -594,3 +594,24 @@ export async function estimateCustomBridgeDepositL2Gas(
l2Value: l2Value,
});
}

export function encodeData(data: any[]): string {
const types = data.map(item => {
if (typeof item === 'number') {
return 'uint256';
Copy link
Contributor

Choose a reason for hiding this comment

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

what if I need uint128 or int64?

Copy link
Author

Choose a reason for hiding this comment

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

good question. I thought the same while setting up this function too (another reason why I left this PR as draft). I would think now then that it is not possible to deduce these lower levels types without the developer specifying them either through an ABI or some form of input. This changes the syntax for constructing both the Paymaster and abstractWallet classes, considering they both can potentially have methods that serialise data.

} else if (typeof item === 'string') {
return 'string';
} else if (typeof item === 'boolean') {
return 'bool';
} else if (Array.isArray(item)) {
throw new Error('Nested arrays are not supported');
} else if (typeof item === 'object' && item !== null) {
throw new Error('Nested objects are not supported');
}
throw new Error(`Unsupported data type: ${typeof item}`);
});

const coder = new AbiCoder();
const encodedData = coder.encode(types, data);
return encodedData;
}
58 changes: 54 additions & 4 deletions src/wallet.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { EIP712Signer } from "./signer";
import { ProgressCallback, ethers } from "ethers";
import { AdapterL1, AdapterL2 } from "./adapters";
import { Paymaster } from "./paymaster";
import { Provider } from "./provider";
import { EIP712_TX_TYPE, serializeEip712 } from "./utils";
import { ethers, ProgressCallback } from "ethers";
import { EIP712Signer } from "./signer";
import { TransactionLike, TransactionRequest, TransactionResponse } from "./types";
import { AdapterL1, AdapterL2 } from "./adapters";
import { DEFAULT_GAS_PER_PUBDATA_LIMIT, EIP712_TX_TYPE, serializeEip712 } from "./utils";

export class Wallet extends AdapterL2(AdapterL1(ethers.Wallet)) {
// @ts-ignore
Expand Down Expand Up @@ -124,3 +125,52 @@ export class Wallet extends AdapterL2(AdapterL1(ethers.Wallet)) {
return await this.provider.broadcastTransaction(await this.signTransaction(populatedTx));
}
}

interface SigningFunction {
(this: AbstractWallet, transaction: TransactionRequest): Promise<string>;
Copy link
Contributor

@Romsters Romsters Dec 15, 2023

Choose a reason for hiding this comment

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

does it need access tothis (AbstractWallet)?

Copy link
Author

@idea404 idea404 Dec 18, 2023

Choose a reason for hiding this comment

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

It could otherwise accept the wallet as a parameter as:

    (abstractWallet: AbstractWallet, transaction: TransactionRequest): Promise<string>;

Both approaches are fine by me

}

export class AbstractWallet extends Wallet {
readonly accountAddress: string;
paymaster: Paymaster;
customSigningFunction: SigningFunction;

constructor(
accountAddress: string,
privateKey: string,
providerL2?: Provider,
providerL1?: ethers.Provider,
paymaster: Paymaster | null = null,
customSigningFunction: SigningFunction = signingFunction,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a default here? Or should we just make it required?

Copy link
Author

Choose a reason for hiding this comment

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

I find it makes the syntax for declaration simpler by adding a default here. I prefer it because of that. I can imagine some developers might not be interested in overriding the signing function and only seek to use AA for the custom logic you can add to accounts on-chain. In these cases I can imagine they'd be happy not having to be concerned with the signing logic, or paymaster logic either (if they do not wish to use a paymaster).

) {
super(privateKey, providerL2);
this.accountAddress = accountAddress;
this.paymaster = paymaster;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this.paymaster here? How I can I control it's usage if it's here?

Copy link
Author

Choose a reason for hiding this comment

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

this.paymaster is called in the signingFunction logic inside of signTransaction which is called for signing any transaction. The goal here was to provide a way for the developer to specify using a paymaster by default on any transaction. Currently the syntax in signingFunction is:

   ...
    transaction.customData = this.paymaster ? {
        gasPerPubdata: DEFAULT_GAS_PER_PUBDATA_LIMIT,
        paymasterParams: this.paymaster.getPaymasterParams(),
    } : {};
    ...

But I think it would be better to change it actually to:

  ...
   transaction.customData ??= this.paymaster ? {
       gasPerPubdata: DEFAULT_GAS_PER_PUBDATA_LIMIT,
       paymasterParams: this.paymaster.getPaymasterParams(),
   } : {};
   ...

Considering the developer might want to specify some transactions on the frontend where a paymaster is applied, and some others not. But if the developer wants a paymaster to be used in all transactions, then they can simply declare the paymaster in this constructor here.

this.providerL1 = providerL1;
this.customSigningFunction = customSigningFunction.bind(this);
}

override transfer(transaction: { to: string; amount: ethers.BigNumberish; token?: string; overrides?: ethers.Overrides; }): Promise<TransactionResponse> {
transaction.overrides = { type: EIP712_TX_TYPE };
return super.transfer(transaction);
}

override getAddress(): Promise<string> {
return Promise.resolve(this.accountAddress);
}

override async signTransaction(transaction: TransactionRequest): Promise<string> {
return this.customSigningFunction(transaction);
}
}

export async function signingFunction(this: AbstractWallet, transaction: TransactionRequest): Promise<string> {
transaction.customData = this.paymaster ? {
gasPerPubdata: DEFAULT_GAS_PER_PUBDATA_LIMIT,
paymasterParams: this.paymaster.getPaymasterParams(),
} : {};
transaction.customData = this._fillCustomData(transaction.customData);
transaction.customData.customSignature = await this.eip712.sign(transaction);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does it work? What key is used to sign it?

Copy link
Author

Choose a reason for hiding this comment

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

This will use the privateKey parameter passed to the constructor here. Ultimately, a SigningKey function is added at ethers.BaseWallet, the class that ethers.Wallet extends (and this class is extended by zks.Wallet), using this passed in private key that is used for signatures.

I find it also tricky to know what exactly is going on with the class as it extends another class about 5 times.. But this approach adds the least lines of code too.

// @ts-ignore
return (0, serializeEip712)(transaction);
}
Loading