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

refactor(experimental): Add getBlockProduction API #1263

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

mcintyre94
Copy link
Contributor

Summary

  • Add the getBlockProduction API + unit tests
  • Add jest-extended which provides some extra jest matchers

Test Plan

pnpm turbo test:unit:node test:unit:browser

Misc notes:

@socket-security
Copy link

socket-security bot commented Apr 14, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No dependency changes detected in pull request

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

@mcintyre94 mcintyre94 force-pushed the get-block-production branch 6 times, most recently from ab6148f to 007faa5 Compare April 18, 2023 18:13
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

image

@@ -6,6 +6,7 @@ export const SolanaJsonRpcErrorCode = {
JSON_RPC_SERVER_ERROR_BLOCK_NOT_AVAILABLE: -32004,
JSON_RPC_SERVER_ERROR_BLOCK_STATUS_NOT_AVAILABLE_YET: -32014,
JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX: -32010,
JSON_RPC_SERVER_ERROR_LAST_SLOT_TOO_LARGE: -32602,
Copy link
Collaborator

Choose a reason for hiding this comment

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

That error code is actually a JSON-RPC reserved one that means ‘invalid params.’ https://www.jsonrpc.org/specification#error_object

.getBlockProduction({
range: {
firstSlot: 0n,
lastSlot: 2n ** 63n - 1n, // u64:MAX; safe bet it'll be too high.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason that this doesn't work is actually because the validator hasn't advanced to that slot yet. Apparently we code this as ‘invalid params (32602).’

Kind of sounds like we need an actual error code for this in the RPC, similar to JSON_RPC_SERVER_ERROR_MIN_CONTEXT_SLOT_NOT_REACHED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed the 32602 error to JSON_RPC_INVALID_PARAMS for now, since that best matches the current behaviour. Would a github issue on the solana monorepo be the best place to track adding a custom error code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! Totally. That RPC method could do with throwing a more precise error.

packages/rpc-core/src/rpc-methods/getBlockProduction.ts Outdated Show resolved Hide resolved
packages/rpc-core/src/rpc-methods/getBlockProduction.ts Outdated Show resolved Hide resolved
packages/rpc-core/src/rpc-methods/getBlockProduction.ts Outdated Show resolved Hide resolved
packages/rpc-core/src/rpc-methods/getBlockProduction.ts Outdated Show resolved Hide resolved
@mcintyre94 mcintyre94 force-pushed the get-block-production branch 4 times, most recently from 2c5352c to 0322ad5 Compare April 21, 2023 16:26
@steveluscher
Copy link
Collaborator

steveluscher commented Apr 22, 2023

Dang it, the goal with that return type for the identity case was to make this a type error:

const bp = await rpc
    .getBlockProduction({
        commitment,
        identity: 'abc' as Base58EncodedAddress,
    })
    .send();
bp.value.byIdentity['def' as Base58EncodedAddress]; // SHOULD BE A TYPE ERROR

…but that's not happening yet. Essentially the string type is flowing through that type, rather than the string literal.

Think this is possible to type correctly?

@mcintyre94 mcintyre94 force-pushed the get-block-production branch 2 times, most recently from cc40bac to b01e07e Compare April 24, 2023 14:23
@mcintyre94
Copy link
Contributor Author

mcintyre94 commented Apr 24, 2023

I think this should work with:

    getBlockProduction<TIdentity extends Base58EncodedAddress>(
        config: Readonly<{
            commitment?: Commitment;
            identity: TIdentity;
            range?: SlotRange;
        }>
    ): GetBlockProductionApiResponseBase & GetBlockProductionApiResponseWithSingleIdentity<TIdentity>;

See example playground

My local environment has gone screwy so I need to fix that and test this a bit more locally, but I think this should be right!

@steveluscher
Copy link
Collaborator

This has type errors now when you just supply commitment (see the tests file for the error) and still doesn't constrain the type of byIdentity to having only the one key. If that second thing isn't possible with Typescript, then that's cool ¯_(°ペ)_/¯

- Add the `getBlockProduction` API + unit tests
- Add jest-extended which provides some extra jest matchers

```
pnpm turbo test:unit:node test:unit:browser
```
@mcintyre94 mcintyre94 force-pushed the get-block-production branch from b01e07e to 5ccf373 Compare April 25, 2023 10:53
@mcintyre94
Copy link
Contributor Author

mcintyre94 commented Apr 25, 2023

Ah, thanks, fixed those up! On the second thing, it definitely should work, but also.. isn't. I don't know if some layer of indirection is throwing it here

A more realistic playground using basically the same types here does work!

I think it's probably worth parking for now, but it would be really nice to have that type safety!

@steveluscher steveluscher merged commit edfcbdc into solana-labs:master Apr 25, 2023
@mcintyre94 mcintyre94 deleted the get-block-production branch April 26, 2023 09:18
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

🎉 This PR is included in version 1.76.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants