-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix jsonrpc payload and response types (#4743) #4761
Fix jsonrpc payload and response types (#4743) #4761
Conversation
* Make JsonRpcPayload's `params` field optional Currently jsonrpc.js uses `params: params || []` in the `toPayload` function, so this type update makes the `params` field optional to match. * Fix JsonRpcResponse type Update `id` to accept `string | number` - this now matches the `isValidResponse` function in `jsonrpc.js`. Update `error` to accept an object with optional `code`, `data`, and non-optional `message` fields to more closely match the [JSON RPC spec](https://www.jsonrpc.org/specification#error_object) and the `ErrorResponse` function in `errors.js`. * Remove errant spaces * Add PR #443 to CHANGELOG Co-authored-by: jdevcs <86780488+jdevcs@users.noreply.github.com>
Your Render PR Server URL is https://web3-js-pr-4761.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c81btqfho1kl89ok8p10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've refrained from making any changes to 1.x's types because it's considered a breaking change for those using typescript
@spacesailor24 The JSON RPC error property is 100% wrong in every case as is, so I think that should be fixed as no one can rely on that as it is, and if they somehow are, their runtime code is likely broken. This is technically not a breaking change in Web3's types, as it aligns it with runtime behavior, but for Ganache to have to make the change on our side in v7 it certainly would be breaking (and wrong). Please reconsider merging this into 1.x. |
@davidmurdoch Okay, yea that totally makes sense. We had a blanket policy of not making 1.x type changes, but I should've spent more time understanding this one - weird that the types were even written this way |
Pull Request Test Coverage Report for Build 1975050328
💛 - Coveralls |
e2e_windows were not passing in this PR so opened current PR for checks / fixes .
4743:
params
field optionalCurrently jsonrpc.js uses
params: params || []
in thetoPayload
function, so this type update makes theparams
fieldoptional to match.
Update
id
to acceptstring | number
- this now matches theisValidResponse
function injsonrpc.js
.Update
error
to accept an object with optionalcode
,data
,and non-optional
message
fields to more closely match theJSON RPC spec
and the
ErrorResponse
function inerrors.js
.Remove errant spaces
Add PR Invalid contract output parsing #443 to CHANGELOG
Co-authored-by: jdevcs 86780488+jdevcs@users.noreply.github.com
Description
Please include a summary of the changes and be sure to follow our Contribution Guidelines.
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
and testeddist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.