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: update hook interface with tests #258

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: update hook interface with tests #258

wants to merge 2 commits into from

Conversation

cpb8010
Copy link
Contributor

@cpb8010 cpb8010 commented Jan 28, 2025

WIP: still failing some tests

Description

Create sample hooks to test
Create tests to cover hook manager interface
Show combined hook format

Additional context

So because hooks are global and unskippable it's pretty trivial to brick an account with a bad hook (even by accident).

Would love for some safety protections here in the future (like with a trusted module registry).

@cpb8010 cpb8010 self-assigned this Jan 28, 2025
@cpb8010 cpb8010 added documentation Improvements or additions to documentation invalid This doesn't seem right project: contracts labels Jan 28, 2025
Removes the need for an extra bool flag.
Still broken on some tests
Still not sure why some fail
@jcsec-security
Copy link
Collaborator

Do we plan on merging some of these tests with the current ones or just discard the PR? @cpb8010

@cpb8010
Copy link
Contributor Author

cpb8010 commented Feb 5, 2025

I would like to revisit this eventually with Lev since it has some breaking changes from his tests.

The goal with this was to better support hooks that can act as both executor, validation, and as a validation module without complicating the install and remove process too much (using ERC165 instead of a flag)

I don't know how important this is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation invalid This doesn't seem right project: contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants