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

Rust code generation #66

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

Rust code generation #66

wants to merge 31 commits into from

Conversation

amirylm
Copy link

@amirylm amirylm commented Nov 1, 2024

Introducing Rust code generation.

  • generalized code for go templates
  • added rs folder for rust:
    • (golang) code and template for generating the rust module
    • generated rust module chainselectors with tests

NOTES:

  • focusing on EVM at the moment
  • rust toolchain is a soft requirement, docker container is used as a fallback if rust is not installed

Copy link

@PabloMansanet PabloMansanet left a comment

Choose a reason for hiding this comment

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

Looking good! Just left a few style suggestions and some mild safety ones, but nothing that's a showstopper (though I think anyhow -> thiserror is definitely worth doing).

I suppose there are ways we could reduce the boilerplate by storing the string representation, ID and selector in a const array or something similar at the top, but I don't think it's worth it to be stingy with the LOC of a generated file, so this approach looks good to me.

rs/generated_chains.rs.tmpl Outdated Show resolved Hide resolved
rs/generated_chains.rs.tmpl Outdated Show resolved Hide resolved
rs/generated_chains.rs.tmpl Outdated Show resolved Hide resolved
rs/generated_chains.rs.tmpl Outdated Show resolved Hide resolved
Debug, Hash, PartialEq, Eq, Clone, Copy, serde::Serialize, serde::Deserialize, PartialOrd, Ord,
)]
#[repr(u64)]
pub enum ChainName {

Choose a reason for hiding this comment

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

I may be bikeshedding here, but is this really a ChainName? I would think this enum would be the Chain, and that Name would specifically refer to the stringified version of it.

Copy link
Author

Choose a reason for hiding this comment

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

We inherit the naming, we use this type to read strings from configs or keystore (i.e. actual chain names).

Agree that now might be a good time to align

rs/genchains_rs.go Outdated Show resolved Hide resolved
rs/chainselectors/src/lib.rs Outdated Show resolved Hide resolved
rs/chainselectors/src/lib.rs Outdated Show resolved Hide resolved
amirylm and others added 10 commits November 4, 2024 12:14
Co-authored-by: PabloMansanet <pablomansanet@gmail.com>
Co-authored-by: PabloMansanet <pablomansanet@gmail.com>
- use thiserror
- reduce `ChainSelector`, `ChainId` types
- fn from_chain_selector
Copy link

@PabloMansanet PabloMansanet left a comment

Choose a reason for hiding this comment

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

Looks good to me now! 👍

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