Add RPC schema validation #387
Conversation
Add modified RPC specs Add assert_valid_rpc_schema to tests fixup! Add assert_valid_rpc_schema to tests fixup! Add modified RPC specs fixup! Add modified RPC specs Remove unused imports Add comments, cleanup Make `load_schemas` private Update test/rpc/schema.py Co-authored-by: war-in <[email protected]> Move `assert_valid_rpc_schema` below private functions Formatting and lint Add assert_valid_rpc_request decorator initial version Use assert_valid_rpc_request in rpc endpoints Update tests Add gateway_felt Fix rpc endpoints Fix tests Formatting and lint Rename decorator Use better error messages Add better comment Add unevaluatedProperties to oneOf schemas Rename decorator Add message to wrappers Replace `all` with `not any` Simplify _request_schemas_for_method Add handling for both args and kwargs provided Make comments more detailed Use ordered dict Extend validation logic Add schema validation tests Format and lint Add _extract_methods helper function Fix rpc spec Replace additionalProperties with unevaluatedProperties, remove nested unevaluatedProperties Add `__str__` to error wrappers Bump jsonschema, change to standard dependency, bump web3 Format Make schema validation errors more descriptive Use better validation error messages in routes Make rpc_base_transaction_receipt conform to rpc spec correctly Add disable rpc validation arguments Test disabling rpc validation Update docstring Remove valid schema assertions from tests Add docstring Fix lint Remove print Extend schema validation tests Make invalid block_id error message more descriptive Make incorrect params format error message more descriptive Add initial handling for optional arguments in rpc methods Restore getEvents test Change unnecessary test Fix rpc storage test Update rpc schema tests docstrings Fix docstring example Use better sounding errors Add schema optional value validation test Inline _construct_method_name Add missing `required` to rpc_spec_write.py Update _response_schema_for_method to include required properties Update test Update lock after rebase Make `load_json_schema` private again Remove unused rpc spec
There was a problem hiding this comment.
I don't understand why the testing of validation is necessary? My understanding was that the validation is here to provide another layer of security regarding the input/output format.
Other than testing, the observations I left are very minor.
Also, there are conflicts with the base branch, nothing major.
I think that incorrectly working validation can be more harmful than good: If it were to raise errors on correctly formatted inputs or outputs, it would make devnet's rpc unusable. Also, it accepting incorrect inputs or outputs (which it still might do, but tried to limit the cases of this with these tests), will reduce the chances of us introducing new bugs to the rpc because "it is being validated anyway". |
…ror code in correct range
…eck to all PredefinedRpcErrorCode values
|
I did some basic benchmarking by running Personally it is not problematic (and validation can be disabled), but that's something to have in mind. |
# Conflicts: # poetry.lock # starknet_devnet/blueprints/rpc/misc.py
Usage related changes
Closes #379
Since 0.2.1 RPC spec had some problems, I've bundled both read and write api with some of my modifications (changed oneOf to anyOf in couple of places, defined missing
requiredin write api, etc.)Note: I didn't modify
requiredin most parts of the read specification. Until we migrate to new RPC spec (0.3.0 or different), the validation might still not catch some errors, namely some missing parameters in responses if the specification uses inline schema instead of$ref.Development related changes
Checklist:
./scripts/format.sh./scripts/lint.sh