chainlink icon indicating copy to clipboard operation
chainlink copied to clipboard

Move block rate and log limit into toml config

Open ferglor opened this issue 1 year ago • 2 comments

https://smartcontract-it.atlassian.net/browse/AUTO-10027

In this PR, we are:

  • Specifying defaults for the automation block rate and log limit in the toml config, based on the existing hard coded values here
  • Cleaning up the old hard coded defaults
  • Updating the oc2 config types in code to supply these new values

The configuration values are as follows:

Block rate

  • For arbitrum we specify a block rate of 4
  • For all other supported chains, we specify a block rate of 1

Log limit

  • For eth we specify a log limit of 20
  • For optimism, BSC, polygon, avax, and base we specify a log limit of 5
  • For all other supported chains, we specify a log limit of 2

Open questions

  • Not sure if all the chains I've added defaults for are still supported - is there a list of currently supported chains?

ferglor avatar Jun 26 '24 17:06 ferglor

We also have this config values through ocr offchain config which overrides these, not fully sure if these should within toml config also, will add to confusion.

one way to reduce confusion could be to name these as "FallbackBlockRate" and "FallbackLogLimit", but we should also think through why hardcoded values are not sufficient

I'm happy to keep the hardcoded values (means we can just close this PR!), but my understanding was that we should have default values per chain set in the toml config, is that not the case?

ferglor avatar Jul 05 '24 12:07 ferglor

keeping in toml might still be ok, but at least we should make it clear that these are only fallback values. I don't think we've had config yet which lives in both toml and offchain config yet so I would say discuss with @anirudhwarrier what would make sense here. (e.g. for new chains do we update toml or offchain config or both?)

infiloop2 avatar Jul 05 '24 13:07 infiloop2

keeping in toml might still be ok, but at least we should make it clear that these are only fallback values. I don't think we've had config yet which lives in both toml and offchain config yet so I would say discuss with @anirudhwarrier what would make sense here. (e.g. for new chains do we update toml or offchain config or both?)

Discussed with @anirudhwarrier and he's okay with scrapping this PR and relying on the defaults in code; is that okay with you @infiloop2 ?

ferglor avatar Jul 05 '24 15:07 ferglor

keeping in toml might still be ok, but at least we should make it clear that these are only fallback values. I don't think we've had config yet which lives in both toml and offchain config yet so I would say discuss with @anirudhwarrier what would make sense here. (e.g. for new chains do we update toml or offchain config or both?)

After reconsidering this, I think TOML config is overkill for a fallback config. I think this change should be discarded.

anirudhwarrier avatar Jul 05 '24 15:07 anirudhwarrier