-
Notifications
You must be signed in to change notification settings - Fork 80
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: merge pending state update and state update. #128
base: master
Are you sure you want to change the base?
Conversation
previously caused a conflict where state update objects was valid against both state update and pending state update so the schema validation failed for get_state_update api call since it specifies "oneOf".
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @nagmo-starkware and the rest of your teammates on Graphite |
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ArielElp, @EvyatarO, and @nagmo-starkware)
api/starknet_api_openrpc.json
line 1204 at r1 (raw file):
}, "required": [ "storage_diffs",
The problem here is that one of the non-pending fields can appear and you don't require that if one of them appears all of them appear
This is also a bit unreadable. Maybe in this object we can put required at the top of the fields?
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @EvyatarO and @ShahakShama)
api/starknet_api_openrpc.json
line 1204 at r1 (raw file):
Previously, ShahakShama wrote…
The problem here is that one of the non-pending fields can appear and you don't require that if one of them appears all of them appear
This is also a bit unreadable. Maybe in this object we can put required at the top of the fields?
Can we do the same trick with additional_properties = false in PENDING_STATE_UPDATE?
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ArielElp, @EvyatarO, and @ShahakShama)
api/starknet_api_openrpc.json
line 1204 at r1 (raw file):
Previously, ArielElp wrote…
Can we do the same trick with additional_properties = false in PENDING_STATE_UPDATE?
no we can't. state_update
references pending_state_update
if we put additional_properties=false
for the pending status update then the state update schema fails since internally there's a part that says it can't have the additional fields (that the state update adds).
this exact example is described in the docs: https://json-schema.org/understanding-json-schema/reference/object.html#extending-closed-schemas
what do you think of this as a solution: https://json-schema.org/understanding-json-schema/reference/conditionals.html#dependentrequired ?
This change is