Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

Fix send id #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix send id #5

wants to merge 2 commits into from

Conversation

jadbox
Copy link

@jadbox jadbox commented Apr 17, 2020

When using sendAsync, the payload parameter may contain an existing object id. I've added a check for if the id is set and to use it instead of generating a new id.

@moodysalem
Copy link
Member

Why should caller ever be responsible for setting id?

@jadbox
Copy link
Author

jadbox commented May 7, 2020

@moodysalem It's because web3js pub sub likes to generate its own IDs for sendAsync actions so it knows which message is the rpc response. It's a weird use-case, but it's needed.

): Promise<
| JsonRpcSucessfulResponseMessage<TResult>
| JsonRpcErrorResponseMessage<TErrorData>
> {
const id = getUniqueId();
const id = Number.isNaN(optionalId as any)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should check for undefined values, if optionalId value is undefined then id will be undefined too

@federico-bohn
Copy link

This fix solve an issue that I have when send rpc messages from my app (inside iframe) to metamask:
Wrong response id "3610003435135761" (expected: "13") in {"jsonrpc":"2.0","id":13,"method":"eth_getBlockByNumber","params":["latest",false]}

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

Successfully merging this pull request may close these issues.

3 participants