feat: add config with papyrus_config

[lev-starkware]

This change is 

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @lev-starkware)

---

crates/gateway/src/gateway_test.rs line 38 at r1 (raw file):

const DEFAULT_BAD_CONFIG_PATH: &str = "./src/json_files_for_testing/bad_gateway_config.json";
const DEFAULT_BAD_ADDRESS_CONFIG_PATH: &str =
    "./src/json_files_for_testing/bad_gateway_address_config.json";

Please:

1. Define the path to the directory as a constant; use it to define the path to the specific files.
2. I personally don't like the names "good" and "bad". I prefer the "good" files will be just the file name, and the "bad" ones will be "erroneous". If you disagree let's discuss. If not, please reflect that in the constant names as well.

Code quote:

const DEFAULT_GOOD_CONFIG_PATH: &str = "./src/json_files_for_testing/good_gateway_config.json";
const DEFAULT_BAD_CONFIG_PATH: &str = "./src/json_files_for_testing/bad_gateway_config.json";
const DEFAULT_BAD_ADDRESS_CONFIG_PATH: &str =
    "./src/json_files_for_testing/bad_gateway_address_config.json";

---

crates/gateway/src/gateway_test.rs line 69 at r1 (raw file):

    let config_file = std::fs::File::open(Path::new(DEFAULT_BAD_ADDRESS_CONFIG_PATH)).unwrap();
    let load_config =
        load_and_process_config::<GatewayConfig>(config_file, Command::new(""), vec![]).unwrap();

Can these two be unified into a single function that takes the config name and returns its loaded value, that is, as a test utils func?
Furthermore, do we want such a config-name-to-loaded-config function as an actual (not test) util?

Code quote:

    let config_file = std::fs::File::open(Path::new(DEFAULT_BAD_ADDRESS_CONFIG_PATH)).unwrap();
    let load_config =
        load_and_process_config::<GatewayConfig>(config_file, Command::new(""), vec![]).unwrap();

---

crates/gateway/src/gateway_test.rs line 77 at r1 (raw file):

        ),
    }
}

Please put these in a different test file, i.e., "gateway_config_test.rs".

Code quote:

#[test]
fn good_config_test() {
    // Read the good config file and validate it.
    let config = GatewayConfig::default();

    let config_file = File::open(Path::new(DEFAULT_GOOD_CONFIG_PATH)).unwrap();
    let load_config =
        load_and_process_config::<GatewayConfig>(config_file, Command::new(""), vec![]).unwrap();
    assert!(load_config.validate().is_ok());
    assert_eq!(load_config.bind_address, config.bind_address);
}

#[test]
fn bad_config_test() {
    // Read the config file with the bad field path and validate it.
    let config_file = std::fs::File::open(Path::new(DEFAULT_BAD_CONFIG_PATH)).unwrap();
    let load_config =
        load_and_process_config::<GatewayConfig>(config_file, Command::new(""), vec![]);
    match load_config {
        Ok(_) => panic!("Expected an error, but got a config."),
        Err(e) => assert_eq!(e.to_string(), "missing field `bind_address`".to_owned()),
    }
}

#[test]
fn bad_config_address_test() {
    // Read the config file with the bad bind_address values and validate it
    let config_file = std::fs::File::open(Path::new(DEFAULT_BAD_ADDRESS_CONFIG_PATH)).unwrap();
    let load_config =
        load_and_process_config::<GatewayConfig>(config_file, Command::new(""), vec![]).unwrap();
    match load_config.validate() {
        Ok(_) => panic!("Expected an error, but got a config."),
        Err(e) => assert_eq!(
            e.field_errors()["bind_address"][0].code,
            "Invalid Socket address.".to_owned()
        ),
    }
}

---

crates/gateway/src/lib.rs line 4 at r1 (raw file):

pub mod gateway;

use serde::{Deserialize, Serialize};

Is this needed?

---

crates/gateway/src/lib.rs line 13 at r1 (raw file):

use validator::{Validate, ValidationError};

/// Configuration for a gateway.

Rephrase to "Gateway configuration"

[lev-starkware]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/gateway/src/gateway_test.rs line 38 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please:

1. Define the path to the directory as a constant; use it to define the path to the specific files.
2. I personally don't like the names "good" and "bad". I prefer the "good" files will be just the file name, and the "bad" ones will be "erroneous". If you disagree let's discuss. If not, please reflect that in the constant names as well.

1. OK
2. Let's talk

---

crates/gateway/src/gateway_test.rs line 69 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can these two be unified into a single function that takes the config name and returns its loaded value, that is, as a test utils func?
Furthermore, do we want such a config-name-to-loaded-config function as an actual (not test) util?

OK

---

crates/gateway/src/gateway_test.rs line 77 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please put these in a different test file, i.e., "gateway_config_test.rs".

OK

---

crates/gateway/src/lib.rs line 4 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this needed?

Yes, it's needed. Look at the "#derive" for GatewayConfig.

---

crates/gateway/src/lib.rs line 13 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Rephrase to "Gateway configuration"

In papyrus:
"/// Configuration for a memory mapped file."

If it's good for them why it's not good for us?

[Itay-Tsabary-Starkware]

Reviewed 7 of 9 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @lev-starkware)

---

crates/gateway/src/config_test.rs line 21 at r2 (raw file):

fn good_config_test() {
    // Read the good config file and validate it.
    let config = GatewayConfig::default();

Required? Can/should this be in a different "validate_default_config_test"?

Code quote:

let config = GatewayConfig::default();

---

crates/gateway/src/lib.rs line 13 at r1 (raw file):

Previously, lev-starkware wrote…

In papyrus:
"/// Configuration for a memory mapped file."

If it's good for them why it's not good for us?

Writing concisely is a virtue.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 38.33%. Comparing base (81ae9f4) to head (7cbcaeb).

Files	Patch %	Lines
crates/gateway/src/lib.rs	40.90%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   36.84%   38.33%   +1.49%     
==========================================
  Files           4        5       +1     
  Lines          38       60      +22     
  Branches       38       60      +22     
==========================================
+ Hits           14       23       +9     
- Misses         24       37      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lev-starkware]

Reviewable status: 4 of 10 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

crates/gateway/src/config_test.rs line 21 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Required? Can/should this be in a different "validate_default_config_test"?

Changed

---

crates/gateway/src/lib.rs line 13 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Writing concisely is a virtue.

Changed to " // The gateway configuration."

---

crates/gateway/src/gateway_test.rs line 38 at r1 (raw file):

Previously, lev-starkware wrote…

1. OK
2. Let's talk

2. Done

[Itay-Tsabary-Starkware]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved

[dafnamatsry]

Reviewed 1 of 9 files at r1, 1 of 3 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lev-starkware)

---

crates/gateway/src/config_test.rs line 21 at r3 (raw file):

fn test_valid_config() {
    // Read the valid config file and validate its content.
    let expected_config = GatewayConfig {

What is the purpose of having the valid config duplicated here and in the test file?

---

crates/gateway/src/config_test.rs line 24 at r3 (raw file):

        bind_address: String::from("0.0.0.0:8080"),
    };
    let load_config = get_config_file(VALID_CONFIG_FILE).unwrap();

config

Code quote:

load_config

---

crates/gateway/src/config_test.rs line 40 at r3 (raw file):

#[test]
fn test_config_with_invalid_address() {

I think all the invalid test cases should programmatically create an invalid GatewayConfig, instead of having a bad config file for each case.
Something like:

let mut config = get_config_file(VALID_CONFIG_FILE).unwrap();
config.bind_address = "bad_address"

...

---

crates/gateway/src/lib.rs line 19 at r3 (raw file):

#[derive(Clone, Debug, Serialize, Deserialize, Validate, PartialEq)]
pub struct GatewayConfig {
    /// The gateway bind address.

Suggestion:

/// The gateway socket address, in the form of "IP:port".

---

crates/gateway/src/json_files_for_testing/gateway_config.json line 1 at r3 (raw file):

{

Maybe rename the folder to test_config_files?

[lev-starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dafnamatsry)

---

crates/gateway/src/config_test.rs line 21 at r3 (raw file):

Previously, dafnamatsry wrote…

What is the purpose of having the valid config duplicated here and in the test file?

The purpose is to compare the whole structures. I changed the assert_eq line.

---

crates/gateway/src/config_test.rs line 24 at r3 (raw file):

Previously, dafnamatsry wrote…

config

Changed to loaded_config.

---

crates/gateway/src/config_test.rs line 40 at r3 (raw file):

Previously, dafnamatsry wrote…

I think all the invalid test cases should programmatically create an invalid GatewayConfig, instead of having a bad config file for each case.
Something like:

let mut config = get_config_file(VALID_CONFIG_FILE).unwrap();
config.bind_address = "bad_address"

...

Done

---

crates/gateway/src/lib.rs line 19 at r3 (raw file):

#[derive(Clone, Debug, Serialize, Deserialize, Validate, PartialEq)]
pub struct GatewayConfig {
    /// The gateway bind address.

Done

---

crates/gateway/src/json_files_for_testing/gateway_config.json line 1 at r3 (raw file):

Previously, dafnamatsry wrote…

Maybe rename the folder to test_config_files?

I used the already existing folder. Only added file to it.

[Itay-Tsabary-Starkware]

Reviewed 2 of 7 files at r4, all commit messages.
Reviewable status: 5 of 10 files reviewed, 7 unresolved discussions (waiting on @dafnamatsry and @lev-starkware)

---

crates/gateway/src/config_test.rs line 30 at r4 (raw file):

#[test]
fn test_address_validator() {
    // Read the vqalid config file and check that the validator finds no errors.

typo

Code quote:

vqalid

---

crates/gateway/src/config_test.rs line 35 at r4 (raw file):

    if let Err(e) = config.validate() {
        panic!("Unexpected error {e}.");
    }

Please add a comment that this validates the loaded config.

Code quote:

    if let Err(e) = config.validate() {
        panic!("Unexpected error {e}.");
    }

[lev-starkware]

Reviewable status: 5 of 10 files reviewed, 6 unresolved discussions (waiting on @dafnamatsry and @Itay-Tsabary-Starkware)

---

crates/gateway/src/config_test.rs line 30 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

typo

Done.

---

crates/gateway/src/config_test.rs line 35 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please add a comment that this validates the loaded config.

Already in the comment, But removed the empty line between 'let mut' and 'if'.

[Itay-Tsabary-Starkware]

Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry)

[dafnamatsry]

Reviewed 7 of 7 files at r4, 1 of 2 files at r5.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware and @lev-starkware)

---

crates/gateway/src/config_test.rs line 35 at r4 (raw file):

    if let Err(e) = config.validate() {
        panic!("Unexpected error {e}.");
    }

assert!(config.validate().is_ok());

Code quote:

    if let Err(e) = config.validate() {
        panic!("Unexpected error {e}.");
    }

---

crates/gateway/src/config_test.rs line 46 at r4 (raw file):

            "Invalid Socket address.".to_owned()
        ),
    }

assert_matches!(config.validate(), Err(ValidationError(...))

Code quote:

    match config.validate() {
        Ok(_) => panic!("Expected an error, but got a config."),
        Err(e) => assert_eq!(
            e.field_errors()["bind_address"][0].code,
            "Invalid Socket address.".to_owned()
        ),
    }

[lev-starkware]

Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Itay-Tsabary-Starkware)

---

crates/gateway/src/config_test.rs line 35 at r4 (raw file):

Previously, dafnamatsry wrote…

assert!(config.validate().is_ok());

Done

---

crates/gateway/src/config_test.rs line 46 at r4 (raw file):

Previously, dafnamatsry wrote…

assert_matches!(config.validate(), Err(ValidationError(...))

Done

[dafnamatsry]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)