pkg/settings/cresettings: add PerWorkflow.ChainAllowed#1727
pkg/settings/cresettings: add PerWorkflow.ChainAllowed#1727pavel-raykov merged 1 commit intomainfrom
Conversation
✅ API Diff Results - No breaking changes |
93495a6 to
115c3d4
Compare
115c3d4 to
09bd063
Compare
| "ConsensusCallsLimit": "2000", | ||
| "LogLineLimit": "1kb", | ||
| "LogEventLimit": "1000", | ||
| "ChainAllowed": { |
There was a problem hiding this comment.
I wonder if we care to name it something like AdditionalChain instead of ChainAllowed since the latter seems to imply that the empty value forbids them all, whereas it is just meant that "secret" chains are not allowed yet.
There was a problem hiding this comment.
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.
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.
In the deployments repo
There was a problem hiding this comment.
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.
| return s.open, nil | ||
| } | ||
|
|
||
| func (s *simpleGateLimiter) AllowErr(ctx context.Context) error { |
There was a problem hiding this comment.
why AllowErr and not just Allow ?
There was a problem hiding this comment.
Aligning with the other limiters. We could support both, but the error has metadata that we to expose
09bd063 to
5d28905
Compare
5d28905 to
12afb36
Compare
12afb36 to
b372ad9
Compare
f2067f2 to
863a664
Compare
| "Default": "false", | ||
| "Values": { | ||
| "12922642891491394802": "true", | ||
| "3379446385462418246": "true" |
There was a problem hiding this comment.
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.
| "ConsensusCallsLimit": "2000", | ||
| "LogLineLimit": "1kb", | ||
| "LogEventLimit": "1000", | ||
| "ChainAllowed": { |
There was a problem hiding this comment.
Why not make it a separate entry following the capability name like "evm"? Or anything outside "PerWorkflow"
There was a problem hiding this comment.
Well we have chain agnostic selectors rather than per-family chain IDs, and we want this to affect reads and writes. Being under PerWorkflow is what allows us to override and grant access at the workflow level.
863a664 to
7e5ac06
Compare
7e5ac06 to
6d890a3
Compare
https://smartcontract-it.atlassian.net/browse/PLEX-2121
This PR adds a
limits.GateLimiterto be used with a new cresettingPerWorkflow.ChainAllowed. This setting holds both the typical overridable value, as well as a map by chain selector. The latter allows chains to be made accessible via an override on the org, owner, or workflow ID. The former enabled (dis)allowing by default for a particular org, owner, or workflow.Supports: