test(ethereum): fix broken validations in tests #3496
Conversation
Primary Changes --------------- 1. Refactored negative test case exception assertions for cactus-plugin-ledger-connector-ethereum. Removed try-catch blocks, replaced with declarations through jest-extended's own API. 2. Made comments on specific tests where the tests should fail but are actually passing and thus cannot be refactored before being investigated further. Fixes hyperledger#3475 Signed-off-by: ashnashahgrover <[email protected]>
Primary Changes --------------- 1. Added code to deployContract in plugin-ledger-connector-ethereum.ts that checks if any unexpected arguments are present, and if so throws an error. 2. Modified test on line 169 in geth-invoke-web3-method-v1.test.ts so that required argument for invokeRawWeb3EthMethod is missing as required by the test description. Fixes hyperledger#3487 Signed-off-by: ashnashahgrover <[email protected]>
ashnashahgrover
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
| // Validate the keys in the request object | ||
| const validKeys = [ | ||
| "web3SigningCredential", | ||
| "contract", | ||
| "constructorArgs", | ||
| "gasConfig", | ||
| "value", | ||
| ]; | ||
| const extraKeys = Object.keys(req).filter( | ||
| (key) => !validKeys.includes(key), | ||
| ); | ||
|
|
||
| if (extraKeys.length > 0) { | ||
| throw new Error(`Invalid parameters: ${extraKeys.join(", ")}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
@ashnashahgrover The Open API spec is already declaring the request object with additional properties being forbidden, so in theory the openapi validator should reject any request with random other properties in them even without this change.
Could you please confirm if this logic is working (without the code you added here) and if it's not then please also open a separate issue to fix the root cause in the open API validation logic.
The relevant part of the spec I'm talking about:
"additionalProperties": false,
"DeployContractV1Request": {
"type": "object",
"required": ["web3SigningCredential", "contract"],
"additionalProperties": false,
"properties": {
"web3SigningCredential": {
"$ref": "#/components/schemas/Web3SigningCredential"
},
"contract": {
"oneOf": [
{
"$ref": "#/components/schemas/ContractJsonDefinition"
},
{
"$ref": "#/components/schemas/ContractKeychainDefinition"
}
],
"nullable": false
},
"constructorArgs": {
"description": "The list of arguments to pass in to the constructor of the contract being deployed.",
"type": "array",
"default": [],
"items": {}
},
"gasConfig": {
"$ref": "#/components/schemas/GasTransactionConfig"
},
"value": {
"type": "string",
"description": "Ether balance to send on deployment.",
"nullable": false
}
}
},
There was a problem hiding this comment.
Yes, I had the same query @petermetz , to which @ashnashahgrover has mentioned that even with additional properties, it still pass, which was very strange.
@ashnashahgrover can you confirm on the above once again?
There was a problem hiding this comment.
@jagpreetsinghsasan @ashnashahgrover OK, that sounds unfortunately like a completely different bug in the validation itself to be fixed. Could you please open a bug with detailed reproduction steps and then we can work on fixing that separately.
There was a problem hiding this comment.
Yes even with additional properties it fails, and I am investigating why. Will open a seperate bug for this!
Commit to be reviewed
test(ethereum): fix broken validations in tests
Fixes #3487
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.