-
Notifications
You must be signed in to change notification settings - Fork 140
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: eip-7702 support in aa-sdk/core, and sma7702 support in account-kit/smart-contracts #1287
base: main
Are you sure you want to change the base?
Changes from all commits
03a8666
a84a897
654c08f
4939fe8
bbb1d2d
5ed901f
764e4fc
4880d89
e272112
bc3994f
dd101aa
ddd6b04
7d20b9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { AccountNotFoundError } from "../../errors/account.js"; | ||
import type { UserOperationStruct } from "../../types.js"; | ||
import type { ClientMiddlewareFn } from "../types"; | ||
import { defaultGasEstimator } from "./gasEstimator.js"; | ||
|
||
/** | ||
* A middleware function to estimate the gas usage of a user operation with an optional custom gas estimator. | ||
* This function is only compatible with accounts using EntryPoint v0.7.0. | ||
* | ||
* @param {ClientMiddlewareFn} [gasEstimator] An optional custom gas estimator function | ||
* @returns {Function} A function that takes user operation structure and parameters, estimates gas usage, and returns the estimated user operation | ||
*/ | ||
export const default7702GasEstimator: ( | ||
gasEstimator?: ClientMiddlewareFn | ||
) => ClientMiddlewareFn = | ||
(gasEstimator?: ClientMiddlewareFn) => async (struct, params) => { | ||
const gasEstimator_ = gasEstimator ?? defaultGasEstimator(params.client); | ||
|
||
const account = params.account ?? params.client.account; | ||
if (!account) { | ||
throw new AccountNotFoundError(); | ||
} | ||
|
||
const entryPoint = account.getEntryPoint(); | ||
if (entryPoint.version !== "0.7.0") { | ||
throw new Error( | ||
"This middleware is only compatible with EntryPoint v0.7.0" | ||
); | ||
} | ||
|
||
// todo: this is currently overloading the meaning of the getImplementationAddress method, replace with a dedicated method or clarify intention in docs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed offline, but I think it's a good idea to allow passing an optional override to That methods intention is meant to return the address at which the logic for the SCA lives and I think that holds here:
|
||
const implementationAddress = await account.getImplementationAddress(); | ||
|
||
// todo: do we need to omit this from estimation if the account is already 7702 delegated? Not omitting for now. | ||
|
||
(struct as UserOperationStruct<"0.7.0">).authorizationContract = | ||
implementationAddress; | ||
|
||
const estimatedUO = await gasEstimator_(struct, params); | ||
|
||
estimatedUO.authorizationContract = undefined; // Strip out authorizationContract after estimation. | ||
|
||
return estimatedUO; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import { toHex } from "viem"; | ||
import { isSmartAccountWithSigner } from "../../account/smartContractAccount.js"; | ||
import { AccountNotFoundError } from "../../errors/account.js"; | ||
import { ChainNotFoundError } from "../../errors/client.js"; | ||
import type { ClientMiddlewareFn } from "../types"; | ||
import { defaultUserOpSigner } from "./userOpSigner.js"; | ||
|
||
/** | ||
* Provides a default middleware function for signing user operations with a client account when using ERC-7702 to upgrade local accounts to smart accounts. | ||
* If the SmartAccount doesn't support `signAuthorization`, then this just runs the provided `signUserOperation` middleware | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment here, needs an |
||
* @param {ClientMiddlewareFn} [userOpSigner] Optional user operation signer function | ||
* @returns {Function} An async function that processes the user operation and returns the authorized operation with an authorization tuple if necessary | ||
*/ | ||
export const default7702UserOpSigner: ( | ||
userOpSigner?: ClientMiddlewareFn | ||
) => ClientMiddlewareFn = | ||
(userOpSigner?: ClientMiddlewareFn) => async (struct, params) => { | ||
const userOpSigner_ = userOpSigner ?? defaultUserOpSigner; | ||
|
||
const uo = await userOpSigner_(struct, params); | ||
|
||
const account = params.account ?? params.client.account; | ||
const { client } = params; | ||
|
||
if (!account || !isSmartAccountWithSigner(account)) { | ||
throw new AccountNotFoundError(); | ||
} | ||
|
||
const signer = account.getSigner(); | ||
|
||
if (!signer.signAuthorization) { | ||
return uo; | ||
} | ||
|
||
if (!client.chain) { | ||
throw new ChainNotFoundError(); | ||
} | ||
|
||
const code = (await client.getCode({ address: account.address })) ?? "0x"; | ||
// TODO: this isn't the cleanest because now the account implementation HAS to know that it needs to return an impl address | ||
// even if the account is not deployed | ||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anyway to make that clearer? If we add the optional parameter mentioned above, then we could update the docs on We could also update the jsdoc for this middleware as well to make it clear that the account MUST return something when calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is an issue on the semantic meaning of "getImplementationAddress" - there's ambiguity of what the function does:
We could try to do something with the type system to attempt to assert that this middleware is only added to a client for which the account provided has the definition overwritten. But that sounds super messy, so I'm not particularly inclined to do it. I think the real fix for this is to:
|
||
|
||
const implAddress = await account.getImplementationAddress(); | ||
|
||
const expectedCode = "0xef0100" + implAddress.slice(2); | ||
|
||
if (code.toLowerCase() === expectedCode.toLowerCase()) { | ||
return uo; | ||
} | ||
|
||
const accountNonce = await params.client.getTransactionCount({ | ||
address: account.address, | ||
}); | ||
|
||
const { | ||
r, | ||
s, | ||
v, | ||
yParity = v ? v - 27n : undefined, | ||
} = await signer.signAuthorization({ | ||
chainId: client.chain.id, | ||
contractAddress: implAddress, | ||
nonce: accountNonce, | ||
}); | ||
|
||
if (yParity === undefined) { | ||
throw new Error("Invalid signature: missing yParity or v"); | ||
} | ||
Comment on lines
+67
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like the responsibility of the signer probably no? |
||
|
||
return { | ||
...uo, | ||
authorizationTuple: { | ||
chainId: client.chain.id, | ||
nonce: toHex(accountNonce), // deepHexlify doesn't encode number(0) correctly, it returns "0x" | ||
address: implAddress, | ||
r, | ||
s, | ||
yParity: toHex(yParity), | ||
}, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import { | |
privateKeyToAccount, | ||
} from "viem/accounts"; | ||
import type { SmartAccountSigner } from "./types.js"; | ||
import type { Authorization } from "viem/experimental"; | ||
|
||
/** | ||
* Represents a local account signer and provides methods to sign messages and transactions, as well as static methods to create the signer from mnemonic or private key. | ||
|
@@ -95,6 +96,33 @@ export class LocalAccountSigner< | |
return this.inner.signTypedData(params); | ||
}; | ||
|
||
/** | ||
* Signs an unsigned authorization using the provided private key account. | ||
* | ||
* @example | ||
* ```ts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we add At some point we'll want to do this for ALL examples retroactively as well, but that will cause too many build breaks I fear |
||
* import { LocalAccountSigner } from "@aa-sdk/core"; | ||
* import { generatePrivateKey } from "viem"; | ||
* | ||
* const signer = LocalAccountSigner.mnemonicToAccountSigner(generatePrivateKey()); | ||
* const signedAuthorization = await signer.signAuthorization({ | ||
* contractAddress: "0x1234123412341234123412341234123412341234", | ||
* chainId: 1, | ||
* nonce: 3, | ||
* }); | ||
* ``` | ||
* | ||
* @param {Authorization<number, false>} unsignedAuthorization - The unsigned authorization to be signed. | ||
* @returns {Promise<Authorization<number, true>>} A promise that resolves to the signed authorization. | ||
*/ | ||
|
||
signAuthorization( | ||
this: LocalAccountSigner<PrivateKeyAccount>, | ||
unsignedAuthorization: Authorization<number, false> | ||
): Promise<Authorization<number, true>> { | ||
return this.inner.experimental_signAuthorization(unsignedAuthorization); | ||
} | ||
|
||
/** | ||
* Returns the address of the inner object in a specific hexadecimal format. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add an
@example
block here as well and it should look like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
twoslash
piece will ensure that the code is valid when the docs site is built AND add the on hover interaction to the docs