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

test(ethereum): fix broken validations in tests #3496

Conversation

ashnashahgrover
Copy link
Contributor

Commit to be reviewed

test(ethereum): fix broken validations in tests

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 #3487

Pull Request Requirements

  • Rebased onto upstream/main branch 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.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

Comment on lines +946 to +961
// 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(", ")}`);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
          }
        }
      },

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes even with additional properties it fails, and I am investigating why. Will open a seperate bug for this!

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-cacti#3475

Signed-off-by: ashnashahgrover <ashnashahgrover777@gmail.com>
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-cacti#3487

Signed-off-by: ashnashahgrover <ashnashahgrover777@gmail.com>
@petermetz
Copy link
Contributor

Closing this due to inactivity, please feel free to re-open anytime if/when bandwidth is available to take it to the finish line.

@petermetz petermetz closed this Oct 17, 2024
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.

test(ethereum): fix broken validations in ethereum plugin connector tests
3 participants