Add id to all JSON RPC calls like in advanceBlock

[cnasikas]

Hi

The advanceBlock function does not provide an id as a property. The web3-provider-ws package uses the id as follows:

WebsocketProvider.prototype._addResponseCallback = function(payload, callback) {
    var id = payload.id || payload[0].id;
    var method = payload.method || payload[0].method;

    this.responseCallbacks[id] = callback;
    this.responseCallbacks[id].method = method;

    // more code....
};

The line var id = payload.id || payload[0].id; will produce an error when an id is undefined

The same checks is presented in the new version of web3.

Thanks a lot!

[nventuro]

Hello @cnasikas, thanks for reporting this! It that behavior present in v1.0-beta.37 of web3-provider-ws, or was it introduced recently? We may want to add tests using different providers to make sure the test helpers can be used with all of them.

I'm curious, why are you running tests using a websocket provider?

[cnasikas]

Hi @nventuro

You are welcome! If you remember from #35 we use your helpers for different purpose than running tests.

The behavior is presented in v1.0-beta.37. I do not know if it presented in previous versions. In v1.0-beta.55 is presented also.

[frangio]

@cnasikas Do you know if this id is documented anywhere?

[cnasikas]

From https://www.jsonrpc.org/specification#request_object and https://github.com/ethereum/wiki/wiki/JSON-RPC

id
An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]

[cnasikas]

As far I understand the id is used as an identifier for a listener. For example in web3js beta.55:

if (isArray(payload)) {
    id = payload[0].id;
} else {
    id = payload.id;
}

this.once(id, (response) => {
    if (timeout) {
        clearTimeout(timeout);
    }

    this.removeListener('error', reject);

    return resolve(response);
});

[frangio]

Makes sense.

You're not guaranteed to get your answers back in the order you asked for them; the id is to help you sort that out.

From Stack Overflow.

[frangio]

Does the id need to be unique every time? I can't find any info on this.

[cnasikas]

That's a good question. I was thinking myself the same. What will happen if you have two requests with the same id ? If the ID is only used as an event name when you register to a listener then if you register multiple callbacks (one for each requests) to the same event normally all will be fired, right ?

[cnasikas]

I think it should be unique just in case but I am not sure if it is the correct approach.

[cnasikas]

Metamask's id generator function:

https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/random-id.js

[nventuro]

We can probably address this in a 0.4.1 bugfix release.

@cnasikas have you by chance tried this fix locally, to check that it does solve your issue? Also, if you could provide a short snippet that triggers the bug, for us to add as a new test, that'd be great :)

[cnasikas]

@nventuro Sorry for the late response. I tried to fix it locally as follows and it resolves my issue (the id should be change to something unique).

const _ = require('lodash')
const { promisify } = require('util')

const advanceBlock = async (num = 1) => {
  for (const n in _.range(num)) {
    await promisify(web3.currentProvider.send.bind(web3.currentProvider))({
      jsonrpc: '2.0',
      method: 'evm_mine',
      id: n
    })
  }
}

await advanceBlock()

Snippet that triggers the bug:

const Web3 = require('web3')

const web3 = new Web3(new Web3.providers.WebsocketProvider('ws://localhost:7545'))

require('openzeppelin-test-helpers/configure')({ web3 })

const { time } = require('openzeppelin-test-helpers')

const main = async () => {
  await time.advanceBlock()
}

main()
  .catch(err => console.log(err))

[nventuro]

Closed via #92