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

Enforce EL RPC method types in the RPC method resolver #8672

Closed
mehdi-aouadi opened this issue Oct 2, 2024 · 1 comment · Fixed by #8677
Closed

Enforce EL RPC method types in the RPC method resolver #8672

mehdi-aouadi opened this issue Oct 2, 2024 · 1 comment · Fixed by #8677
Assignees
Labels
enhancement 🕵️‍♀️ New feature or request

Comments

@mehdi-aouadi
Copy link
Contributor

Currently the json RPC methods extending the abstract AbstractEngineJsonRpcMethod used by the ExecutionClientHandler might have different type parameter from the one supplied to the EngineJsonRpcMethodsResolver::getMethod which could lead to class cast exceptions.
Example:

DummyRpcMethodClass extends AbstractEngineJsonRpcMethod<ReturnTypeClass>
ExecutionClientHandlerImpl {
...
@Override
  public SafeFuture<BadReturnTypeClass> dummyRpcMethod(
    return engineMethodsResolver
        .getMethod(
            EngineApiMethod.ENGINE_DUMMY_RPC_METHOD,
            () -> spec.atSlot(slot).getMilestone(),
            BadReturnTypeClass.class)
        .execute(paramsBuilder.build());
  }
...
}

In the example above, the dummyRpcMethod shoudl have ReturnTypeClass instead of BadReturnTypeClass.
We could make the EngineJsonRpcMethodsResolver detect the RPC method return type dynamically.
Possibles approaches could be to define the return type in the method itself or make use of reflection to dynamically determine it...

@mehdi-aouadi
Copy link
Contributor Author

A better approach I've found is to refactor the unit tests and make them check the return type of the execution engine's response futures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🕵️‍♀️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant