Implement json param validation #2118
Conversation
… blockHash, Tx Hash and Hex Index parameters and deserializers
….. related methods having parameter validation
|
I think this PR should be merged into the branch json_rpc_param_validation |
| return callArguments; | ||
| } | ||
|
|
||
| private boolean isPropertyValidHex(String propertyValue) { |
There was a problem hiding this comment.
I think we should use existent parameters as properties instead of repeat validations along the different parameter classes.
| return HexUtils.isHex(propertyValue.toLowerCase(), hasPrefix ? 2 : 0); | ||
| } | ||
|
|
||
| private void validateAddressProperty(String propertyValue, String propertyName) { |
There was a problem hiding this comment.
We have Address param with its validation for this.
| } | ||
| } | ||
|
|
||
| private void validateDataProperty(String propertyValue) { |
There was a problem hiding this comment.
we could use HexDataPam instead
| public class BlockIdentifierParam implements Serializable { | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private static final List<String> STRING_IDENTIFIERS = Arrays.asList("earliest", "latest", "pending"); |
There was a problem hiding this comment.
These definitions are being repeated in other places. I thin we should have a common class for them. This is a proposal I did in other PR to solve a similar issue. https://github.com/rsksmart/rskj/pull/2080/files#diff-e15384bdcacb6015dc99ec6de04a4cf59bcab14c86685b1174a995653e88d41b
|
|
||
| String eth_getBalance(String address) throws Exception; | ||
|
|
||
| String eth_getBalance(String address, Map<String, String> blockRef) throws Exception; // NOSONAR |
There was a problem hiding this comment.
This signature is not fully supported, the new changes doesn't support requireCanonical field as it is being done by the map.
This was commented in slack. I think we should add unit test, because we should be able to detect it automatically.
| } | ||
| } | ||
|
|
||
| private void validateMap(Map<String, String> inputs) { |
There was a problem hiding this comment.
As commented above this implementation does not support requireCanonical functionality
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| public class BlockRefParamTest { |
There was a problem hiding this comment.
These are all the example cases we should be supported by BlockRefParam class
- "earliest"
- "0x0"
- { "blockNumber": "0x0" }
- { "blockHash": "0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3" } (hash of the genesis block on the Ethereum main chain)
- { "blockHash": "0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3", "requireCanonical": true }
- { "blockHash": "0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3", "requireCanonical": false }
And I think at least we should test them to be sure that we are supporting them.
|
Closing this PR in order to create another one with the correct target branch. |
|
New PR: #2121 |
Description
Motivation and Context
We want to align GETH's and RSKJ's behavior when handling requests errors to avoid returning "Internal server error" from RSKJ.
How Has This Been Tested?
Using the payloads from the notion document linked in the ticket CORE-3026 for the methods, for the payloads that should return an error, said error should not be "Internal server error". For example:
Should return:
{"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"Invalid block identifier 'first'"}}Types of changes
Checklist: