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

(BIDS 3226) API: Block Details v2 (Internal) #637

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

remoterami
Copy link
Contributor

No description provided.

@remoterami remoterami requested a review from LuccaBitfly July 29, 2024 10:13
@remoterami remoterami force-pushed the BIDS-3226/api-blocks branch from 2cc3f17 to d2e2cd6 Compare July 29, 2024 11:21
Copy link

cloudflare-workers-and-pages bot commented Jul 29, 2024

Deploying beaconchain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 189f0be
Status: ✅  Deploy successful!
Preview URL: https://8155019b.beaconchain.pages.dev
Branch Preview URL: https://bids-3226-api-blocks.beaconchain.pages.dev

View logs

@remoterami remoterami force-pushed the BIDS-3226/api-blocks branch from d2e2cd6 to c8f6185 Compare July 29, 2024 11:34
backend/pkg/api/handlers/common.go Outdated Show resolved Hide resolved
backend/pkg/api/handlers/common.go Outdated Show resolved Hide resolved
frontend/types/api/block.ts Show resolved Hide resolved
@remoterami remoterami requested a review from MauserBitfly July 30, 2024 09:42
Copy link
Contributor

@LuccaBitfly LuccaBitfly left a comment

Choose a reason for hiding this comment

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

lgtm so far, pls make one of everything for slots too 😅

Copy link
Contributor

@LuccaBitfly LuccaBitfly left a comment

Choose a reason for hiding this comment

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

lgtm, don't forget to let FE team approve structs

Copy link
Contributor

@MauserBitfly MauserBitfly 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 from the FE point of view, just have one remark

/**
* General
*/
block: number /* uint64 */;
Copy link
Contributor

Choose a reason for hiding this comment

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

Some properties overlap between the BlockOverview and the BlockExecutionPayload.
Wouldn't it be enough to have them just in the BlockExecutionPayload ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was just done because of our approach to design the internal api closely tailored to the designs and keep the frontend as simple as possible. But you're right, that information is redundant and can be removed, see the new changes; I've kept the duplicates in the base BlockOverview though because BlockExecutionPayload is intended to be present for POS blocks only

Copy link
Contributor

@MauserBitfly MauserBitfly left a comment

Choose a reason for hiding this comment

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

LGTM

@remoterami remoterami merged commit 9e18c47 into staging Aug 2, 2024
3 checks passed
@remoterami remoterami deleted the BIDS-3226/api-blocks branch August 2, 2024 08:04
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