[op-batcher] Change data availabilty kind at runtime with rpc #10837
Conversation
| @@ -81,6 +82,63 @@ func NewBatchSubmitter(setup DriverSetup) *BatchSubmitter { | |||
| } | |||
| } | |||
|
|
|||
| func (l *BatchSubmitter) SanityCheckConfigUpdate(daType flags.DataAvailabilityType, frameCount *uint) error { | |||
There was a problem hiding this comment.
nit: og prefix on variable names not consistent with the codebase
| return err | ||
| } | ||
| // cant fail, we already sanity checked it | ||
| a.b.ChangeDataAvailability(kind, frameCount) |
There was a problem hiding this comment.
Since SanityCheckConfigUpdate and ChangeDataAvailability are not coupled, it results in a future dev being able to call ChangeDataAvailability without calling sanity check. I think they should be coupled more tightly, ie sanity check is internal or the impl is directly inside change and change returns an error
There was a problem hiding this comment.
agree & initially I had ChangeDataAvailability return error if bad config but it would mean a potentially needless stopping of batch submitting.
| @@ -34,6 +38,36 @@ func GetAdminAPI(api *adminAPI) gethrpc.API { | |||
| } | |||
| } | |||
|
|
|||
| func (a *adminAPI) UseDAType( | |||
There was a problem hiding this comment.
I think this would be better if there was a getter + setter where the getter returns the whole config and the setter lets you set the whole config. This is to prevent the need to add more RPC endpoints for particular things. The tradeoff here is around a lot of the config being standard and there isn't a huge need to change it
There was a problem hiding this comment.
Ack - returning whole config in caused an import cycle
|
@sebastianst @axelKingsley wondering if I can get your input, anything explicitly wrong in logic? |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Closing the channel manager can result in extra batches dropped by op-node with an old timestamp but that is ignorable