Only allow enabling/disabling recovery alerts for modules of given Safe #1267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fmrsabino
suggested changes
Mar 11, 2024
Comment on lines
73
to
83
| try { | ||
| const { safes } = await this.safeRepository.getSafesByModule({ | ||
| chainId, | ||
| moduleAddress, | ||
| }); | ||
|
|
||
| return safes.some((safe) => getAddress(safe) === getAddress(safeAddress)); | ||
| } catch (e) { | ||
| this.loggingService.debug(e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We can merge the try/catch block with the one above. Alternatively you can extract this part to a new Guard in favour of composability (e.g.: SafeHasModuleGuard).
However for the context of this PR and since we only perform this check here I would say that we can merge the try/catch block first.
fmrsabino
suggested changes
Mar 12, 2024
Comment on lines
73
to
82
| try { | ||
| const { safes } = await this.safeRepository.getSafesByModule({ | ||
| chainId, | ||
| moduleAddress, | ||
| }); | ||
|
|
||
| return safes.some((safe) => getAddress(safe) === getAddress(safeAddress)); | ||
| } catch (e) { | ||
| this.loggingService.debug(e); | ||
| return false; |
There was a problem hiding this comment.
Same here 🙏
fmrsabino
approved these changes
Mar 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We currently prevent the user from enabling/disabling recovery alerts unless they submit a valid signature. This does, not, however, mean that the module is tied to the specified Safe.
This includes a check that the specified
moduleAddressis enabled on thatsafeAddress.Changes
moduleAddressis enabled onsafeAddressinEnableRecoveryAlertsGuard/DisableRecoveryAlertsGuard