Skip to content

Conversation

syjn99
Copy link
Member

@syjn99 syjn99 commented Aug 5, 2025

What was wrong?

We might need some constants different from the beacon chain. (e.g., INTERVALS_PER_SLOT is 4)

Part of #393.

How was it fixed?

This PR simply add a module in ream-consensus-misc for constant. Also my clippy complains about let-chain so I fixed it also. (Therefore make pr is now happy)

To-Do

@unnawut
Copy link
Contributor

unnawut commented Aug 5, 2025

I'm not sure if ream_consensus_misc needs to host both beacon and lean stuff? They seem entirely independent. Even misc could go into ream_consensus_beacon/lean. But maybe I don't know there's a reason for splitting misc from consensus crate?

But if so I think splitting the crates into one of these pairs make more sense, but just 2 cents:

  1. ream_consensus_beacon::misc::constants & ream_consensus_lean::misc::constants
  2. ream_misc_beacon::constants & ream_misc_lean::constants but personally 1 makes more sense.

@syjn99
Copy link
Member Author

syjn99 commented Aug 5, 2025

They seem entirely independent. Even misc could go into ream_consensus_beacon/lean. But maybe I don't know there's a reason for splitting misc from consensus crate?

@unnawut Yeah indeed. I also like the first option, will do that way!

@syjn99
Copy link
Member Author

syjn99 commented Aug 5, 2025

@unnawut I tried, but removing the existing dependency makes another dependency issues there. As the task size might be too big, I'd say taking this as an issue.

@unnawut
Copy link
Contributor

unnawut commented Aug 5, 2025

@unnawut I tried, but removing the existing dependency makes another dependency issues there. As the task size might be too big, I'd say taking this as an issue.

Sgtm. Actually I think this is cosmetic enough we might not even need to tackle at all so an issue is probably not needed.

@syjn99 syjn99 added this pull request to the merge queue Aug 5, 2025
Merged via the queue into ReamLabs:master with commit 9c5f310 Aug 5, 2025
15 checks passed
@jihoonsong
Copy link
Contributor

FYI, the consensus spec is being changed to use milliseconds and basis points: ethereum/consensus-specs#4476

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.

3 participants