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: add initial skeleton for 6900 account support #248

Merged
merged 12 commits into from
Jan 8, 2024
Merged

Conversation

moldy530
Copy link
Collaborator

@moldy530 moldy530 commented Nov 14, 2023

Pull Request Checklist

  • Did you add new tests and confirm existing tests pass? (yarn test)
  • Did you update relevant docs? (docs are found in the site folder, see guidleines below)
  • Do your commits follow the Conventional Commits standard?
  • Does your PR title also follow the Conventional Commits standard?
  • If you have a breaking change, is it [correctly reflected in your commit message](https://www.conventionalcommits.org/en/v1.0.0/#examples? (e.g. feat!: breaking change)
  • Did you run lint (yarn lint:check) and fix any issues? (yarn lint:fix)
  • Is the base branch you're merging into development and not main?

PR-Codex overview

This PR focuses on adding plugin configuration and management functionality to the smart contract account system.

Detailed summary

  • Added plugin configuration files for multi-owner, session key, and token receiver plugins.
  • Added plugin generation configurations for multi-owner, session key, and token receiver plugins.
  • Updated the provider schema to include connection configuration options.
  • Updated the account base class to include an extend method for extending the account functionality.
  • Added types for plugin hooks and injected hooks information.
  • Updated the wagmi.config.ts file to generate plugin files based on the plugin configurations.
  • Updated the Alchemy schema to include the connection configuration schema.
  • Updated the provider interface to include a generic type parameter for provider decorators.
  • Updated the uninstallPlugin function to include a generic type parameter for the smart account provider.
  • Added the IStandardExecutorAbi to the account base class.

The following files were skipped due to too many changes: packages/accounts/src/msca/plugin-manager/uninstallPlugin.ts, packages/accounts/src/msca/utils.ts, packages/core/src/provider/base.ts, packages/accounts/src/msca/account-loupe/decorator.ts, packages/accounts/src/msca/plugins/types.ts, packages/accounts/src/msca/plugin-manager/installPlugin.ts, packages/accounts/src/index.ts, examples/aa-simple-dapp/src/hooks/useAlchemyProvider.ts, packages/accounts/src/msca/abis/IAccountLoupe.ts, packages/accounts/src/msca/account-loupe/types.ts, packages/core/src/account/types.ts, packages/accounts/src/msca/abis/MultiOwnerMSCAFactory.ts, packages/accounts/src/msca/abis/IPluginManager.ts, packages/accounts/src/msca/abis/MultiOwnerTokenReceiverMSCAFactory.ts, packages/accounts/src/msca/multi-owner-account.ts, packages/accounts/src/msca/plugins/session-key/plugin.ts, packages/accounts/scripts/plugingen.ts, packages/accounts/src/msca/plugins/multi-owner/plugin.ts, packages/accounts/src/msca/builder.ts, packages/accounts/src/msca/plugins/token-receiver/plugin.ts, packages/accounts/src/msca/abis/IPlugin.ts, packages/accounts/plugindefs/token-receiver/abi.ts, packages/accounts/plugindefs/session-key/abi.ts, packages/accounts/plugindefs/multi-owner/abi.ts, yarn.lock

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@denniswon denniswon force-pushed the msca-base branch 2 times, most recently from 718db97 to 928a8eb Compare November 16, 2023 16:52
@moldy530 moldy530 force-pushed the msca-base branch 4 times, most recently from 6161184 to 55dfbdc Compare November 16, 2023 21:44

export interface Plugin<D> {
meta: { name: string; version: string };
accountDecorators: (a: BaseSmartContractAccount) => D;
Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Collaborator Author

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";
Copy link
Collaborator Author

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

Copy link
Contributor

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

Copy link
Collaborator Author

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

Comment on lines 58 to 86
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",
});
};
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/OMGWINNING/account-contracts/blob/master/src/plugins/owner/MultiOwnerPlugin.sol#L136

All signatures for 1271 validation in the MSCA (and LightAccount) require this wrapping

Comment on lines 44 to 60
.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]],
}),
]);
})
Copy link
Contributor

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?

Copy link
Collaborator Author

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),
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Comment on lines +31 to +52
| "signMessage"
| "signTypedData"
| "signUserOperationHash"
| "getDummySignature"
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Comment on lines +8 to +9
// TODO: need to make this configurable to run in CI without requiring this
rpcUrl?: string;
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Contributor

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?

Copy link
Collaborator Author

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

.vscode/settings.json Outdated Show resolved Hide resolved
Copy link
Contributor

@avasisht23 avasisht23 left a 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",
Copy link
Contributor

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.

Copy link
Collaborator Author

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;
Copy link
Contributor

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?

Copy link
Contributor

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?

Comment on lines +8 to +9
// TODO: need to make this configurable to run in CI without requiring this
rpcUrl?: string;
Copy link
Contributor

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) => ({
Copy link
Contributor

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?

Copy link
Collaborator Author

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

packages/accounts/scripts/plugingen.ts Outdated Show resolved Hide resolved
Copy link
Contributor

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?

Copy link
Collaborator Author

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

packages/accounts/src/msca/abis/IStandardExecutor.ts Outdated Show resolved Hide resolved
packages/accounts/src/msca/abis/MultiOwnerMSCAFactory.ts Outdated Show resolved Hide resolved
Copy link
Contributor

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?

Copy link
Collaborator Author

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

@avasisht23
Copy link
Contributor

avasisht23 commented Nov 27, 2023

moldy530 and others added 12 commits January 8, 2024 11:43
* feat: add some plugin generation logic

* chore: add generate commands

* style: apply PR suggestion

Co-authored-by: Dennis Won <[email protected]>

* 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 <[email protected]>
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: avasisht23 <[email protected]>
* 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 <[email protected]>

* Update packages/core/src/account/types.ts

Co-authored-by: Michael Moldoveanu <[email protected]>

* Update packages/core/src/account/types.ts

Co-authored-by: Michael Moldoveanu <[email protected]>

* Update packages/accounts/src/msca/account-loupe/decorator.ts

Co-authored-by: Michael Moldoveanu <[email protected]>

* fix: fix lint and build

---------

Co-authored-by: Michael Moldoveanu <[email protected]>
* feat: msca plugingen to accept multichain address format

* Update packages/accounts/scripts/plugingen.ts

Co-authored-by: Michael Moldoveanu <[email protected]>

* Update packages/accounts/wagmi.config.ts

Co-authored-by: Michael Moldoveanu <[email protected]>

* feat: changed address field in plugingenconfig to addresses

---------

Co-authored-by: Michael Moldoveanu <[email protected]>
…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
@moldy530 moldy530 merged commit eb842bf into main Jan 8, 2024
2 checks passed
@moldy530 moldy530 deleted the msca-base branch January 8, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants