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

feat/validator tests #388

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
17 changes: 9 additions & 8 deletions validator-cli/.env.dist
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,29 @@ PRIVATE_KEY=
# Devnet RPCs
RPC_CHIADO=https://rpc.chiadochain.net
RPC_ARB_SEPOLIA=https://sepolia-rollup.arbitrum.io/rpc
RPC_GNOSIS=https://rpc.chiadochain.net
RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
RPC_SEPOLIA=

# Testnet or Mainnet RPCs
RPC_ARB=
RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
RPC_ETH=
RPC_GNOSIS=https://rpc.chiadochain.net
Comment on lines +9 to +11
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 3, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consolidate duplicate RPC endpoints.

The same RPC URLs are defined multiple times:

  • RPC_ARB_SEPOLIA and RPC_ARB both point to https://sepolia-rollup.arbitrum.io/rpc
  • RPC_CHIADO and RPC_GNOSIS both point to https://rpc.chiadochain.net

Consider consolidating these into single variables with clear naming to avoid confusion:

# Testnet RPCs
-RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
+RPC_ARB_TESTNET=https://sepolia-rollup.arbitrum.io/rpc  # Arbitrum Sepolia
-RPC_GNOSIS=https://rpc.chiadochain.net
+RPC_GNOSIS_TESTNET=https://rpc.chiadochain.net  # Gnosis Chiado
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
RPC_ETH=
RPC_GNOSIS=https://rpc.chiadochain.net
# Testnet RPCs
RPC_ARB_TESTNET=https://sepolia-rollup.arbitrum.io/rpc # Arbitrum Sepolia
RPC_ETH=
RPC_GNOSIS_TESTNET=https://rpc.chiadochain.net # Gnosis Chiado

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RPC urls are same but the env vars are different for devnet and testnet/mainnet. So it's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mani99brar, understood. Thanks for clarifying your approach regarding separate environment variables for devnet vs. testnet/mainnet. No changes needed here!


✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/.env.dist:9-11
Timestamp: 2025-01-06T06:17:34.081Z
Learning: User (mani99brar) intentionally keeps the same RPC URLs under different environment variables to distinguish between devnet and testnet/mainnet usage, and this is by design rather than an oversight.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


# Testnet or Mainnet Addresses
# VEA Arbitrum to Ethereum
VEAINBOX_ARB_TO_ETH_ADDRESS=0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06
VEAOUTBOX_ARB_TO_ETH_ADDRESS=0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9
# VEA Arbitrum to GNOSIS
VEAINBOX_ARB_TO_GNOSIS_ADDRESS=0x854374483572FFcD4d0225290346279d0718240b
VEAOUTBOX_ARB_TO_GNOSIS_ADDRESS=0x2f1788F7B74e01c4C85578748290467A5f063B0b
VEAROUTER_ARB_TO_GNOSIS_ADDRESS=0x5BE03fDE7794Bc188416ba16932510Ed1277b193
GNOSIS_AMB_ADDRESS=0x8448E15d0e706C0298dECA99F0b4744030e59d7d

VEAOUTBOX_CHAIN_ID=421611

# Devnet Addresses
VEAINBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS=0x906dE43dBef27639b1688Ac46532a16dc07Ce410
VEAOUTBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS=0x906dE43dBef27639b1688Ac46532a16dc07Ce410

#For arbToGnosis bridge
VEAINBOX_ARB_TO_GNOSIS_ADDRESS=0x854374483572FFcD4d0225290346279d0718240b
VEAOUTBOX_ARB_TO_GNOSIS_ADDRESS=0x2f1788F7B74e01c4C85578748290467A5f063B0b
VEAROUTER_ARB_TO_GNOSIS_ADDRESS=0x5BE03fDE7794Bc188416ba16932510Ed1277b193
GNOSIS_AMB_ADDRESS=0x8448E15d0e706C0298dECA99F0b4744030e59d7d

TRANSACTION_BATCHER_CONTRACT_ADDRESS_SEPOLIA=0xe7953da7751063d0a41ba727c32c762d3523ade8
TRANSACTION_BATCHER_CONTRACT_ADDRESS_CHIADO=0xcC0a08D4BCC5f91ee9a1587608f7a2975EA75d73
10 changes: 10 additions & 0 deletions validator-cli/jest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import type { Config } from "jest";

const config: Config = {
preset: "ts-jest",
testEnvironment: "node",
collectCoverage: true,
collectCoverageFrom: ["**/*.ts"],
};

export default config;
8 changes: 6 additions & 2 deletions validator-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@
"yarn": "4.2.2"
},
"scripts": {
"start": "npx ts-node ./src/ArbToEth/watcher.ts",
"start": "npx ts-node ./src/watcher.ts",
"start-chiado-devnet": "npx ts-node ./src/devnet/arbToChiado/happyPath.ts",
"start-sepolia-devnet": "npx ts-node ./src/devnet/arbToSepolia/happyPath.ts",
"start-sepolia-testnet": "npx ts-node ./src/ArbToEth/watcherArbToEth.ts",
"start-arbitrum-to-gnosis": "npx ts-node ./src/ArbToEth/watcherArbToGnosis.ts"
"start-arbitrum-to-gnosis": "npx ts-node ./src/ArbToEth/watcherArbToGnosis.ts",
"test": "jest --coverage"
},
"dependencies": {
"@arbitrum/sdk": "4.0.1",
Expand All @@ -28,6 +29,9 @@
"web3-batched-send": "^1.0.3"
},
"devDependencies": {
"@types/jest": "^29.5.14",
"jest": "^29.7.0",
"ts-jest": "^29.2.5",
"ts-node": "^10.9.2"
}
}
307 changes: 307 additions & 0 deletions validator-cli/src/ArbToEth/transactionHandler.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,307 @@
import { ArbToEthTransactionHandler, ContractType } from "./transactionHandler";
import { MockEmitter, defaultEmitter } from "../utils/emitter";
import { BotEvents } from "../utils/botEvents";
import { ClaimNotSetError } from "../utils/errors";
import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";

describe("ArbToEthTransactionHandler", () => {
let epoch: number = 100;
let veaInbox: any;
let veaOutbox: any;
let veaInboxProvider: any;
let veaOutboxProvider: any;
let claim: ClaimStruct = null;

beforeEach(() => {
veaInboxProvider = {
getTransactionReceipt: jest.fn(),
getBlock: jest.fn(),
};
veaOutbox = {
estimateGas: jest.fn(),
withdrawChallengeDeposit: jest.fn(),
["challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))"]: jest.fn(),
};
veaInbox = {
sendSnapshot: jest.fn(),
};
claim = {
stateRoot: "0x1234",
claimer: "0x1234",
timestampClaimed: 1234,
timestampVerification: 0,
blocknumberVerification: 0,
honest: 0,
challenger: "0x1234",
};
});
describe("constructor", () => {
it("should create a new TransactionHandler without claim", () => {
const transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider
);
expect(transactionHandler).toBeDefined();
expect(transactionHandler.epoch).toEqual(epoch);
expect(transactionHandler.veaOutbox).toEqual(veaOutbox);
expect(transactionHandler.emitter).toEqual(defaultEmitter);
});

it("should create a new TransactionHandler with claim", () => {
const transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
defaultEmitter,
claim
);
expect(transactionHandler).toBeDefined();
expect(transactionHandler.epoch).toEqual(epoch);
expect(transactionHandler.veaOutbox).toEqual(veaOutbox);
expect(transactionHandler.claim).toEqual(claim);
expect(transactionHandler.emitter).toEqual(defaultEmitter);
});
});

describe("checkTransactionStatus", () => {
let transactionHandler: ArbToEthTransactionHandler;
let finalityBlock: number = 100;
const mockEmitter = new MockEmitter();
beforeEach(() => {
transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
mockEmitter
);
veaInboxProvider.getBlock.mockResolvedValue({ number: finalityBlock });
});

it("should return false if transaction is not final", async () => {
jest.spyOn(mockEmitter, "emit");
veaInboxProvider.getTransactionReceipt.mockResolvedValue({
blockNumber: finalityBlock - (transactionHandler.requiredConfirmations - 1),
});
const trnxHash = "0x123456";
const status = await transactionHandler.checkTransactionStatus(trnxHash, ContractType.INBOX);
expect(status).toBeTruthy();
expect(mockEmitter.emit).toHaveBeenCalledWith(
BotEvents.TXN_NOT_FINAL,
trnxHash,
transactionHandler.requiredConfirmations - 1
);
});

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 3, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Mismatch Between Test Name and Assertions

The test title says "should return false if transaction is not final", but it checks expect(status).toBeTruthy(). This contradictory behavior can confuse maintainers.

Use one of the following fixes to align the test name or logic:

-it("should return false if transaction is not final", async () => {
+it("should return true if transaction is not final", async () => {

Or invert the assertion:

-expect(status).toBeTruthy();
+expect(status).toBeFalsy();

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@mani99brar, thanks for confirming the fix! Let me know if you need further assistance.

it("should return true if transaction is pending", async () => {
jest.spyOn(mockEmitter, "emit");
veaInboxProvider.getTransactionReceipt.mockResolvedValue(null);
const trnxHash = "0x123456";
const status = await transactionHandler.checkTransactionStatus(trnxHash, ContractType.INBOX);
expect(status).toBeTruthy();
expect(mockEmitter.emit).toHaveBeenCalledWith(BotEvents.TXN_PENDING, trnxHash);
});

it("should return false if transaction is final", async () => {
jest.spyOn(mockEmitter, "emit");
veaInboxProvider.getTransactionReceipt.mockResolvedValue({
blockNumber: finalityBlock - transactionHandler.requiredConfirmations,
});
const trnxHash = "0x123456";
const status = await transactionHandler.checkTransactionStatus(trnxHash, ContractType.INBOX);
expect(status).toBeFalsy();
expect(mockEmitter.emit).toHaveBeenCalledWith(
BotEvents.TXN_FINAL,
trnxHash,
transactionHandler.requiredConfirmations
);
});

it("should return false if transaction hash is null", async () => {
const trnxHash = null;
const status = await transactionHandler.checkTransactionStatus(trnxHash, ContractType.INBOX);
expect(status).toBeFalsy();
});
});

describe("challengeClaim", () => {
let transactionHandler: ArbToEthTransactionHandler;
const mockEmitter = new MockEmitter();
beforeEach(() => {
transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
mockEmitter
);
transactionHandler.claim = claim;
});

it("should emit CHALLENGING event and throw error if claim is not set", async () => {
jest.spyOn(mockEmitter, "emit");
transactionHandler.claim = null;
await expect(transactionHandler.challengeClaim()).rejects.toThrow(ClaimNotSetError);
expect(mockEmitter.emit).toHaveBeenCalledWith(BotEvents.CHALLENGING);
});

it("should not challenge claim if txn is pending", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(true);
transactionHandler.transactions.challengeTxn = "0x1234";
await transactionHandler.challengeClaim();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(
transactionHandler.transactions.challengeTxn,
ContractType.OUTBOX
);
expect(veaOutbox.estimateGas).not.toHaveBeenCalled();
});

it("should challenge claim", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(false);
const mockChallenge = jest.fn().mockResolvedValue({ hash: "0x1234" }) as any;
(mockChallenge as any).estimateGas = jest.fn().mockResolvedValue(BigInt(100000));
veaOutbox["challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))"] = mockChallenge;
await transactionHandler.challengeClaim();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(null, ContractType.OUTBOX);
expect(transactionHandler.transactions.challengeTxn).toEqual("0x1234");
});

it.todo("should set challengeTxn as completed when txn is final");
});

describe("withdrawDeposit", () => {
let transactionHandler: ArbToEthTransactionHandler;
const mockEmitter = new MockEmitter();
beforeEach(() => {
transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
mockEmitter
);
veaOutbox.withdrawChallengeDeposit.mockResolvedValue("0x1234");
transactionHandler.claim = claim;
});

it("should withdraw deposit", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(false);
veaOutbox.withdrawChallengeDeposit.mockResolvedValue({ hash: "0x1234" });
await transactionHandler.withdrawChallengeDeposit();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(null, ContractType.OUTBOX);
expect(transactionHandler.transactions.withdrawChallengeDepositTxn).toEqual("0x1234");
});

it("should not withdraw deposit if txn is pending", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(true);
transactionHandler.transactions.withdrawChallengeDepositTxn = "0x1234";
await transactionHandler.withdrawChallengeDeposit();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(
transactionHandler.transactions.withdrawChallengeDepositTxn,
ContractType.OUTBOX
);
});

it("should throw an error if claim is not set", async () => {
transactionHandler.claim = null;
await expect(transactionHandler.withdrawChallengeDeposit()).rejects.toThrow(ClaimNotSetError);
});

it("should emit WITHDRAWING event", async () => {
jest.spyOn(mockEmitter, "emit");
await transactionHandler.withdrawChallengeDeposit();
expect(mockEmitter.emit).toHaveBeenCalledWith(BotEvents.WITHDRAWING);
});
});

describe("sendSnapshot", () => {
let transactionHandler: ArbToEthTransactionHandler;
const mockEmitter = new MockEmitter();
beforeEach(() => {
transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
mockEmitter
);
transactionHandler.claim = claim;
});

it("should send snapshot", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(false);
veaInbox.sendSnapshot.mockResolvedValue({ hash: "0x1234" });
await transactionHandler.sendSnapshot();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(null, ContractType.INBOX);
expect(transactionHandler.transactions.sendSnapshotTxn).toEqual("0x1234");
});

it("should not send snapshot if txn is pending", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(true);
transactionHandler.transactions.sendSnapshotTxn = "0x1234";
await transactionHandler.sendSnapshot();
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(
transactionHandler.transactions.sendSnapshotTxn,
ContractType.INBOX
);
expect(veaInbox.sendSnapshot).not.toHaveBeenCalled();
});

it("should throw an error if claim is not set", async () => {
jest.spyOn(mockEmitter, "emit");
transactionHandler.claim = null;
await expect(transactionHandler.sendSnapshot()).rejects.toThrow(ClaimNotSetError);
expect(mockEmitter.emit).toHaveBeenCalledWith(BotEvents.SENDING_SNAPSHOT, epoch);
});
});

describe("resolveChallengedClaim", () => {
let mockMessageExecutor: any;
let transactionHandler: ArbToEthTransactionHandler;
const mockEmitter = new MockEmitter();
beforeEach(() => {
mockMessageExecutor = jest.fn();
transactionHandler = new ArbToEthTransactionHandler(
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
mockEmitter
);
});
it("should resolve challenged claim", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(false);
transactionHandler.transactions.sendSnapshotTxn = "0x1234";
mockMessageExecutor.mockResolvedValue({ hash: "0x1234" });
await transactionHandler.resolveChallengedClaim(
transactionHandler.transactions.sendSnapshotTxn,
mockMessageExecutor
);
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(
transactionHandler.transactions.sendSnapshotTxn,
ContractType.OUTBOX
);
expect(transactionHandler.transactions.executeSnapshotTxn).toEqual("0x1234");
});

it("should not resolve challenged claim if txn is pending", async () => {
jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(true);
transactionHandler.transactions.sendSnapshotTxn = "0x1234";
await transactionHandler.resolveChallengedClaim(mockMessageExecutor);
expect(transactionHandler.checkTransactionStatus).toHaveBeenCalledWith(
transactionHandler.transactions.sendSnapshotTxn,
ContractType.OUTBOX
);
});
});
});
Loading
Loading