-
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?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd the label graphite-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
6797694
to
ed436d8
Compare
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.
this is looking good!
One thing missing is some client.test at a minimum (though would be great if we could extend it to installValidation.ts
and have unit tests for installValidation.test.ts
)
OOS for this PR is the naming I think, but calling it out here as @howydev and @linnall look at consolidating to the Prism naming as just looking at this PR it's clear that it'll be much better devex externally to have both a Prism account factory and Prism client factory for release
/** | ||
* 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. | ||
* |
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:
@example
```ts twoslash
....
```
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
); | ||
} | ||
|
||
// 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 comment
The 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 toSmartContractAccount
for this
That methods intention is meant to return the address at which the logic for the SCA lives and I think that holds here:
- proxies point to a different address than the SCA address
- no proxy accounts should probably return the address of the SCA itself?
- 7702 returns the address containing the logic we're delegating to for the EOA
// 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 |
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.
Is there anyway to make that clearer? If we add the optional parameter mentioned above, then we could update the docs on toSmartContractAccount
to highlight what that address is for?
We could also update the jsdoc for this middleware as well to make it clear that the account MUST return something when calling getImplementationAddress
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.
I think this is an issue on the semantic meaning of "getImplementationAddress" - there's ambiguity of what the function does:
- does it get the current implementation address from the account's proxy? <-- what it is default-initialized to
- does it get the intended implementation address for a proxy?
- does it get the intended delegation address for 7702? <-- what this middleware expects user to overwrite it to
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:
- define the
getImplementationAddress
to mean getting the intended implementation address, agnostic to SCA / 7702-upgraded EOA - Move the "current proxy implementation getter" to a utility function / utility action on a public client
/** | ||
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here, needs an @example
tag which has twoslash
enabled
if (yParity === undefined) { | ||
throw new Error("Invalid signature: missing yParity or v"); | ||
} |
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.
This seems like the responsibility of the signer probably no?
|
||
export type MAV2Account< | ||
TSigner extends SmartAccountSigner = SmartAccountSigner | ||
> = SmartContractAccountWithSigner<"MAV2Account", TSigner, "0.7.0"> & { |
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.
I think this base type should probably be generic on source
as well since source
should be unique per instance (ie. the first generic parameter in SmartContractAccountWithSigner
)
signer: TSigner; | ||
entryPoint?: EntryPointDef<"0.7.0", Chain>; | ||
signerEntity?: SignerEntity; |
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.
oh yea I remember seeing this during hackathon and thinking it odd. Why can't we merge signer
and signerEntity
?
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.
cc @howydev
/** | ||
* Creates an SMAV2 account using defined parameters including chain, signer, salt, factory address, and more. | ||
* Handles account initialization code, nonce generation, transaction encoding, and more to construct a modular account with optional validation hooks. | ||
* |
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.
nit: add example here
* Creates a SMAv2 account client using the provided configuration parameters. | ||
* | ||
* @example | ||
* ```ts |
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.
* ```ts | |
* ```ts twoslash |
import { http } from "viem"; | ||
|
||
const client = createSmartAccountClient({ | ||
transport: http("rpc-url"), | ||
chain: sepolia, | ||
...alchemyGasManagerMiddleware("policyId"), |
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.
oh I saw this in a another PR somewhere too... This looks like a bug in main 🤔
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.
I think this is a git issue somewhere - it was fixed in a different PR that's already merged
c0d52b7
to
847ab00
Compare
* feat: adds initial impl of signAuthorization * docs: adds signAuthorization documentation * fix: rename var
847ab00
to
7d20b9f
Compare
Pull Request Checklist
yarn test
)site
folder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:write
)PR-Codex overview
This PR updates the
viem
package version across multiplepackage.json
files and introduces new functionalities related to signing authorizations and gas estimation in theaa-sdk
andaccount-kit
projects.Detailed summary
viem
version from^2.20.0
to^2.22.6
in variouspackage.json
files.signAuthorization
method toLocalAccountSigner
andBaseAlchemySigner
.default7702UserOpSigner
anddefault7702GasEstimator
middleware functions.createSMA7702AccountClient
and related functionalities insmart-contracts
.