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

BatchRequest assumes JSON RPC requests are returned in order #4250

Closed
floatcoder opened this issue Aug 17, 2021 · 3 comments · Fixed by #4596
Closed

BatchRequest assumes JSON RPC requests are returned in order #4250

floatcoder opened this issue Aug 17, 2021 · 3 comments · Fixed by #4596
Assignees
Labels
1.x 1.0 related issues Bug Addressing a bug

Comments

@floatcoder
Copy link

floatcoder commented Aug 17, 2021

Expected behavior

The response to a batch JSON RPC is not enforced to be in order (according to the spec). Despite most endpoints following this convention, some (e.g. Erigon) do not.

This means that results can be misparsed, or worse, silently corrupted despite the response being valid.

For example if we batched the following calls from G-UNI FLOAT/USDC contract:

  • getUnderlyingBalances(uint256,uint256)
  • totalSupply(uint256)

We can receive the response:

[
  {
    "jsonrpc": "2.0",
    "id": 2,
    "result": "0x0000000000000000000000000000000000000000000000058002c3cbfcb15f17"
  },
  {
    "jsonrpc": "2.0",
    "id": 1,
    "result": "0x000000000000000000000000000000000000000000000000000001dee1b1ea2a00000000000000000000000000000000000000000000c419a8b582118ad911b6"
  }
]

Which is valid and should decode to:

  • (amount0Current, amount1Current) = 2056780900906, 926057156010074212733366
  • totalSupply = 101457870636241215255

Actual behavior

In actuality, the response for totalSupply is attempted to be decoded as a tuple.

index.js:1 G-UNI USDC/FLOAT:getUnderlyingBalances.call Error: data out-of-bounds (length=32, offset=64, code=BUFFER_OVERRUN, version=abi/5.0.7)
    at Logger.makeError (index.ts:213)
    at Logger.throwError (index.ts:225)
    at Reader._peekBytes (abstract-coder.ts:182)
    at Reader.readBytes (abstract-coder.ts:196)
    at Reader.readValue (abstract-coder.ts:203)
    at NumberCoder.decode (number.ts:44)
    at array.ts:108
    at Array.forEach (<anonymous>)
    at unpack (array.ts:89)
    at TupleCoder.decode (tuple.ts:27)
    at AbiCoder.decode (abi-coder.ts:113)
    at ABICoder.push../node_modules/web3-eth-contract/node_modules/web3-eth-abi/lib/index.js.ABICoder.decodeParametersWith (index.js:303)
    at ABICoder.push../node_modules/web3-eth-contract/node_modules/web3-eth-abi/lib/index.js.ABICoder.decodeParameters (index.js:284)
    at push../node_modules/web3-eth-contract/lib/index.js.Contract._decodeMethodReturn (index.js:469)
    at batch.js:58
    at Array.forEach (<anonymous>)
    at batch.js:49
    at index.js:186
    at XMLHttpRequest.request.onreadystatechange (index.js:98)

Environment

yarn 1.22.4

web3-eth-contract@1.5.1, web3-eth-contract@^1.3.1:
  version "1.5.1"
  resolved "https://registry.yarnpkg.com/web3-eth-contract/-/web3-eth-contract-1.5.1.tgz#4c59abe3e575920e83e8f8c1bc0bf8a18f975623"
  integrity sha512-LRzFnogxeZagxHVpJ9cDK5Y8oQFUNtNL8s5w4IjvZ/JDoBQXPJuwhySwjftL3Hlk3znziMFqAH6snoxjvHnoag==
  dependencies:
    "@types/bn.js" "^4.11.5"
    web3-core "1.5.1"
    web3-core-helpers "1.5.1"
    web3-core-method "1.5.1"
    web3-core-promievent "1.5.1"
    web3-core-subscriptions "1.5.1"
    web3-eth-abi "1.5.1"
    web3-utils "1.5.1"
@floatcoder
Copy link
Author

An easy fix would be to perform a sort by id at https://github.com/ChainSafe/web3.js/blob/1.x/packages/web3-core-requestmanager/src/batch.js#L51, happy to open a MR to do that if that's the preferred fix.

@luu-alex luu-alex added the 1.x 1.0 related issues label Aug 19, 2021
@spacesailor24
Copy link
Contributor

Hi there, thank you for bringing this to our attention with so much detail!

The team's efforts right now is focused on getting the rewrite (4.x branch) done so we have a more straightforward code base to work with. If you are able to get a pull request up for this, we can review it and get it merged in, otherwise we don't have a guarantee on when this issue will be addressed

@spacesailor24 spacesailor24 added the Bug Addressing a bug label Aug 23, 2021
@jdevcs jdevcs assigned luu-alex and unassigned nazarhussain Sep 13, 2021
@philknows
Copy link

philknows commented Sep 15, 2021

Echoing above from @spacesailor24 , there is no guarantee on when this will be addressed as resources are being used for 4.x branch to help better address issues such as this. We will push to get someone to check #4274 when possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Bug Addressing a bug
Projects
None yet
6 participants