ChainSafe / forest

🌲 Rust Filecoin Node Implementation
https://forest.chainsafe.io
Apache License 2.0
624 stars 149 forks source link

Fix message deserialisation in `jsonrpsee` #4004

Open elmattic opened 6 months ago

elmattic commented 6 months ago

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

Acceptance Criteria

Other information and links

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

niklasad1 commented 3 months ago

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...

elmattic commented 3 months ago

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