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 API docs updates #32747

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Aug 7, 2023

Problem

We've noticed a few things that are inaccurate in the JSON RPC docs while building out the new Web3 JS library.

Some methods have parameters marked as optional that are actually required, and other similar minor errors.

Summary of Changes

  • getBlocksWithLimit: Make limit a required parameter
  • getHighestSnapshotSlot: Response field incremental should be of type <u64|null> not <u64|undefined>
  • getSignatureStatuses: Make signatures a required parameter
  • getVersion:
    • Response code block should include feature-set
    • Added types of both response fields (ie. solana-core: string & feature-set: number)

Fixes #32696


I'm curious why the response for RpcVersionInfo is set to Kebab case, rather than Camel case. Surely there's a good reason for this, but it throws off client library management since the rest of the methods appear to adhere to Camel case. Just curious why this is done (Source)


Note: This will remain a draft until we've finished implementing all remaining RPC HTTP methods (see solana-labs/solana-web3.js#1449) and Web Socket methods (see solana-labs/solana-web3.js#1505)

@mergify mergify bot added community Community contribution need:merge-assist labels Aug 7, 2023
@mergify mergify bot requested a review from a team August 7, 2023 18:48
@buffalojoec buffalojoec force-pushed the rpc-docs-fixes branch 3 times, most recently from 7459f79 to 864312a Compare August 7, 2023 19:19
@joncinque joncinque requested a review from steviez August 14, 2023 18:29
@steviez
Copy link
Contributor

steviez commented Aug 14, 2023

Note: This will remain a draft until we've finished implementing all remaining RPC HTTP methods

@buffalojoec - Based on the status of the linked PR's, I don't think you have finished what you had mentioned wanting to finish. That being said, I don't necessarily think you have to collect all of the and wait to push until you have completely gone through all methods. I don't imagine this PR would grow that big, but for example, if we started to hit 100's of lines changed, breaking up into smaller PR's would preferred if possible.

Either is fine with me - you can sit on this until you've finished or you can push fixes in incrementally as you find them. Either way, when you'd like me to take a look, please move the PR out of Draft state and @-mention me in a comment to be sure I get a notification (I don't think moving from draft to ready-for-review sends a notification)

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Aug 14, 2023

Note: This will remain a draft until we've finished implementing all remaining RPC HTTP methods

@buffalojoec - Based on the status of the linked PR's, I don't think you have finished what you had mentioned wanting to finish. That being said, I don't necessarily think you have to collect all of the and wait to push until you have completely gone through all methods. I don't imagine this PR would grow that big, but for example, if we started to hit 100's of lines changed, breaking up into smaller PR's would preferred if possible.

Either is fine with me - you can sit on this until you've finished or you can push fixes in incrementally as you find them. Either way, when you'd like me to take a look, please move the PR out of Draft state and @-mention me in a comment to be sure I get a notification (I don't think moving from draft to ready-for-review sends a notification)

Sounds good to me. We actually are on the last HTTP methods and - as you said - we can always pen a new PR with any additional findings. Why don't we go ahead and mark this ready and we can explore opening another new one if we spot anything else?

@steviez

@buffalojoec buffalojoec marked this pull request as ready for review August 14, 2023 19:36
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@buffalojoec
Copy link
Contributor Author

LGTM

I can't merge! 😥

@steviez steviez merged commit de4eee1 into solana-labs:master Aug 15, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON RPC Method getBlocksWithLimit Requires 2 Parameters but Docs Say 1
2 participants