From ac24511811b343c2062ef2c097f522e24440d010 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 07:07:01 +0000 Subject: [PATCH 1/2] test: add failing reproductions for v0.40-dev bugs Adds intentionally-failing unit tests under tests/bugs/ that reproduce real instability / wrong-result bugs found in the v0.40-dev line. Each test pins the correct behavior and documents the source location, root cause, a concrete reproduction, and a suggested fix direction. A follow-up change should fix the underlying bug in src/ so these go green. Bugs covered: - contracts/index.ts arg parsing: crash on non-integer numeric args (BigInt("1.5")); empty-string arg becomes number 0; odd-length b# hex drops the last nibble; JSON args containing a float silently degrade to a raw string (structure lost). - wallet/browserSend.ts: nextLabel is cleared only on success, so a failed send leaks its signing label onto the next transaction. - wallet/browserBridge.ts: POST /api/connected accepts an empty/malformed body and marks the bridge connected with an empty-string address. - staking/StakingAction.ts: --staking-address without --network mutates the shared BUILT_IN_NETWORKS singleton, leaking the address process-wide. - vesting/validatorSetIdentity.ts: --extra-cid 0x... is UTF-8 re-encoded instead of hex passthrough, corrupting on-chain identity bytes. - staking/validatorExit.ts: a confirmed exit is reported as "Failed to exit" when the cosmetic post-exit getEpochInfo() read transiently fails. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Ja7QCkNnYByMDLou3qah6H --- tests/bugs/README.md | 20 +++++ .../browserBridgeConnectedValidation.test.ts | 84 ++++++++++++++++++ tests/bugs/browserSendLabelLeak.test.ts | 88 +++++++++++++++++++ tests/bugs/contractArgParsing.test.ts | 68 ++++++++++++++ .../stakingNetworkSingletonMutation.test.ts | 76 ++++++++++++++++ ...lidatorExitReportsFailureOnSuccess.test.ts | 83 +++++++++++++++++ tests/bugs/vestingSetIdentityExtraCid.test.ts | 76 ++++++++++++++++ 7 files changed, 495 insertions(+) create mode 100644 tests/bugs/README.md create mode 100644 tests/bugs/browserBridgeConnectedValidation.test.ts create mode 100644 tests/bugs/browserSendLabelLeak.test.ts create mode 100644 tests/bugs/contractArgParsing.test.ts create mode 100644 tests/bugs/stakingNetworkSingletonMutation.test.ts create mode 100644 tests/bugs/validatorExitReportsFailureOnSuccess.test.ts create mode 100644 tests/bugs/vestingSetIdentityExtraCid.test.ts diff --git a/tests/bugs/README.md b/tests/bugs/README.md new file mode 100644 index 00000000..43b26488 --- /dev/null +++ b/tests/bugs/README.md @@ -0,0 +1,20 @@ +# Bug reproduction tests (intentionally failing) + +These test files reproduce real bugs found in the `v0.40-dev` line by a bug hunt. +**They are expected to FAIL against the current code** — each one pins the +correct behavior so the fix can be verified by making the test go green. A +follow-up change should fix the underlying bug in `src/` (not weaken the test). + +Every file has a header comment with the exact source location, root cause, a +concrete reproduction, and the suggested fix direction. + +| Test file | Source | Bug (instability / wrong result) | +|---|---|---| +| `contractArgParsing.test.ts` | `src/commands/contracts/index.ts` | `--args` parsing: `parseScalar("1.5")` throws an uncaught `SyntaxError` (crash); an empty-string arg becomes the number `0`; odd-length `b#` hex silently drops the last nibble; a JSON arg containing a float silently degrades to a raw string (structure lost). | +| `browserSendLabelLeak.test.ts` | `src/lib/wallet/browserSend.ts` | `nextLabel` is cleared only after a successful send, so a rejected/failed transaction leaks its signing label onto the next, unrelated transaction (user sees the wrong prompt). | +| `browserBridgeConnectedValidation.test.ts` | `src/lib/wallet/browserBridge.ts` | `POST /api/connected` accepts an empty/malformed body and marks the bridge `connected` with an empty-string address; `getState()` then reports `connected: true` while every write path rejects as "not connected". | +| `stakingNetworkSingletonMutation.test.ts` | `src/commands/staking/StakingAction.ts` (+ `src/lib/actions/BaseAction.ts` `resolveNetwork`) | Using `--staking-address` without `--network` mutates the shared, module-level chain singleton, leaking the staking address into every later network resolution in the process. | +| `vestingSetIdentityExtraCid.test.ts` | `src/commands/vesting/validatorSetIdentity.ts` | `vesting validator set-identity --extra-cid 0x...` UTF-8 re-encodes the value instead of hex passthrough (as staking does), writing corrupted identity bytes on-chain. | +| `validatorExitReportsFailureOnSuccess.test.ts` | `src/commands/staking/validatorExit.ts` | A confirmed on-chain exit is reported as "Failed to exit" (non-zero exit, tx hash discarded) when the purely-cosmetic post-exit `getEpochInfo()` read transiently fails. | + +Run just these: `npx vitest run tests/bugs` diff --git a/tests/bugs/browserBridgeConnectedValidation.test.ts b/tests/bugs/browserBridgeConnectedValidation.test.ts new file mode 100644 index 00000000..2805e788 --- /dev/null +++ b/tests/bugs/browserBridgeConnectedValidation.test.ts @@ -0,0 +1,84 @@ +import {describe, test, expect, afterEach} from "vitest"; +import {BrowserWalletBridge, type BridgeChainParams} from "../../src/lib/wallet/browserBridge"; + +/** + * BUG: POST /api/connected accepts an empty / malformed body and marks the + * bridge "connected" with an empty-string address. + * + * src/lib/wallet/browserBridge.ts: + * - readJson (~L777-782): malformed JSON and an empty body both resolve to `{}` + * instead of rejecting, so the 400 path (handleJsonReadError) is never hit. + * - /api/connected handler (~L583): `const address = (body?.address ?? "") as Address;` + * defaults a missing address to "" and stores it unconditionally. + * + * Result: getState() reports `connected: true, address: ""`, but every write + * path guards with `if (!this.connectedAddress)` (empty string is falsy) and + * rejects with "wallet-not-connected". The session is simultaneously + * "connected" (status/UX) and "not connected" (can't sign) — an inconsistent + * state that surfaces as a bogus `wallet status: connected, address: ""`. + * + * Fix direction: reject a missing / non-`0x` address with 400 and do NOT flip + * the connection state. + * + * This test FAILS on the buggy code (connected becomes true) and should PASS + * once an empty/invalid address is rejected. + */ + +const CHAIN: BridgeChainParams = { + chainId: 4221, + chainName: "Genlayer Bradbury Testnet", + rpcUrls: ["https://rpc.example"], + nativeCurrency: {name: "GEN Token", symbol: "GEN", decimals: 18}, + blockExplorerUrls: ["https://explorer.example"], +}; + +function parse(url: string): {origin: string; token: string} { + const u = new URL(url); + const origin = `${u.protocol}//${u.host}`; + const token = new URLSearchParams(u.hash.slice(1)).get("s")!; + return {origin, token}; +} + +const activeBridges: BrowserWalletBridge[] = []; + +afterEach(async () => { + await Promise.all(activeBridges.map(b => b.close().catch(() => {}))); + activeBridges.length = 0; +}); + +describe("BUG: /api/connected must validate the address", () => { + test("an empty body must NOT mark the bridge connected", async () => { + const bridge = new BrowserWalletBridge({chain: CHAIN, openUrl: async () => {}, handleSigint: false}); + activeBridges.push(bridge); + const {url} = await bridge.start(); + const {origin, token} = parse(url); + + // Valid token + origin, but no address in the (empty) body. + const res = await fetch(`${origin}/api/connected`, { + method: "POST", + headers: {"X-Bridge-Token": token, "Content-Type": "application/json", Origin: origin}, + body: "", + }); + + // The bridge must not consider itself connected with no signer address. + expect(bridge.getState().connected).toBe(false); + expect(bridge.getState().address).not.toBe(""); + // And it should signal the bad request rather than 200 OK. + expect(res.status).toBeGreaterThanOrEqual(400); + }); + + test("a malformed JSON body must NOT mark the bridge connected", async () => { + const bridge = new BrowserWalletBridge({chain: CHAIN, openUrl: async () => {}, handleSigint: false}); + activeBridges.push(bridge); + const {url} = await bridge.start(); + const {origin, token} = parse(url); + + await fetch(`${origin}/api/connected`, { + method: "POST", + headers: {"X-Bridge-Token": token, "Content-Type": "application/json", Origin: origin}, + body: "not-json{{{", + }); + + expect(bridge.getState().connected).toBe(false); + }); +}); diff --git a/tests/bugs/browserSendLabelLeak.test.ts b/tests/bugs/browserSendLabelLeak.test.ts new file mode 100644 index 00000000..182fd350 --- /dev/null +++ b/tests/bugs/browserSendLabelLeak.test.ts @@ -0,0 +1,88 @@ +import {describe, test, expect, vi, beforeEach} from "vitest"; + +/** + * BUG: stale `nextLabel` leaks to the next transaction after a failed send. + * + * src/lib/wallet/browserSend.ts (eip1193Provider `eth_sendTransaction`, ~L217-229): + * + * const result = await transport.sendTransaction({ ..., label: nextLabel ?? "GenLayer transaction" }); + * const hash = assertResultSigner(result, signerAddress); + * nextLabel = undefined; // <-- only reached when the send SUCCEEDS + * + * `nextLabel` is cleared only after a successful send. If the wallet rejects the + * transaction (user declines, tx error, timeout) the label survives and is + * attached to the NEXT, unrelated transaction. The label is exactly what the + * bridge page renders next to the wallet confirmation, so the user is shown the + * wrong signing prompt (e.g. confirms "Deploy Counter.py" for a different call). + * + * Fix direction: consume the label BEFORE the send (or clear it in a finally), + * so it never carries over past the call it was set for. + * + * This test FAILS on the buggy code (the second send is labelled with the stale + * "Deploy Counter.py") and should PASS once the label is reset regardless of + * outcome. + */ + +const bridgeSend = vi.fn(); +const bridgeClose = vi.fn().mockResolvedValue(undefined); +const bridgeStart = vi.fn().mockResolvedValue({url: "http://127.0.0.1:12345/#s=tok"}); +const bridgeWaitForConnection = vi.fn().mockResolvedValue("0xConnected0000000000000000000000000000001"); + +vi.mock("../../src/lib/wallet/browserBridge", () => ({ + BrowserWalletBridge: vi.fn().mockImplementation(() => ({ + start: bridgeStart, + waitForConnection: bridgeWaitForConnection, + sendTransaction: bridgeSend, + close: bridgeClose, + getUrl: () => "http://127.0.0.1:12345/#s=tok", + })), +})); + +const publicCall = vi.fn().mockResolvedValue({data: "0x"}); +const waitForReceipt = vi.fn(); +vi.mock("viem", async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + createPublicClient: vi.fn(() => ({call: publicCall, waitForTransactionReceipt: waitForReceipt})), + }; +}); + +import {openBrowserWalletSession} from "../../src/lib/wallet/browserSend"; + +const CHAIN: any = { + id: 4221, + name: "Genlayer Bradbury Testnet", + rpcUrls: {default: {http: ["https://rpc.example"]}}, + nativeCurrency: {name: "GEN Token", symbol: "GEN", decimals: 18}, + blockExplorers: {default: {url: "https://explorer.example"}}, +}; + +describe("BUG: browserSend nextLabel leaks after a failed send", () => { + beforeEach(() => { + vi.clearAllMocks(); + bridgeStart.mockResolvedValue({url: "http://127.0.0.1:12345/#s=tok"}); + bridgeWaitForConnection.mockResolvedValue("0xConnected0000000000000000000000000000001"); + publicCall.mockResolvedValue({data: "0x"}); + }); + + test("a rejected send must not carry its label into the next transaction", async () => { + const session = await openBrowserWalletSession({chain: CHAIN, rpcUrl: "https://rpc.example"}); + + // First send: the wallet rejects it. Its label must NOT survive. + bridgeSend.mockRejectedValueOnce(new Error("User rejected the request")); + session.setNextLabel("Deploy Counter.py"); + await expect( + session.eip1193Provider.request({method: "eth_sendTransaction", params: [{to: "0xA", data: "0x"}]}), + ).rejects.toThrow(/rejected/i); + + // Second, unrelated send should use the default label — NOT the stale one. + bridgeSend.mockResolvedValueOnce("0xhash"); + await session.eip1193Provider.request({ + method: "eth_sendTransaction", + params: [{to: "0xB", data: "0x"}], + }); + + expect(bridgeSend).toHaveBeenLastCalledWith(expect.objectContaining({label: "GenLayer transaction"})); + }); +}); diff --git a/tests/bugs/contractArgParsing.test.ts b/tests/bugs/contractArgParsing.test.ts new file mode 100644 index 00000000..4a98dabd --- /dev/null +++ b/tests/bugs/contractArgParsing.test.ts @@ -0,0 +1,68 @@ +import {vi, describe, test, expect} from "vitest"; + +// contracts/index.ts transitively imports deploy.ts (esbuild). Mock it so the +// module loads in the jsdom test environment, exactly as parseArg.test.ts does. +vi.mock("esbuild", () => ({ + buildSync: vi.fn(), +})); + +import {parseArg, parseScalar} from "../../src/commands/contracts"; + +/** + * BUGS in src/commands/contracts/index.ts argument parsing (parseScalar / + * parseArg / coerceValue). These are pure functions reached by every + * `--args` value on deploy / call / write, so a wrong result here is sent + * straight to the contract. + */ + +describe("BUG: parseScalar crashes on a non-integer numeric arg", () => { + // parseScalar (~L56-57): a numeric-looking string that isn't a safe integer + // falls through to `BigInt(value)`, and BigInt("1.5") throws a SyntaxError. + // There is no try/catch and no global handler, so `genlayer deploy --args 1.5` + // dies with a raw stack trace instead of a friendly message. + test('parseScalar("1.5") must not throw', () => { + expect(() => parseScalar("1.5")).not.toThrow(); + }); + + test('parseScalar("-0.5") must not throw', () => { + expect(() => parseScalar("-0.5")).not.toThrow(); + }); +}); + +describe("BUG: an empty-string arg is coerced to the number 0", () => { + // parseScalar (~L56): Number("") === 0 (not NaN) and 0 is a safe integer, + // so an intentionally-empty string argument becomes integer 0 on-chain. + test('parseScalar("") stays an empty string', () => { + expect(parseScalar("")).toBe(""); + }); + + test('parseScalar(" ") is not silently turned into 0', () => { + expect(parseScalar(" ")).not.toBe(0); + }); +}); + +describe("BUG: odd-length b# hex bytes silently drop the last nibble", () => { + // BYTES_PREFIX_RE allows an odd number of hex chars and hexToBytes does + // `new Uint8Array(hex.length / 2)`, truncating 1.5 -> 1. "b#abc" loses the + // trailing "c" with no error — a corrupted byte payload sent silently. + test('parseScalar("b#abc") must reject odd-length hex instead of truncating', () => { + expect(() => parseScalar("b#abc")).toThrow(); + }); +}); + +describe("BUG: a JSON arg containing a float degrades to a literal string", () => { + // parseArg: coerceValue runs inside the JSON.parse try block; BigInt(1.5) + // throws (RangeError) and is swallowed by the `catch`, so the whole JSON + // argument silently degrades to parseScalar('[1.5]') -> the raw string + // "[1.5]". The structured list/object the user passed is lost. + test('parseArg("[1.5]") preserves the array structure (not the raw string)', () => { + const result = parseArg("[1.5]"); + expect(result[0]).not.toBe("[1.5]"); + expect(Array.isArray(result[0])).toBe(true); + }); + + test('parseArg with a float field keeps an object, not a string', () => { + const result = parseArg('{"price":1.5}'); + expect(typeof result[0]).not.toBe("string"); + }); +}); diff --git a/tests/bugs/stakingNetworkSingletonMutation.test.ts b/tests/bugs/stakingNetworkSingletonMutation.test.ts new file mode 100644 index 00000000..9dbba01c --- /dev/null +++ b/tests/bugs/stakingNetworkSingletonMutation.test.ts @@ -0,0 +1,76 @@ +import {describe, test, vi, beforeEach, afterEach, expect} from "vitest"; +import {StakingInfoAction} from "../../src/commands/staking/stakingInfo"; +import {resolveNetwork} from "../../src/lib/actions/BaseAction"; + +/** + * BUG: StakingAction mutates the shared, module-level network singleton when + * `--staking-address` is used without `--network`. + * + * src/lib/actions/BaseAction.ts resolveNetwork() returns the SHARED + * BUILT_IN_NETWORKS[...] / localnet objects (no clone). StakingAction.getNetwork + * only defensively copies on the `--network` flag branch: + * + * if (config.network) return {...resolveNetwork(config.network, ...)}; // copy + * return resolveNetwork(this.getConfig().network, ...); // SHARED + * + * getReadOnlyStakingClient / getStakingClient / getBrowserStakingClient then do + * `network.stakingContract = {address: config.stakingAddress, ...}`, writing + * through to the shared chain object. Every later resolveNetwork() in the same + * process (a second action, the wizard's multiple clients, the whole test + * suite) inherits the leaked staking address. + * + * Fix direction: clone on the config-fallback branch too (or clone inside + * resolveNetwork), so per-command overrides never touch the singleton. + * + * This test FAILS today: after one read-only client build with a custom + * staking address and no network, the shared localnet singleton carries that + * address. + */ + +vi.mock("genlayer-js", () => ({ + createClient: vi.fn(() => ({})), + createAccount: vi.fn(() => ({address: "0xMockedAddress"})), + formatStakingAmount: vi.fn((v: bigint) => `${v}`), + parseStakingAmount: vi.fn((v: string) => BigInt(v)), + abi: {STAKING_ABI: [], VALIDATOR_WALLET_ABI: []}, +})); + +vi.mock("genlayer-js/chains", () => ({ + localnet: {id: 1, name: "localnet", rpcUrls: {default: {http: ["http://localhost:8545"]}}}, + studionet: {id: 2, name: "studionet", rpcUrls: {default: {http: ["https://studionet.genlayer.com"]}}}, + testnetAsimov: {id: 3, name: "testnet-asimov", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, + testnetBradbury: {id: 4, name: "testnet-bradbury", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, +})); + +const CUSTOM_STAKING_ADDRESS = "0xC0ffee0000000000000000000000000000000001"; + +describe("BUG: staking mutates the shared network singleton", () => { + let action: StakingInfoAction; + + beforeEach(() => { + vi.clearAllMocks(); + action = new StakingInfoAction(); + // Deterministic: no configured network → resolveNetwork(undefined) => localnet singleton. + vi.spyOn(action as any, "getConfig").mockReturnValue({}); + vi.spyOn(action as any, "getCustomNetworks").mockReturnValue({}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("--staking-address without --network must not mutate the localnet singleton", async () => { + // Sanity: the singleton has no staking address to begin with. + expect((resolveNetwork(undefined) as any).stakingContract?.address).toBeUndefined(); + + // Build a read-only client with a custom staking address and NO network flag. + try { + await (action as any).getReadOnlyStakingClient({stakingAddress: CUSTOM_STAKING_ADDRESS}); + } catch { + // The client build isn't what we're testing; the mutation happens first. + } + + // The shared singleton must be untouched. + expect((resolveNetwork(undefined) as any).stakingContract?.address).not.toBe(CUSTOM_STAKING_ADDRESS); + }); +}); diff --git a/tests/bugs/validatorExitReportsFailureOnSuccess.test.ts b/tests/bugs/validatorExitReportsFailureOnSuccess.test.ts new file mode 100644 index 00000000..aef5f9dd --- /dev/null +++ b/tests/bugs/validatorExitReportsFailureOnSuccess.test.ts @@ -0,0 +1,83 @@ +import {describe, test, vi, beforeEach, afterEach, expect} from "vitest"; +import {ValidatorExitAction} from "../../src/commands/staking/validatorExit"; + +/** + * BUG: a SUCCESSFUL validator exit is reported as "Failed to exit" when the + * post-exit informational epoch read transiently fails. + * + * src/commands/staking/validatorExit.ts (~L44-66): + * const result = await client.validatorExit({validator, shares}); // tx CONFIRMED on-chain + * const epochInfo = await client.getEpochInfo(); // cosmetic read, SAME try + * ... + * this.succeedSpinner("Exit initiated successfully!", output); + * } catch (error) { this.failSpinner("Failed to exit", ...); } + * + * `getEpochInfo()` is only used to pick a human-readable note, but it runs in + * the same try block AFTER the exit transaction has already been mined. A + * transient RPC hiccup on that read lands in the catch, so the CLI prints + * "Failed to exit" and exits non-zero — discarding the real tx hash — for an + * exit that actually succeeded. (validatorJoin.preflight deliberately swallows + * getEpochInfo failures for exactly this reason.) + * + * Fix direction: move the getEpochInfo read out of the critical try, or + * swallow its failure and still report success with the tx hash. + * + * This test FAILS today (failSpinner is called for a confirmed exit). + */ + +vi.mock("genlayer-js", () => ({ + createClient: vi.fn(() => ({})), + createAccount: vi.fn(() => ({address: "0xMockedAddress"})), + formatStakingAmount: vi.fn((v: bigint) => `${v}`), + parseStakingAmount: vi.fn((v: string) => BigInt(v)), + abi: {STAKING_ABI: [], VALIDATOR_WALLET_ABI: []}, +})); + +vi.mock("genlayer-js/chains", () => ({ + localnet: {id: 1, name: "localnet", rpcUrls: {default: {http: ["http://localhost:8545"]}}}, + studionet: {id: 2, name: "studionet", rpcUrls: {default: {http: ["https://studionet.genlayer.com"]}}}, + testnetAsimov: {id: 3, name: "testnet-asimov", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, + testnetBradbury: {id: 4, name: "testnet-bradbury", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, +})); + +describe("BUG: validator-exit reports failure when the cosmetic epoch read fails", () => { + let action: ValidatorExitAction; + let client: {validatorExit: ReturnType; getEpochInfo: ReturnType}; + + beforeEach(() => { + vi.clearAllMocks(); + action = new ValidatorExitAction(); + vi.spyOn(action as any, "isBrowserWallet").mockReturnValue(false); + vi.spyOn(action as any, "startSpinner").mockImplementation(() => {}); + vi.spyOn(action as any, "setSpinnerText").mockImplementation(() => {}); + vi.spyOn(action as any, "succeedSpinner").mockImplementation(() => {}); + vi.spyOn(action as any, "failSpinner").mockImplementation(() => {}); + client = { + // Exit tx is confirmed on-chain. + validatorExit: vi.fn().mockResolvedValue({ + transactionHash: "0xExitHash", + blockNumber: 10n, + gasUsed: 21000n, + }), + // The purely-informational epoch read hiccups. + getEpochInfo: vi.fn().mockRejectedValue(new Error("RPC timeout")), + }; + vi.spyOn(action as any, "getStakingClient").mockResolvedValue(client); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("a confirmed exit is reported as success, not 'Failed to exit'", async () => { + await action.execute({ + validator: "0xValidatorWallet00000000000000000000000001", + shares: "100", + } as any); + + expect(client.validatorExit).toHaveBeenCalled(); + // The exit succeeded on-chain — the user must not be told it failed. + expect(action["failSpinner"]).not.toHaveBeenCalledWith("Failed to exit", expect.anything()); + expect(action["succeedSpinner"]).toHaveBeenCalled(); + }); +}); diff --git a/tests/bugs/vestingSetIdentityExtraCid.test.ts b/tests/bugs/vestingSetIdentityExtraCid.test.ts new file mode 100644 index 00000000..5fefd157 --- /dev/null +++ b/tests/bugs/vestingSetIdentityExtraCid.test.ts @@ -0,0 +1,76 @@ +import {describe, test, vi, beforeEach, afterEach, expect} from "vitest"; +import {VestingValidatorSetIdentityAction} from "../../src/commands/vesting/validatorSetIdentity"; + +/** + * BUG: `vesting validator set-identity --extra-cid 0x...` double-encodes hex. + * + * src/commands/vesting/validatorSetIdentity.ts L33 (keystore) and L89 (browser): + * const extraCid = options.extraCid ? toHex(new TextEncoder().encode(options.extraCid)) : "0x"; + * + * The flag is documented as "IPFS CID or hex bytes (0x...)" and every other + * command in the repo passes `extraCid` straight to the SDK, whose + * `encodeExtraCid` does hex passthrough (`if (extraCid.startsWith("0x")) return + * extraCid`). The staking counterpart (staking/setIdentity.ts) forwards the raw + * value for exactly this reason. The vesting command instead UTF-8-encodes the + * raw string unconditionally, so a hex payload like `0xabcd` is turned into + * `0x307861626364` (the ASCII bytes of the text "0xabcd") and written on-chain. + * + * Fix direction: forward `options.extraCid` to the SDK (or pass through when it + * starts with "0x") instead of UTF-8 re-encoding. + * + * This test FAILS today (the client receives the double-encoded value). + */ + +vi.mock("genlayer-js", () => ({ + createClient: vi.fn(), + createAccount: vi.fn(() => ({address: "0xMockedAddress"})), + abi: {VESTING_ABI: []}, +})); + +vi.mock("genlayer-js/chains", () => ({ + localnet: {id: 1, name: "localnet", rpcUrls: {default: {http: ["http://localhost:8545"]}}}, + studionet: {id: 2, name: "studionet", rpcUrls: {default: {http: ["https://studionet.genlayer.com"]}}}, + testnetAsimov: {id: 3, name: "testnet-asimov", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, + testnetBradbury: {id: 4, name: "testnet-bradbury", rpcUrls: {default: {http: ["https://testnet.genlayer.com"]}}}, +})); + +describe("BUG: vesting validator set-identity double-encodes --extra-cid hex", () => { + let action: VestingValidatorSetIdentityAction; + let client: {vestingValidatorSetIdentity: ReturnType}; + + beforeEach(() => { + vi.clearAllMocks(); + action = new VestingValidatorSetIdentityAction(); + vi.spyOn(action as any, "startSpinner").mockImplementation(() => {}); + vi.spyOn(action as any, "setSpinnerText").mockImplementation(() => {}); + vi.spyOn(action as any, "succeedSpinner").mockImplementation(() => {}); + vi.spyOn(action as any, "failSpinner").mockImplementation(() => {}); + vi.spyOn(action as any, "log").mockImplementation(() => {}); + // Force the keystore lane. + vi.spyOn(action as any, "isBrowserWallet").mockReturnValue(false); + vi.spyOn(action as any, "resolveBeneficiaryVesting").mockResolvedValue("0xVesting"); + client = { + vestingValidatorSetIdentity: vi + .fn() + .mockResolvedValue({transactionHash: "0xTH", blockNumber: 1n, gasUsed: 2n}), + }; + vi.spyOn(action as any, "getVestingClient").mockResolvedValue(client); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("a 0x-prefixed extra-cid is forwarded verbatim, not UTF-8 re-encoded", async () => { + await action.execute({ + walletAddress: "0xWallet0000000000000000000000000000000001", + moniker: "M", + extraCid: "0xabcd", + } as any); + + expect(action["failSpinner"]).not.toHaveBeenCalled(); + expect(client.vestingValidatorSetIdentity).toHaveBeenCalledWith( + expect.objectContaining({extraCid: "0xabcd"}), + ); + }); +}); From 5e9072916a350a73ecbffec4b186287a5ecb14fd Mon Sep 17 00:00:00 2001 From: Edgars Date: Sat, 25 Jul 2026 22:57:22 +0100 Subject: [PATCH 2/2] fix: resolve v0.40-dev bug hunt findings --- src/commands/contracts/index.ts | 24 ++++++++---- src/commands/staking/StakingAction.ts | 2 +- src/commands/staking/validatorExit.ts | 23 +++++++++--- src/commands/vesting/validatorSetIdentity.ts | 5 +-- src/lib/actions/BaseAction.ts | 39 +++++++++++++++++--- src/lib/wallet/browserBridge.ts | 21 +++++++---- src/lib/wallet/browserSend.ts | 7 +++- tests/libs/browserBridge.test.ts | 2 +- tests/libs/sessionClient.test.ts | 2 +- tests/libs/sessionDaemon.test.ts | 2 +- tests/libs/sessionResolver.test.ts | 2 +- 11 files changed, 94 insertions(+), 35 deletions(-) diff --git a/src/commands/contracts/index.ts b/src/commands/contracts/index.ts index f4909eb3..5f525128 100644 --- a/src/commands/contracts/index.ts +++ b/src/commands/contracts/index.ts @@ -14,6 +14,9 @@ const BYTES_PREFIX_RE = /^b#([0-9a-fA-F]*)$/; const HEX_RE = /^0x[0-9a-fA-F]+$/; function hexToBytes(hex: string): Uint8Array { + if (hex.length % 2 !== 0) { + throw new Error("Hex bytes must contain an even number of digits"); + } const bytes = new Uint8Array(hex.length / 2); for (let i = 0; i < bytes.length; i++) { bytes[i] = parseInt(hex.slice(i * 2, i * 2 + 2), 16); @@ -26,7 +29,10 @@ export function coerceValue(value: unknown): unknown { if (typeof value === "boolean") return value; if (typeof value === "number") { if (Number.isSafeInteger(value)) return value; - return BigInt(value); + if (Number.isInteger(value)) return BigInt(value); + // GenVM calldata has no float type. Preserve the user's value as a string + // rather than throwing or collapsing the surrounding JSON structure. + return value.toString(); } if (Array.isArray(value)) return value.map(coerceValue); if (typeof value === "object" && value !== null) { @@ -53,20 +59,24 @@ export function parseScalar(value: string): unknown { if (bytesMatch) return hexToBytes(bytesMatch[1]); if (HEX_RE.test(value)) return BigInt(value); - if (!isNaN(Number(value)) && Number.isSafeInteger(Number(value))) return Number(value); - if (!isNaN(Number(value))) return BigInt(value); + if (value.trim() === "") return value; + const numericValue = Number(value); + if (!Number.isNaN(numericValue) && Number.isSafeInteger(numericValue)) return numericValue; + if (!Number.isNaN(numericValue) && Number.isInteger(numericValue)) return BigInt(value); return value; } export function parseArg(value: string, previous: any[] = []): any[] { + let parsed: unknown; try { - const parsed = JSON.parse(value); - if (typeof parsed === "object" || Array.isArray(parsed)) { - return [...previous, coerceValue(parsed)]; - } + parsed = JSON.parse(value); } catch { // not JSON, fall through to scalar parsing + return [...previous, parseScalar(value)]; + } + if (typeof parsed === "object" && parsed !== null) { + return [...previous, coerceValue(parsed)]; } return [...previous, parseScalar(value)]; } diff --git a/src/commands/staking/StakingAction.ts b/src/commands/staking/StakingAction.ts index 62c762eb..84a4d63a 100644 --- a/src/commands/staking/StakingAction.ts +++ b/src/commands/staking/StakingAction.ts @@ -44,7 +44,7 @@ export class StakingAction extends BaseAction { private getNetwork(config: StakingConfig): GenLayerChain { // Priority: --network option > global config > localnet default if (config.network) { - return {...resolveNetwork(config.network, this.getCustomNetworks())}; + return resolveNetwork(config.network, this.getCustomNetworks()); } return resolveNetwork(this.getConfig().network, this.getCustomNetworks()); diff --git a/src/commands/staking/validatorExit.ts b/src/commands/staking/validatorExit.ts index 3f4ced2e..a5574950 100644 --- a/src/commands/staking/validatorExit.ts +++ b/src/commands/staking/validatorExit.ts @@ -46,9 +46,16 @@ export class ValidatorExitAction extends StakingAction { shares, }); - // Check epoch to determine note - const epochInfo = await client.getEpochInfo(); - const isEpochZero = epochInfo.currentEpoch === 0n; + // This read only improves the explanatory note. The exit is already + // confirmed, so a transient failure here must not turn success into an + // error or hide the transaction hash. + let isEpochZero = false; + try { + const epochInfo = await client.getEpochInfo(); + isEpochZero = epochInfo.currentEpoch === 0n; + } catch { + // Keep the conservative unbonding-period note. + } const output = { transactionHash: result.transactionHash, @@ -97,9 +104,13 @@ export class ValidatorExitAction extends StakingAction { shares, }); - // Check epoch to determine note - const epochInfo = await client.getEpochInfo(); - const isEpochZero = epochInfo.currentEpoch === 0n; + let isEpochZero = false; + try { + const epochInfo = await client.getEpochInfo(); + isEpochZero = epochInfo.currentEpoch === 0n; + } catch { + // The exit is already confirmed; keep the conservative note. + } this.succeedSpinner("Exit initiated successfully!", { transactionHash: result.transactionHash, diff --git a/src/commands/vesting/validatorSetIdentity.ts b/src/commands/vesting/validatorSetIdentity.ts index 11a75d81..40217472 100644 --- a/src/commands/vesting/validatorSetIdentity.ts +++ b/src/commands/vesting/validatorSetIdentity.ts @@ -1,6 +1,5 @@ import {VestingAction, VestingConfig} from "./VestingAction"; import type {Address} from "genlayer-js/types"; -import {toHex} from "viem"; export interface VestingValidatorSetIdentityOptions extends VestingConfig { walletAddress: string; @@ -30,7 +29,7 @@ export class VestingValidatorSetIdentityAction extends VestingAction { try { const client = await this.getVestingClient(options); const vesting = await this.resolveBeneficiaryVesting(client, options); - const extraCid = options.extraCid ? toHex(new TextEncoder().encode(options.extraCid)) : "0x"; + const extraCid = options.extraCid || "0x"; this.setSpinnerText(`Setting identity for vesting validator wallet ${options.walletAddress}...`); @@ -86,7 +85,7 @@ export class VestingValidatorSetIdentityAction extends VestingAction { try { const client = this.getBrowserVestingClient(options, session); const vesting = await this.resolveBeneficiaryVesting(client, options); - const extraCid = options.extraCid ? toHex(new TextEncoder().encode(options.extraCid)) : "0x"; + const extraCid = options.extraCid || "0x"; session.setNextLabel("Set vesting validator identity"); const result = await client.vestingValidatorSetIdentity({ diff --git a/src/lib/actions/BaseAction.ts b/src/lib/actions/BaseAction.ts index 65072fed..42e2064c 100644 --- a/src/lib/actions/BaseAction.ts +++ b/src/lib/actions/BaseAction.ts @@ -26,16 +26,45 @@ export const BUILT_IN_NETWORKS: Record = { "testnet-bradbury": testnetBradbury, }; +function cloneNetwork(network: GenLayerChain): GenLayerChain { + const rpcUrls = Object.fromEntries( + Object.entries(network.rpcUrls).map(([key, urls]) => [ + key, + { + ...urls, + http: [...urls.http], + ...(urls.webSocket ? {webSocket: [...urls.webSocket]} : {}), + }, + ]), + ) as GenLayerChain["rpcUrls"]; + + return { + ...network, + nativeCurrency: {...network.nativeCurrency}, + rpcUrls, + ...(network.blockExplorers + ? { + blockExplorers: Object.fromEntries( + Object.entries(network.blockExplorers).map(([key, explorer]) => [ + key, + {...explorer}, + ]), + ), + } + : {}), + } as GenLayerChain; +} + /** * Resolves a stored network config to a fresh chain object. * Handles both new format (alias string) and old format (JSON object) for backwards compat. */ export function resolveNetwork(stored: string | undefined, customNetworks?: CustomNetworksConfig): GenLayerChain { - if (!stored) return localnet; + if (!stored) return cloneNetwork(localnet); // Try as alias first (new format) if (BUILT_IN_NETWORKS[stored]) { - return BUILT_IN_NETWORKS[stored]; + return cloneNetwork(BUILT_IN_NETWORKS[stored]); } const customNetwork = customNetworks?.[stored]; @@ -48,7 +77,7 @@ export function resolveNetwork(stored: string | undefined, customNetworks?: Cust // (`network add `) — not the base chain's name. Otherwise a "clarke" // network shows up as "Genlayer Bradbury Testnet" everywhere (wizard picker, // network info, prompts), which is confusing. - return {...applyCustomNetworkProfile(baseNetwork, customNetwork), name: stored}; + return cloneNetwork({...applyCustomNetworkProfile(baseNetwork, customNetwork), name: stored}); } // Backwards compat: try parsing as JSON (old format) @@ -58,10 +87,10 @@ export function resolveNetwork(stored: string | undefined, customNetworks?: Cust const alias = Object.entries(BUILT_IN_NETWORKS) .find(([_, chain]) => chain.name === parsed.name)?.[0]; if (alias) { - return BUILT_IN_NETWORKS[alias]; + return cloneNetwork(BUILT_IN_NETWORKS[alias]); } // Custom network - use as-is - return parsed; + return cloneNetwork(parsed); } catch { throw new Error(`Unknown network: ${stored}`); } diff --git a/src/lib/wallet/browserBridge.ts b/src/lib/wallet/browserBridge.ts index b0716f49..e4f7198f 100644 --- a/src/lib/wallet/browserBridge.ts +++ b/src/lib/wallet/browserBridge.ts @@ -1,7 +1,7 @@ import http from "node:http"; import {randomUUID, timingSafeEqual} from "node:crypto"; import type {AddressInfo} from "node:net"; -import {hexToBigInt} from "viem"; +import {hexToBigInt, isAddress} from "viem"; import type {Address, Hash} from "genlayer-js/types"; import {openUrl as defaultOpenUrl} from "../clients/system"; import {BRIDGE_PAGE_HTML} from "./bridgePage"; @@ -580,7 +580,10 @@ export class BrowserWalletBridge { if (path === "/api/connected" && req.method === "POST") { void this.readJson(req) .then(body => { - const address = (body?.address ?? "") as Address; + const address = body?.address; + if (typeof address !== "string" || !isAddress(address)) { + throw new Error("invalid wallet address"); + } this.connectedAddress = address; if (this.connectTimer) { clearTimeout(this.connectTimer); @@ -776,15 +779,19 @@ export class BrowserWalletBridge { settled = true; try { const raw = Buffer.concat(chunks).toString("utf-8"); - resolve(raw ? JSON.parse(raw) : {}); - } catch { - resolve({}); + if (!raw.trim()) { + reject(new Error("empty JSON body")); + return; + } + resolve(JSON.parse(raw)); + } catch (err) { + reject(err); } }); - req.on("error", () => { + req.on("error", err => { if (settled) return; settled = true; - resolve({}); + reject(err); }); }); } diff --git a/src/lib/wallet/browserSend.ts b/src/lib/wallet/browserSend.ts index d3ca9844..c99afcb3 100644 --- a/src/lib/wallet/browserSend.ts +++ b/src/lib/wallet/browserSend.ts @@ -214,6 +214,10 @@ export function buildBrowserSession( nonce?: string; type?: string; }; + const label = nextLabel ?? "GenLayer transaction"; + // A label belongs to exactly one send attempt, including a rejected + // or failed attempt. Consume it before awaiting the transport. + nextLabel = undefined; const result = await transport.sendTransaction({ to: req.to as Address, data: (req.data ?? "0x") as `0x${string}`, @@ -222,10 +226,9 @@ export function buildBrowserSession( gasPrice: req.gasPrice !== undefined ? hexToBigInt(req.gasPrice as `0x${string}`) : undefined, nonce: req.nonce !== undefined ? Number(hexToBigInt(req.nonce as `0x${string}`)) : undefined, type: req.type, - label: nextLabel ?? "GenLayer transaction", + label, }); const hash = assertResultSigner(result, signerAddress); - nextLabel = undefined; return hash; } default: diff --git a/tests/libs/browserBridge.test.ts b/tests/libs/browserBridge.test.ts index e9aa99b1..96d31f45 100644 --- a/tests/libs/browserBridge.test.ts +++ b/tests/libs/browserBridge.test.ts @@ -31,7 +31,7 @@ const CHAIN: BridgeChainParams = { blockExplorerUrls: ["https://explorer.example"], }; -const ADDRESS = "0xConnectedAddress0000000000000000000000000" as `0x${string}`; +const ADDRESS = "0x0000000000000000000000000000000000000001" as `0x${string}`; /** Parse origin + token out of the bridge URL (http://127.0.0.1:/#s=). */ function parse(url: string): {origin: string; token: string} { diff --git a/tests/libs/sessionClient.test.ts b/tests/libs/sessionClient.test.ts index 81fd9953..e7dde3ce 100644 --- a/tests/libs/sessionClient.test.ts +++ b/tests/libs/sessionClient.test.ts @@ -14,7 +14,7 @@ const CHAIN: BridgeChainParams = { nativeCurrency: {name: "GEN Token", symbol: "GEN", decimals: 18}, blockExplorerUrls: ["https://explorer.example"], }; -const ADDRESS = "0xConnected0000000000000000000000000000001" as `0x${string}`; +const ADDRESS = "0x0000000000000000000000000000000000000001" as `0x${string}`; function parse(url: string) { const u = new URL(url); diff --git a/tests/libs/sessionDaemon.test.ts b/tests/libs/sessionDaemon.test.ts index 773a675d..e8a89ad6 100644 --- a/tests/libs/sessionDaemon.test.ts +++ b/tests/libs/sessionDaemon.test.ts @@ -6,7 +6,7 @@ import {ConfigFileManager} from "../../src/lib/config/ConfigFileManager"; import {runWalletSessionDaemon, type DaemonHandle} from "../../src/lib/wallet/sessionDaemon"; import {descriptorPath, readDescriptor} from "../../src/lib/wallet/sessionDescriptor"; -const ADDRESS = "0xConnected0000000000000000000000000000001" as `0x${string}`; +const ADDRESS = "0x0000000000000000000000000000000000000001" as `0x${string}`; describe("runWalletSessionDaemon", () => { let dir: string; diff --git a/tests/libs/sessionResolver.test.ts b/tests/libs/sessionResolver.test.ts index ff5b6d5f..ab510dd2 100644 --- a/tests/libs/sessionResolver.test.ts +++ b/tests/libs/sessionResolver.test.ts @@ -22,7 +22,7 @@ vi.mock("../../src/lib/wallet/browserSend", async importActual => { return {...actual, openRemoteWalletSession: vi.fn(actual.openRemoteWalletSession)}; }); -const ADDRESS = "0xConnected0000000000000000000000000000001" as `0x${string}`; +const ADDRESS = "0x1234567890abcdef1234567890abcdef12345678" as `0x${string}`; /** * A fetch stub answering /api/ping + /api/state for a discovered daemon, so a