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

feat: Extend support for env-based configuration #631

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ pub struct Args {
}

pub fn get_config(args: Args) -> eyre::Result<Config> {
Ok(if let Some(path) = args.config.as_ref() {
Config::from_file(path)?
if let Some(path) = args.config.as_ref() {
Config::from_file(path)
} else {
Config::from_env()
sergey-melnychuk marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
128 changes: 108 additions & 20 deletions crates/core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::path::PathBuf;
use std::str::FromStr;

use ethers::types::Address;
use eyre::{eyre, Result};
use eyre::{eyre, Context, Result};
use helios::client::{Client, ClientBuilder};
use helios::config::checkpoints;
use helios::config::networks::Network;
Expand Down Expand Up @@ -84,25 +84,37 @@ impl Default for Config {
}

impl Config {
pub fn from_env() -> Self {
Self {
network: Network::from_str(
&std::env::var("NETWORK").unwrap_or_default(),
)
.unwrap_or(Network::MAINNET),
eth_execution_rpc: std::env::var("ETH_EXECUTION_RPC")
.unwrap_or_default(),
starknet_rpc: std::env::var("STARKNET_RPC").unwrap_or_default(),
data_dir: PathBuf::from(
std::env::var("DATA_DIR").unwrap_or_default(),
),
poll_secs: u64::from_str(
&std::env::var("POLL_SECS").unwrap_or_default(),
)
.unwrap_or(DEFAULT_POLL_SECS),
rpc_addr: rpc_addr(),
fee_token_addr: fee_token_addr(),
}
pub fn from_env() -> Result<Self> {
Self::from_vars(|key| std::env::var(key).ok())
}

fn from_vars<F>(get: F) -> Result<Self>
where
F: Fn(&'static str) -> Option<String> + 'static,
{
let require = |var_key: &'static str| {
get(var_key).ok_or_else(|| eyre!("The \"{}\" env var must be set or a configuration file must be specified", var_key))
};

Ok(Self {
network: Network::from_str(&get("NETWORK").unwrap_or_default())
Copy link
Contributor

Choose a reason for hiding this comment

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

if the provided network is invalid, then mainnet is used as a default which might not be always desired behavior; I think an error must be returned when provided network is invalid - with respective test case.

.unwrap_or(Network::MAINNET),
eth_execution_rpc: require("ETH_EXECUTION_RPC")?,
starknet_rpc: require("STARKNET_RPC")?,
data_dir: PathBuf::from(get("DATA_DIR").unwrap_or_default()),
poll_secs: u64::from_str(&get("POLL_SECS").unwrap_or_default())
Copy link
Contributor

@sergey-melnychuk sergey-melnychuk Apr 19, 2024

Choose a reason for hiding this comment

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

if provided poll seconds value is invalid also the default is used - probably safer to return an error; will also require one more test case for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the original behavior. It's out of scope of this PR. It doesn't help with with my original task. It would be another breaking change.

.unwrap_or(DEFAULT_POLL_SECS),
rpc_addr: match get("RPC_ADDR") {
Some(addr) => SocketAddr::from_str(&addr)
.context("Invalid value for `RPC_ADDR`")?,
None => rpc_addr(),
},
fee_token_addr: match get("FEE_TOKEN_ADDR") {
Some(addr) => FieldElement::from_hex_be(&addr)
.context("Invalid value for `FEE_TOKEN_ADDR`")?,
None => fee_token_addr(),
},
})
}

pub fn from_file(path: &str) -> Result<Self> {
Expand Down Expand Up @@ -201,3 +213,79 @@ impl Config {
.expect("incorrect helios client config")
}
}

#[cfg(test)]
mod tests {
use std::collections::HashMap;

use super::*;

fn case(vars: &[(&'static str, &'static str)]) -> Result<Config> {
let vars: HashMap<&str, &str> = HashMap::from_iter(vars.to_vec());
Config::from_vars(move |s| vars.get(s).map(|s| s.to_string()))
}

static MIN_CONFIG_VARS: &[(&'static str, &'static str)] =
&[("ETH_EXECUTION_RPC", "url"), ("STARKNET_RPC", "url")];

#[test]
fn test_min_config_requirements() {
assert!(case(&[("ETH_EXECUTION_RPC", "url"),]).is_err());
assert!(case(&[("STARKNET_RPC", "url"),]).is_err());

assert!(case(&MIN_CONFIG_VARS).is_ok());
}

#[test]
fn test_unspecified_network_is_mainnet() {
let config = case(&MIN_CONFIG_VARS).unwrap();
assert_eq!(config.network, Network::MAINNET);
}

#[test]
fn test_rpc_address_is_validated() {
let result = case(&[
("ETH_EXECUTION_RPC", "url"),
("STARKNET_RPC", "url"),
("RPC_ADDR", "invalid_value"),
]);
assert!(result.is_err());

let result = case(&[
("ETH_EXECUTION_RPC", "url"),
("STARKNET_RPC", "url"),
("RPC_ADDR", "127.0.0.1:3333"),
]);
assert!(result.is_ok());
assert_eq!(
SocketAddr::from_str("127.0.0.1:3333").unwrap(),
result.unwrap().rpc_addr
);

// Default test case
let config = case(&MIN_CONFIG_VARS).unwrap();
assert_eq!(config.rpc_addr, rpc_addr());
}

#[test]
fn test_fee_token_addr_is_validated() {
let result = case(&[
("ETH_EXECUTION_RPC", "url"),
("STARKNET_RPC", "url"),
("FEE_TOKEN_ADDR", "invalid_value"),
]);
assert!(result.is_err());

let result = case(&[
("ETH_EXECUTION_RPC", "url"),
("STARKNET_RPC", "url"),
("FEE_TOKEN_ADDR", "1"),
]);
assert!(result.is_ok());
assert_eq!(FieldElement::ONE, result.unwrap().fee_token_addr);

// Default test case
let config = case(&MIN_CONFIG_VARS).unwrap();
assert_eq!(config.fee_token_addr, fee_token_addr());
}
}
Loading