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(gateway): create error type that matches the spec #487

Closed
wants to merge 1 commit into from

Conversation

yair-starkware
Copy link
Contributor

@yair-starkware yair-starkware commented Jul 16, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.50%. Comparing base (e0d1c62) to head (b5e1f78).
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   83.33%   83.50%   +0.17%     
==========================================
  Files          37       40       +3     
  Lines        1776     1952     +176     
  Branches     1776     1952     +176     
==========================================
+ Hits         1480     1630     +150     
- Misses        218      238      +20     
- Partials       78       84       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yair-starkware yair-starkware force-pushed the yair/gw_spec_errors branch 3 times, most recently from 627a962 to 7fb4fcc Compare July 16, 2024 12:46
Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)


crates/gateway/resources/starknet_write_api.json line 0 at r1 (raw file):
Move this file to crates/mempool_test_utils/test_files


crates/gateway/resources/starknet_write_api.json line 0 at r1 (raw file):
What is this file? Is this an external resource? Can/ Should we document is somewhere?

Copy link
Contributor

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @yair-starkware)


crates/gateway/src/gateway_test.rs line 144 at r1 (raw file):

        assert_eq!(err_schema.get("data").is_some(), err.data().is_some());
    }

what about checking if all json errors appear in the enum?

Copy link
Contributor Author

@yair-starkware yair-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)


crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Move this file to crates/mempool_test_utils/test_files

Not sure I agree - it is the spec of the gateway, not just for tests


crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

What is this file? Is this an external resource? Can/ Should we document is somewhere?

Starknet RPC has a repo for the specs: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_write_api.json


crates/gateway/src/gateway_test.rs line 144 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

what about checking if all json errors appear in the enum?

Done.

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)


crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Starknet RPC has a repo for the specs: https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_write_api.json

How do we make sure the copy we have is up to date?


crates/gateway/src/gateway_test.rs line 127 at r2 (raw file):

        // Use the error serialization to get the error name, and then use it to get the error
        // schema.
        let err_schema = match serde_json::to_value(&err).unwrap() {

Suggestion:

spec_err_schema

Copy link
Contributor Author

@yair-starkware yair-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)


crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, dafnamatsry wrote…

How do we make sure the copy we have is up to date?

Is it necessary? It doesn't change often, and when it does, it is published in a public Slack channel.
We can use a git submodule, but I don't think it is worth it.
Another option is using the Papyrus RPC methodspecVersion and asserting that it matches the file, but it brings more complications.


crates/gateway/src/gateway_test.rs line 127 at r2 (raw file):

        // Use the error serialization to get the error name, and then use it to get the error
        // schema.
        let err_schema = match serde_json::to_value(&err).unwrap() {

Done.

Copy link
Contributor

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)


crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Is it necessary? It doesn't change often, and when it does, it is published in a public Slack channel.
We can use a git submodule, but I don't think it is worth it.
Another option is using the Papyrus RPC methodspecVersion and asserting that it matches the file, but it brings more complications.

Ok, let's keep it like that for now.
Can you add a TODO to think about a way to automatically keep track of changes to this file?

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@yair-starkware
Copy link
Contributor Author

crates/gateway/resources/starknet_write_api.json line at r1 (raw file):

Previously, dafnamatsry wrote…

Ok, let's keep it like that for now.
Can you add a TODO to think about a way to automatically keep track of changes to this file?

Done

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yair-starkware)


crates/gateway/src/lib.rs line 22 at r4 (raw file):

// TODO(yair): Find a way to detect changes in the RPC spec
// (https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_write_api.json).
// Currently, the spec file is copied manually.

Why is this TODO here and not somewhere else?
Not that I have a better suggestion for its location.

Code quote:

// TODO(yair): Find a way to detect changes in the RPC spec
// (https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_write_api.json).
// Currently, the spec file is copied manually.

@yair-starkware
Copy link
Contributor Author

crates/gateway/src/lib.rs line 22 at r4 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why is this TODO here and not somewhere else?
Not that I have a better suggestion for its location.

Where else can I put it?
It's about a crate level resource

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yair-starkware)

@yair-starkware
Copy link
Contributor Author

Opened a PR in the sequencer repo: starkware-libs/sequencer#120

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.

5 participants