refactor: approved relays, jobs and selectors #7
Conversation
…ork-core into refactor/relays-selectors
There was a problem hiding this comment.
Looking good. Some extra comments:
- A lot is being managed through constants. This may be prone to error when deploying to multiple chains. I would guess, but not entirely sure, that in different chains the jobs will pay not in
ETHfor example but in the other chain's native token.Keep3r_V2will be deployed in another address as well. So, perhaps having names such asETHand events that areETHReceivedmay not be 100% compatible with the idea of multi-chain deployment, as well as fixed addresses.
Consider using immutables to at least have the user think through what parameters he's inputting.
- The
GelatoRelayis powerful. I mean this in the sense that anyAutomationVaultis banking on the protocol's reputation to avoid them from calling a bunch of vaults withFeeData==vaultBalanceand drain them. Now, one thing we should be extremely careful about is howGelatoitself checks the parameters their own keepers/executors use.
A good question to answer is:
- What happens when a random user tells Gelato: "Hey I have put some money in
1Balanceplease callGelatoRelaywith these malicious parameters." Do we know what checks they perform? Do we know if they just obey and do it?
| } | ||
|
|
||
| unchecked { | ||
| ++_i; | ||
| } | ||
|
|
||
| // If the relay has no enabled callers, remove it from the list of relays | ||
| if (_relayCallers[_relay].length() == 0) { |
There was a problem hiding this comment.
This should be outside of the for loop, to avoid rechecking on each iteration.
| // Create the new automation vault with the owner | ||
| _automationVault = new AutomationVault(_owner); | ||
| _automationVault = new AutomationVault{salt: keccak256(abi.encodePacked(msg.sender, _salt))}(_owner); |
There was a problem hiding this comment.
This add a bit of extra gas cost, so it's optional but when a CREATE2 deployment fails, it returns address(0). You may want to add a check for it to avoid adding address(0) to the automation vaults mapping
There was a problem hiding this comment.
I didnt know nice one 👍
There was a problem hiding this comment.
@0xng are we sure that this happens?, im trying to test this situation and when the deployment fails reverts and not return address (0)
There was a problem hiding this comment.
@ashitakah interesting, you may be right, I got this idea from the evm.codes CREATE2 line that says:
Deployment can fail due to:
- A contract already exists at the destination address.
- Insufficient value to transfer.
- Sub context reverted.
- Insufficient gas to execute the initialisation code.
- Call depth limit reached.
- Note that these failures only affect the return value and do not cause the calling context to revert (unlike the error cases below).
But also tested one of these cases (contract already exists at the destination address), and it also reverts. So then we should be good without adding the check.
ashitakah
changed the title
| // Get the list of selectors | ||
| _selectors = _relayJobSelectors[_relay][_job].values(); |
There was a problem hiding this comment.
Should there be a method to get a relay's approved jobs?
There was a problem hiding this comment.
Should be in the front end part. Ardy can collect all jobs and check if the job is for that relay
| // Create the exec data needed variables | ||
| ExecData memory _execDatum; | ||
| ExecData memory _dataToExecute; |
There was a problem hiding this comment.
In views of normalization, I suggest naming this variable _dataInfo.
There was a problem hiding this comment.
Also, the naming was review'd
| abstract contract Deploy is Script { | ||
| abstract contract DeployNativeETH is Script { |
There was a problem hiding this comment.
If you deploy in L2 you can choose another native. I know that at the end of the day is the same, however I think that is better to specify that.
| function relays() external view returns (address[] memory __relays); | ||
| function relays() external view returns (address[] memory _listRelays); |
There was a problem hiding this comment.
_relaysList and _jobsList sounds better to me.
There was a problem hiding this comment.
I changed it because another comments told me that sounds better hahahha
No description provided.