-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
The invalid type Record<never, never> removed from return type #4609
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { v4 as uuidV4 } from 'uuid'; | |
/** | ||
* A JSON-RPC request conforming to the EIP-1193 specification. | ||
*/ | ||
type Eip1193Request<Params extends JsonRpcParams> = { | ||
type Eip1193Request<Params extends JsonRpcParams = JsonRpcParams> = { | ||
id?: JsonRpcId; | ||
jsonrpc?: JsonRpcVersion2; | ||
method: string; | ||
|
@@ -26,11 +26,9 @@ type Eip1193Request<Params extends JsonRpcParams> = { | |
* @param eip1193Request - The EIP-1193 request to convert. | ||
* @returns The corresponding JSON-RPC request. | ||
*/ | ||
export function convertEip1193RequestToJsonRpcRequest< | ||
Params extends JsonRpcParams, | ||
>( | ||
eip1193Request: Eip1193Request<Params>, | ||
): JsonRpcRequest<Params | Record<never, never>> { | ||
export function convertEip1193RequestToJsonRpcRequest( | ||
eip1193Request: Eip1193Request, | ||
): JsonRpcRequest { | ||
const { id = uuidV4(), jsonrpc = '2.0', method, params } = eip1193Request; | ||
return params | ||
? { | ||
|
@@ -78,15 +76,10 @@ export class SafeEventEmitterProvider extends SafeEventEmitter { | |
* @param eip1193Request - The request to send. | ||
* @returns The JSON-RPC response. | ||
*/ | ||
async request<Params extends JsonRpcParams, Result extends Json>( | ||
eip1193Request: Eip1193Request<Params>, | ||
): Promise<Result> { | ||
async request(eip1193Request: Eip1193Request): Promise<Json> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't We should check whether this change breaks downstream packages using a preview build of this branch. |
||
const jsonRpcRequest = | ||
convertEip1193RequestToJsonRpcRequest(eip1193Request); | ||
const response = await this.#engine.handle< | ||
Params | Record<never, never>, | ||
Result | ||
>(jsonRpcRequest); | ||
const response = await this.#engine.handle(jsonRpcRequest); | ||
|
||
if ('result' in response) { | ||
return response.result; | ||
|
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.
Same as above, I think this function needs to be generic over
Params
. It would potentially be unusable downstream without an exposed way to narrow theparams
property of the return object.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.
In EIP 1193 optional
params
isunknown[] | object
. And here we have assigned the same usingJsonRpcParams
(it's a default type for params). So it has to be of this type. Just thinking, in which case we need this to be beyond this type?