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

Chopsticks client is not compatible with Papi #801

Closed
Ad96el opened this issue Aug 14, 2024 · 10 comments · Fixed by #820
Closed

Chopsticks client is not compatible with Papi #801

Ad96el opened this issue Aug 14, 2024 · 10 comments · Fixed by #820

Comments

@Ad96el
Copy link

Ad96el commented Aug 14, 2024

Description

When attempting to create a client instance in PAPI, Chopsticks throws the following error:

ERROR (ws): Invalid request: {"jsonrpc":"2.0","id":"1-1","method":"chainHead_v1_follow","params":[true]}
app: "chopsticks"

After some investigation, it appears that the desired RPC call is not available after forking.

Steps to Reproduce

  1. Connect to any network using Polkadot.js and verify if the chainHead_v1_follow method is available under Developer > RPC calls > rpc > methods.
  2. Fork the network.
  3. Check if the method is still listed. It should no longer be available.
@ermalkaleci
Copy link
Collaborator

Chopsticks is a client itself and it and has its own methods, this one is not implemented

@xlc
Copy link
Member

xlc commented Aug 14, 2024

yeah we need to implement those new methods

@Ad96el Ad96el changed the title Chopsticks node is not compatible with Papi Chopsticks client is not compatible with Papi Aug 15, 2024
@Ad96el
Copy link
Author

Ad96el commented Aug 15, 2024

@xlc, thanks for the response. Could you estimate which version will include the fix?

@xlc
Copy link
Member

xlc commented Aug 15, 2024

Unfortunately, I can't provide an estimate as we don't have extra resources atm
However happy to mentor and review if anyone is interested working on this

@rflechtner
Copy link

There are ways for papi to work with clients that do not yet implement these rpc methods (https://papi.how/requirements#polkadot-sdk-110--x--1110) - as long as the client at least acts as a client on polkadot api > v1.1.0 would. We're still running into issues with this method though, which strangely results in the following error message from the chopsticks client:

Invalid request: {"jsonrpc":"2.0","id":"__INTERNAL_ID","method":"rpc_methods","params":[]}

Any idea what the issue may be here? May it be that the id coming from papi is throwing it off?

@ermalkaleci
Copy link
Collaborator

id needs to be number

@voliva
Copy link
Contributor

voliva commented Aug 15, 2024

Mhh I see we have to document this better, but Polkadot-API is built on top of the new Polkadot JSON-RPC spec, and requires chainHead, rpc, chainSpec and transaction group of functions to be implemented in the node.

The compatibility layer from polkadot-api/polkadot-sdk-compat fixes some implementation bugs of this spec that were present in older versions of Polkadot-SDK, and that we can easily fix on the client-side. But the RPC methods defined in the new specification must be there.

Another issue might be this:

id needs to be number

However, the JSON-RPC spec (the global one) specifies that the id parameter of the request object must be an integer, a string or null: https://www.jsonrpc.org/specification#request_object - And it is properly handled by polkadot-sdk nodes too.

All of the requests coming from PAPI use string id, in the shape of {interger}-{integer} (because for some use cases it needs to multiplex across different chains, so the first integer represents the chainId, and the second the requestId)

@josepot
Copy link

josepot commented Aug 15, 2024

However happy to mentor and review if anyone is interested working on this

We would be super happy to collaborate on this. We can assign some resources for the upcoming OpenGov proposal to tackle this. Meaning that I would be super happy to be mentored and contribute 🙂

id needs to be number

As @voliva pointed out this is bad, as it makes the API not JSON-RPC compliant. In other words: this API seems to be tightly coupled to an implementation detail of PJS... which kinda defeats the point of having a JSON-RPC interface.

@xlc
Copy link
Member

xlc commented Aug 16, 2024

Here are some pointers for people wanting to contribute:

To update JSON RPC id type schema:

To implement new RPC methods:

Add a new file if needed (e.g. chainHead.ts): https://github.com/AcalaNetwork/chopsticks/tree/master/packages/core/src/rpc/substrate

and have it added to the handlers: https://github.com/AcalaNetwork/chopsticks/blob/7c4a7a92a4ea774ae10f1a2b1349063f760990e0/packages/core/src/rpc/substrate/index.ts

Implement the handler methods. Use existing methods as an example. e.g.

export const chain_subscribeNewHead: Handler<void, string> = async (context, _params, { subscribe }) => {

and that's pretty much it.

The JSON RPC spec should contain all the info about what's needed: https://paritytech.github.io/json-rpc-interface-spec/

However, I will suggest just use papi to connect to Chopsticks, and implement whatever missing RPC methods it complaints until it runs to get an MVP version out.

The API docs for Chopsticks core types could be helpful: https://acalanetwork.github.io/chopsticks/docs/core/README.html

In theory, the existing core types should be enough to implement all the RPC handlers but let me know if that's not the case.

@xlc
Copy link
Member

xlc commented Sep 15, 2024

Making a new release with papi support https://github.com/AcalaNetwork/chopsticks/releases/tag/0.15.0

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

Successfully merging a pull request may close this issue.

6 participants