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: use interfaces from reference implementation and bump version #4

Conversation

jaypaik
Copy link
Contributor

@jaypaik jaypaik commented Sep 4, 2024

Removing the 6900 interfaces from this lib in favor of using the interfaces from https://github.com/erc6900/reference-implementation.

BasePlugin has also been deleted. We should use BaseModule from the reference implementation.

Also renaming this package to @erc6900/modular-account-libs (from modular-account-libs)

@jaypaik jaypaik requested a review from a team September 4, 2024 18:53
@jaypaik
Copy link
Contributor Author

jaypaik commented Sep 4, 2024

Hmm, I should update this to use PackedUserOperation. Update incoming.

@jaypaik
Copy link
Contributor Author

jaypaik commented Sep 4, 2024

@adamegyed why did we remove the explicit dependency on the 4337 ref impl again here: db6c47f ?

Comment on lines 10 to 11
// Magic value for hooks that should always revert.
ModuleEntity internal constant _PRE_HOOK_ALWAYS_DENY = ModuleEntity.wrap(bytes24(uint192(2)));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should re-copy this from the RI, because it's missing some parts like the byte layout diagram, and contains old fields like _PRE_HOOK_ALWAYS_DENY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, I did copy this from the reference implementation 😂 . So I guess that needs to be cleaned up?

Copy link
Contributor

Choose a reason for hiding this comment

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

oof, yeah we should clean that up. will get on it...

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamegyed
Copy link
Contributor

why did we remove the explicit dependency on the 4337 ref impl again here: db6c47f ?

When this repo gets added as a dependency to another repo, it may be added either as a git submodule (foundry) or as an NPM package. If this repo depends on any external imports, those imports are not necessarily going to be available to the user unless this repo uses the same import scheme as the user. Because the user may pick either of the two, this repo should have no dependencies, except those which are needed in tests.

@adamegyed
Copy link
Contributor

Also renaming this package to @erc6900/modular-account-libs (from modular-account-libs)

Could you verify that this still works with the github: form of adding NPM imports? I thought there was some dependency between the name in package.json and the name used in the dependency statement.

@jaypaik
Copy link
Contributor Author

jaypaik commented Sep 4, 2024

Also renaming this package to @erc6900/modular-account-libs (from modular-account-libs)

Could you verify that this still works with the github: form of adding NPM imports? I thought there was some dependency between the name in package.json and the name used in the dependency statement.

Yes this works! Already using this line in package.json (after erc6900/reference-implementation#181):

"@erc6900/reference-implementation": "github:erc6900/reference-implementation#v0.8.0-rc.1",

Will just need to update modular-account's package.json later.

@jaypaik
Copy link
Contributor Author

jaypaik commented Sep 4, 2024

When this repo gets added as a dependency to another repo, it may be added either as a git submodule (foundry) or as an NPM package. If this repo depends on any external imports, those imports are not necessarily going to be available to the user unless this repo uses the same import scheme as the user. Because the user may pick either of the two, this repo should have no dependencies, except those which are needed in tests.

We should expect the repo importing this package to import https://github.com/eth-infinitism/account-abstraction for those interfaces, right? Why would we expect them to use the interfaces that we provide here?

@adamegyed
Copy link
Contributor

We should expect the repo importing this package to import https://github.com/eth-infinitism/account-abstraction for those interfaces, right? Why would we expect them to use the interfaces that we provide here?

The previous state of this repo didn't have that dependency. The downside with requiring users to also install separate dependencies is that they must map things in the same way as this repo's code does. So for instance, this PR uses imports from "@erc6900/reference-implementation/...". If the user imports the RI but maps it differently, then this won't work, and they'll have to either map it twice, or change all of their code's mappings. Having no dependencies was my attempt at resolving this issue.

@jaypaik
Copy link
Contributor Author

jaypaik commented Sep 4, 2024

We should expect the repo importing this package to import https://github.com/eth-infinitism/account-abstraction for those interfaces, right? Why would we expect them to use the interfaces that we provide here?

The previous state of this repo didn't have that dependency. The downside with requiring users to also install separate dependencies is that they must map things in the same way as this repo's code does. So for instance, this PR uses imports from "@erc6900/reference-implementation/...". If the user imports the RI but maps it differently, then this won't work, and they'll have to either map it twice, or change all of their code's mappings. Having no dependencies was my attempt at resolving this issue.

Oof. Solidity dependencies really need better tooling. I imagine this is fine if importing as an npm package but causes problems when used as a submodule?

@adamegyed
Copy link
Contributor

I think it's equally a problem between the two different forms. But thinking a bit more about this, maybe we're ok with making it a requirement for users of this library to also install additional dependencies, and handle remappings. We should just call it out in the README.

@jaypaik jaypaik merged commit d64adb5 into develop Sep 5, 2024
3 checks passed
@jaypaik jaypaik deleted the 09-04-feat_use_interfaces_from_reference_implementation_and_bump_version branch September 5, 2024 15:30
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.

2 participants