Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move block rate and log limit into toml config #13700

Closed
wants to merge 19 commits into from

Conversation

ferglor
Copy link
Collaborator

@ferglor ferglor commented Jun 26, 2024

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 2
  • For all other supported chains, we specify a block rate of 1

Log limit

  • For arbitrum we specify a log limit of 1
  • 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 1

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 ferglor requested review from a team as code owners June 26, 2024 17:45
ogtownsend
ogtownsend previously approved these changes Jun 27, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also update for zksync and gnosis(xdai)?

Copy link
Contributor

@infiloop2 infiloop2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ferglor
Copy link
Collaborator Author

ferglor commented Jul 5, 2024

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?

@infiloop2
Copy link
Contributor

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?)

@ferglor
Copy link
Collaborator Author

ferglor commented Jul 5, 2024

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 ?

@anirudhwarrier
Copy link
Contributor

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.

@ferglor ferglor closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants