-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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 master FF for review #18445
GraphQL master FF for review #18445
Conversation
…a is reported properly
Co-Authored-By: Arachnid <[email protected]>
internal/ethapi/api.go
Outdated
func (s *PublicBlockChainAPI) EstimateGas(ctx context.Context, args CallArgs) (hexutil.Uint64, error) { | ||
return DoEstimateGas(ctx, s.b, args, rpc.PendingBlockNumber) | ||
return s.doEstimateGas(ctx, s.b, args, rpc.PendingBlockNumber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arachnid I would like you to confirm this change. My understanding reading your PR was that you wanted to make DoCall
and DoEstimate
independent. And in fact, this is what is currently breaking the build. Could you and @kshinn clarify what you want to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was one of the changes I made to bring this inline with the direction of the design of the rest of the calls (delegating to a private method). If the intent was to move away from that direction, then the changes I made in the last commit can be omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kshinn This breaks existing code (per the build):
# github.com/ethereum/go-ethereum/graphql
graphql/grahpql.go:894:30: undefined: ethapi.DoCall
graphql/grahpql.go:915:14: undefined: ethapi.DoEstimateGas
Would you mind either reverting the change or fixing that code? I'm okay with either if @gballet is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mind at all. I'll play with getting this working and push and update to this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way, as long as it works.
@gballet I think we're ready to go here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Done, thanks for your impressive work and patience! |
I've written up a draft EIP for standardising this functionality here: https://github.com/Arachnid/EIPs/blob/graphql/EIPS/eip-draft-graphql.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
@Arachnid Here is your PR with master merged in. There are some updates to
internal/ethapi/api.go
that I made in the last commit to match other design changes I was seeing in when I merged in master. Those changes should be isolated to the last commit in case they need to be peeled off.