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

graphql: validate block params #27876

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Aug 7, 2023

Block takes a number and a hash. The spec is unclear on this. My interpretation is either of these should be provided and we should error in case both are passed in.

Fixes #25899

graphql/graphql.go Outdated Show resolved Hide resolved
Co-authored-by: Martin Holst Swende <martin@swende.se>
@wsb1994
Copy link

wsb1994 commented Aug 8, 2023

Hi, I personally disagree with the direction this goes -> Not because this is a bad idea, but because there is a language centric method of validation for things like this already in go-validator, here is an example of how that might look, although I'll allow you and the reviewer to decide on what that means for this PR and allow you to retain the edit :)!

func (r *Resolver) Block(ctx context.Context, args struct {
	Number *Long
	Hash   *common
}) (*Block, error) {
	// Validate input arguments
	err := validateBlockArgs(args)
	if err != nil {
		return nil, err
	}

above is the validation function, using go-validate, which would then run the validation and return an error inline.

This is a much more go-thonic and sustainable way to do validation throughout etherium. This could compromise a substantial value add for other areas of the etherium project as well, providing more sustainable and expandable validation in many areas of the codebase.

@karalabe
Copy link
Member

karalabe commented Aug 8, 2023

@wsb1994 I don't think we want to go down the route of annotating validation rules in struct tags. We use a handful of consensus structures which have wildly different validation criteria based on which entrypoint they appear in our codebase. Whether network propagation, RPC APIs, sync vs. block processing, etc.

All in all I am not a fan of declarative rules, they almost always lack some flexibility that results in the same old custom code needing to be written anyway.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.12.2 milestone Aug 10, 2023
@holiman holiman merged commit 5e89ff4 into ethereum:master Aug 10, 2023
2 checks passed
@fjl fjl modified the milestones: 1.12.2, 1.13.0 Aug 11, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Block takes a number and a hash. The spec is unclear on what should happen in this case, leaving it an implemenation detail. With this change, we return an error in case both number and hash are passed in.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This pull request was closed.
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.

GraphQL: "block" query ignores hash if both hash and number are present
5 participants