Skip to content

Conversation

ltitanb
Copy link
Contributor

@ltitanb ltitanb commented Apr 28, 2025

Issue Addressed

We (Helix relay), have recently switched our consensus types to Lighthouse. During integration we made a few small quality of life changes that would be great to upstream so we can get rid of the fork. Overall all the changes are quite small and don't change any core logic.

Proposed Changes

Make a blanket implementation of SignedRoot for all impls of TreeHash

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2025

CLA assistant check
All committers have signed the CLA.

@realbigsean realbigsean added the ready-for-review The code is ready for review label May 12, 2025
mergify bot pushed a commit that referenced this pull request May 12, 2025
ref: #7367


  Implement `TestRandom` for a few types
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Making SignedRoot opt-in was an intentional design choice to statically prevent signing of objects that should never be signed.

At a glance, I would prefer to keep it that way. Which type did you find yourself wanting SignedRoot on that didn't have it already implemented? Could we add that implementation instead? Or could you add it downstream?

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 13, 2025
@ltitanb
Copy link
Contributor Author

ltitanb commented May 13, 2025

Noted, will follow the same approach downstream!

@ltitanb ltitanb closed this May 13, 2025
mergify bot pushed a commit that referenced this pull request May 16, 2025
ref: #7367


  Implement `From<Hash256>` for `ExecutionBlockHash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants