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

fix(node): remove genesis interpreter #1118

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Conversation

cryptoAtwill
Copy link
Contributor

This is a follow up to consistent genesis PR. In that PR, the implementation has been migrate, but some tests are still using the old GenesisInterpreter and quite coupled/leaked the implementation into unit tests.

This PR removes GenesisInterpreter completely, update tests and encapsulates all the genesis requirements into new module.

@cryptoAtwill cryptoAtwill requested a review from a team as a code owner August 21, 2024 08:03
@cryptoAtwill cryptoAtwill changed the base branch from main to simple-genesis August 21, 2024 08:03
@@ -91,7 +87,7 @@ where
pub fn new(
client: C,
validator_ctx: Option<ValidatorContext<C>>,
contracts_dir: PathBuf,
_contracts_dir: PathBuf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we don't remove this arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I missed it. Was using it as a placeholder so that not to break so many code at once. Will remove it.

Copy link
Contributor

@raulk raulk left a comment

Choose a reason for hiding this comment

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

-900 LoC reduction, score! 🎉🎉🎉

@raulk raulk merged commit 47556e9 into simple-genesis Aug 21, 2024
18 checks passed
@raulk raulk deleted the remove-genesis-interpreter branch August 21, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants