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: track installed modules #138

Merged
merged 16 commits into from
Aug 15, 2024

Conversation

highskore
Copy link
Contributor

@highskore highskore commented Aug 9, 2024

  • adds listeners for ModuleInstalled(uint256, address) and ModuleUninstalled(uint256, address) events
  • when a module is installed it stores the installed module per account in a linked list using writeInstalledModule(), when a module is uninstalled it is removed using removeInstalledModule().
  • allows querying of all currently installed modules per account instance using getInstalledModules()
  • currently the tests are skipped for Kernel account type, because the Kernel implementation doesn't always emit an event when a module is installed

@highskore highskore marked this pull request as draft August 9, 2024 15:49
@highskore highskore marked this pull request as ready for review August 9, 2024 16:13
@highskore highskore marked this pull request as draft August 9, 2024 16:24
@highskore highskore marked this pull request as ready for review August 11, 2024 14:29
@highskore
Copy link
Contributor Author

highskore commented Aug 13, 2024

@kopy-kat looks like they made some changes to forge fmt recently which causes the lint ci to fail, not sure if it's a good idea to push the formatted repo changes within this pr (the forge fmt diff is pretty big)

@kopy-kat
Copy link
Member

Ah ok, good shout - lets leave it unformatted. And I will format in a separate PR. Should I already review this pr or are you still working on it?

@highskore
Copy link
Contributor Author

@kopy-kat Yeah should be ready for review, currently the tests are skipped for Kernel (we would have to update dependency to their dev branch for the tests to pass)

src/test/utils/ERC4337Helpers.sol Show resolved Hide resolved
src/test/utils/Storage.sol Show resolved Hide resolved
test/Diff.t.sol Outdated
Comment on lines 156 to 165
InstalledModule[] memory modules = instance.getInstalledModules();
AccountType env = ModuleKitHelpers.getAccountType();
assertTrue(modules.length == (env == AccountType.SAFE ? 3 : 2));
uint256 expectedIndex = env == AccountType.SAFE ? 1 : 0;
assertTrue(
modules[expectedIndex].moduleAddress == newValidator
&& modules[expectedIndex + 1].moduleAddress == newValidator1
&& modules[expectedIndex].moduleType == MODULE_TYPE_VALIDATOR
&& modules[expectedIndex + 1].moduleType == MODULE_TYPE_VALIDATOR
);
Copy link
Member

Choose a reason for hiding this comment

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

it looks like a lot of this code is reused a few times - could we add an internal func here to make it more readable?

Copy link
Contributor Author

@highskore highskore Aug 14, 2024

Choose a reason for hiding this comment

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

done in refactor(getModulesAndAssert): move logic to reusable function, lmk if you had a different approach in mind. I wanted to remove as much lines as possible from the actuals tests so I had to abi.encode the params because you can't inline pass memory arrays

@kopy-kat
Copy link
Member

this looks good to me, thanks! could you please merge from main and fix the conflicts before we merge

@kopy-kat kopy-kat merged commit d2ed91a into rhinestonewtf:main Aug 15, 2024
3 of 6 checks passed
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