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

Fix Invalid type assignment in SafeEventEmitterProvider #4608

Open
kanthesha opened this issue Aug 14, 2024 · 4 comments · May be fixed by #4609
Open

Fix Invalid type assignment in SafeEventEmitterProvider #4608

kanthesha opened this issue Aug 14, 2024 · 4 comments · May be fixed by #4609
Assignees

Comments

@kanthesha
Copy link
Contributor

kanthesha commented Aug 14, 2024

The Record<never, never> type assigned to response of convertEip1193RequestToJsonRpcRequest does not belong here (link to the code), no possible return value is represented by that type.

We need to fix/simplify the type assignment.

@kanthesha kanthesha changed the title Simplify types assignment in SafeEventEmitterProvider Fix Invalid type assignment in SafeEventEmitterProvider Aug 14, 2024
@kanthesha kanthesha self-assigned this Aug 14, 2024
@kanthesha kanthesha added bug Something isn't working and removed bug Something isn't working labels Aug 14, 2024
@kanthesha kanthesha linked a pull request Aug 14, 2024 that will close this issue
3 tasks
@kanthesha
Copy link
Contributor Author

export function convertEip1193RequestToJsonRpcRequest<
  Params extends JsonRpcParams,
>(
  eip1193Request: Eip1193Request<Params>,
): JsonRpcRequest<Params | Record<never, never>> {
  const { id = uuidV4(), jsonrpc = '2.0', method, params } = eip1193Request;
  return params
    ? {
        id,
        jsonrpc,
        method,
        params,
      }
    : {
        id,
        jsonrpc,
        method,
      };
}

In the above function, the type of params in JsonRpcRequest is (((Record<string, Json> | Json[]) & ExactOptionalGuard) & JsonRpcParams) | undefined> and type of JsonRpcParams is Json[] | Record<string, Json>.
So we need Record<never, never> to widen the type so that it matches with the type of params in JsonRpcRequest.

We can try replacing Params extends JsonRpcParams with Params extends JsonRpcRequest['params']. But this way, we get type error in the : JsonRpcRequest<Params>, because JsonRpcRequest has type JsonRpcRequest<Params extends JsonRpcParams = JsonRpcParams>.

I think, if we have to fix this, we need to align the JsonRpcRequest generic type with the type of params in JsonRpcRequest.

Additional Reference:
Actual JsonRpcRequest type

export type JsonRpcRequest<Params extends JsonRpcParams = JsonRpcParams> = InferWithParams<typeof JsonRpcRequestStruct, Params>;

which will expand to

type JsonRpcRequest<Params extends JsonRpcParams = JsonRpcParams> = {
    params?: (Record<string, Json> | Json[]) & ExactOptionalGuard;
    id: string | number | null;
    method: string;
    jsonrpc: "2.0";
}

cc: @Gudahtt

@Gudahtt
Copy link
Member

Gudahtt commented Aug 20, 2024

So we need Record<never, never> to widen the type so that it matches with the type of params in JsonRpcRequest.

Hmm, I'm not really following how those difference in types is resolved by this addition. How does adding Record<never, never> widen the type? It's unclear to me which piece of the left type requires that.

@kanthesha
Copy link
Contributor Author

kanthesha commented Aug 21, 2024

It is something to do with ExactOptionalGuard! tested by remove that in JsonRpcRequest, then it works fine without Record<never, never> in response (: JsonRpcRequest<Params>).

@mcmire
Copy link
Contributor

mcmire commented Oct 28, 2024

Just seeing this issue now. I think this was my attempt to get something working with convertEip1193RequestToJsonRpcRequest without fully understanding the intricacies of the JsonRpcRequest type. In using Record<never, never> I was trying to represent the possibility that params on the given eip1193Request could be ignored and therefore not set on the JsonRpcRequest being built, but I agree that it's not the right way to solve this problem. I'll have to think more about what a good solution here would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@mcmire @Gudahtt @kanthesha and others