Add moduleTypeId check to installModule/uninstallModule. #28
Conversation
src/MSABasic.sol
Outdated
| onlyEntryPointOrSelf | ||
| { | ||
|
|
||
| if (!IModule(module).isModuleType(moduleTypeId)) revert MismatchModuleTypeId(moduleTypeId); |
There was a problem hiding this comment.
maybe we should check if module passed supports interfaceId for IModule?
@kopy-kat
There was a problem hiding this comment.
isModuleType should be sufficient right?
There was a problem hiding this comment.
yes it is sufficient. we should write more tests though
kopy-kat
left a comment
kopy-kat
left a comment
There was a problem hiding this comment.
tysm @doublespending! left a couple of comments but this looks good
src/MSABasic.sol
Outdated
| onlyEntryPointOrSelf | ||
| { | ||
|
|
||
| if (!IModule(module).isModuleType(moduleTypeId)) revert MismatchModuleTypeId(moduleTypeId); |
There was a problem hiding this comment.
isModuleType should be sufficient right?
src/MSAAdvanced.sol
Outdated
| { | ||
| (address hook, bytes memory hookData) = _preCheck(); | ||
|
|
||
| if (!IModule(module).isModuleType(moduleTypeId)) revert MismatchModuleTypeId(moduleTypeId); |
There was a problem hiding this comment.
imo this is not needed since this check is already done on installation right?
There was a problem hiding this comment.
Yes, we can remove it.
There was a problem hiding this comment.
which methods are these?
There was a problem hiding this comment.
wdym? my point is that we already check moduletype onInstallation, but I dont think theres a need to check on uninstallation since it doesnt rly matter anyways (bc the module wont be used anymore)
There was a problem hiding this comment.
should be good imo, on uninstall if module address is wrong or if it doesn't match typeid it would revert with sentinel list or in module itself. will share if anything post writing more tests
src/MSABasic.sol
Outdated
| onlyEntryPointOrSelf | ||
| { | ||
|
|
||
| if (!IModule(module).isModuleType(moduleTypeId)) revert MismatchModuleTypeId(moduleTypeId); |
There was a problem hiding this comment.
Yes, we can remove it.
There was a problem hiding this comment.
which methods are these?
|
lmk what you think of the comments and we can merge if it looks good |
|
LGTM thanks ser |
3 participants
No description provided.