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

chore(registry): Dogfood Test Rollup Config #308

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

refcell
Copy link
Collaborator

@refcell refcell commented Nov 23, 2024

Description

Dogfoods the hardcoded test rollup config and uses the new From<&RollupConfig> for HardForkConfiguration.

@refcell refcell added A-genesis Area: genesis crate C-feat Category: New feature or request labels Nov 23, 2024
@refcell
Copy link
Collaborator Author

refcell commented Nov 23, 2024

📚 $\text{Stack Overview}$

Pulls submitted in this stack:

This comment was automatically generated by st.

@refcell refcell self-assigned this Nov 23, 2024
@refcell refcell added A-registry Area: registry crate C-chore Category: general cleanup and removed C-feat Category: New feature or request labels Nov 23, 2024
@@ -55,6 +55,19 @@ pub struct HardForkConfiguration {
pub holocene_time: Option<u64>,
}

impl From<&RollupConfig> for HardForkConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

these types are in the same crate.

this impl would be less horrible if we also add this natively to RollupConfig.

I guess from<&> wouldn't be an issue for RollupConfig because we assume this will likely be Arc'ed.
but still, very much dislike standalone from ref impls

Comment on lines -142 to +137
canyon_time: Some(1704992401),
delta_time: Some(1708560000),
ecotone_time: Some(1710374401),
fjord_time: Some(1720627201),
granite_time: Some(1726070401),
holocene_time: None,
},
hardfork_configuration: (&crate::configs::BASE_MAINNET_CONFIG).into(),
Copy link
Member

Choose a reason for hiding this comment

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

see, this is horrible,

granite_time: Some(1726070401),
holocene_time: None,
},
hardfork_configuration: (&crate::configs::BASE_MAINNET_CONFIG).into(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hardfork_configuration: (&crate::configs::BASE_MAINNET_CONFIG).into(),
hardfork_configuration: crate::configs::BASE_MAINNET_CONFIG.hardfork_config(),

is much nicer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, that makes sense to me, will change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-genesis Area: genesis crate A-registry Area: registry crate C-chore Category: general cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants