-
Notifications
You must be signed in to change notification settings - Fork 111
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
Send a notification if a Reality Module with unsecured parameters is created #91
Send a notification if a Reality Module with unsecured parameters is created #91
Conversation
Tx: https://goerli.etherscan.io/tx/0x435453b42850e0b1b6a36d1086dc01e3a3f7b8afbccbab9052f37248315b84fe |
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.
Nice work!
The main thing is that we should be able to get the exact event that triggered the autotask (it can be more than one CreateModuleProxy event in one transaction).
In the current state, it will run on the first CreateModuleProxy event in every transaction, not necessarily the one creating a Reality Module proxy. And also, if there are multiple modules created in one tx, the autotask will trigger multiple times (as it should), but it will, for each trigger, look at the first CreateModuleProxy even (the same event each time it's triggered and also maybe not the event creating a Reality Module).
If it for some reason is not possible to get the exact event that triggered it, and we have to handle the whole transaction. Then we should do something like this:
const newRealityModuleProxies = logs
.filter((log) => log.topics != null) // only has logs
.filter((log) => log.topics[0] === keccak) // only ModuleProxyCreation events
.map((log) => {
const [proxy, mastercopy] = utils.defaultAbiCoder
.decode(["address", "address"], [log.topics[1], log.topics[2]])
.map((_) => _.toLowerCase());
return {
proxy,
mastercopy,
};
}) // decode the logs
.filter(({ mastercopy }) => mastercopy != masterCopyAddress.toLowerCase()) // only Modules using the masterCopyAddress
.map(({ proxy }) => proxy); // we only keep the proxy address
To pick out all relevant CreateModuleProxy events and handle all of them (even though this might result in multiple notifications in Discord. Multiple notifications for a bad setup are probably better than non.
But the best approve is clearly to be able to retrieve the exact event that triggered the Autotask (very strange if that is not possible).
not just the first in a tx
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.
Nice improvements :)
I added a couple of commits and added comments to explain the changes in the code.
I also added a few new review comments. Please, take a look.
Also, for the ENS checks, we can only do this on Gerli and Mainnet (like you say).
* @returns | ||
*/ | ||
const decode = async (logs, utils) => { | ||
const decode = async (logs, realityMastercopy, utils) => { |
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.
-
This is a very generic name. Could we change it to something more descriptive? For instance,
extractModuleProxyCreations
. -
We should also check that the logs are emitted from our ProxyFactory contract.
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 can't see that you have addressed any of these concerns
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
Create an autotask script that is going to add inside of the sentinel script
Closes #97.