Skip to content

feat: implement new beacon APIs(accessors for pending_deposits/pending_partial_withdrawals) #7006

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

Merged
merged 5 commits into from
Mar 17, 2025

Conversation

syjn99
Copy link
Contributor

@syjn99 syjn99 commented Feb 15, 2025

Issue Addressed

Resolves #7003

Proposed Changes

Added two endpoints as ethereum/beacon-APIs#500 proposed:

  • /eth/v1/beacon/states/{state_id}/pending_deposits
  • /eth/v1/beacon/states/{state_id}/pending_partial_withdrawals

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.

Looks good at a glance, you're definitely on the right track. I've triggered CI so you can see if the tests pass. You can also run locally, just not in debug mode

@michaelsproul
Copy link
Member

michaelsproul commented Feb 17, 2025

You can fix the cargo format failure by running cargo fmt --all

@syjn99
Copy link
Contributor Author

syjn99 commented Feb 20, 2025

RUST_MIN_STACK=16777216 cargo test beacon_get_state_info_electra -p http_api --test bn_http_api_tests -- --nocapture

@michaelsproul Hi, thanks to #7007, I found that my test successfully passed with the commit. However, I'm afraid that default BeaconChainHarness will always return empty list of pending_deposits/pending_partial_withdrawals as it is initialized with default value. Do I have to add a case for comparing not emptied list? If then, would you mind letting me know where the good place to start?

@michaelsproul michaelsproul marked this pull request as ready for review February 20, 2025 11:43
@michaelsproul michaelsproul added v7.0.0 New release c. Q1 2025 v7.0.0-beta.clean Clean release post Holesky rescue labels Mar 11, 2025
@michaelsproul
Copy link
Member

I think we should include this in v7.0.0 as we have some users asking about it.

@syjn99 Do you mind rebasing your changes on the release-v7.0.0 branch? 🙏

@michaelsproul michaelsproul changed the base branch from unstable to release-v7.0.0 March 11, 2025 22:55
@michaelsproul michaelsproul requested a review from jxs as a code owner March 11, 2025 22:55
@michaelsproul
Copy link
Member

When you rebase, just take out all the commits from unstable (use rebase -i).

@syjn99 syjn99 force-pushed the feat/beacon-api-500 branch from effa234 to 764e591 Compare March 12, 2025 01:01
@syjn99 syjn99 requested a review from michaelsproul March 12, 2025 01:03
@michaelsproul michaelsproul requested a review from macladson March 12, 2025 02:12
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.

LGTM

Just pushed one small commit to use List in the serialization rather than Vec. This saves some copying of data (List and Vec have the same serialized representation anyway, so we can mix and match them).

@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Mar 17, 2025
mergify bot added a commit that referenced this pull request Mar 17, 2025
@mergify mergify bot merged commit 50b5a72 into sigp:release-v7.0.0 Mar 17, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v7.0.0-beta.clean Clean release post Holesky rescue v7.0.0 New release c. Q1 2025
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants