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(katana-rpc): add ContractErrorData when it applies #1297

Merged
merged 7 commits into from
Dec 18, 2023

Conversation

glihm
Copy link
Collaborator

@glihm glihm commented Dec 16, 2023

Add the actual data for the ContractError (40) which is the only one in the specification of have such inner error object.

When we will be compatible with 0.6.0, TransactionExecutionError will also have one.

@kariy I don't know if it's the most elegant way to do so. But introducing the object into the ContractError variant break the Copy trait.

Copy link
Member

@kariy kariy left a comment

Choose a reason for hiding this comment

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

I think it's more idiomatic to make the ContractError variant as a struct.

Removing the Copy trait should be fine.

@glihm
Copy link
Collaborator Author

glihm commented Dec 17, 2023

I think it's more idiomatic to make the ContractError variant as a struct.

Removing the Copy trait should be fine.

That's a nice option, I've made a new proposition. The only point is that, we can't cast into i32 anymore if we have internal fields in the variants.
For this reason, I've proposed to move the error code (which is a bit less readable IMO for the code). But we have a better extensibility in the future I think when new error may have data fields.

I've also tried to introduce a new type ApiRpcError, that katana and starknet APIs can return. And then we only use the convertion from ApiRpcError into jsonrpsee::Error. But I'm not sure if it's a good think to add a new layer of abstraction here.

WDYT?

crates/katana/rpc/src/api/starknet.rs Outdated Show resolved Hide resolved
crates/katana/rpc/src/api/starknet.rs Outdated Show resolved Hide resolved
crates/katana/rpc/src/api/starknet.rs Outdated Show resolved Hide resolved
crates/katana/rpc/src/api/starknet.rs Outdated Show resolved Hide resolved
@glihm glihm requested a review from kariy December 17, 2023 23:33
@kariy kariy merged commit fc24a5a into dojoengine:main Dec 18, 2023
9 checks passed
@glihm glihm deleted the feat/katana-error-data branch December 20, 2023 16:50
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