Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

JsonRpcError decoding to include message #1336

Merged
merged 2 commits into from
Apr 26, 2019
Merged

JsonRpcError decoding to include message #1336

merged 2 commits into from
Apr 26, 2019

Conversation

CjHare
Copy link
Contributor

@CjHare CjHare commented Apr 26, 2019

PR description

With JsonRpcErrors now having multiple errors with the same code, the message need to be included when decoding the JSON to correctly map the error. (bug identified in EthSigner, fix ported)

Fixed Issue(s)

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Given the current state of things this does seem like an improvement but it doesn't feel like we're in the right place.

When does Pantheon parse a JsonRpcError from JSON and why would we expect it to have exactly the same message as we would have used? It seems like if we parsed a JsonRpcError we should just return an object with the supplied code and message and the message probably shouldn't mean anything much to us programmatically and just be something to help users understand what the code means.

@CjHare
Copy link
Contributor Author

CjHare commented Apr 26, 2019

@ajsutton an interesting point, I'm not entirely sure (or can't recall) the use cases where the JSON errors are needed decoding by Pantheon ...but as the code was already there, I really just keenon keeping things logical correct and consistent across the PegaSys products 🙂

@ajsutton
Copy link
Contributor

Maybe we should try deleting it and see if any tests fail? :)

@CjHare
Copy link
Contributor Author

CjHare commented Apr 26, 2019

👍 I'll experiment with deleting it (different PR maybe 🤔)

@CjHare CjHare merged commit 9a5c073 into PegaSysEng:master Apr 26, 2019
@CjHare CjHare deleted the jsonrpcerror-decode branch April 26, 2019 06:21
ajsutton pushed a commit to ajsutton/pantheon that referenced this pull request Apr 28, 2019
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants