-
Notifications
You must be signed in to change notification settings - Fork 36
Add a variation of the Safe{Wallet} Guard timelock for Safe{Wallet} version 1.4.1 #399
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
base: dcmt/upgrade-timelock
Are you sure you want to change the base?
Conversation
🛡️ Immunefi PR ReviewsWe’ve assigned 5 code reviewer(s) to this PR. They’ll begin the review shortly and leave feedback directly in the pull request. This review is based on the current state of your pull request. If you make changes after the review starts, they won’t be reflected here. To ensure the review includes your latest updates, you’ll need to open a new pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed updated code to be compatible with Safe 1.4. Looks good to me, did not find any major issues.
Left a few nice-to-have type comments
- OxStrausses
| } | ||
|
|
||
| function _checkAfterExecution(ISafeMinimal _safe) private { | ||
| function _checkAfterExecutionReturnBool(ISafeMinimal _safe) internal view returns (bool result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to add comment above this function, that this is rewritten _checkAfterExecution function, but that returns bool value instead of revert
- OxStrausses
| pragma solidity =0.8.25; | ||
|
|
||
| // This enum is derived from the code deployed to 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA | ||
| import {IERC165} from "@forge-std/interfaces/IERC165.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For contract that are going to be deployed better to import from OpenZeppelin.
- OxStrausses
No description provided.