cacti icon indicating copy to clipboard operation
cacti copied to clipboard

fix(connector-ethereum): additional properties to deployContract does not throw error

Open ashnashahgrover opened this issue 1 year ago • 2 comments

Describe the bug

The deployContract function in cactus-plugin-ledger-connector-ethereum takes DeployContractV1Request as an argument, which already declares additional properties as forbidden in the Open API spec as can be seen here:

"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
          }
        }
      }

However in various tests that input additional properties, and expect an error to be thrown, the test actually passes.

To Reproduce

Run the test beginning on this line.

The test should throw an error, but it passes without an error.

Expected behavior

The test should throw an error saying additional properties are forbidden.

Logs/Stack traces Run the test this way, with the fail statement commented out, as that was actually causing the test to fail rather than the additional parameters provided (fake: 4 is the additional parameter):

  test("deployContract with additional parameters should fail", async () => {
    try {
      const result = await apiClient.deployContract({
        contract: {
          contractJSON: HelloWorldContractJson,
        },
        web3SigningCredential: {
          ethAccount: WHALE_ACCOUNT_ADDRESS,
          secret: "",
          type: Web3SigningCredentialType.GethKeychainPassword,
        },
        gas: 1000000,
        fake: 4,
      } as DeployContractV1Request);
      // fail("Expected deployContract call to fail but it succeeded.");
      console.log("deployContract actually returned a response", result);
    } catch (error) {
      console.log("deployContract failed as expected");
    }
  });

And it will return this in the log:

deployContract actually returned a response { status: 200, statusText: 'OK', headers: Object [AxiosHeaders] { 'x-powered-by': 'Express', 'content-type': 'application/json; charset=utf-8', 'content-length': '993', etag: 'W/"3e1-Cc+on/RU7altdwH6OGOLX2/wWiM"', date: 'Tue, 03 Sep 2024 01:00:19 GMT', connection: 'close' }, config: { transitional: { silentJSONParsing: true, forcedJSONParsing: true, clarifyTimeoutError: false }, ...

Full log here: testPassed.txt

Hyperledger Cactus Plugins/Connectors Used

  • Ethereum

Additional context

ashnashahgrover avatar Sep 03 '24 00:09 ashnashahgrover

@ashnashahgrover Thank you very much, this will help us keep track of the bug!

petermetz avatar Sep 04 '24 22:09 petermetz

From the conversation here: https://github.com/hyperledger/cacti/pull/3496#discussion_r1732296908

TheJagpreet avatar Sep 19 '24 03:09 TheJagpreet

@aldousalvarez @jagpreetsinghsasan After any epic rabbit hole diving session I came to realize that this is not supported by the express openapi validator package that we use so please put this back in the backlog.

https://github.com/cdimascio/express-openapi-validator/issues/104

petermetz avatar Oct 04 '24 21:10 petermetz

@aldousalvarez @jagpreetsinghsasan After any epic rabbit hole diving session I came to realize that this is not supported by the express openapi validator package that we use so please put this back in the backlog.

cdimascio/express-openapi-validator#104

Yes, I had a look into this as well. Putting it back in backlog.

TheJagpreet avatar Oct 07 '24 04:10 TheJagpreet