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

chore: clean up unused code #207

Merged
merged 17 commits into from
Dec 9, 2024
Merged

chore: clean up unused code #207

merged 17 commits into from
Dec 9, 2024

Conversation

cpb8010
Copy link
Contributor

@cpb8010 cpb8010 commented Nov 27, 2024

Description

Removed the following:

  • R1 validation and keys were unreachable after changing the init interface
  • K1 validation was unused since it was validated directly without a module (simple signature instead of modular signature)
  • Fallback modules were only implemented in storage, not in the account.
  • The general module list wasn't used except for a general purpose auth check
  • Exec modules could have been used, but haven't been tested and aren't supported by the factory to be installed at init.
  • Call disable during validation module removal (matching hook removal)

Combined the following:

  • IModule and IModuleValidator (duplicated interfaces)
  • Init and addHook (duplicated)

Additional context

Less dead code means a faster audit, even if this code was mostly audited before it was incomplete in some places. Most of this was added in the direction 7579 compatibility or was leftover from Clave after the validation interface was made into a module.

This doesn't functionally change the factory interface, so we'll don't yet need to update the SDK

@cpb8010 cpb8010 added help wanted Extra attention is needed project: contracts labels Nov 27, 2024
@cpb8010 cpb8010 requested a review from ly0va November 27, 2024 07:20
@cpb8010 cpb8010 self-assigned this Nov 27, 2024
@cpb8010
Copy link
Contributor Author

cpb8010 commented Dec 3, 2024

I realize this interacts more with the validation interface cleanup than I expected. I'm going to try to reformat these so the cleanup is more of a setup for removing the validation hook interface (which is what's really required to drop lots of this stuff)

@cpb8010 cpb8010 force-pushed the cleanup-unused branch 2 times, most recently from 3f7a58e to 5c09a2e Compare December 4, 2024 15:27
@cpb8010 cpb8010 marked this pull request as ready for review December 4, 2024 15:33
R1 validation and keys were unreachable after changing the init
interface,
K1 validation was unused since it was validated directly without a
module (simple signature instead of modular signature)
Fallback modules were entirely unused.
Modules and exec modules could have been used, but were not and weren't
supported by the factory to be installed at init.
dropping unused arg
Moving to a single module to elimiate duplicate code
this was unused and too similar to the validator module
This is much more direct path to installing instead of using the install
to re-enter the account
removing only locally tests pass
This broke locally, but now fails in CI, maybe the converse is also true
This was recently renamed, and we're still on an older version
This wasn't necessary to pass in ci, but now fails locally :(
This is just a test setup issue combined with a era-test-node change
And finally R1 validator interfaces
@cpb8010
Copy link
Contributor Author

cpb8010 commented Dec 5, 2024

I got some good feedback on this outside of this PR, so I'll post my responses here for better visibility:

  • My previous remark about validation is because I'm entirely reworking the validation module interface to include the functionality of the validation hook (in this PR: Session Validation Module Interface Update #208. I have no intentions of adding functionally empty modules back.
  • OnlySelfOrModule is poorly named! There's already an onlySelf modifier, even more to consolidate!
  • disable() in WebAuthValidator should satisfy ERC165, but we don't actually have enough context to disable the module with just the sender address. I can make a note to either reject this or create a new storage layout so we can do this lookup. The code comment should include this context.
  • The SsoAccount.sol initialize now calls _installHook instead of routing this via the generic module's init function. This removes the need for modules to have permissions to add other modules or hooks. This is part of the setup for the next PR that entirely removes validation hooks in favor of a single validation module interface.
  • The tests pass the session key contract as a hook and as a validation module, so it only needs to be initialized once. We probably want to allow future hooks to have their init methods called, so can add that back so it at least gets called when doing the externally facing addHook

@cpb8010 cpb8010 marked this pull request as draft December 5, 2024 18:33
Easy rename after cleanup
The normal case appears to be no data, so the key is added elsewhere?
This matches the behaviour of the hook
@cpb8010 cpb8010 marked this pull request as ready for review December 6, 2024 09:50
Copy link
Member

@ly0va ly0va left a comment

Choose a reason for hiding this comment

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

looks good, but needs some fixes

currently if we want to uninstall the session key module, we have to uninstall it as both validator and as hook. if we do it in the wrong order, it will lock itself in a bad state (since we only check for isHook in isInitialized). So we have to fix isInitialized or even skip that check in uninstall entirely.

src/validators/WebAuthValidator.sol Outdated Show resolved Hide resolved
test/SessionKeyTest.ts Outdated Show resolved Hide resolved
src/validators/SessionKeyValidator.sol Outdated Show resolved Hide resolved
@ly0va
Copy link
Member

ly0va commented Dec 6, 2024

We probably want to allow future hooks to have their init methods called, so can add that back so it at least gets called when doing the externally facing addHook

yes we should probably let hooks get inited, but with our current interface adding more hooks is problematic, especially if they expect hookData, since we have to properly pass it for every call and we might not know what data they expect.

so it should be fine for now, we might even consider disabling adding hooks temporarily, after initialization

Updating disable to be more robust!
@ly0va ly0va merged commit 2eeb05a into main Dec 9, 2024
3 checks passed
@ly0va ly0va deleted the cleanup-unused branch December 9, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed project: contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants