-
Notifications
You must be signed in to change notification settings - Fork 969
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
Create a separate interface for module guard #758
Conversation
Pull Request Test Coverage Report for Build 9223136085Details
💛 - Coveralls |
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.
Some nits.
Subjective: I like TxGuard
or TransactionGuard
better than just Guard
.
Co-authored-by: Shebin John <[email protected]>
…-contracts into feature/module-tx-guard
Co-authored-by: Shebin John <[email protected]>
Co-authored-by: Shebin John <[email protected]>
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.
LGTM. I'd still wait for Nick's or Shebin's approval since this is a highly complex and impactful addition.
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
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.
Looks great! One minor nit for comment, and more suggestions for the migration.
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
80e0265
to
ff20341
Compare
Thanks for approval! I will merge it this week after a final self-review as this is an important change in the account contract. |
d71ff3a
to
0ce9061
Compare
Fixes #755
Summary of changes:
The PR creates a separate interface for Module guards instead of having a single
Guard
interface for both module transactions and Safe transactions.The new Module guard interface i.e, IModuleGuard has two functions:
checkModuleTransaction
checkAfterExecution
The updated addresses in migration contracts are taken from logs from the tests.
Rename interface
Guard
toITransactionGuard
.Fix typo: Rename
ModuleTransasctionDetails
toModuleTransactionDetails
Codesize:
Main branch:
This PR (+571 bytes):
Changes in PR:
Open for discussion:
Guard
toITransactionGuard
? -> YessetGuard
function tosetTransactionGuard
? : Impacts Safe interfaceChangedGuard
event toChangedTransactionGuard
? Impacts services monitoring this event