-
Notifications
You must be signed in to change notification settings - Fork 24
Add ERC-7579 modules docs and EIP-7702 note in accounts docs #157
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
Conversation
Co-authored-by: Arr00 <[email protected]>
We should add the interfaces to the API section of the docs. |
Yeah, the issue with those is a bit more complex since they come from the vanilla repository. #30 |
Co-authored-by: Arr00 <[email protected]>
contracts/mocks/docs/account/modules/MyERC7579SocialRecovery.sol
Outdated
Show resolved
Hide resolved
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'm not sure where and exactly how, but I would like to briefly mention the benefits of implicitly using ERC-7913 building blocks on the Social Recovery example, since I think, and this is very opinionated, that it is a valuable feature that makes a compelling reason enough to choose this over other implementations, on top of the great composability and extensibility provided. Wdyt? @ernestognw
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 don't think standards should be sold. I mean that a good standard solves an issue cleanly, and I think this is the case. The recommendation should be "just use ERC-7913" because it's simply the best way to approach support for arbitrary authorization types.
Instead, maybe users would benefit from understanding how to bring ERC-7913 to their use cases. They should be guided about when to use each variant (SingerERC7913, MultisignerERC7913, ERC7579Multisig, ERC7913Utils, ERC7913RSAVerifier, ERC7913ZKVerifier, ERC7913P256Verifier and beyond)
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 get what you say. Is it that the reasons of why using an ERC7579Multisig aren't explained? That'd be true.
I just updated with a bit more context in 8980e0c
Co-authored-by: Arr00 <[email protected]>
Co-authored-by: Arr00 <[email protected]>
Co-authored-by: Gonzalo Othacehe <[email protected]>
contracts/mocks/docs/account/modules/MyERC7579DelayedSocialRecovery.sol
Outdated
Show resolved
Hide resolved
contracts/mocks/docs/account/modules/MyERC7579DelayedSocialRecovery.sol
Outdated
Show resolved
Hide resolved
Thanks for reviewing both! Merging since the previous change was minimal |
Co-authored-by: Arr00 <[email protected]> Co-authored-by: Gonzalo Othacehe <[email protected]>
Follow up to #121