-
Notifications
You must be signed in to change notification settings - Fork 133
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: add initial skeleton for 6900 account support #248
Conversation
750aad0
to
3552335
Compare
733b177
to
81406f6
Compare
718db97
to
928a8eb
Compare
6161184
to
55dfbdc
Compare
|
||
export interface Plugin<D> { | ||
meta: { name: string; version: string }; | ||
accountDecorators: (a: BaseSmartContractAccount) => D; |
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 expose a method or the rpcProvider
property on ISmartContractAccount
so we can use that here instead of BaseSmartContractAccount
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.
by having made it readonly you can now, right?
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 did but not on the ISmartContractAccount, I can update the ISmartContractAccount interface here if we're cool with that change
@@ -0,0 +1 @@ | |||
export { MultiOwnerPluginGenConfig } from "./multi-owner/config.js"; |
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 can probably do better here by just reading all the config.js
files found within this folder and exporting their contents. we might need to do default exports for the configs though
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.
hmm but will take break the vscode module navigation ? or you mean reading all config.ts files and export their contents by writing into a new file for exports in out
or something
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.
yea the way I was thinking would break module navigation good call... we could make the plugin gen script overwrite this file though that's a good idea
382246c
to
a5d466e
Compare
const signWith1271Wrapper = async (msg: Hash): Promise<`0x${string}`> => { | ||
const multiOwnerAugmented = | ||
acct.extendWithPluginMethods(MultiOwnerPlugin); | ||
|
||
const [, name, version, chainId, verifyingContract, salt] = | ||
await multiOwnerAugmented.readEip712Domain(); | ||
|
||
return params.owner.signTypedData({ | ||
domain: { | ||
chainId: Number(chainId), | ||
name, | ||
salt, | ||
verifyingContract, | ||
version, | ||
}, | ||
types: { | ||
ERC6900Message: [{ name: "message", type: "bytes" }], | ||
}, | ||
message: { | ||
ERC6900Message: { | ||
message: toBytes(msg), | ||
}, | ||
}, | ||
primaryType: "ERC6900Message", | ||
}); | ||
}; |
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 signing with 1271?
can you point to the reference where the contract implements function isValidSignature
? Wondering how you came up with the primaryType ERC6900Message
. Is that degined in the eip712Domain
function of MultiOwnerPlugin
?
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.
All signatures for 1271 validation in the MSCA (and LightAccount) require this wrapping
.withFactory(async (acct) => { | ||
const ownerAddress = await params.owner.getAddress(); | ||
return concatHex([ | ||
acct.getFactoryAddress(), | ||
encodeFunctionData({ | ||
abi: MultiOwnerMSCAFactoryAbi, | ||
functionName: "createAccount", | ||
// TODO: this needs to support creating accounts with multiple owners | ||
args: [params.index, [ownerAddress]], | ||
}), | ||
]); | ||
}) |
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.
so MultiOwnerMSCAFactory
is always the factory that create MSCAs? There isn't a separate MSCAFactory
right?
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.
There can be any number of factories for an MSCA which is why I added this withFactory
concept. The other factory that exists already is the MultiOwnerWithTokenReceiver factory
>() => | ||
createBaseSmartAccountParamsSchema<TTransport>().extend({ | ||
owner: SignerSchema, | ||
index: z.bigint().optional().default(0n), |
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.
what does this index
specify?
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.
index is used as a salt, if you want to create many accounts with the same set of initial owners but have them be at other addresses use can use the salt to differentiate them
| "signMessage" | ||
| "signTypedData" | ||
| "signUserOperationHash" | ||
| "getDummySignature" |
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.
these are abstraction functions except the execute functions from SmartContractAccount correct?
Then why also signUserOperationHash
? Don't we just use signMessage
for signing uos?
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.
for 1271 validation, we sign messages differently that we do for UO hashes. The MSCA (and LightAccount) expects the UO to be personal sign of the hash. 1271 expects a sign typed data following the envelope defined above
// TODO: need to make this configurable to run in CI without requiring this | ||
rpcUrl?: string; |
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.
what do you mean configurable here?
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 I forgot to remove the TODO because it is optional now
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 idea is that we shouldn't have to hardcode an RPC URL here. This is useful for testing locally if you want to hit a local anvil instance
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 a better way to write this type so it's dependent on environment to see if rpcUrl is defined?
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 can probs just do that in the plugin gen code
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.
would like to review the builder + multi-owner-account files live as a team, but left comments elsewhere
@@ -29,6 +29,8 @@ | |||
"./package.json": "./package.json" | |||
}, | |||
"scripts": { | |||
"generate": "npx wagmi generate", |
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.
learn: have you worked with the wagmi cli? if not / maybe if you remember when you first did, did you just look up a way to generate abis and some stackoverflow pointed you to it?
otherwise, curious how you arrived at this solution.
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 used it before, but then looked at their codebase for how they define plugins so I could write my own
|
||
export interface Plugin<D> { | ||
meta: { name: string; version: string }; | ||
accountDecorators: (a: BaseSmartContractAccount) => D; |
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.
by having made it readonly you can now, right?
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.
should the file be "plugin-gen.ts" to be kebab case?
// TODO: need to make this configurable to run in CI without requiring this | ||
rpcUrl?: string; |
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 a better way to write this type so it's dependent on environment to see if rpcUrl is defined?
const pluginRegEx: RegExp = /[pP]lugin/g; | ||
|
||
export default defineConfig( | ||
pluginConfigs.map((config) => ({ |
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: is there a way to check if there's no diff to the generated file? this re-writes every plugin auto-gen file each time the yarn script is run, right?
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.
yea it does and we probs can, but we can probs leverage nx
and lerna
to do that by adding an nx
config for generate
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: want to include a link to the repo + etherscan / some way to validate the abi?
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.
replied elsewhere too, but let's keep those out of here for now since none of these things are in production yet
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: should this folder be plugin-defs
?
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.
yea I think that's a fair change
4f99d57
to
dc4c98b
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
* feat: add some plugin generation logic * chore: add generate commands * style: apply PR suggestion Co-authored-by: Dennis Won <denniswon@users.noreply.github.com> * refactor: create plugin config concept * feat: add multi-owner-msca impl that leverages the plugin gen (#263) * feat: add multi-owner-msca impl that leverages the plugin gen * feat: proposal for msca builder pattern (#264) * feat: proposal for msca builder pattern * refactor: rework plugin gen to create read methods for the account --------- Co-authored-by: Dennis Won <denniswon@users.noreply.github.com>
* feat: add initial skeleton for 6900 account support * feat: add some plugin generation logic (#262) * feat: add some plugin generation logic * chore: add generate commands * style: apply PR suggestion Co-authored-by: Dennis Won <denniswon@users.noreply.github.com> * refactor: create plugin config concept * feat: add multi-owner-msca impl that leverages the plugin gen (#263) * feat: add multi-owner-msca impl that leverages the plugin gen * feat: proposal for msca builder pattern (#264) * feat: proposal for msca builder pattern * refactor: rework plugin gen to create read methods for the account --------- Co-authored-by: Dennis Won <denniswon@users.noreply.github.com> * chore: update plugin gen * feat: add plugin manager decorator for MSCA * feat: add a mechanism for adding provider decorators to accounts (#287) * feat: add a mechanism for adding provider decorators to accounts * chore: update lerna and package json to lint generated files * refactor: rename MSCA to IMSCA --------- Co-authored-by: Dennis Won <denniswon@users.noreply.github.com> Co-authored-by: avasisht23 <ajay@alchemy.com>
* refactor: move required by types to aa-core utils types folder * feat: extend msca account with account loupe decorators * feat: move extend method on account from msca to base account * Update packages/core/src/account/types.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * Update packages/core/src/account/types.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * Update packages/core/src/account/types.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * Update packages/accounts/src/msca/account-loupe/decorator.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * fix: fix lint and build --------- Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com>
* feat: msca plugingen to accept multichain address format * Update packages/accounts/scripts/plugingen.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * Update packages/accounts/wagmi.config.ts Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com> * feat: changed address field in plugingenconfig to addresses --------- Co-authored-by: Michael Moldoveanu <michael.moldoveanu@alchemy.com>
…lt token receiver plugin (#316) * feat: msca plugingen to accept multichain address format * feat: msca token receiver plugin with opt out option to exclude default token receiver plugin
* feat: msca plugingen to accept multichain address format * feat: msca token receiver plugin with opt out option to exclude default token receiver plugin * feat: msca token receiver plugin with opt out option to exclude default token receiver plugin * feat: add session key plugin abi and plugingen
Pull Request Checklist
yarn test
)site
folder, see guidleines below)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:fix
)development
and notmain
?PR-Codex overview
This PR focuses on adding plugin configuration and management functionality to the smart contract account system.
Detailed summary
extend
method for extending the account functionality.