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

bug(rust): CLI argument parser doesn't work for some configurations. #2662

Closed
Tracked by #2215
josedev-union opened this issue Aug 16, 2023 · 8 comments · Fixed by #2666
Closed
Tracked by #2215

bug(rust): CLI argument parser doesn't work for some configurations. #2662

josedev-union opened this issue Aug 16, 2023 · 8 comments · Fixed by #2666
Assignees
Labels
agent bug Something isn't working tech-debt

Comments

@josedev-union
Copy link
Contributor

josedev-union commented Aug 16, 2023

The following configurations are not parsed from either env var or cli args.
Configurations whose key includes multiple words are not parsed from CLI args correctly. Here are typical ones.(edited)

chains.<chain_name>.addresses.interchainGasPaymaster
chains.<chain_name>.addresses.validatorAnnounce

Btw, when I use a config file, it works.
Here is a discord link for more details https://discord.com/channels/935678348330434570/984123861144600587/1139906822510219284

@josedev-union
Copy link
Contributor Author

I think i found the reason.
In the CommandLineArguments, the key is converted to lowercase but RawCoreContractAddresses uses camelCase serde. i.e. RawCoreContractAddresses.interchain_gas_paymaster is seralized to interchainGasPaymaster and vice versa.
On the other side, the cli argument --chains.<chain_name>.addresses.interchainGasPaymaster is parsed as chains.<chain_name>.addresses.interchaingaspaymaster and it doesn't match with the camelCased result.

#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct RawCoreContractAddresses {
mailbox: Option<String>,
interchain_gas_paymaster: Option<String>,
validator_announce: Option<String>,
}

So this issue is not only affecting to chains.x.addresses but also all the other configurations which include multiple words in the key. For example, chains.${CHAIN_NAME}.finalityBlocks.

@josedev-union josedev-union changed the title rust: ArgumentParser doesn't work for some configurations. bug(rust): ArgumentParser doesn't work for some configurations. Aug 16, 2023
@josedev-union
Copy link
Contributor Author

I can reproduce this issue with the following code snippet

use serde_json;


#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct TestHyperlaneArgParse {
    single: Option<String>,
    camel_case_upper: Option<String>,
    camel_case_lower: Option<String>,
    snake_case_upper: Option<String>,
    snake_case_lower: Option<String>,
}

fn main() {
    let json_data = r#"
        {
            "single": "single",
            "camelCaseUpper": "camel_case_upper",
            "camelcaselower": "camel_case_lower",
            "snake_Case_Upper": "snake_case_upper",
            "snake_case_lower": "snake_case_lower"
        }
    "#;

    let deserialized: Result<TestHyperlaneArgParse, serde_json::Error> = serde_json::from_str(json_data);
    match deserialized {
        Ok(data) => {
            println!("{:?}", data);
        },
        Err(err) => {
            println!("Error: {:?}", err);
        }
    }
}

Its output is

TestHyperlaneArgParse { single: Some("single"), camel_case_upper: Some("camel_case_upper"), camel_case_lower: None, snake_case_upper: None, snake_case_lower: None }

@josedev-union josedev-union changed the title bug(rust): ArgumentParser doesn't work for some configurations. bug(rust): CLI argument parser doesn't work for some configurations. Aug 16, 2023
@mattiekat
Copy link
Contributor

Can you elaborate on what "Not parsed correctly means". The argument parser was intentionally designed to lowercase the strings to be insensitive to either the literal originchainname or originChainName or OriginChainName and so on. The JSON config parser is more rigid as it has a better definition of what is expected, and the ENV parser is, well, env format unfortunately and that is why it is SCREAMING_SNAKE_CASE.

@mattiekat
Copy link
Contributor

Wait, okay, I see, the issue is that you are not able to specify the core contract address via arg due to casing issues. I think this reasonably slipped through testing because to the best of my knowledge we have always specified this via config file.

@josedev-union
Copy link
Contributor Author

Wait, okay, I see, the issue is that you are not able to specify the core contract address via arg due to casing issues. I think this reasonably slipped through testing because to the best of my knowledge we have always specified this via config file.

yes, the problem is that the parser does lower-case while obj deserializer uses camelCase.
I see. Config files work as expected but we deploy all agents on K8s clusters by using the helm chart in this repo. And that helm chart only relys on env vars.
I found this issue for env vars first, and after some troubleshotting, found that cli args don't work either.

@mattiekat
Copy link
Contributor

As a stop gap I recommend creating a config file using your helm chart. What we do internally is we ship the docker image with the chains config you see in the repo; this means our helm files don't need to generate the config file.

@mattiekat
Copy link
Contributor

This bug might block the planned changes in #2215

@mattiekat mattiekat added bug Something isn't working agent tech-debt config labels Aug 16, 2023
@josedev-union
Copy link
Contributor Author

As a stop gap I recommend creating a config file using your helm chart. What we do internally is we ship the docker image with the chains config you see in the repo; this means our helm files don't need to generate the config file.

yeap, thanks for your suggestion

@mattiekat mattiekat self-assigned this Aug 16, 2023
mattiekat added a commit that referenced this issue Aug 18, 2023
### Description

This is a fix for an issue in the arguments and environment where we are
unable to correctly support attribute names due to the casing. The main
issue cannot be fully fixed practically by this PR because it would
cause some configuration cases to break. For now it adds manual
exceptions for the command line argument case and leaves envs as they
were. There are then two new parsers that can be used with the new
config format.

### Drive-by changes

Also adds this to argument parser since it was trivial to copy/paste it
and it will allow us to harden parsing when we switch over.

### Related issues

- Fixes #2662 
- Fixes #2663 
- Progress on #2215 

### Backward compatibility

Yes

### Testing

Unit Tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent bug Something isn't working tech-debt
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants