Conversation
|
|
||
| EnvConfig memory config = Env.load(vm.envString("NETWORK")); | ||
|
|
||
| GasService gasService = new GasService(config.network.buildBatchLimits()); |
There was a problem hiding this comment.
Shouldn't the new GasService also be included in the env JSON?
There was a problem hiding this comment.
Yeah! Good one, I'll add as part of the script
|
@lemunozm one note: I think we should actually move |
|
I agree 👍🏻. Also, from the last iteration regarding this, now we have 2 interfaces Is it ok if we move this to |
Seems fine yes! |
lemunozm
force-pushed
the
swappable-gas-service
branch
from
028e478 to
1df5d5c
Compare
wischli
left a comment
wischli
left a comment
There was a problem hiding this comment.
Just a few nits and one raised concern that I wanted to discuss but doesn't require any codechange probably.
|
|
||
| // Rely opsGuardian | ||
| report.multiAdapter.rely(address(report.opsGuardian)); | ||
| report.gateway.rely(address(report.opsGuardian)); |
There was a problem hiding this comment.
I am not sure if we should give the OpsGuardian a Gateway ward as it allows for updating the message processor or replacing adapters or even calling handle directly to bypass multi-adapter quorum for inbound messages. So I feel like this is in conflict with our trust model. Though, IIUC, this would only be possible in a future version of the OpsGuardian which would support allow it to forward arbitrary calls which it currently cannot?!
There was a problem hiding this comment.
This is a very good catch! Definitely a security issue imo.
There was a problem hiding this comment.
Note that the ward is from the OpsGuardian, not OpsSafe. The OpsGuardian can only do what is coded on it. No one can call handle() or bypass communication actions.
There was a problem hiding this comment.
Yeah you are right, I was misreading it, this is okay. We just need to be cautious with what we add to OpsGuardian, but that is anyways important.
| emit File(what, data); | ||
| } | ||
|
|
||
| function setGasService(IGasService gasService) external onlySafe { |
There was a problem hiding this comment.
Nits:
- I think we should an event here such in case it matters for the SDK/Apps.
- Missing UTs for this setter
- Missing
@inheritdocheader
There was a problem hiding this comment.
The events are already in the file() methods called by this method, not sure if we want to add another event for this 🤔, in the same way we dont add an event for createPool or init adapters.
Good point with the UTs and documentation and the rest of the NITs!
|
|
||
| /// @notice Updates the gas service | ||
| /// @param gasService the new gas service to use | ||
| function setGasService(IGasService gasService) external; |
There was a problem hiding this comment.
Nit: We should add a gateway view ABI to this interface
| @@ -39,6 +40,10 @@ interface IOpsGuardian { | |||
There was a problem hiding this comment.
Nit: Needs to be updated for the new gateway addition
github-actions
bot
added
audit:needed
|
Auto-labeled: |
|
Coverage after merging swappable-gas-service into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
4 participants
Product requirements
Design notes
Solution A (failed).
I initially allow direct modification to GasService values using OpsAdmin making GasService mutable. But this comes with some issues:
Solution B
script/deploy-gas-service.shto push a safe OpsAdmin propostion to all changes to acept this new GasService.