Skip to content

Conversation

@KolbyML
Copy link
Contributor

@KolbyML KolbyML commented Apr 24, 2025

Motivation

This PR is an extension on #7245

I was trying to use the endpoint to create a https://github.com/ethereum/portal-network-specs/blob/master/beacon-chain/beacon-network.md#historicalsummaries HistoricalSummariesWithProof, but the endpoint didn't return the Fork or the slot as we need the epoch.

Description

  • I added slot to the HistoricalSummariesResponseType type
  • I used ExecutionOptimisticFinalizedAndVersionCodec to expose the fork
  • I changed /eth/v1/lodestar/historical_summaries/{state_id} to /eth/v1/lodestar/states/{state_id}/historical_summaries to be more in line with the other state endpoints https://ethereum.github.io/beacon-APIs/#/Beacon

@KolbyML KolbyML requested a review from a team as a code owner April 24, 2025 00:04
@CLAassistant
Copy link

CLAassistant commented Apr 24, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ nflaig
❌ KolbyML
You have signed the CLA already but the status is still pending? Let us recheck it.

@KolbyML
Copy link
Contributor Author

KolbyML commented Apr 24, 2025

@nflaig ready for another look 🚀

@KolbyML KolbyML requested a review from nflaig April 24, 2025 08:55
nflaig
nflaig previously approved these changes Apr 24, 2025
Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

LGTM

@nflaig
Copy link
Member

nflaig commented Apr 24, 2025

@nflaig
Copy link
Member

nflaig commented Apr 24, 2025

I was trying to use the endpoint to create a https://github.com/ethereum/portal-network-specs/blob/master/beacon-chain/beacon-network.md#historicalsummaries HistoricalSummariesWithProof, but the endpoint didn't return the Fork or the slot as we need the epoch.

does it make sense to return epoch instead of slot directly from the api?

@KolbyML
Copy link
Contributor Author

KolbyML commented Apr 24, 2025

I was trying to use the endpoint to create a https://github.com/ethereum/portal-network-specs/blob/master/beacon-chain/beacon-network.md#historicalsummaries HistoricalSummariesWithProof, but the endpoint didn't return the Fork or the slot as we need the epoch.

does it make sense to return epoch instead of slot directly from the api?

I will fix the test tomorrow, I was trying to match status-im/nimbus-eth2#6675 Nimbus's implementation to a degree. I struggle reading Nim. But without the slot/epoch and fork info it is awkward to use Lodestar's current api for our usecase. We can easily derive the epoch ourselves so I think it is fine if we keep it as a slot.

@nflaig
Copy link
Member

nflaig commented Apr 24, 2025

We can easily derive the epoch ourselves so I think it is fine if we keep it as a slot.

Whatever works best for you, I don't think anyone else is using the api

@codecov
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.29%. Comparing base (dd729f5) to head (a07bc54).
Report is 2 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #7742      +/-   ##
============================================
- Coverage     56.29%   56.29%   -0.01%     
============================================
  Files           837      837              
  Lines         58044    58048       +4     
  Branches       4469     4468       -1     
============================================
- Hits          32678    32677       -1     
- Misses        25298    25303       +5     
  Partials         68       68              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KolbyML
Copy link
Contributor Author

KolbyML commented Apr 24, 2025

It looks like you updated the test yourself, is there anything else I need to do on my end?

@nflaig
Copy link
Member

nflaig commented Apr 24, 2025

It looks like you updated the test yourself, is there anything else I need to do on my end?

seems good to me, CI is happy as well now

@nflaig
Copy link
Member

nflaig commented Apr 24, 2025

merging this, can do follow up changes if something still needs to be adapted

@nflaig nflaig merged commit 1733258 into ChainSafe:unstable Apr 24, 2025
20 checks passed
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.30.0 🎉

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.

4 participants