pluginconfigs: axe unnecessary config types #70
Conversation
| MaxReportTransmissionCheckAttempts uint `json:"maxReportTransmissionCheckAttempts"` | ||
|
|
||
| // SyncTimeout is the timeout for syncing the commit plugin reader. | ||
| SyncTimeout time.Duration `json:"syncTimeout"` |
There was a problem hiding this comment.
If we don't use defaults for these let's add them validation for them.
There was a problem hiding this comment.
we have multiple readers and better clarify here.
There was a problem hiding this comment.
We have defaults for this that are applied in the plugin constructors. I'm wondering if there should be a standard place where defaults are applied. In the plugin constructor seems fine to me at the moment though.
| // The maximum number of times to check if the previous report has been transmitted | ||
| MaxReportTransmissionCheckAttempts uint `json:"maxReportTransmissionCheckAttempts"` | ||
|
|
||
| // SyncTimeout is the timeout for syncing the commit plugin reader. |
There was a problem hiding this comment.
| // SyncTimeout is the timeout for syncing the commit plugin reader. | |
| // SyncTimeout is the timeout for syncing the commit plugin ccip reader. |
| // SyncTimeout is the timeout for syncing the commit plugin reader. | ||
| SyncTimeout time.Duration `json:"syncTimeout"` | ||
|
|
||
| // SyncFrequency is the frequency at which the commit plugin reader should sync. |
There was a problem hiding this comment.
| // SyncFrequency is the frequency at which the commit plugin reader should sync. | |
| // SyncFrequency is the frequency at which the commit plugin ccip reader should sync. |
| PriceSources: map[types.Account]ArbitrumPriceSource{}, | ||
| }, | ||
| true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Add tests for sync durations and timeouts?
There was a problem hiding this comment.
Sync duration and timeout have defaults that are applied in the plugin constructors, so I think its OK if they're zero.
Although there could be an argument for not having these defaults since its less visible...
There was a problem hiding this comment.
I think it's better to be enforced through config. Better for visibility and understanding the important parts when reading the config.
There was a problem hiding this comment.
Then will rip out the defaults and enforce that they're specified in the offchain config
Test Coverage
|
|
Will re-implement in another branch |
What
Axe unnecessary config types.
Why
They're confusing and unnecessary
Fixed tests here: smartcontractkit/ccip#1325