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: fix the manifest wording in the repo #209

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

nikita-quantstamp
Copy link
Contributor

We have some leftover wording from the old manifest setup in the code, so I removed that from the comment, as validation functions are no longer part of the manifest.

I was going to update the wording in the standard itself from manifest to execution manifest, as that more accurately reflects the current state of the manifest, but we use it interchangeably through the ERC, so I'm not too sure there. Should we commit to one of them? What do people think?

Copy link

Summary by Octane

New Contracts

No new contracts were added in this PR.

Updated Contracts

  • IModularAccount.sol: Removed validation against the manifest for installing validation functions.

🔗 Commit Hash: a2ada19

Copy link

Overview

Octane AI analysis has finished. No vulnerabilities were found. Cheers! 🎉🎉🎉


🔗 Commit Hash: a2ada19

@jaypaik
Copy link
Collaborator

jaypaik commented Dec 3, 2024

I was going to update the wording in the standard itself from manifest to execution manifest, as that more accurately reflects the current state of the manifest, but we use it interchangeably through the ERC, so I'm not too sure there. Should we commit to one of them? What do people think?

I think maybe in the installation/uninstallation section we can change "module manifest" to "module's manifest" or "module's execution manifest". But otherwise seems fine to leave other things be, and have it be implied that "manifest" refers to ExecutionManifest.

@jaypaik jaypaik requested a review from a team December 3, 2024 18:40
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

Happy to just keep the manifest wording as is. And with #210, we'll be removing the standard from the reference implementation, so we don't have to make that change here.

@jaypaik jaypaik merged commit 7284028 into develop Dec 5, 2024
4 checks passed
@jaypaik jaypaik deleted the 12-03-feat_fix_manifest_wording branch December 5, 2024 21:11
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