hive: engine-cancun: fix return error code and bit of refactoring #9307
Conversation
| if version >= clparams.DenebVersion && payloadAttributes.ParentBeaconBlockRoot == nil { | ||
| return nil, &engine_helpers.InvalidPayloadAttributesErr // Beacon Root missing | ||
| if version >= clparams.DenebVersion && payloadAttributes.ParentBeaconBlockRoot == nil { // Beacon Root missing | ||
| return nil, &rpc.InvalidParamsError{Message: "Beacon Root missing"} |
There was a problem hiding this comment.
This is returning -32602? I think this should still be -38003 with the message
There was a problem hiding this comment.
Yes, for some reason this is what hive expects it to return. https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/suites/cancun/tests.go#L534.
There was a problem hiding this comment.
As per https://github.com/ethereum/execution-apis/blob/main/src/engine/cancun.md#specification-1 if the payloadAttributes structure has issues it should return -38003. Maybe we need to check with Hive team
There was a problem hiding this comment.
I agree, I also had this in my mind.
There was a problem hiding this comment.
Or there might be a place we did not implemented yet or something?
There was a problem hiding this comment.
I think the Hive tests need to be updated. See #8982 & ethereum/execution-apis#498
| TOO_LARGE_REQUEST = -38004 | ||
| UNSUPPORTED_FORK_ERROR = -38005 | ||
| ) | ||
|
|
There was a problem hiding this comment.
The engine API specific errors are meant for CL <> EL API-play. I think this should be kept with other engineapi code
There was a problem hiding this comment.
Right, but I am just not a fan of code duplicating thing. It just doesn't lead to a good... but let's not merge this for now
There was a problem hiding this comment.
I see it as compartmentalization not duplicating
There was a problem hiding this comment.
I think you are partially right, I understand what you mean, but I think it's batter to keep error related thing in one place, it's just how I would do it. You can call it design choice :). Anyway this is not a huge performance boost, so we can leave it as is
There was a problem hiding this comment.
These aren't errors per se, but certain specific return values - the way it woks with engine api. rpc encompasses a lot more
There was a problem hiding this comment.
I see them as error codes. If any error code in the future will change you will have to go all over the code to change them manually instead of changing a value in a single place. It probably makes sense to extract errors from rpc as well in the future in case we will do major refactoring, but not now. we are leaving it as is, right?
There was a problem hiding this comment.
One way to go about it, yes. Maybe calling upon @yperbasis to settle the tie
In general it is a bad practice to keep 2 or more separate code parts that do the same thing. So in this PR I removed errors in engine_helpers and created a function in rpc that creates an error based on error code. Additionally I am thinking to remove all errors in rpc.errors except methodNotFoundError and subscriptionNotFoundError since they seem to have constant return message.