forest icon indicating copy to clipboard operation
forest copied to clipboard

Fix message deserialisation in `jsonrpsee`

Open elmattic opened this issue 11 months ago • 2 comments

Issue summary

This is a follow-up of https://github.com/ChainSafe/forest/pull/3978.

For testing our Filecoin.ChainNotify code, we relied on some .js scripts (see link below) to open channels and cancel them. However, this script had to be modified when sending the cancel message payload by adding the id field to the JSON payload:

this.sendWs({
  jsonrpc: '2.0',
  method: 'xrpc.cancel',
  params: [json.id],
  // This was added as a workaround
  id: null,
})

The modification is because jsonrpsee can't deserial the message if the id field is missing. This hinders the correct cancelation of a channel in Forest. The id field is optional according to the spec. So, this is likely a bug in this crate.

Task summary

  • [ ] Fork the jsonrpsee crate
  • [ ] Make the fix and test it
  • [ ] Merge the PR into the upstream branch

Acceptance Criteria

  • [ ] Canceling a channel works, even if id field is missing

Other information and links

https://github.com/filecoin-shipyard/js-lotus-client-provider-browser/blob/master/index.js#L163-L166

elmattic avatar Feb 28 '24 09:02 elmattic

From the JSON-RPC 2.0 spec

A Notification is a Request object without an "id" member. A Request object that is a Notification signifies the Client's lack of interest in the corresponding Response object, and as such no Response object needs to be returned to the client. The Server MUST NOT reply to a Notification, including those that are within a batch request.

Thus, without the id field it's regarded a notification and jsonrpsee ignores it. So I don't agree but feel free to open an issue in jsonrpsee if you don't agree...

niklasad1 avatar May 24 '24 07:05 niklasad1

@niklasad1 Thanks for your insight! I'm unsure what to think now, so I will keep this issue on the back burner.

elmattic avatar May 24 '24 13:05 elmattic