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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
438a800
test(ethereum): refactor jest test negative test cases
ashnashahgrover Aug 12, 2024
b216088
test(ethereum): fix broken validations in tests
ashnashahgrover Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...-plugin-ledger-connector-ethereum/src/main/typescript/plugin-ledger-connector-ethereum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,22 @@ export class PluginLedgerConnectorEthereum
): Promise<RunTransactionResponse> {
Checks.truthy(req, "deployContract() request arg");

// 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(", ")}`);
}

Comment on lines +946 to +961
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!

if (isWeb3SigningCredentialNone(req.web3SigningCredential)) {
throw new Error(`Cannot deploy contract with pre-signed TX`);
}
Expand Down
70 changes: 33 additions & 37 deletions .../test/typescript/integration/geth-contract-deploy-and-invoke-using-json-object-v1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
LogLevelDesc,
IListenOptions,
Servers,
LoggerProvider,
} from "@hyperledger/cactus-common";
import { PluginRegistry } from "@hyperledger/cactus-core";
import { Configuration, Constants } from "@hyperledger/cactus-core-api";
Expand All @@ -50,6 +51,12 @@ const containerImageName = "ghcr.io/hyperledger/cacti-geth-all-in-one";
const containerImageVersion = "2023-07-27-2a8c48ed6";

describe("Ethereum contract deploy and invoke using keychain tests", () => {
const logLevel: LogLevelDesc = "info";
const log = LoggerProvider.getOrCreate({
label: "geth-contract-deploy-and-invoke-using-json-object-v1.test.ts",
level: logLevel,
});

const keychainEntryKey = uuidV4();
let testEthAccount: {
address: HexString;
Expand Down Expand Up @@ -227,24 +234,23 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
});

test("deployContract without contractJSON should fail", async () => {
try {
await apiClient.deployContract({
await expect(
apiClient.deployContract({
contract: {} as ContractJsonDefinition,
web3SigningCredential: {
ethAccount: WHALE_ACCOUNT_ADDRESS,
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
});
fail("Expected deployContract call to fail but it succeeded.");
} catch (error) {
console.log("deployContract failed as expected");
}
}),
).rejects.toThrow();

log.info("deployContract failed as expected");
});

test("deployContract with additional parameters should fail", async () => {
try {
await apiClient.deployContract({
await expect(
apiClient.deployContract({
contract: {
contractJSON: HelloWorldContractJson,
},
Expand All @@ -255,11 +261,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
},
gas: 1000000,
fake: 4,
} as DeployContractV1Request);
fail("Expected deployContract call to fail but it succeeded.");
} catch (error) {
console.log("deployContract failed as expected");
}
} as DeployContractV1Request),
).rejects.toThrow();

log.info("deployContract failed as expected");
});

//////////////////////////////////
Expand All @@ -285,8 +290,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
expect(setNameOut).toBeTruthy();
expect(setNameOut.data).toBeTruthy();

try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractJSON: HelloWorldContractJson,
contractAddress,
Expand All @@ -299,11 +304,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
});
fail("Expected getContractInfoKeychain call to fail but it succeeded.");
} catch (error) {
expect(error).toBeTruthy();
}
}),
).rejects.toBeTruthy();

const getNameOut = await apiClient.invokeContractV1({
contract: {
Expand Down Expand Up @@ -366,8 +368,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
expect(setNameOut).toBeTruthy();
expect(setNameOut.data).toBeTruthy();

try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractJSON: HelloWorldContractJson,
contractAddress,
Expand All @@ -383,11 +385,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: testEthAccount.privateKey,
type: Web3SigningCredentialType.PrivateKeyHex,
},
});
fail("Expected getContractInfoKeychain call to fail but it succeeded.");
} catch (error) {
expect(error).toBeTruthy();
}
}),
).rejects.toBeTruthy();

const invokeGetNameOut = await apiClient.invokeContractV1({
contract: {
Expand All @@ -410,8 +409,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
});

test("invokeContractV1 without methodName should fail", async () => {
try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractJSON: HelloWorldContractJson,
contractAddress,
Expand All @@ -423,12 +422,9 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
} as InvokeContractV1Request);
fail(
"Expected deployContractSolBytecodeV1 call to fail but it succeeded.",
);
} catch (error) {
console.log("deployContractSolBytecodeV1 failed as expected");
}
} as InvokeContractV1Request),
).rejects.toThrow();

log.info("deployContractSolBytecodeV1 failed as expected");
});
});
97 changes: 48 additions & 49 deletions ...src/test/typescript/integration/geth-contract-deploy-and-invoke-using-keychain-v1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
LogLevelDesc,
IListenOptions,
Servers,
LoggerProvider,
} from "@hyperledger/cactus-common";
import { PluginRegistry } from "@hyperledger/cactus-core";
import { Configuration, Constants } from "@hyperledger/cactus-core-api";
Expand Down Expand Up @@ -54,6 +55,11 @@ const containerImageName = "ghcr.io/hyperledger/cacti-geth-all-in-one";
const containerImageVersion = "2023-07-27-2a8c48ed6";

describe("Ethereum contract deploy and invoke using keychain tests", () => {
const log = LoggerProvider.getOrCreate({
label: "geth-contract-deploy-and-invoke-using-keychain-v1.test.ts",
level: testLogLevel,
});

const keychainEntryKey = uuidV4();
let testEthAccount: {
address: HexString;
Expand Down Expand Up @@ -230,8 +236,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
});

test("deployContract without contractName should fail", async () => {
try {
await apiClient.deployContract({
await expect(
apiClient.deployContract({
contract: {
keychainId: keychainPlugin.getKeychainId(),
} as ContractKeychainDefinition,
Expand All @@ -240,16 +246,15 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
});
fail("Expected deployContract call to fail but it succeeded.");
} catch (error) {
console.log("deployContract failed as expected");
}
}),
).rejects.toThrow();

log.info("deployContract failed as expected");
});

test("deployContract with additional parameters should fail", async () => {
try {
await apiClient.deployContract({
await expect(
apiClient.deployContract({
contract: {
contractName: HelloWorldContractJson.contractName,
keychainId: keychainPlugin.getKeychainId(),
Expand All @@ -261,11 +266,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
},
gas: 1000000,
fake: 4,
} as DeployContractV1Request);
fail("Expected deployContract call to fail but it succeeded.");
} catch (error) {
console.log("deployContract failed as expected");
}
} as DeployContractV1Request),
).rejects.toThrow();

log.info("deployContract failed as expected");
});

//////////////////////////////////
Expand All @@ -291,8 +295,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
expect(setNameOut).toBeTruthy();
expect(setNameOut.data).toBeTruthy();

try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractName: HelloWorldContractJson.contractName,
keychainId: keychainPlugin.getKeychainId(),
Expand All @@ -305,11 +309,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
});
fail("Expected invokeContractV1 call to fail but it succeeded.");
} catch (error) {
expect(error).toBeTruthy();
}
}),
).rejects.toThrow();

log.info("invokeContractV1 failed as expected");

const getNameOut = await apiClient.invokeContractV1({
contract: {
Expand Down Expand Up @@ -385,16 +388,15 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
});

test("runTransactionV1 without transaction config should fail", async () => {
try {
await apiClient.runTransactionV1({
await expect(
apiClient.runTransactionV1({
web3SigningCredential: {
type: Web3SigningCredentialType.None,
},
} as RunTransactionRequest);
fail("Expected runTransactionV1 call to fail but it succeeded.");
} catch (error) {
console.log("runTransactionV1 failed as expected");
}
} as RunTransactionRequest),
).rejects.toThrow();

log.info("runTransactionV1 failed as expected");
});

test("invoke Web3SigningCredentialType.PrivateKeyHex", async () => {
Expand All @@ -420,8 +422,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
expect(setNameOut).toBeTruthy();
expect(setNameOut.data).toBeTruthy();

try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractName: HelloWorldContractJson.contractName,
keychainId: keychainPlugin.getKeychainId(),
Expand All @@ -437,11 +439,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: testEthAccount.privateKey,
type: Web3SigningCredentialType.PrivateKeyHex,
},
});
fail("Expected invokeContractV1 call to fail but it succeeded.");
} catch (error) {
expect(error).toBeTruthy();
}
}),
).rejects.toThrow();

log.info("invokeContractV1 failed as expected");

const invokeGetNameOut = await apiClient.invokeContractV1({
contract: {
Expand Down Expand Up @@ -490,8 +491,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
expect(setNameOut).toBeTruthy();
expect(setNameOut.data).toBeTruthy();

try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractName: HelloWorldContractJson.contractName,
keychainId: keychainPlugin.getKeychainId(),
Expand All @@ -507,11 +508,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
});
fail("Expected invokeContractV1 call to fail but it succeeded.");
} catch (error) {
expect(error).toBeTruthy();
}
}),
).rejects.toThrow();

log.info("invokeContractV1 failed as expected");

const invokeGetNameOut = await apiClient.invokeContractV1({
contract: {
Expand All @@ -530,8 +530,8 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
});

test("invokeContractV1 without methodName should fail", async () => {
try {
await apiClient.invokeContractV1({
await expect(
apiClient.invokeContractV1({
contract: {
contractName: HelloWorldContractJson.contractName,
keychainId: keychainPlugin.getKeychainId(),
Expand All @@ -543,11 +543,10 @@ describe("Ethereum contract deploy and invoke using keychain tests", () => {
secret: "",
type: Web3SigningCredentialType.GethKeychainPassword,
},
} as InvokeContractV1Request);
fail("Expected invokeContractV1 call to fail but it succeeded.");
} catch (error) {
console.log("invokeContractV1 failed as expected");
}
} as InvokeContractV1Request),
).rejects.toThrow();

log.info("invokeContractV1 failed as expected");
});

// @todo - move to separate test suite
Expand Down
Loading
Loading