Separating env variables in respective crates #373
Conversation
Deploying cw-orchestrator with
|
| Latest commit: |
dea2334
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://751a97c5.cw-orchestrator.pages.dev |
| Branch Preview URL: | https://update-separate-env-variable.cw-orchestrator.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Buckram123
left a comment
Buckram123
left a comment
There was a problem hiding this comment.
Separating them is nice
cw-orch-daemon/src/env.rs
Outdated
| impl DaemonEnvVars { | ||
| pub fn load() -> Result<Self, CwEnvError> { | ||
| let mut env_values = DaemonEnvVars::default(); | ||
|
|
There was a problem hiding this comment.
Not really sure why we load all of the env variables every time we use only one of them.
Options I see:
- load it once and store in https://doc.rust-lang.org/std/cell/struct.OnceCell.html after first load
- DaemonEnvVars::env_variable_name()? to only load one of them
There was a problem hiding this comment.
Agreed, would be nice to use cell here.
There was a problem hiding this comment.
Using OnceCell or lazy_static here means it's not possible to change those env variables in code, once it's initialized.
Because this is hidden from the user, unexpected behavior can happen which would be hard to debug for the user.
I would prefer the second solution !
There was a problem hiding this comment.
I wouldn't lazy static it but initialize it with the daemon state instead. That way the user can still execute code (and set envs) before this is loaded.
cw-orch-daemon/src/env.rs
Outdated
| pub const MIN_GAS_ENV_NAME: &str = "CW_ORCH_MIN_GAS"; | ||
| pub const MAX_TX_QUERIES_RETRY_ENV_NAME: &str = "CW_ORCH_MAX_TX_QUERY_RETRIES"; | ||
| pub const MIN_BLOCK_SPEED_ENV_NAME: &str = "CW_ORCH_MIN_BLOCK_SPEED"; | ||
| pub const DISABLE_WALLET_BALANCE_ASSERTION_ENV_NAME: &str = |
There was a problem hiding this comment.
Maybe a nit but are we defaulting to having the env-var named as disable and basically defaulting to setting env-vars to true if they want to enable something (so default all our bool env vars to false) or do we change it to the env-vars always indicating the feature and having the env var toggle them. I.e. rename these "disable" env vars to "wallet_balance_assertion", "log_activation", etc. and having them set to a default value.
This PR aims at separating env variables in their respective crates.
Closes ORC-120