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

feat(jsonrpc): add error data field from JSON RPC spec #483

Closed
wants to merge 9 commits into from

Conversation

glihm
Copy link
Contributor

@glihm glihm commented Oct 26, 2023

Currently, when the JSON RPC error is ContractError(40), we loose the data field that is only present for the CONTRACT_ERROR error type from the spec.

This PR aims at adding the revert_error that is present in data to the error message.
I've tried to modified the structure, but due to the current state of the spec, @xJonathanLEI do you think that is enough, or we should prepare a struct that can be forward compatible with other possible error coming in the future?

In anyway, it's useful to have a more explicit error message in the case of contract error. We may add a revert_reason field?

Any comment feedback very welcomed!

@glihm
Copy link
Contributor Author

glihm commented Oct 27, 2023

@xJonathanLEI changed this one to also add the data error when the error is a more generic one and not only starknet contract error.

@xJonathanLEI
Copy link
Owner

When you say there's a data field in the CONTRACT_ERROR variant, you're actually referencing v0.5.x of the specs (btw please use permalink instead when referencing files). starknet-rs supports v0.4.0 only at the moment. It's wrong to expect a data field.

This PR shouldn't be merged at least until we move to v0.5.x.

@glihm
Copy link
Contributor Author

glihm commented Oct 29, 2023

When you say there's a data field in the CONTRACT_ERROR variant, you're actually referencing v0.5.x of the specs (btw please use permalink instead when referencing files). starknet-rs supports v0.4.0 only at the moment. It's wrong to expect a data field.

This PR shouldn't be merged at least until we move to v0.5.x.

You're totally right for the CONTRACT_ERROR variant. And the we shouldn't add the data behavior on this specific error until v0.5.x.

However the PR also addresses the JSON RPC spec, where the error object contains a data field that we are not using as it can be omitted but can also be present following the spec. https://www.jsonrpc.org/specification#error_object

Can we please consider this part of the PR and I remove the CONTRACT_ERROR part?

@glihm glihm changed the title feat: add contract error data into the rpc error message feat: add error data field from JSON RPC spec Oct 29, 2023
@glihm
Copy link
Contributor Author

glihm commented Oct 29, 2023

I Will rename and and change the context accordingly to have a better scope on this PR.

@glihm
Copy link
Contributor Author

glihm commented Nov 6, 2023

@xJonathanLEI I've changed the PR to only address the compatibility with the JSON RPC spec (not the JSON RPC of starknet, but the underlying protocol).
When the new Starknet JSON RPC spec is supported, we will be able to leverage this field for the contract error or any other coming in the future.

WDYT?

@glihm glihm changed the title feat: add error data field from JSON RPC spec feat(jsonrpc): add error data field from JSON RPC spec Nov 24, 2023
@xJonathanLEI
Copy link
Owner

As much as I agree that we definitely need the ability to parse the data field from JSON-RPC errors, I don't think representing everything as a String is the optimal way of doing this.

@xJonathanLEI
Copy link
Owner

Superseded by #513.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants