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: cainome cli update and new config parsing #22

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Conversation

glihm
Copy link
Collaborator

@glihm glihm commented Mar 16, 2024

No description provided.

#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct ContractParserConfig {
/// The file extension that should be considered as a Sierra file.
pub sierra_extension: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could add

pub struct ContractParserConfig {
  #[serde(default = ".contract_class.json")]
  pub sierra_extension: String,
  
  #[serde(default)]
  pub type_aliases: HashMap<String, String>,
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

When i think about it I have some doubts :)). So imo we can add this or leave it as it is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps we could add

pub struct ContractParserConfig {
  #[serde(default = ".contract_class.json")]
  pub sierra_extension: String,
  
  #[serde(default)]
  pub type_aliases: HashMap<String, String>,
}

Isn't the default implementation already defined below?

Copy link
Collaborator

@piniom piniom Mar 17, 2024

Choose a reason for hiding this comment

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

Yes, you're right, I've missed that. We could then have

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(default)] 
pub struct ContractParserConfig {
    pub sierra_extension: String,
    pub type_aliases: HashMap<String, String>,
}

This would allow the fields to be left unspecified when the user wants to have the default

Copy link
Collaborator Author

@glihm glihm Mar 17, 2024

Choose a reason for hiding this comment

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

Yes, you're right, I've missed that. We could then have

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(default)] 
pub struct ContractParserConfig {
    pub sierra_extension: String,
    pub type_aliases: HashMap<String, String>,
}

This would allow the fields to be left unspecified when the user wants to have the default

Yep I did that at first, but I wanted to enforce the sierra extension. But as you mention, we may prefer having a 100% default value, and having the .contract_class.json only in the example file? This is what you suggest?

Because we want an extension by default, and not an empty string. That's why I preferred to have a value in the default. :)

@piniom
Copy link
Collaborator

piniom commented Mar 16, 2024

Hey @glihm, thank you for this.

I have 3 points:

  1. When i build using cargo install --features build-binary --path . -- I get a non critical warning:
    image

  2. The generated files look like this
    image

It seems ok when i run a rust fmt on the file, maybe we could format it so that this step is not needed?

  1. How can i specify different contract names for the contracts when i have two contracts in my build directory?
    image

@glihm
Copy link
Collaborator Author

glihm commented Mar 17, 2024

  1. When i build using cargo install --features build-binary --path . -- I get a non critical warning:

You should use --all-features. An other reason why I want to finish the cainomeup script, to ensure anybody can run it without worrying about impl details. :)

  1. The generated files look like this
    It seems ok when i run a rust fmt on the file, maybe we could format it so that this step is not needed?

That's a very good point. We should check if this can be done from rust code to avoid linter to generate a diff after code generation.

  1. How can i specify different contract names for the contracts when i have two contracts in my build directory?

You mean like we have type aliases, adding a contract name aliasing in the parser configuration?

@piniom
Copy link
Collaborator

piniom commented Mar 17, 2024

Hey @glihm
Ad. 1. Thanks!
Ad. 2. I recommend this, it should work for our usecase
https://crates.io/crates/prettyplease/0.1.25
Ad 3.
Something similar to this, so that i can specify the Type of the contract. I don't know if it can be done or not, it's not critical, just a nice thing to have

pub mod account {
    use cainome::rs::abigen;

    abigen!(
        CartridgeAccount, //HERE
        include_str!("../../../target/dev/cartridge_account_Account.contract_class.json"),
        type_aliases {
            webauthn_session::session_component::Event as SessionComponentEvent;
            webauthn_auth::component::webauthn_component::Event as WebauthnComponentEvent;
        }
    );
}

pub mod erc20 {
    use cainome::rs::abigen;
    abigen!(
        Erc20Contract,  //HERE
        include_str!("../../../target/dev/cartridge_account_ERC20.contract_class.json"),
        type_aliases {
            openzeppelin::token::erc20::erc20::ERC20Component::Event as ERC20ComponentEvent;
            openzeppelin::access::ownable::ownable::OwnableComponent::Event as OwnableComponentEvent;
        }
    );
}

@glihm
Copy link
Collaborator Author

glihm commented Mar 17, 2024

Something similar to this, so that i can specify the Type of the contract. I don't know if it can be done or not, it's not critical, just a nice thing to have

Yes exactly, we can do that by adding a key in the parser configuration:

{
    "contracts": {
        "cartridge_account_Account", "CartridgeAccount",
        "file_name_of_contract", "MyContractName",
    }
}

This is a details but I think you get an important point. Because the name of the contract into the abigen! expansion is what allows to refer to this specific contract in the code. So yeah, I think it's important, good catch.

@glihm
Copy link
Collaborator Author

glihm commented Mar 17, 2024

I recommend this, it should work for our usecase
https://crates.io/crates/prettyplease/0.1.25

Thanks! Let's try with this then. 👍

@glihm glihm merged commit 075ef03 into main Apr 24, 2024
5 checks passed
@glihm glihm deleted the fix/cainome-cli-update branch May 30, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants