Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

feat: add commission fields matching RPC spec #29435

Merged
merged 2 commits into from
Dec 30, 2022

Conversation

R-K-H
Copy link
Contributor

@R-K-H R-K-H commented Dec 29, 2022

Problem

The @solana/web3.js SDK InflationReward type and rewards in the meta for transaction status metadata object properties do not match the JSON RPC spec in that they are missing the commission field. This is causing an issue with the types when attempting to access field value.

See:

Summary of Changes

Adds the commission field in the src/connection.ts

  • InflationReward
  • RewardsResult
  • GetInflationRewardResult
  • BlockResponse
  • ParsedBlockResponse
  • VersionedBlockResponse
  • ConfirmedBlock

Adds commission field in the mockRpcResponse in the tests/connection.test.ts file.

@github-actions github-actions bot added the web3.js Related to the JavaScript client label Dec 29, 2022
@mergify mergify bot added the community Community contribution label Dec 29, 2022
@mergify mergify bot requested a review from a team December 29, 2022 16:42
@steveluscher steveluscher changed the title Add commission fields matching RPC spec feat: add commission fields matching RPC spec Dec 29, 2022
@steveluscher steveluscher added the automerge Merge this Pull Request automatically once CI passes label Dec 29, 2022
Copy link
Contributor

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

giphy-1

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Dec 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Dec 29, 2022

automerge label removed due to a CI failure

Copy link
Contributor

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

Looks like you have some Typescript errors. Want to fix those up and resubmit, @R-K-H?

@mergify mergify bot dismissed steveluscher’s stale review December 30, 2022 00:39

Pull request has been modified.

@R-K-H R-K-H force-pushed the update_reward_type branch from ef1b4d8 to abbfc60 Compare December 30, 2022 00:52
@R-K-H
Copy link
Contributor Author

R-K-H commented Dec 30, 2022

Sure thing, I've updated the PR and refactored the previous git commit message to pass lint there as well. I've ran the tests locally with one error from VoteProgram on live test (unrelated I think).

Aside: Looks like it didn't like optional() and since undefined wasn't in the design pattern of current implementation, I just went ahead and continued the pattern for other undefined fields coming from the RPC spec with the null being acceptable.

@@ -767,6 +767,8 @@ export type InflationReward = {
amount: number;
/** post balance of the account in lamports */
postBalance: number;
/** vote account commission when the reward was credited */
commission: number | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is right? The JSON-RPC docs describe this property as commission: <u8|undefined>.

Suggested change
commission: number | null;
commission?: number;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct! Will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I'm really just asking. It's a distinct possibility that the docs are wrong. #27655

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it is returning null from the request, when doing the live testing it does seem to require that the field is nullable. And I know from my own use that making the request to mainnet the JSON response is returning the commission (at least for InflationReward) as a number.

But I'm fixing right now which I suppose supporting the optional(nullable(number())) more closely matches the JSON RPC spec with it being ABLE to be undefined (although I'm not certain it does return without the field).

Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing for undefined would definitely protect against a response from an old RPC fataling the client. I don't think there are any RPCs old enough to return undefined for this property but ¯_(°ペ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed and agreed. Updated.

@R-K-H R-K-H force-pushed the update_reward_type branch from abbfc60 to e24e84c Compare December 30, 2022 01:43
@mergify mergify bot dismissed steveluscher’s stale review December 30, 2022 01:44

Pull request has been modified.

@codecov
Copy link

codecov bot commented Dec 30, 2022

Codecov Report

Merging #29435 (e24e84c) into master (d392378) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #29435   +/-   ##
=======================================
  Coverage    76.5%    76.5%           
=======================================
  Files          54       54           
  Lines        3129     3129           
  Branches      470      470           
=======================================
  Hits         2396     2396           
  Misses        568      568           
  Partials      165      165           

@steveluscher steveluscher added the automerge Merge this Pull Request automatically once CI passes label Dec 30, 2022
@steveluscher steveluscher merged commit a2db104 into solana-labs:master Dec 30, 2022
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Jan 10, 2023
…na-labs#29435)

* fix: adds commission field matching RPC spec

* fix: update optional to follow type pattern
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Jan 10, 2023
…na-labs#29435)

* fix: adds commission field matching RPC spec

* fix: update optional to follow type pattern
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Jan 10, 2023
…na-labs#29435)

* fix: adds commission field matching RPC spec

* fix: update optional to follow type pattern
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Jan 10, 2023
…na-labs#29435)

* fix: adds commission field matching RPC spec

* fix: update optional to follow type pattern
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes community Community contribution web3.js Related to the JavaScript client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants