-
Notifications
You must be signed in to change notification settings - Fork 25
pkg/settings/cresettings: add PerWorkflow.ChainAllowed #1727
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package contexts | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| ) | ||
|
|
||
| const chainSelectorCtxKey key = "chainSelectorCtx" | ||
|
|
||
| // WithChainSelector returns a new context that includes the chain selector. | ||
| // Use ChainSelectorValue to get the value. | ||
| func WithChainSelector(ctx context.Context, cs uint64) context.Context { | ||
| return context.WithValue(ctx, chainSelectorCtxKey, cs) | ||
| } | ||
|
|
||
| // ChainSelectorValue returns the chain selector, if one was set via WithChainSelector. | ||
| func ChainSelectorValue(ctx context.Context) (uint64, error) { | ||
| val := Value[uint64](ctx, chainSelectorCtxKey) | ||
| if val == 0 { | ||
| return 0, errors.New("context missing chain selector") | ||
| } | ||
| return val, nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,13 @@ | |
| "ConsensusCallsLimit": "2000", | ||
| "LogLineLimit": "1kb", | ||
| "LogEventLimit": "1000", | ||
| "ChainAllowed": { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not make it a separate entry following the capability name like "evm"? Or anything outside "PerWorkflow"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well we have chain agnostic selectors rather than per-family chain IDs, and we want this to affect reads and writes. Being under |
||
| "Default": "false", | ||
| "Values": { | ||
| "12922642891491394802": "true", | ||
| "3379446385462418246": "true" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why only these two?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could include more chains, but it would be assuming implementation details about the deployed system, and it can't serve as the source of truth because we need to be able to roll out new chains without making code changes here. I originally had it empty, but we use these dev/testnets (EVM chain ID 1337 and 2337) in tests, so including them seemed worthwhile. |
||
| } | ||
| }, | ||
| "CRONTrigger": { | ||
| "FastestScheduleInterval": "30s", | ||
| "RateLimit": "every30s:1" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,13 @@ ConsensusCallsLimit = '2000' | |
| LogLineLimit = '1kb' | ||
| LogEventLimit = '1000' | ||
|
|
||
| [PerWorkflow.ChainAllowed] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't it cause downtime until we push new jobs with limits allowing eth and other chains?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll update the settings before updating the nodes: https://github.com/smartcontractkit/chainlink-deployments/pull/8514 |
||
| Default = 'false' | ||
|
|
||
| [PerWorkflow.ChainAllowed.Values] | ||
| 12922642891491394802 = 'true' | ||
| 3379446385462418246 = 'true' | ||
|
|
||
| [PerWorkflow.CRONTrigger] | ||
| FastestScheduleInterval = '30s' | ||
| RateLimit = 'every30s:1' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we care to name it something like
AdditionalChaininstead ofChainAllowedsince the latter seems to imply that the empty value forbids them all, whereas it is just meant that "secret" chains are not allowed yet.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty does forbid them all. We don't have any pre-existing notion of public or private to build on - we just have the chain selector ID.
We could include a default set of public chains here, but I was concerned that is an implementation detail, and regardless it will evolve over time and different between environments and zones, so it can be stale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, so how one will keep it up to date - will we have a file with all chainIDs somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the deployments repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, so how deployments repo is better than a set of public chains here? Is it because the deployments repo is a "source of truth" and is better maintained? Also, currently where would such list of public chainIDs be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/smartcontractkit/chainlink-deployments/pull/8514