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

JSON RPC interface spec: chainHead_v1 #813

Merged
merged 12 commits into from
Sep 8, 2024

Conversation

voliva
Copy link
Contributor

@voliva voliva commented Aug 26, 2024

More work for #801

This is the most important group of functions to be implemented from the spec. This makes PAPI already compatible except to send transactions (which needs the transaction_v1 group of functions).

It's not fully spec-compliant, specifically for the chainHead_v1_storage call, which has 2 variations that are more complex (closestDescendantMerkleValue, hash, descendantsHashes, and dealing with childTrie), but none of those features are used by PAPI. I rather make small incremental PRs, so I would initially focus on having full PAPI compatibility, and later on we can make it full spec compliant.

I'd also like to add tests, what do you prefer? Should I raise them as a separate PR or would you like it in this one? I'm thinking on keep raising incremental PRs, first I'll work on the tests for chainHead_v1, then implement transaction_v1 and its tests, and lastly chainSpec_v1 (the last group used by PAPI, but the least important).

@ermalkaleci
Copy link
Collaborator

Please add tests into this PR

@voliva voliva marked this pull request as draft August 26, 2024 11:23
@voliva
Copy link
Contributor Author

voliva commented Aug 27, 2024

@ermalkaleci I see that the e2e tests use PolkadotJS to test everything works as expected, but PolkadotJS doesn't use the new RPC-interface

Is it alright if I add polkadot-api (or a lower level sub-package) as a devDependency to run the e2e tests for the new rpc spec?

@ermalkaleci
Copy link
Collaborator

@ermalkaleci I see that the e2e tests use PolkadotJS to test everything works as expected, but PolkadotJS doesn't use the new RPC-interface

Is it alright if I add polkadot-api (or a lower level sub-package) as a devDependency to run the e2e tests for the new rpc spec?

of course, go ahead

@voliva voliva marked this pull request as ready for review September 3, 2024 08:38
@voliva
Copy link
Contributor Author

voliva commented Sep 3, 2024

I've finished adding tests - I had a bit of back-and-forth because I wasn't sure which level should I use for them.

One of the issues I had is that the block the tests run with is using a snapshot with a very old version of the metadata (v9 if I'm not wrong), which is not compatible with the higher-level papi libraries (since it relies on metadata v14+ to create the codecs)

Settled with @polkadot-api/substrate-client, since this just interfaces with the JSON RPC interface spec without encoding or decoding SCALE. I had to use polkadot.js to create some of the keys for the test.

@xlc
Copy link
Member

xlc commented Sep 3, 2024

you could add a new env for papi tests if it helps

packages/e2e/src/helper.ts Outdated Show resolved Hide resolved
packages/e2e/src/helper.ts Show resolved Hide resolved
@voliva voliva requested a review from xlc September 5, 2024 08:20
@xlc xlc merged commit c9f9ada into AcalaNetwork:master Sep 8, 2024
6 checks passed
@voliva voliva deleted the chainHead branch September 9, 2024 06:30
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 this pull request may close these issues.

3 participants