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

chore(api): Dropped node api context generic #2456

Merged
merged 15 commits into from
Feb 2, 2025
Merged

Conversation

abi87
Copy link
Collaborator

@abi87 abi87 commented Feb 1, 2025

Cleaned up the NodeAPIs context.
Apologies to @fridrik01 for stealing this

@@ -46,8 +47,8 @@ func (h *Handler[ContextT]) GetState(c ContextT) (any, error) {
}
return beacontypes.StateResponse{
// TODO: The version should be retrieved based on the slot
Version: "deneb", // stubbed
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to avoid this, we should introduce:

  • a latest version
  • a version.String method

cc @calbera

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense. latest version might not be necessary since we want to provide the version at which the incoming request is queried for. Would probably just require returning chainSpec.ActiveForkVersionForSlot(requestedSlot).String()

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 0% with 108 lines in your changes missing coverage. Please review.

Project coverage is 32.36%. Comparing base (d4cf42b) to head (047ac57).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
node-core/components/api_handlers.go 0.00% 16 Missing ⚠️
cmd/beacond/defaults.go 0.00% 9 Missing ⚠️
node-api/handlers/handlers.go 0.00% 7 Missing ⚠️
node-core/components/api.go 0.00% 7 Missing ⚠️
node-api/server/server.go 0.00% 6 Missing ⚠️
node-api/handlers/beacon/validators.go 0.00% 5 Missing ⚠️
node-api/handlers/routes.go 0.00% 4 Missing ⚠️
node-api/handlers/beacon/handler.go 0.00% 3 Missing ⚠️
node-api/handlers/builder/handler.go 0.00% 3 Missing ⚠️
node-api/handlers/config/handler.go 0.00% 3 Missing ⚠️
... and 24 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2456      +/-   ##
==========================================
- Coverage   32.37%   32.36%   -0.01%     
==========================================
  Files         350      350              
  Lines       15589    15592       +3     
  Branches       20       20              
==========================================
  Hits         5047     5047              
- Misses      10179    10182       +3     
  Partials      363      363              
Files with missing lines Coverage Δ
node-core/components/service_registry.go 0.00% <ø> (ø)
node-api/engines/echo/engine.go 0.00% <0.00%> (ø)
node-api/handlers/beacon/blobs.go 0.00% <0.00%> (ø)
node-api/handlers/beacon/block.go 0.00% <0.00%> (ø)
node-api/handlers/beacon/genesis.go 0.00% <0.00%> (ø)
node-api/handlers/beacon/randao.go 0.00% <0.00%> (ø)
node-api/handlers/beacon/routes.go 0.00% <0.00%> (ø)
node-api/handlers/proof/block_proposer.go 0.00% <0.00%> (ø)
node-api/handlers/proof/execution_fee_recipient.go 0.00% <0.00%> (ø)
node-api/handlers/proof/execution_number.go 0.00% <0.00%> (ø)
... and 25 more

@abi87 abi87 force-pushed the drop-node-api-context-generic branch from a957553 to f91e75e Compare February 1, 2025 03:56
import "github.com/labstack/echo/v4"

type (
Context = echo.Context
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to handler package

)

type Context = echo.Context
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved here from echo package to avoid import cycle

type VersionResponse struct {
Data struct {
Version string `json:"version"`
} `json:"data"`
}

response := VersionResponse{}
response.Data.Version = "1.0.0"
response.Data.Version = "1.1.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated here just to signal that we should fix this

@abi87 abi87 force-pushed the drop-node-api-context-generic branch from 228dd3d to 7bedccc Compare February 1, 2025 04:05
@abi87 abi87 marked this pull request as ready for review February 1, 2025 04:19
@abi87 abi87 requested a review from a team as a code owner February 1, 2025 04:19
@abi87 abi87 changed the title chore(api): Drop node api context generic chore(api): Dropped node api context generic Feb 1, 2025
Copy link
Contributor

@rezbera rezbera left a comment

Choose a reason for hiding this comment

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

LGTM - Much needed

@abi87 abi87 merged commit 928024c into main Feb 2, 2025
19 checks passed
@abi87 abi87 deleted the drop-node-api-context-generic branch February 2, 2025 15:02
shotes pushed a commit that referenced this pull request Feb 3, 2025
shotes pushed a commit that referenced this pull request Feb 3, 2025
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