From 191c2784bbf5d811953e2bcb8408659486221a23 Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Mon, 7 Sep 2026 18:26:10 +0300 Subject: [PATCH 1/7] fix: #2330 expand tuples in overloaded ABI function keys Function keys for overloaded ABI functions were built from the raw ABI types, so a struct parameter became the literal "tuple". The resulting key is not a fragment ethers accepts, and two overloads differing only inside the struct produced the same key, so the user's choice was not recoverable. Selecting either Permit2 `permit` overload from the published ABI produced `permit(address,tuple,bytes)` and failed at encode with "invalid function fragment", while the canonical spelling was not found by the lookup. Both key writers now expand tuples through canonicalType, which the selector shown next to the same dropdown entry already used. The reader matches canonical signatures and keeps accepting the legacy raw spelling wherever it still identifies one overload, so saved workflows and API or MCP callers that supply a key themselves keep working. canonicalType stays strict. Canonicalising an entry is guarded at the lookup instead, so an entry that cannot be canonicalised is simply not a canonical match: findAbiFunction stays total, the UI helpers that call it outside a try/catch keep failing closed, and one malformed entry cannot hide the healthy functions beside it. Where a legacy key matches several overloads the choice was never stored and cannot be recovered. The execution paths now report that as its own error naming the signatures to pick from, rather than resolving to whichever overload came first and returning its inputs, outputs and stateMutability to the caller. simulate encodes with the signature derived from the resolved entry rather than the key as supplied, since resolving a legacy key is not enough to make it encodable. The Diamond facet merge in fetch-abi deduplicates on a computed selector. Building that selector from raw types made two tuple overloads collide, so one was dropped from the merged ABI at fetch time; it now goes through computeSelector, and an entry that cannot be canonicalised is left out of deduplication instead of discarded. --- app/api/web3/fetch-abi/route.ts | 26 ++- .../config/action-config-renderer.tsx | 14 +- lib/abi/function-key.ts | 44 +++- lib/abi/utils.ts | 153 ++++++++++--- lib/execute/simulate.ts | 24 +- .../web3/steps/batch-write-contract-core.ts | 16 +- plugins/web3/steps/read-contract-core.ts | 21 +- plugins/web3/steps/write-contract-core.ts | 18 +- tests/unit/abi-function-key.test.ts | 127 +++++++++++ tests/unit/abi-utils.test.ts | 215 +++++++++++++++++- 10 files changed, 598 insertions(+), 60 deletions(-) create mode 100644 tests/unit/abi-function-key.test.ts diff --git a/app/api/web3/fetch-abi/route.ts b/app/api/web3/fetch-abi/route.ts index 742c67c38..572968fb4 100644 --- a/app/api/web3/fetch-abi/route.ts +++ b/app/api/web3/fetch-abi/route.ts @@ -1,6 +1,10 @@ import { eq } from "drizzle-orm"; import { ethers } from "ethers"; import { NextResponse } from "next/server"; +import { + type AbiItemComponent, + computeSelector, +} from "@/lib/abi/utils"; import { toChecksumAddress } from "@/lib/address-utils"; import { apiError } from "@/lib/api-error"; import { db } from "@/lib/db"; @@ -312,18 +316,32 @@ async function getDiamondFacets( } /** - * Get function selector for an ABI item + * Get function selector for an ABI item. + * + * Goes through computeSelector so tuple parameters expand into their component + * types. A signature built from the raw `input.type` renders every struct as + * the literal "tuple", so two different tuple-taking overloads hash to the same + * value -- and this selector is what combineAbis dedupes facet ABIs on, which + * would drop one of them from the merged ABI entirely. + * + * Returns null for an entry that cannot be canonicalised, which keeps the entry + * out of deduplication rather than discarding it: the ABI is fetched from an + * explorer, and one malformed entry must not decide the fate of the functions + * around it. */ function getFunctionSelector(abiItem: { type: string; name?: string; - inputs?: Array<{ type: string; name?: string }>; + inputs?: Array<{ type: string; name?: string; components?: AbiItemComponent[] }>; }): string | null { if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { return null; } - const signature = `${abiItem.name}(${abiItem.inputs.map((i) => i.type).join(",")})`; - return ethers.id(signature).slice(0, 10); // First 4 bytes + try { + return computeSelector(abiItem.name, abiItem.inputs); + } catch { + return null; + } } /** diff --git a/components/workflow/config/action-config-renderer.tsx b/components/workflow/config/action-config-renderer.tsx index 4984026a4..d31a62aea 100644 --- a/components/workflow/config/action-config-renderer.tsx +++ b/components/workflow/config/action-config-renderer.tsx @@ -29,11 +29,12 @@ import { ArrayInputField } from "@/components/workflow/config/array-input-field" import { MalformedAbiArgsNotice } from "@/components/workflow/config/malformed-abi-notice"; import { TupleInputField } from "@/components/workflow/config/tuple-input-field"; import { + type AbiFunctionInput, isValidAbiInput, resolveFunctionInputs, } from "@/lib/abi/function-inputs"; import { parseAbiFunctionArgs } from "@/lib/abi/parse-args"; -import { computeSelector } from "@/lib/abi/utils"; +import { canonicalType, computeSelector } from "@/lib/abi/utils"; import { evaluateShowWhen } from "@/lib/workflow/editor/show-when"; import { parseAddressBookSelection } from "@/lib/address-book-selection"; import { toChecksumAddress } from "@/lib/address-utils"; @@ -518,8 +519,17 @@ export function AbiFunctionSelectField({ .join(", "); const selector = complete ? computeSelector(func.name, inputs) : null; const isOverloaded = (nameCounts.get(func.name) ?? 0) > 1; + // The stored key must expand tuples the same way the selector above + // does: a raw `input.type` renders a struct as the literal "tuple", + // which ethers rejects as a fragment and which cannot tell two + // overloads apart when they differ only inside the struct. Falls back + // to the raw types when the ABI is too malformed to canonicalise -- + // the same condition that already suppresses the selector. + const keyTypes = complete + ? inputs.map((input: AbiFunctionInput) => canonicalType(input)) + : inputTypes; const key = isOverloaded - ? `${func.name}(${inputTypes.join(",")})` + ? `${func.name}(${keyTypes.join(",")})` : func.name; return { key, diff --git a/lib/abi/function-key.ts b/lib/abi/function-key.ts index 6c171a855..b6d158d87 100644 --- a/lib/abi/function-key.ts +++ b/lib/abi/function-key.ts @@ -1,29 +1,49 @@ import "server-only"; - -type AbiEntry = { - type: string; - name: string; - inputs?: Array<{ type: string }>; -}; +import { + type AbiFunctionItem, + type AbiItem, + canonicalType, +} from "@/lib/abi/utils"; /** * Build a fully qualified function key to disambiguate overloaded ABI functions. * Returns "deposit(uint256,address)" when the ABI has multiple `deposit` overloads, * or the plain function name when unambiguous. + * + * Tuple parameters are expanded into their canonical component types, so an + * overload taking a struct becomes "swap((address,uint256))" -- the spelling + * `ethers` accepts. Building it from the raw ABI types instead yields the + * literal "tuple", which neither encodes nor distinguishes two overloads that + * differ only inside the struct. + * + * Overloads are counted by the resolved entry's own name rather than by + * `functionName`, because callers pass whatever key was stored -- which is + * already a qualified signature for anything configured through the function + * selector. Counting by that string matches no ABI entry, and the key would be + * handed back unchanged. */ export function getAbiFunctionKey( - parsedAbi: AbiEntry[], + parsedAbi: AbiItem[], functionName: string, - functionAbi: AbiEntry + functionAbi: AbiFunctionItem ): string { + const name = functionAbi.name; const matchingFunctions = parsedAbi.filter( - (item) => item.type === "function" && item.name === functionName + (item) => item?.type === "function" && item.name === name ); if (matchingFunctions.length <= 1) { - return functionName; + return name; } - const inputTypes = (functionAbi.inputs ?? []).map((i) => i.type); - return `${functionName}(${inputTypes.join(",")})`; + const inputs = Array.isArray(functionAbi.inputs) ? functionAbi.inputs : []; + try { + return `${name}(${inputs.map((i) => canonicalType(i)).join(",")})`; + } catch { + // The ABI is user-supplied and this entry is malformed enough that no + // canonical signature exists. Fall back to the key as it was passed in: + // the call still fails, but at the encoder, naming the fragment -- rather + // than here, silently, against whichever overload happened to be first. + return functionName; + } } diff --git a/lib/abi/utils.ts b/lib/abi/utils.ts index 64aa98cef..39e140c45 100644 --- a/lib/abi/utils.ts +++ b/lib/abi/utils.ts @@ -16,7 +16,7 @@ type AbiInput = { * e.g. a tuple with (uint32, bytes32) becomes "(uint32,bytes32)" * and a tuple[] becomes "(uint32,bytes32)[]" */ -function canonicalType(input: AbiInput): string { +export function canonicalType(input: AbiInput): string { // The ABI is user-supplied, so `type` can be absent at runtime. Fail loudly // rather than fabricating a signature that would encode the wrong call. if (typeof input?.type !== "string") { @@ -63,41 +63,138 @@ export type AbiItem = { export type AbiFunctionItem = AbiItem & { name: string }; /** - * Find a function in a parsed ABI by key. + * Canonical signature of a function entry, e.g. + * `send((uint32,bytes32),address)`. This is the spelling `ethers` accepts. * - * The key can be a plain name (`"send"`) or a qualified signature - * (`"send(address,uint256,bytes)"`). Plain names match when the ABI - * contains at most one function with that name. Qualified signatures - * are used for overloaded functions. + * Returns undefined when the entry cannot be canonicalised at all -- the ABI + * is user-pasted JSON, so an input may be missing its `type` or carry a + * malformed `components`. Callers treat such an entry as "not a canonical + * match" and keep looking, rather than failing the whole lookup: one broken + * entry must not hide the healthy functions next to it. */ -export function findAbiFunction( +function canonicalSignature(item: AbiItem): string | undefined { + try { + const inputs = Array.isArray(item.inputs) ? item.inputs : []; + return `${item.name}(${inputs.map((i) => canonicalType(i)).join(",")})`; + } catch { + return; + } +} + +/** + * Signature built from the raw ABI types, the spelling qualified keys were + * stored in before tuples were expanded. Undefined when any input is missing + * its `type`, so a corrupt entry cannot be matched by a key that stringifies + * to the same text. + */ +function legacySignature(item: AbiItem): string | undefined { + const inputs = Array.isArray(item.inputs) ? item.inputs : []; + if (!inputs.every((i) => typeof i?.type === "string")) { + return; + } + return `${item.name}(${inputs.map((i) => i.type).join(",")})`; +} + +/** Why a qualified key did not resolve to exactly one function. */ +export type AbiFunctionResolution = + | { status: "found"; entry: AbiFunctionItem; canonicalKey: string } + | { status: "not_found" } + | { status: "ambiguous"; candidates: AbiFunctionItem[] }; + +/** + * Resolve a function key to a single ABI entry, reporting *why* it failed. + * + * Prefer this over `findAbiFunction` wherever the caller can surface an error, + * because `undefined` alone cannot tell "no such function" from "this key + * matches several overloads". + * + * Qualified keys are matched canonically first (`send((uint32,bytes32),address)`) + * and then against the legacy raw spelling (`send(tuple,address)`), which older + * saved workflows and external API callers still send. A legacy key resolves + * when it identifies exactly one overload; it is reported as `ambiguous` only + * when two overloads share the same raw spelling, which is the one case where + * the choice was never recoverable from what was stored. + */ +export function resolveAbiFunction( abi: AbiItem[], key: string | undefined | null -): AbiFunctionItem | undefined { +): AbiFunctionResolution { if (!key) { - return; + return { status: "not_found" }; } + const parenIdx = key.indexOf("("); + const name = parenIdx === -1 ? key : key.slice(0, parenIdx); + + const named = abi.filter( + (item): item is AbiFunctionItem => + item != null && item.type === "function" && item.name === name + ); + if (parenIdx === -1) { - return abi.find( - (item): item is AbiFunctionItem => - item != null && item.type === "function" && item.name === key - ); + // Plain names keep their long-standing first-match behaviour. + const entry = named[0]; + return entry + ? { status: "found", entry, canonicalKey: canonicalSignature(entry) ?? key } + : { status: "not_found" }; } - const name = key.slice(0, parenIdx); - const typesStr = key.slice(parenIdx + 1, -1); - const targetTypes = typesStr === "" ? [] : typesStr.split(","); - - return abi.find((item): item is AbiFunctionItem => { - if (item == null || item.type !== "function" || item.name !== name) { - return false; - } - const rawInputs = Array.isArray(item.inputs) ? item.inputs : []; - const inputTypes = rawInputs.map((i) => i?.type); - if (inputTypes.length !== targetTypes.length) { - return false; - } - return inputTypes.every((t, idx) => t === targetTypes[idx]); - }); + const canonical = named.filter((item) => canonicalSignature(item) === key); + if (canonical.length === 1) { + return { status: "found", entry: canonical[0], canonicalKey: key }; + } + + const legacy = named.filter((item) => legacySignature(item) === key); + if (legacy.length === 1) { + const entry = legacy[0]; + return { + status: "found", + entry, + canonicalKey: canonicalSignature(entry) ?? key, + }; + } + if (legacy.length > 1) { + return { status: "ambiguous", candidates: legacy }; + } + + return { status: "not_found" }; +} + +/** + * Explain an ambiguous legacy key, naming the overloads to choose between. + * + * The stored key is a raw-type signature that two overloads share, so which + * one the user picked was never recorded. Nothing can recover it -- the message + * has to send them back to the function selector. + */ +export function describeAmbiguousKey( + key: string, + candidates: AbiFunctionItem[] +): string { + const options = candidates + .map((c) => canonicalSignature(c) ?? legacySignature(c) ?? c.name) + .join(", "); + return `Function '${key}' matches ${candidates.length} overloads in this ABI, so the one to call cannot be determined. Re-select the function to store its full signature: ${options}`; +} + +/** + * Find a function in a parsed ABI by key. + * + * The key can be a plain name (`"send"`) or a qualified signature, either + * canonical (`"send((uint32,bytes32),address)"`) or in the legacy raw spelling + * (`"send(tuple,address)"`). Plain names return the first function with that + * name. Qualified signatures select one overload. + * + * Total by design: it never throws, so the UI helpers that call it outside a + * try/catch (`resolveFunctionInputs`, `deriveStateMutability`) keep failing + * closed on a malformed ABI. It returns undefined when a legacy key matches + * several overloads -- use `resolveAbiFunction` where that needs saying out + * loud. + */ +export function findAbiFunction( + abi: AbiItem[], + key: string | undefined | null +): AbiFunctionItem | undefined { + const resolution = resolveAbiFunction(abi, key); + return resolution.status === "found" ? resolution.entry : undefined; } diff --git a/lib/execute/simulate.ts b/lib/execute/simulate.ts index 52d5b4f02..69b70f72e 100644 --- a/lib/execute/simulate.ts +++ b/lib/execute/simulate.ts @@ -2,7 +2,11 @@ import "server-only"; import { ethers, isError } from "ethers"; import { coerceArgsForAbi, reshapeArgsForAbi } from "@/lib/abi/struct-args"; -import { type AbiItem, findAbiFunction } from "@/lib/abi/utils"; +import { + type AbiItem, + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { describeNativeShortfall, getNativeSymbol, @@ -594,8 +598,16 @@ export async function simulateContractCall( } const abiArray = abiArrayOrError; - const abiFn = findAbiFunction(abiArray as AbiItem[], input.functionName); - if (!abiFn) { + const resolution = resolveAbiFunction(abiArray as AbiItem[], input.functionName); + if (resolution.status === "ambiguous") { + return failure( + from, + to, + value, + describeAmbiguousKey(input.functionName, resolution.candidates) + ); + } + if (resolution.status !== "found") { return failure( from, to, @@ -603,6 +615,7 @@ export async function simulateContractCall( `Function ${input.functionName} not found in ABI` ); } + const abiFn = resolution.entry; const argsOrError = parseFunctionArgs(input.functionArgs); if (typeof argsOrError === "string") { @@ -615,7 +628,10 @@ export async function simulateContractCall( iface = new ethers.Interface(abiArray as ethers.InterfaceAbi); const coerced = coerceArgsForAbi(argsOrError, abiFn); const reshaped = reshapeArgsForAbi(coerced, abiFn); - encodedData = iface.encodeFunctionData(input.functionName, reshaped); + // Encode with the signature derived from the resolved entry, not with the + // key as supplied: an API or MCP caller may send the legacy raw spelling + // (`f(tuple)`), which resolves here but is not a fragment ethers accepts. + encodedData = iface.encodeFunctionData(resolution.canonicalKey, reshaped); } catch (err) { return failure( from, diff --git a/plugins/web3/steps/batch-write-contract-core.ts b/plugins/web3/steps/batch-write-contract-core.ts index c25ccb11a..be3dad7db 100644 --- a/plugins/web3/steps/batch-write-contract-core.ts +++ b/plugins/web3/steps/batch-write-contract-core.ts @@ -18,7 +18,10 @@ import { eq } from "drizzle-orm"; import { ethers } from "ethers"; import { coerceArgsForAbi, reshapeArgsForAbi } from "@/lib/abi/struct-args"; import { validateArgsForAbi } from "@/lib/abi/validate-args"; -import { findAbiFunction } from "@/lib/abi/utils"; +import { + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { getAbiFunctionKey } from "@/lib/abi/function-key"; import { db } from "@/lib/db"; import { workflowExecutions } from "@/lib/db/schema"; @@ -392,13 +395,20 @@ function buildCallWithMeta( return { ok: false, error: `Call at index ${index}: ABI must be a JSON array` }; } - const functionAbi = findAbiFunction(parsedAbi, rawCall.abiFunction); - if (!functionAbi) { + const resolution = resolveAbiFunction(parsedAbi, rawCall.abiFunction); + if (resolution.status === "ambiguous") { + return { + ok: false, + error: `Call at index ${index}: ${describeAmbiguousKey(rawCall.abiFunction, resolution.candidates)}`, + }; + } + if (resolution.status !== "found") { return { ok: false, error: `Call at index ${index}: Function '${rawCall.abiFunction}' not found in ABI`, }; } + const functionAbi = resolution.entry; const functionKey = getAbiFunctionKey(parsedAbi, rawCall.abiFunction, functionAbi); const { args, error: argsError } = coerceAndValidateArgs( diff --git a/plugins/web3/steps/read-contract-core.ts b/plugins/web3/steps/read-contract-core.ts index adfcbe8f0..38c2410a1 100644 --- a/plugins/web3/steps/read-contract-core.ts +++ b/plugins/web3/steps/read-contract-core.ts @@ -15,7 +15,10 @@ import { validateArgsForAbi } from "@/lib/abi/validate-args"; import { ErrorCategory, logUserError } from "@/lib/logging"; import { getChainIdFromNetwork } from "@/lib/rpc/network-utils"; import { getRpcProvider } from "@/lib/rpc/provider-factory"; -import { findAbiFunction } from "@/lib/abi/utils"; +import { + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { getErrorMessage } from "@/lib/utils"; import { getAbiFunctionKey } from "@/lib/abi/function-key"; import { getChainAdapter } from "@/lib/web3/chain-adapter"; @@ -143,9 +146,20 @@ async function readContractInner( return { success: false, error: "ABI must be a JSON array", errorClass: ExecutionErrorType.USER }; } - const functionAbi = findAbiFunction(parsedAbi, abiFunction); + const resolution = resolveAbiFunction(parsedAbi, abiFunction); + + if (resolution.status === "ambiguous") { + const error = describeAmbiguousKey(abiFunction, resolution.candidates); + logUserError( + ErrorCategory.VALIDATION, + "[Read Contract] Ambiguous function key:", + abiFunction, + { plugin_name: "web3", action_name: "read-contract" } + ); + return { success: false, error, errorClass: ExecutionErrorType.USER }; + } - if (!functionAbi) { + if (resolution.status !== "found") { logUserError( ErrorCategory.VALIDATION, "[Read Contract] Function not found in ABI:", @@ -159,6 +173,7 @@ async function readContractInner( }; } + const functionAbi = resolution.entry; const abiFunctionKey = getAbiFunctionKey(parsedAbi, abiFunction, functionAbi); // Parse function arguments diff --git a/plugins/web3/steps/write-contract-core.ts b/plugins/web3/steps/write-contract-core.ts index 41b6e442f..f041c9478 100644 --- a/plugins/web3/steps/write-contract-core.ts +++ b/plugins/web3/steps/write-contract-core.ts @@ -25,7 +25,10 @@ import { import { getChainIdFromNetwork } from "@/lib/rpc/network-utils"; import { getRpcProvider } from "@/lib/rpc/provider-factory"; import { rpcRelayErrorClass } from "@/lib/rpc/providers"; -import { findAbiFunction } from "@/lib/abi/utils"; +import { + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { getErrorMessage, resolveFailOnError } from "@/lib/utils"; import { getAbiFunctionKey } from "@/lib/abi/function-key"; import { generateId } from "@/lib/utils/id"; @@ -274,9 +277,17 @@ export async function writeContractCore( }; } - const functionAbi = findAbiFunction(parsedAbi, abiFunction); + const resolution = resolveAbiFunction(parsedAbi, abiFunction); + + if (resolution.status === "ambiguous") { + return { + success: false, + error: describeAmbiguousKey(abiFunction, resolution.candidates), + errorClass: ExecutionErrorType.USER, + }; + } - if (!functionAbi) { + if (resolution.status !== "found") { return { success: false, error: `Function '${abiFunction}' not found in ABI`, @@ -284,6 +295,7 @@ export async function writeContractCore( }; } + const functionAbi = resolution.entry; const abiFunctionKey = getAbiFunctionKey(parsedAbi, abiFunction, functionAbi); // Parse function arguments diff --git a/tests/unit/abi-function-key.test.ts b/tests/unit/abi-function-key.test.ts new file mode 100644 index 000000000..42da84d84 --- /dev/null +++ b/tests/unit/abi-function-key.test.ts @@ -0,0 +1,127 @@ +import { ethers } from "ethers"; +import { describe, expect, it, vi } from "vitest"; + +vi.mock("server-only", () => ({})); + +import { getAbiFunctionKey } from "@/lib/abi/function-key"; +import { + type AbiItem, + findAbiFunction, + resolveAbiFunction, +} from "@/lib/abi/utils"; + +const TOKEN = "0x4200000000000000000000000000000000000006"; + +const SWAP_ABI: AbiItem[] = [ + { + type: "function", + name: "swap", + stateMutability: "nonpayable", + inputs: [{ name: "amount", type: "uint256" }], + }, + { + type: "function", + name: "swap", + stateMutability: "nonpayable", + inputs: [ + { + name: "params", + type: "tuple", + components: [ + { name: "token", type: "address" }, + { name: "amount", type: "uint256" }, + ], + }, + ], + }, + { + type: "function", + name: "transfer", + stateMutability: "nonpayable", + inputs: [ + { name: "to", type: "address" }, + { name: "amount", type: "uint256" }, + ], + }, +]; + +function keyFor(abi: AbiItem[], storedKey: string): string { + const resolution = resolveAbiFunction(abi, storedKey); + if (resolution.status !== "found") { + throw new Error(`expected ${storedKey} to resolve, got ${resolution.status}`); + } + return getAbiFunctionKey(abi, storedKey, resolution.entry); +} + +describe("getAbiFunctionKey", () => { + it("returns the plain name when the function is not overloaded", () => { + expect(keyFor(SWAP_ABI, "transfer")).toBe("transfer"); + }); + + it("expands a tuple parameter into its component types", () => { + expect(keyFor(SWAP_ABI, "swap((address,uint256))")).toBe( + "swap((address,uint256))" + ); + }); + + it("qualifies a bare name that is overloaded", () => { + const entry = findAbiFunction(SWAP_ABI, "swap"); + if (!entry) { + throw new Error("expected swap to resolve"); + } + expect(getAbiFunctionKey(SWAP_ABI, "swap", entry)).toBe("swap(uint256)"); + }); + + it("rebuilds the key when the stored value is already qualified", () => { + // Overloads are counted by the entry's own name. Counting by the passed-in + // string would match no ABI entry, and a legacy key would be handed back + // unchanged -- which is the shape that fails to encode. + expect(keyFor(SWAP_ABI, "swap(tuple)")).toBe("swap((address,uint256))"); + }); + + it("leaves a non-tuple overload key unchanged", () => { + expect(keyFor(SWAP_ABI, "swap(uint256)")).toBe("swap(uint256)"); + }); +}); + +describe("resolve then encode", () => { + // The seam the two halves meet at: a key that resolves is not necessarily a + // key ethers accepts. Every case below runs all the way to call data. + const iface = new ethers.Interface(SWAP_ABI as ethers.InterfaceAbi); + const tupleArgs = [{ token: TOKEN, amount: 1n }]; + + it("encodes the tuple overload from its canonical key", () => { + const data = iface.encodeFunctionData( + keyFor(SWAP_ABI, "swap((address,uint256))"), + tupleArgs + ); + expect(data.slice(0, 10)).toBe("0xc546f7f6"); + }); + + it("encodes the tuple overload from a stored legacy key", () => { + const data = iface.encodeFunctionData( + keyFor(SWAP_ABI, "swap(tuple)"), + tupleArgs + ); + expect(data.slice(0, 10)).toBe("0xc546f7f6"); + }); + + it("encodes the scalar overload of the same name", () => { + const data = iface.encodeFunctionData(keyFor(SWAP_ABI, "swap(uint256)"), [ + 1n, + ]); + expect(data.slice(0, 10)).toBe("0x94b918de"); + }); + + it("encodes an unambiguous request-supplied legacy key", () => { + // API and MCP callers send the key themselves, so both spellings have to + // survive resolution and reach the encoder in a form ethers accepts. + const resolution = resolveAbiFunction(SWAP_ABI, "swap(tuple)"); + if (resolution.status !== "found") { + throw new Error("expected the legacy key to resolve"); + } + expect(() => + iface.encodeFunctionData(resolution.canonicalKey, tupleArgs) + ).not.toThrow(); + }); +}); diff --git a/tests/unit/abi-utils.test.ts b/tests/unit/abi-utils.test.ts index 02f54e840..bbd5b8179 100644 --- a/tests/unit/abi-utils.test.ts +++ b/tests/unit/abi-utils.test.ts @@ -2,8 +2,11 @@ import { describe, expect, it } from "vitest"; import { type AbiItem, + canonicalType, computeSelector, + describeAmbiguousKey, findAbiFunction, + resolveAbiFunction, } from "@/lib/abi/utils"; const SELECTOR_PATTERN = /^0x[\da-f]{8}$/; @@ -172,7 +175,19 @@ describe("findAbiFunction", () => { expect(result?.inputs).toHaveLength(2); }); - it("finds the other overload by qualified signature", () => { + it("finds the tuple overload by canonical signature", () => { + const result = findAbiFunction( + OVERLOADED_ABI, + "send((uint32,bytes32),address)" + ); + expect(result).toBeDefined(); + expect(result?.stateMutability).toBe("payable"); + }); + + it("still finds the tuple overload by its legacy raw signature", () => { + // Keys stored before tuples were expanded spell a struct as "tuple". They + // stay valid wherever they identify one overload, so saved workflows and + // external API callers keep working. const result = findAbiFunction(OVERLOADED_ABI, "send(tuple,address)"); expect(result).toBeDefined(); expect(result?.stateMutability).toBe("payable"); @@ -210,3 +225,201 @@ describe("findAbiFunction", () => { expect(result?.type).toBe("function"); }); }); + +const COLLIDING_ABI: AbiItem[] = [ + { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { type: "address", name: "owner" }, + { + type: "tuple", + name: "permitSingle", + components: [ + { name: "token", type: "address" }, + { name: "amount", type: "uint160" }, + ], + }, + { type: "bytes", name: "signature" }, + ], + }, + { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { type: "address", name: "owner" }, + { + type: "tuple", + name: "permitBatch", + components: [ + { name: "spender", type: "address" }, + { name: "deadline", type: "uint256" }, + ], + }, + { type: "bytes", name: "signature" }, + ], + }, +]; + +describe("canonicalType", () => { + it("expands a tuple into its component types", () => { + expect( + canonicalType({ + type: "tuple", + components: [ + { name: "a", type: "uint32" }, + { name: "b", type: "bytes32" }, + ], + }) + ).toBe("(uint32,bytes32)"); + }); + + it("keeps the array suffix on a tuple array", () => { + expect( + canonicalType({ + type: "tuple[]", + components: [{ name: "a", type: "uint256" }], + }) + ).toBe("(uint256)[]"); + }); + + it("throws on an input with no type rather than fabricating a signature", () => { + expect(() => + canonicalType({ components: [] } as unknown as { type: string }) + ).toThrow(); + }); +}); + +describe("computeSelector on overloads that differ only inside a struct", () => { + // The Diamond facet merge in app/api/web3/fetch-abi dedupes on this selector. + // Raw ABI types render both structs as the literal "tuple", so the two + // functions would collide and one would be dropped from the merged ABI. + it("gives two tuple overloads distinct selectors", () => { + const first = computeSelector("permit", COLLIDING_ABI[0].inputs ?? []); + const second = computeSelector("permit", COLLIDING_ABI[1].inputs ?? []); + expect(first).not.toBe(second); + }); + + it("collides when the signature is built from raw types instead", () => { + const rawSignature = (item: AbiItem) => + `${item.name}(${(item.inputs ?? []).map((i) => i.type).join(",")})`; + expect(rawSignature(COLLIDING_ABI[0])).toBe(rawSignature(COLLIDING_ABI[1])); + }); +}); + +describe("resolveAbiFunction", () => { + it("reports a canonical key as found", () => { + const result = resolveAbiFunction( + OVERLOADED_ABI, + "send((uint32,bytes32),address)" + ); + expect(result.status).toBe("found"); + }); + + it("returns the canonical key for a legacy raw key", () => { + const result = resolveAbiFunction(OVERLOADED_ABI, "send(tuple,address)"); + expect(result).toMatchObject({ + status: "found", + canonicalKey: "send((uint32,bytes32),address)", + }); + }); + + it("reports a legacy key that two overloads share as ambiguous", () => { + const result = resolveAbiFunction( + COLLIDING_ABI, + "permit(address,tuple,bytes)" + ); + expect(result.status).toBe("ambiguous"); + if (result.status === "ambiguous") { + expect(result.candidates).toHaveLength(2); + } + }); + + it("resolves each colliding overload by its own canonical key", () => { + const single = resolveAbiFunction( + COLLIDING_ABI, + "permit(address,(address,uint160),bytes)" + ); + const batch = resolveAbiFunction( + COLLIDING_ABI, + "permit(address,(address,uint256),bytes)" + ); + expect(single.status).toBe("found"); + expect(batch.status).toBe("found"); + if (single.status === "found" && batch.status === "found") { + expect(single.entry).not.toBe(batch.entry); + } + }); + + it("keeps first-match behaviour for a plain name", () => { + const result = resolveAbiFunction(OVERLOADED_ABI, "send"); + expect(result).toMatchObject({ status: "found" }); + if (result.status === "found") { + expect(result.entry.stateMutability).toBe("payable"); + } + }); + + it("reports an unknown key as not found", () => { + expect(resolveAbiFunction(OVERLOADED_ABI, "missing(uint256)")).toEqual({ + status: "not_found", + }); + }); + + it("finds a healthy function next to an entry that cannot be canonicalised", () => { + const abi = [ + { + type: "function", + name: "broken", + inputs: [{ name: "a" }], + }, + { + type: "function", + name: "broken", + inputs: [{ name: "b", type: "uint256" }], + }, + ] as unknown as AbiItem[]; + const result = resolveAbiFunction(abi, "broken(uint256)"); + expect(result.status).toBe("found"); + }); + + it("does not match a corrupt entry against a stringified key", () => { + const abi = [ + { type: "function", name: "broken", inputs: [{ name: "a" }] }, + ] as unknown as AbiItem[]; + expect(resolveAbiFunction(abi, "broken(undefined)")).toEqual({ + status: "not_found", + }); + }); + + it("tolerates components that are not an array", () => { + const abi = [ + { + type: "function", + name: "weird", + inputs: [{ name: "p", type: "tuple", components: { a: "uint256" } }], + }, + ] as unknown as AbiItem[]; + expect(() => resolveAbiFunction(abi, "weird(tuple)")).not.toThrow(); + expect(resolveAbiFunction(abi, "weird(tuple)").status).toBe("found"); + }); +}); + +describe("describeAmbiguousKey", () => { + it("names the canonical signatures to choose between", () => { + const result = resolveAbiFunction( + COLLIDING_ABI, + "permit(address,tuple,bytes)" + ); + if (result.status !== "ambiguous") { + throw new Error("expected an ambiguous resolution"); + } + const message = describeAmbiguousKey( + "permit(address,tuple,bytes)", + result.candidates + ); + expect(message).toContain("permit(address,(address,uint160),bytes)"); + expect(message).toContain("permit(address,(address,uint256),bytes)"); + }); +}); From 12006c317785bb39d526d66843c99b0a8b5bc192 Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Mon, 7 Sep 2026 18:44:14 +0300 Subject: [PATCH 2/7] fix: #2330 resolve duplicated entries and legacy keys past the encoder Three cases the first pass left open. A legacy key was reported ambiguous by counting matching entries rather than distinct signatures, so an ABI that lists one function twice -- merged facet ABIs and some explorer responses do -- turned a scalar call that resolved on staging into an ambiguity error, and a duplicated tuple function into not found under its canonical key. Ambiguity now means two different overloads share the raw spelling; repeats of one signature collapse to their first occurrence, and the message lists each overload once. simulate encoded with the resolved signature but still decoded the return data with the key as supplied, so a legacy tuple key produced correct call data and then handed the caller raw hex instead of the decoded value, the decode failure having fallen through silently. Both directions now use the canonical key. The contract-call route resolves the key itself before choosing the read or write path, and its own lookup collapsed an ambiguous key to not found, so the message naming the signatures to choose from was unreachable through the API. The route now distinguishes the two. Tests cover each: duplicated scalar and tuple entries, ambiguity counted across distinct overloads only, the legacy-key return value through simulateContractCall, and the 400 body from the route. --- app/api/execute/contract-call/route.ts | 16 +- app/api/web3/fetch-abi/route.ts | 11 +- lib/abi/utils.ts | 36 +++- lib/execute/simulate.ts | 7 +- tests/unit/abi-function-key.test.ts | 4 +- tests/unit/abi-utils.test.ts | 80 ++++++++ .../unit/contract-call-ambiguous-key.test.ts | 178 ++++++++++++++++++ tests/unit/execute-simulate.test.ts | 46 +++++ 8 files changed, 361 insertions(+), 17 deletions(-) create mode 100644 tests/unit/contract-call-ambiguous-key.test.ts diff --git a/app/api/execute/contract-call/route.ts b/app/api/execute/contract-call/route.ts index 4817e6eed..50fd7c819 100644 --- a/app/api/execute/contract-call/route.ts +++ b/app/api/execute/contract-call/route.ts @@ -4,7 +4,11 @@ import "server-only"; import { NextResponse } from "next/server"; import { resolveAbi } from "@/lib/abi/cache"; -import { type AbiItem, findAbiFunction } from "@/lib/abi/utils"; +import { + type AbiItem, + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { enforceExecutionLimit } from "@/lib/billing/execution-guard"; import { enterApiExecuteErrorContext } from "@/lib/db/org-helpers"; import { simulateContractCall } from "@/lib/execute/simulate"; @@ -54,13 +58,17 @@ function findFunctionInAbi( return { error: "ABI must be a JSON array" }; } - const entry = findAbiFunction(parsed, functionName); + const resolution = resolveAbiFunction(parsed, functionName); + + if (resolution.status === "ambiguous") { + return { error: describeAmbiguousKey(functionName, resolution.candidates) }; + } - if (!entry) { + if (resolution.status !== "found") { return { error: `Function '${functionName}' not found in ABI` }; } - return { entry }; + return { entry: resolution.entry }; } async function resolveAbiForRequest( diff --git a/app/api/web3/fetch-abi/route.ts b/app/api/web3/fetch-abi/route.ts index 572968fb4..4896cfaae 100644 --- a/app/api/web3/fetch-abi/route.ts +++ b/app/api/web3/fetch-abi/route.ts @@ -1,10 +1,7 @@ import { eq } from "drizzle-orm"; import { ethers } from "ethers"; import { NextResponse } from "next/server"; -import { - type AbiItemComponent, - computeSelector, -} from "@/lib/abi/utils"; +import { type AbiItemComponent, computeSelector } from "@/lib/abi/utils"; import { toChecksumAddress } from "@/lib/address-utils"; import { apiError } from "@/lib/api-error"; import { db } from "@/lib/db"; @@ -332,7 +329,11 @@ async function getDiamondFacets( function getFunctionSelector(abiItem: { type: string; name?: string; - inputs?: Array<{ type: string; name?: string; components?: AbiItemComponent[] }>; + inputs?: Array<{ + type: string; + name?: string; + components?: AbiItemComponent[]; + }>; }): string | null { if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { return null; diff --git a/lib/abi/utils.ts b/lib/abi/utils.ts index 39e140c45..6ca9659f3 100644 --- a/lib/abi/utils.ts +++ b/lib/abi/utils.ts @@ -135,16 +135,24 @@ export function resolveAbiFunction( // Plain names keep their long-standing first-match behaviour. const entry = named[0]; return entry - ? { status: "found", entry, canonicalKey: canonicalSignature(entry) ?? key } + ? { + status: "found", + entry, + canonicalKey: canonicalSignature(entry) ?? key, + } : { status: "not_found" }; } - const canonical = named.filter((item) => canonicalSignature(item) === key); - if (canonical.length === 1) { - return { status: "found", entry: canonical[0], canonicalKey: key }; + // Every canonical match spells the same signature, so however many entries + // repeat it -- merged facet ABIs do -- they are one function, not overloads. + const canonical = named.find((item) => canonicalSignature(item) === key); + if (canonical) { + return { status: "found", entry: canonical, canonicalKey: key }; } - const legacy = named.filter((item) => legacySignature(item) === key); + const legacy = distinctBySignature( + named.filter((item) => legacySignature(item) === key) + ); if (legacy.length === 1) { const entry = legacy[0]; return { @@ -160,6 +168,24 @@ export function resolveAbiFunction( return { status: "not_found" }; } +/** + * Collapse entries that spell the same canonical signature to their first + * occurrence. A legacy key is ambiguous when it matches *different* overloads, + * not when one function happens to be listed twice. + */ +function distinctBySignature(entries: AbiFunctionItem[]): AbiFunctionItem[] { + const seen = new Set(); + const distinct: AbiFunctionItem[] = []; + for (const entry of entries) { + const signature = canonicalSignature(entry) ?? legacySignature(entry) ?? ""; + if (!seen.has(signature)) { + seen.add(signature); + distinct.push(entry); + } + } + return distinct; +} + /** * Explain an ambiguous legacy key, naming the overloads to choose between. * diff --git a/lib/execute/simulate.ts b/lib/execute/simulate.ts index 69b70f72e..51931a095 100644 --- a/lib/execute/simulate.ts +++ b/lib/execute/simulate.ts @@ -598,7 +598,10 @@ export async function simulateContractCall( } const abiArray = abiArrayOrError; - const resolution = resolveAbiFunction(abiArray as AbiItem[], input.functionName); + const resolution = resolveAbiFunction( + abiArray as AbiItem[], + input.functionName + ); if (resolution.status === "ambiguous") { return failure( from, @@ -697,7 +700,7 @@ export async function simulateContractCall( if (returnData && returnData !== "0x") { try { const decoded = iface.decodeFunctionResult( - input.functionName, + resolution.canonicalKey, returnData ); simulatedReturnValue = diff --git a/tests/unit/abi-function-key.test.ts b/tests/unit/abi-function-key.test.ts index 42da84d84..e394bb121 100644 --- a/tests/unit/abi-function-key.test.ts +++ b/tests/unit/abi-function-key.test.ts @@ -48,7 +48,9 @@ const SWAP_ABI: AbiItem[] = [ function keyFor(abi: AbiItem[], storedKey: string): string { const resolution = resolveAbiFunction(abi, storedKey); if (resolution.status !== "found") { - throw new Error(`expected ${storedKey} to resolve, got ${resolution.status}`); + throw new Error( + `expected ${storedKey} to resolve, got ${resolution.status}` + ); } return getAbiFunctionKey(abi, storedKey, resolution.entry); } diff --git a/tests/unit/abi-utils.test.ts b/tests/unit/abi-utils.test.ts index bbd5b8179..9e73e008c 100644 --- a/tests/unit/abi-utils.test.ts +++ b/tests/unit/abi-utils.test.ts @@ -406,6 +406,86 @@ describe("resolveAbiFunction", () => { }); }); +describe("resolveAbiFunction on duplicated entries", () => { + // Merged facet ABIs and some explorer responses list one function twice. + // Repeats of the same signature are one function, not overloads. + const DUPLICATED_SCALAR: AbiItem[] = [ + { + type: "function", + name: "f", + stateMutability: "view", + inputs: [{ name: "x", type: "uint256" }], + }, + { + type: "function", + name: "f", + stateMutability: "view", + inputs: [{ name: "x", type: "uint256" }], + }, + ]; + + const DUPLICATED_TUPLE: AbiItem[] = [ + { + type: "function", + name: "f", + stateMutability: "view", + inputs: [ + { + name: "p", + type: "tuple", + components: [{ name: "a", type: "address" }], + }, + ], + }, + { + type: "function", + name: "f", + stateMutability: "view", + inputs: [ + { + name: "p", + type: "tuple", + components: [{ name: "a", type: "address" }], + }, + ], + }, + ]; + + it("resolves a scalar function that is listed twice", () => { + expect(resolveAbiFunction(DUPLICATED_SCALAR, "f(uint256)").status).toBe( + "found" + ); + expect(findAbiFunction(DUPLICATED_SCALAR, "f(uint256)")).toBeDefined(); + }); + + it("resolves a tuple function that is listed twice by its canonical key", () => { + expect(resolveAbiFunction(DUPLICATED_TUPLE, "f((address))").status).toBe( + "found" + ); + }); + + it("resolves a tuple function that is listed twice by its legacy key", () => { + expect(resolveAbiFunction(DUPLICATED_TUPLE, "f(tuple)").status).toBe( + "found" + ); + }); + + it("reports ambiguity only across distinct overloads, listing each once", () => { + const abi: AbiItem[] = [...DUPLICATED_TUPLE, ...COLLIDING_ABI]; + const tuple = resolveAbiFunction(abi, "f(tuple)"); + expect(tuple.status).toBe("found"); + + const permit = resolveAbiFunction( + [...COLLIDING_ABI, COLLIDING_ABI[0]], + "permit(address,tuple,bytes)" + ); + expect(permit.status).toBe("ambiguous"); + if (permit.status === "ambiguous") { + expect(permit.candidates).toHaveLength(2); + } + }); +}); + describe("describeAmbiguousKey", () => { it("names the canonical signatures to choose between", () => { const result = resolveAbiFunction( diff --git a/tests/unit/contract-call-ambiguous-key.test.ts b/tests/unit/contract-call-ambiguous-key.test.ts new file mode 100644 index 000000000..1f1e426bd --- /dev/null +++ b/tests/unit/contract-call-ambiguous-key.test.ts @@ -0,0 +1,178 @@ +/** + * POST /api/execute/contract-call resolves the function key itself before it + * decides between the read and write paths. A legacy key that two overloads + * share has to be reported as ambiguous at that point: if the route's own + * lookup collapses it to "not found", the message naming the signatures to + * choose from, which the core steps emit, is never reached. + * + * Run with: pnpm vitest tests/unit/contract-call-ambiguous-key.test.ts + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("server-only", () => ({})); + +const mockValidateApiKey = vi.fn(); +vi.mock("@/app/api/execute/_lib/auth", () => ({ + validateApiKey: (...args: unknown[]) => mockValidateApiKey(...args), +})); + +const mockCheckRateLimit = vi.fn(); +vi.mock("@/app/api/execute/_lib/rate-limit", () => ({ + checkRateLimit: (...args: unknown[]) => mockCheckRateLimit(...args), +})); + +vi.mock("@/app/api/execute/_lib/concurrency-limit", () => ({ + enforceDirectExecutionConcurrency: vi.fn().mockResolvedValue(null), +})); + +const mockValidateContractCallInput = vi.fn(); +vi.mock("@/app/api/execute/_lib/validate", async (importActual) => { + const actual = + await importActual(); + return { + ...actual, + validateContractCallInput: (...args: unknown[]) => + mockValidateContractCallInput(...args), + }; +}); + +vi.mock("@/lib/billing/execution-guard", () => ({ + enforceExecutionLimit: vi + .fn() + .mockResolvedValue({ blocked: false, limitResult: null }), + EXECUTION_LIMIT_ERROR: "Monthly execution limit exceeded", + EXECUTION_DEBT_ERROR: "Executions suspended due to unpaid overage invoice.", +})); + +vi.mock("@/app/api/execute/_lib/wallet-check", () => ({ + requireWallet: vi.fn().mockResolvedValue(null), +})); + +// Neither path may be reached: the ambiguity is a 400 before routing. +const mockReadContractCore = vi.fn(); +vi.mock("@/plugins/web3/steps/read-contract-core", () => ({ + readContractCore: (...args: unknown[]) => mockReadContractCore(...args), +})); +const mockWriteContractCore = vi.fn(); +vi.mock("@/plugins/web3/steps/write-contract-core", () => ({ + writeContractCore: (...args: unknown[]) => mockWriteContractCore(...args), +})); + +const mockBeginIdempotentFromRequest = vi.fn(); +vi.mock("@/lib/idempotency", async (importActual) => { + const actual = await importActual(); + return { + ...actual, + beginIdempotentFromRequest: (...args: unknown[]) => + mockBeginIdempotentFromRequest(...args), + }; +}); + +import { POST } from "@/app/api/execute/contract-call/route"; + +const ADDRESS = "0x1234567890123456789012345678901234567890"; + +// Two overloads that differ only inside the struct, as Permit2's `permit` +// does. Their legacy raw spelling is identical. +const COLLIDING_ABI = JSON.stringify([ + { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { name: "owner", type: "address" }, + { + name: "single", + type: "tuple", + components: [ + { name: "token", type: "address" }, + { name: "amount", type: "uint160" }, + ], + }, + { name: "signature", type: "bytes" }, + ], + outputs: [], + }, + { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { name: "owner", type: "address" }, + { + name: "batch", + type: "tuple", + components: [ + { name: "spender", type: "address" }, + { name: "deadline", type: "uint256" }, + ], + }, + { name: "signature", type: "bytes" }, + ], + outputs: [], + }, +]); + +function post(functionName: string): Request { + return new Request("http://localhost/api/execute/contract-call", { + method: "POST", + headers: { + "Content-Type": "application/json", + Authorization: "Bearer kh_test", + }, + body: JSON.stringify({ + chainId: "8453", + contractAddress: ADDRESS, + functionName, + abi: COLLIDING_ABI, + functionArgs: JSON.stringify([]), + }), + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + mockValidateApiKey.mockResolvedValue({ + organizationId: "org-1", + apiKeyId: "key-1", + }); + mockCheckRateLimit.mockReturnValue({ allowed: true }); + mockValidateContractCallInput.mockReturnValue({ valid: true }); +}); + +describe("contract-call with a legacy key two overloads share", () => { + it("returns 400 naming the signatures to choose from", async () => { + const response = await (POST as (req: Request) => Promise)( + post("permit(address,tuple,bytes)") + ); + const body = (await response.json()) as { error: string; field?: string }; + + expect(response.status).toBe(400); + expect(body.field).toBe("functionName"); + expect(body.error).toContain("matches 2 overloads"); + expect(body.error).toContain("permit(address,(address,uint160),bytes)"); + expect(body.error).toContain("permit(address,(address,uint256),bytes)"); + expect(body.error).not.toContain("not found in ABI"); + }); + + it("never reaches the read or write path", async () => { + await (POST as (req: Request) => Promise)( + post("permit(address,tuple,bytes)") + ); + + expect(mockReadContractCore).not.toHaveBeenCalled(); + expect(mockWriteContractCore).not.toHaveBeenCalled(); + expect(mockBeginIdempotentFromRequest).not.toHaveBeenCalled(); + }); + + it("still reports a genuinely missing function as not found", async () => { + const response = await (POST as (req: Request) => Promise)( + post("approve(address,uint256)") + ); + const body = (await response.json()) as { error: string }; + + expect(response.status).toBe(400); + expect(body.error).toContain("not found in ABI"); + }); +}); diff --git a/tests/unit/execute-simulate.test.ts b/tests/unit/execute-simulate.test.ts index 08ec9ac35..eef86bdb2 100644 --- a/tests/unit/execute-simulate.test.ts +++ b/tests/unit/execute-simulate.test.ts @@ -232,6 +232,52 @@ describe("simulateContractCall", () => { ); }); + it("decodes the return value when the key is a legacy tuple spelling", async () => { + // API callers still send keys stored before tuples were expanded, such as + // `f(tuple)`. Resolving that key is only half the job: the call data and + // the returned bytes both have to go through a signature ethers accepts, + // or the decode fails silently and the caller gets raw hex back. + resetSpies(); + const encoded42 = + "0x000000000000000000000000000000000000000000000000000000000000002a"; + executeWithFailover.mockResolvedValueOnce([BigInt(30_000), encoded42]); + + const result = await simulateContractCall({ + organizationId: "org_test", + network: "1", + contractAddress: CONTRACT_ADDRESS, + abi: JSON.stringify([ + { + type: "function", + name: "f", + inputs: [{ name: "x", type: "uint256" }], + outputs: [{ name: "", type: "uint256" }], + stateMutability: "view", + }, + { + type: "function", + name: "f", + inputs: [ + { + name: "p", + type: "tuple", + components: [{ name: "a", type: "uint256" }], + }, + ], + outputs: [{ name: "", type: "uint256" }], + stateMutability: "view", + }, + ]), + functionName: "f(tuple)", + functionArgs: JSON.stringify([{ a: "7" }]), + }); + + expect(result.success).toBe(true); + if (result.success) { + expect(result.simulatedReturnValue).toBe("42"); + } + }); + it("returns wouldRevert with a decoded reason when failover rejects", async () => { resetSpies(); // Build a CALL_EXCEPTION-shaped error carrying a standard From c9ee6a18e6d2b2dca03996ce453880f6bdce5c3e Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Mon, 7 Sep 2026 19:02:32 +0300 Subject: [PATCH 3/7] test: #2330 cover the Diamond facet merge and the paths review found combineAbis, processAbiString and getFunctionSelector move unchanged from the fetch-abi route into lib/abi/combine-abis.ts, with combineAbis as the only export and the route importing it. The merge is what deduplicates facet ABIs on the computed selector, and it could not be tested where it lived: reaching it meant standing up loupe detection and the explorer fetch. Its tests now pin that two overloads differing only inside a struct survive the merge, within one facet and across facets, that a genuine duplicate is still dropped, that order and non-function entries are preserved, and that an entry with no computable selector is neither dropped nor allowed to drop its neighbours. The execute-simulate route test mocks the ABI helpers and provided only findAbiFunction, so the routes' switch to resolveAbiFunction failed it on a missing export. The mock now builds resolveAbiFunction on the same lookup. BigInt literals in the new key test are replaced with BigInt(), which is what the project's ES2017 target allows. --- app/api/web3/fetch-abi/route.ts | 112 +----------- lib/abi/combine-abis.ts | 118 +++++++++++++ .../execute-simulate-route.test.ts | 21 ++- tests/unit/abi-combine-abis.test.ts | 162 ++++++++++++++++++ tests/unit/abi-function-key.test.ts | 4 +- 5 files changed, 300 insertions(+), 117 deletions(-) create mode 100644 lib/abi/combine-abis.ts create mode 100644 tests/unit/abi-combine-abis.test.ts diff --git a/app/api/web3/fetch-abi/route.ts b/app/api/web3/fetch-abi/route.ts index 4896cfaae..d5c945bc1 100644 --- a/app/api/web3/fetch-abi/route.ts +++ b/app/api/web3/fetch-abi/route.ts @@ -1,7 +1,7 @@ import { eq } from "drizzle-orm"; import { ethers } from "ethers"; import { NextResponse } from "next/server"; -import { type AbiItemComponent, computeSelector } from "@/lib/abi/utils"; +import { combineAbis } from "@/lib/abi/combine-abis"; import { toChecksumAddress } from "@/lib/address-utils"; import { apiError } from "@/lib/api-error"; import { db } from "@/lib/db"; @@ -312,116 +312,6 @@ async function getDiamondFacets( }); } -/** - * Get function selector for an ABI item. - * - * Goes through computeSelector so tuple parameters expand into their component - * types. A signature built from the raw `input.type` renders every struct as - * the literal "tuple", so two different tuple-taking overloads hash to the same - * value -- and this selector is what combineAbis dedupes facet ABIs on, which - * would drop one of them from the merged ABI entirely. - * - * Returns null for an entry that cannot be canonicalised, which keeps the entry - * out of deduplication rather than discarding it: the ABI is fetched from an - * explorer, and one malformed entry must not decide the fate of the functions - * around it. - */ -function getFunctionSelector(abiItem: { - type: string; - name?: string; - inputs?: Array<{ - type: string; - name?: string; - components?: AbiItemComponent[]; - }>; -}): string | null { - if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { - return null; - } - try { - return computeSelector(abiItem.name, abiItem.inputs); - } catch { - return null; - } -} - -/** - * Parse and process a single ABI string - */ -function processAbiString( - abiStr: string, - seenSelectors: Set -): unknown[] { - try { - const abi = JSON.parse(abiStr) as unknown[]; - const items: unknown[] = []; - let functionCount = 0; - let duplicateCount = 0; - - for (const item of abi) { - const abiItem = item as { - type: string; - name?: string; - inputs?: Array<{ type: string; name?: string }>; - }; - - // For functions, check for duplicates by selector - const selector = getFunctionSelector(abiItem); - if (selector) { - functionCount += 1; - if (seenSelectors.has(selector)) { - duplicateCount += 1; - console.log( - `[Diamond] Skipping duplicate function: ${abiItem.name} (selector: ${selector})` - ); - continue; - } - seenSelectors.add(selector); - } - - // Include all items (functions, events, errors, etc.) - items.push(item); - } - - if (functionCount > 0) { - const uniqueFunctions = items.filter( - (i) => (i as { type?: string }).type === "function" - ).length; - console.log( - `[Diamond] Processed ${functionCount} functions (${duplicateCount} duplicates skipped, ${uniqueFunctions} unique)` - ); - } - - return items; - } catch (error) { - logUserError( - ErrorCategory.EXTERNAL_SERVICE, - "[Diamond] Failed to parse facet ABI from Etherscan", - error instanceof Error ? error : new Error(String(error)), - { - service: "etherscan", - component: "diamond-proxy", - } - ); - return []; - } -} - -/** - * Combine multiple ABIs into one, removing duplicates - */ -function combineAbis(abis: string[]): string { - const allItems: unknown[] = []; - const seenSelectors = new Set(); - - for (const abiStr of abis) { - const items = processAbiString(abiStr, seenSelectors); - allItems.push(...items); - } - - return JSON.stringify(allItems); -} - type DiamondFacetResult = { address: string; name: string | null; diff --git a/lib/abi/combine-abis.ts b/lib/abi/combine-abis.ts new file mode 100644 index 000000000..df55fbf1f --- /dev/null +++ b/lib/abi/combine-abis.ts @@ -0,0 +1,118 @@ +import { type AbiItemComponent, computeSelector } from "@/lib/abi/utils"; +import { ErrorCategory, logUserError } from "@/lib/logging"; + +/** + * Get function selector for an ABI item. + * + * Goes through computeSelector so tuple parameters expand into their component + * types. A signature built from the raw `input.type` renders every struct as + * the literal "tuple", so two different tuple-taking overloads hash to the same + * value -- and this selector is what combineAbis dedupes facet ABIs on, which + * would drop one of them from the merged ABI entirely. + * + * Returns null for an entry that cannot be canonicalised, which keeps the entry + * out of deduplication rather than discarding it: the ABI is fetched from an + * explorer, and one malformed entry must not decide the fate of the functions + * around it. + */ +function getFunctionSelector(abiItem: { + type: string; + name?: string; + inputs?: Array<{ + type: string; + name?: string; + components?: AbiItemComponent[]; + }>; +}): string | null { + if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { + return null; + } + try { + return computeSelector(abiItem.name, abiItem.inputs); + } catch { + return null; + } +} + +/** + * Parse and process a single ABI string + */ +function processAbiString( + abiStr: string, + seenSelectors: Set +): unknown[] { + try { + const abi = JSON.parse(abiStr) as unknown[]; + const items: unknown[] = []; + let functionCount = 0; + let duplicateCount = 0; + + for (const item of abi) { + const abiItem = item as { + type: string; + name?: string; + inputs?: Array<{ type: string; name?: string }>; + }; + + // For functions, check for duplicates by selector + const selector = getFunctionSelector(abiItem); + if (selector) { + functionCount += 1; + if (seenSelectors.has(selector)) { + duplicateCount += 1; + console.log( + `[Diamond] Skipping duplicate function: ${abiItem.name} (selector: ${selector})` + ); + continue; + } + seenSelectors.add(selector); + } + + // Include all items (functions, events, errors, etc.) + items.push(item); + } + + if (functionCount > 0) { + const uniqueFunctions = items.filter( + (i) => (i as { type?: string }).type === "function" + ).length; + console.log( + `[Diamond] Processed ${functionCount} functions (${duplicateCount} duplicates skipped, ${uniqueFunctions} unique)` + ); + } + + return items; + } catch (error) { + logUserError( + ErrorCategory.EXTERNAL_SERVICE, + "[Diamond] Failed to parse facet ABI from Etherscan", + error instanceof Error ? error : new Error(String(error)), + { + service: "etherscan", + component: "diamond-proxy", + } + ); + return []; + } +} + +/** + * Combine multiple ABIs into one, removing duplicates. + * + * Used to merge the facet ABIs of a Diamond proxy. Functions are deduplicated + * by selector across all inputs, in order, so the first facet to declare a + * function keeps it; events, errors and other entries pass through untouched. + * A facet that fails to parse contributes nothing and does not affect the + * others. + */ +export function combineAbis(abis: string[]): string { + const allItems: unknown[] = []; + const seenSelectors = new Set(); + + for (const abiStr of abis) { + const items = processAbiString(abiStr, seenSelectors); + allItems.push(...items); + } + + return JSON.stringify(allItems); +} diff --git a/tests/integration/execute-simulate-route.test.ts b/tests/integration/execute-simulate-route.test.ts index 0e7b349c7..32178d085 100644 --- a/tests/integration/execute-simulate-route.test.ts +++ b/tests/integration/execute-simulate-route.test.ts @@ -121,8 +121,8 @@ vi.mock("@/lib/abi/cache", () => ({ resolveAbi: vi.fn(() => Promise.resolve({ abi: "[]" })), })); -vi.mock("@/lib/abi/utils", () => ({ - findAbiFunction: (_abi: unknown, name: string) => { +vi.mock("@/lib/abi/utils", () => { + const findAbiFunction = (_abi: unknown, name: string) => { if (name === "setValue") { return { name, type: "function", stateMutability: "nonpayable" }; } @@ -135,8 +135,21 @@ vi.mock("@/lib/abi/utils", () => ({ }; } return; - }, -})); + }; + // The routes resolve through resolveAbiFunction and only fall back to the + // ambiguity message on a real collision; neither fixture here has one. + return { + findAbiFunction, + resolveAbiFunction: (abi: unknown, name: string) => { + const entry = findAbiFunction(abi, name); + return entry + ? { status: "found", entry, canonicalKey: name } + : { status: "not_found" }; + }, + describeAmbiguousKey: (key: string) => + `Function '${key}' matches several overloads in this ABI`, + }; +}); vi.mock("../../app/api/execute/_lib/condition", () => ({ evaluateCondition: () => ({ met: true }), diff --git a/tests/unit/abi-combine-abis.test.ts b/tests/unit/abi-combine-abis.test.ts new file mode 100644 index 000000000..33287f16c --- /dev/null +++ b/tests/unit/abi-combine-abis.test.ts @@ -0,0 +1,162 @@ +/** + * combineAbis merges the facet ABIs of a Diamond proxy and deduplicates + * functions by selector. The selector has to expand tuple parameters, or two + * overloads that differ only inside a struct collide and the second is dropped + * from the merged ABI at fetch time -- before any lookup could find it. + * + * Run with: pnpm vitest tests/unit/abi-combine-abis.test.ts + */ + +import { describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/logging", () => ({ + ErrorCategory: { EXTERNAL_SERVICE: "external_service" }, + logUserError: vi.fn(), +})); + +import { combineAbis } from "@/lib/abi/combine-abis"; + +type Entry = { type: string; name?: string; inputs?: unknown[] }; + +function functions(merged: string): string[] { + return (JSON.parse(merged) as Entry[]) + .filter((e) => e.type === "function") + .map((e) => `${e.name}/${JSON.stringify(e.inputs)}`); +} + +const PERMIT_SINGLE = { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { name: "owner", type: "address" }, + { + name: "single", + type: "tuple", + components: [ + { name: "token", type: "address" }, + { name: "amount", type: "uint160" }, + ], + }, + { name: "signature", type: "bytes" }, + ], + outputs: [], +}; + +const PERMIT_BATCH = { + type: "function", + name: "permit", + stateMutability: "nonpayable", + inputs: [ + { name: "owner", type: "address" }, + { + name: "batch", + type: "tuple", + components: [ + { name: "spender", type: "address" }, + { name: "deadline", type: "uint256" }, + ], + }, + { name: "signature", type: "bytes" }, + ], + outputs: [], +}; + +const TRANSFER = { + type: "function", + name: "transfer", + stateMutability: "nonpayable", + inputs: [ + { name: "to", type: "address" }, + { name: "amount", type: "uint256" }, + ], + outputs: [{ name: "", type: "bool" }], +}; + +const TRANSFER_EVENT = { + type: "event", + name: "Transfer", + inputs: [ + { name: "from", type: "address", indexed: true }, + { name: "to", type: "address", indexed: true }, + { name: "value", type: "uint256", indexed: false }, + ], + anonymous: false, +}; + +describe("combineAbis", () => { + it("keeps two overloads that differ only inside a struct, within one facet", () => { + const merged = combineAbis([JSON.stringify([PERMIT_SINGLE, PERMIT_BATCH])]); + expect(functions(merged)).toHaveLength(2); + }); + + it("keeps two overloads that differ only inside a struct, across facets", () => { + const merged = combineAbis([ + JSON.stringify([PERMIT_SINGLE]), + JSON.stringify([PERMIT_BATCH]), + ]); + expect(functions(merged)).toHaveLength(2); + }); + + it("drops a genuine duplicate declared by a later facet", () => { + const merged = combineAbis([ + JSON.stringify([TRANSFER]), + JSON.stringify([TRANSFER, PERMIT_SINGLE]), + ]); + expect(functions(merged)).toEqual([ + `transfer/${JSON.stringify(TRANSFER.inputs)}`, + `permit/${JSON.stringify(PERMIT_SINGLE.inputs)}`, + ]); + }); + + it("preserves facet order and passes non-function entries through", () => { + const merged = combineAbis([ + JSON.stringify([TRANSFER_EVENT, TRANSFER]), + JSON.stringify([PERMIT_BATCH]), + ]); + const entries = JSON.parse(merged) as Entry[]; + expect(entries.map((e) => `${e.type}:${e.name}`)).toEqual([ + "event:Transfer", + "function:transfer", + "function:permit", + ]); + }); + + it("keeps the healthy functions beside an entry that cannot be canonicalised", () => { + const broken = { + type: "function", + name: "broken", + inputs: [{ name: "p", type: "tuple", components: [{ name: "a" }] }], + outputs: [], + }; + const merged = combineAbis([ + JSON.stringify([TRANSFER, broken, PERMIT_SINGLE]), + ]); + const names = (JSON.parse(merged) as Entry[]).map((e) => e.name); + expect(names).toEqual(["transfer", "broken", "permit"]); + }); + + it("does not deduplicate entries it cannot compute a selector for", () => { + const broken = { + type: "function", + name: "broken", + inputs: [{ name: "p", type: "tuple", components: [{ name: "a" }] }], + outputs: [], + }; + const merged = combineAbis([JSON.stringify([broken, broken])]); + expect(functions(merged)).toHaveLength(2); + }); + + it("skips a facet that does not parse without affecting the others", () => { + const merged = combineAbis([ + JSON.stringify([TRANSFER]), + "{not json", + JSON.stringify([PERMIT_BATCH]), + ]); + expect(functions(merged)).toHaveLength(2); + }); + + it("returns an empty array for no facets", () => { + expect(combineAbis([])).toBe("[]"); + }); +}); diff --git a/tests/unit/abi-function-key.test.ts b/tests/unit/abi-function-key.test.ts index e394bb121..401de1d4e 100644 --- a/tests/unit/abi-function-key.test.ts +++ b/tests/unit/abi-function-key.test.ts @@ -90,7 +90,7 @@ describe("resolve then encode", () => { // The seam the two halves meet at: a key that resolves is not necessarily a // key ethers accepts. Every case below runs all the way to call data. const iface = new ethers.Interface(SWAP_ABI as ethers.InterfaceAbi); - const tupleArgs = [{ token: TOKEN, amount: 1n }]; + const tupleArgs = [{ token: TOKEN, amount: BigInt(1) }]; it("encodes the tuple overload from its canonical key", () => { const data = iface.encodeFunctionData( @@ -110,7 +110,7 @@ describe("resolve then encode", () => { it("encodes the scalar overload of the same name", () => { const data = iface.encodeFunctionData(keyFor(SWAP_ABI, "swap(uint256)"), [ - 1n, + BigInt(1), ]); expect(data.slice(0, 10)).toBe("0x94b918de"); }); From 294294f4295bdd58565cb7aca55e3e0a07bfad8e Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Tue, 8 Sep 2026 08:44:10 +0300 Subject: [PATCH 4/7] fix: #2330 reject a tuple with no components instead of spelling it "tuple" canonicalType returned the raw type for a tuple that carried no components, so the literal "tuple" this change exists to eliminate could still reach every consumer: resolveAbiFunction reported such an entry as a canonical match and handed back an unencodable canonicalKey, getAbiFunctionKey stored that key, and computeSelector hashed it into a selector that is not the on-chain one -- which combineAbis then deduplicated on, dropping one of two distinct tuple overloads that both lacked components. A tuple without an array of components has no canonical form and now throws like a missing type does, so it is not a canonical match, produces no key and no selector, and stays out of deduplication. getFunctionSelector also treated an absent inputs key as "no selector", which let a zero-argument function that an explorer emitted without the key appear twice in a merged Diamond ABI. Absent inputs are an empty list. Tests cover both shapes and the merge behaviour they affect. --- lib/abi/combine-abis.ts | 6 ++-- lib/abi/utils.ts | 8 ++++- tests/unit/abi-combine-abis.test.ts | 37 ++++++++++++++++++++++ tests/unit/abi-utils.test.ts | 49 +++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 3 deletions(-) diff --git a/lib/abi/combine-abis.ts b/lib/abi/combine-abis.ts index df55fbf1f..14c4215df 100644 --- a/lib/abi/combine-abis.ts +++ b/lib/abi/combine-abis.ts @@ -24,11 +24,13 @@ function getFunctionSelector(abiItem: { components?: AbiItemComponent[]; }>; }): string | null { - if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { + if (abiItem.type !== "function" || !abiItem.name) { return null; } try { - return computeSelector(abiItem.name, abiItem.inputs); + // Explorer-fetched ABIs sometimes omit `inputs` on a zero-argument + // function instead of emitting `[]`; both mean the same selector. + return computeSelector(abiItem.name, abiItem.inputs ?? []); } catch { return null; } diff --git a/lib/abi/utils.ts b/lib/abi/utils.ts index 6ca9659f3..fb413532b 100644 --- a/lib/abi/utils.ts +++ b/lib/abi/utils.ts @@ -22,9 +22,15 @@ export function canonicalType(input: AbiInput): string { if (typeof input?.type !== "string") { throw new Error("ABI input is missing a type"); } - if (!(input.type.startsWith("tuple") && input.components)) { + if (!input.type.startsWith("tuple")) { return input.type; } + // A tuple without its components has no canonical form: returning the raw + // "tuple" here would hand back the one spelling ethers cannot encode, as a + // key, a selector and a canonical signature alike. Treat it as malformed. + if (!Array.isArray(input.components)) { + throw new Error("ABI tuple input is missing its components"); + } const inner = input.components.map((c) => canonicalType(c)).join(","); const suffix = input.type.slice("tuple".length); return `(${inner})${suffix}`; diff --git a/tests/unit/abi-combine-abis.test.ts b/tests/unit/abi-combine-abis.test.ts index 33287f16c..faa8f9eea 100644 --- a/tests/unit/abi-combine-abis.test.ts +++ b/tests/unit/abi-combine-abis.test.ts @@ -147,6 +147,43 @@ describe("combineAbis", () => { expect(functions(merged)).toHaveLength(2); }); + it("keeps two distinct tuple overloads that both lack components", () => { + // Without components no real selector can be computed for either entry. + // A selector hashed from the literal "tuple" would be the same for both + // and one would be dropped as a duplicate of the other. + const first = { + type: "function", + name: "f", + inputs: [{ name: "p", type: "tuple" }], + outputs: [], + }; + const second = { + type: "function", + name: "f", + inputs: [{ name: "q", type: "tuple" }], + outputs: [], + }; + const merged = combineAbis([JSON.stringify([first, second])]); + expect(functions(merged)).toHaveLength(2); + }); + + it("deduplicates a zero-argument function that omits inputs entirely", () => { + // Explorer-fetched ABIs sometimes leave the key out instead of emitting + // an empty array. Both spell the same selector. + const withoutInputs = { + type: "function", + name: "totalSupply", + stateMutability: "view", + outputs: [{ name: "", type: "uint256" }], + }; + const withEmptyInputs = { ...withoutInputs, inputs: [] }; + const merged = combineAbis([ + JSON.stringify([withoutInputs]), + JSON.stringify([withEmptyInputs]), + ]); + expect(functions(merged)).toHaveLength(1); + }); + it("skips a facet that does not parse without affecting the others", () => { const merged = combineAbis([ JSON.stringify([TRANSFER]), diff --git a/tests/unit/abi-utils.test.ts b/tests/unit/abi-utils.test.ts index 9e73e008c..2a7521a26 100644 --- a/tests/unit/abi-utils.test.ts +++ b/tests/unit/abi-utils.test.ts @@ -285,6 +285,20 @@ describe("canonicalType", () => { ).toBe("(uint256)[]"); }); + it('throws on a tuple with no components rather than returning the literal "tuple"', () => { + // The one malformed shape that used to produce a wrong answer silently: + // "tuple" is exactly the spelling ethers rejects, and it would otherwise + // flow into a stored key, a selector and a canonical signature. + expect(() => canonicalType({ type: "tuple" })).toThrow(); + expect(() => canonicalType({ type: "tuple[]" })).toThrow(); + expect(() => + canonicalType({ + type: "tuple", + components: { a: "uint256" } as unknown as AbiItem["inputs"], + }) + ).toThrow(); + }); + it("throws on an input with no type rather than fabricating a signature", () => { expect(() => canonicalType({ components: [] } as unknown as { type: string }) @@ -393,6 +407,41 @@ describe("resolveAbiFunction", () => { }); }); + it("does not report a tuple overload with no components as a canonical match", () => { + const abi: AbiItem[] = [ + { + type: "function", + name: "send", + stateMutability: "payable", + inputs: [ + { name: "p", type: "tuple" }, + { name: "r", type: "address" }, + ], + }, + { + type: "function", + name: "send", + stateMutability: "nonpayable", + inputs: [ + { name: "to", type: "address" }, + { name: "amount", type: "uint256" }, + ], + }, + ]; + // The legacy spelling still identifies the entry, so it resolves -- but + // the key it hands back must not be presented as a canonical signature, + // because no encodable one exists for an entry like this. + const result = resolveAbiFunction(abi, "send(tuple,address)"); + expect(result.status).toBe("found"); + if (result.status === "found") { + expect(result.entry.stateMutability).toBe("payable"); + expect(result.canonicalKey).toBe("send(tuple,address)"); + } + expect(resolveAbiFunction(abi, "send(address,uint256)").status).toBe( + "found" + ); + }); + it("tolerates components that are not an array", () => { const abi = [ { From a01b173df131eec1e6f0152ac577acd9a7031387 Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Wed, 9 Sep 2026 08:25:32 +0300 Subject: [PATCH 5/7] fix: #2330 address tuple key review regressions --- app/api/gas/estimate/route.ts | 36 ++- .../config/action-config-renderer.tsx | 41 ++- .../workflow/config/malformed-abi-notice.tsx | 3 +- lib/abi/function-inputs.ts | 2 +- lib/abi/function-key.ts | 5 +- lib/abi/utils.ts | 27 +- plugins/web3/steps/query-transactions-core.ts | 41 ++- plugins/web3/steps/read-contract-core.ts | 26 +- tests/fixtures/abi-tuple-shapes.ts | 56 ++++ tests/integration/query-transactions.test.ts | 2 +- tests/unit/abi-combine-abis.test.ts | 5 +- tests/unit/abi-function-inputs.test.ts | 18 ++ tests/unit/abi-function-key.test.ts | 25 ++ tests/unit/abi-function-select.test.tsx | 249 ++++++++++++++++++ tests/unit/abi-utils.test.ts | 39 ++- .../unit/contract-call-ambiguous-key.test.ts | 50 ++-- tests/unit/execute-simulate.test.ts | 61 +++++ tests/unit/gas-estimate-function-key.test.ts | 134 ++++++++++ tests/unit/query-transactions-core.test.ts | 79 ++++++ tests/unit/read-contract-core.test.ts | 26 ++ 20 files changed, 853 insertions(+), 72 deletions(-) create mode 100644 tests/fixtures/abi-tuple-shapes.ts create mode 100644 tests/unit/abi-function-select.test.tsx create mode 100644 tests/unit/gas-estimate-function-key.test.ts diff --git a/app/api/gas/estimate/route.ts b/app/api/gas/estimate/route.ts index cd8b2f3b2..52f20491d 100644 --- a/app/api/gas/estimate/route.ts +++ b/app/api/gas/estimate/route.ts @@ -1,5 +1,10 @@ import { ethers } from "ethers"; import { NextResponse } from "next/server"; +import { + type AbiItem, + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { apiError } from "@/lib/api-error"; import ERC20_ABI from "@/lib/contracts/abis/erc20.json"; import { MULTICALL3_ABI, MULTICALL3_ADDRESS } from "@/lib/contracts/multicall3"; @@ -153,6 +158,33 @@ function estimateWriteContract( return badRequest("Invalid ABI JSON"); } + if (!Array.isArray(parsedAbi)) { + return badRequest("ABI must be a JSON array"); + } + // ethers also accepts JSON arrays of human-readable fragments. Normalize + // those entries for the shared resolver without discarding malformed objects. + try { + parsedAbi = parsedAbi.map((entry) => + typeof entry === "string" + ? JSON.parse(ethers.Fragment.from(entry).format("json")) + : entry + ); + } catch { + return badRequest("Invalid ABI fragment"); + } + const resolution = resolveAbiFunction( + parsedAbi as AbiItem[], + config.abiFunction + ); + if (resolution.status === "ambiguous") { + return badRequest( + describeAmbiguousKey(config.abiFunction, resolution.candidates) + ); + } + if (resolution.status !== "found") { + return badRequest(`Function '${config.abiFunction}' not found in ABI`); + } + let args: unknown[] = []; if (config.functionArgs && config.functionArgs.trim() !== "") { try { @@ -173,9 +205,9 @@ function estimateWriteContract( // instead of the inherited method (which lacks `.estimateGas`). let fn: ethers.BaseContractMethod; try { - fn = contract.getFunction(config.abiFunction); + fn = contract.getFunction(resolution.canonicalKey); } catch { - return badRequest(`Function '${config.abiFunction}' not found in ABI`); + return badRequest(`Invalid ABI function '${config.abiFunction}'`); } return fn.estimateGas(...args, { from: walletAddress }); diff --git a/components/workflow/config/action-config-renderer.tsx b/components/workflow/config/action-config-renderer.tsx index d31a62aea..7840675f1 100644 --- a/components/workflow/config/action-config-renderer.tsx +++ b/components/workflow/config/action-config-renderer.tsx @@ -34,7 +34,11 @@ import { resolveFunctionInputs, } from "@/lib/abi/function-inputs"; import { parseAbiFunctionArgs } from "@/lib/abi/parse-args"; -import { canonicalType, computeSelector } from "@/lib/abi/utils"; +import { + canonicalType, + computeSelector, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { evaluateShowWhen } from "@/lib/workflow/editor/show-when"; import { parseAddressBookSelection } from "@/lib/address-book-selection"; import { toChecksumAddress } from "@/lib/address-utils"; @@ -472,18 +476,18 @@ export function AbiFunctionSelectField({ abiValue, functionFilter = "read", }: AbiFunctionSelectProps) { - // Parse ABI and extract functions - const functions = React.useMemo(() => { - if (!abiValue || abiValue.trim() === "") { + const abi = React.useMemo(() => { + try { + const parsed = JSON.parse(abiValue); + return Array.isArray(parsed) ? parsed : []; + } catch { return []; } + }, [abiValue]); + // Parse ABI and extract functions + const functions = React.useMemo(() => { try { - const abi = JSON.parse(abiValue); - if (!Array.isArray(abi)) { - return []; - } - // Filter functions based on functionFilter prop const filterFn = functionFilter === "write" @@ -506,8 +510,8 @@ export function AbiFunctionSelectField({ return filtered.map((func) => { const inputs = Array.isArray(func.inputs) ? func.inputs : []; - // A parameter with no type cannot be encoded, so the function is still - // listed (selecting it explains the problem) but gets no selector. + // Missing types or tuple components cannot be encoded. Keep the entry + // visible, but withhold its selector and show the malformed ABI notice. const complete = inputs.every(isValidAbiInput); const inputTypes = inputs.map((input: { type?: unknown }) => typeof input?.type === "string" ? input.type : "?" @@ -533,6 +537,7 @@ export function AbiFunctionSelectField({ : func.name; return { key, + entry: func, label: `${func.name}(${params})`, stateMutability: func.stateMutability || "nonpayable", selector, @@ -541,7 +546,17 @@ export function AbiFunctionSelectField({ } catch { return []; } - }, [abiValue, functionFilter]); + }, [abi, functionFilter]); + + // Resolve only for display. Opening a saved workflow must not mutate its + // config, and an ambiguous legacy key must not select the first overload. + // Resolve against the entire ABI: a hidden read/write overload can also + // make the stored key ambiguous. + const resolution = resolveAbiFunction(abi, value); + const displayValue = + resolution.status === "found" + ? functions.find((func) => func.entry === resolution.entry)?.key ?? "" + : ""; if (functions.length === 0) { return ( @@ -554,7 +569,7 @@ export function AbiFunctionSelectField({ } return ( - diff --git a/components/workflow/config/malformed-abi-notice.tsx b/components/workflow/config/malformed-abi-notice.tsx index 7034a0973..eddfb959e 100644 --- a/components/workflow/config/malformed-abi-notice.tsx +++ b/components/workflow/config/malformed-abi-notice.tsx @@ -9,7 +9,8 @@ export function MalformedAbiArgsNotice(): React.ReactNode { return (
The parameters for this function could not be read from the ABI. Check - that the ABI above is valid JSON and that every parameter has a type. + that the ABI above is valid JSON, every parameter has a type, and every + tuple has its components.
); } diff --git a/lib/abi/function-inputs.ts b/lib/abi/function-inputs.ts index 85456c18d..1c9efa437 100644 --- a/lib/abi/function-inputs.ts +++ b/lib/abi/function-inputs.ts @@ -26,7 +26,7 @@ export function isValidAbiInput(input: unknown): boolean { } if (components === undefined) { - return true; + return !type.startsWith("tuple"); } return Array.isArray(components) && components.every(isValidAbiInput); diff --git a/lib/abi/function-key.ts b/lib/abi/function-key.ts index b6d158d87..d2186e1ca 100644 --- a/lib/abi/function-key.ts +++ b/lib/abi/function-key.ts @@ -42,8 +42,9 @@ export function getAbiFunctionKey( } catch { // The ABI is user-supplied and this entry is malformed enough that no // canonical signature exists. Fall back to the key as it was passed in: - // the call still fails, but at the encoder, naming the fragment -- rather - // than here, silently, against whichever overload happened to be first. + // no canonical signature can be supplied. Callers must validate this key + // before entering RPC failover; the original spelling is not proof that + // an ethers fragment exists. return functionName; } } diff --git a/lib/abi/utils.ts b/lib/abi/utils.ts index fb413532b..489485679 100644 --- a/lib/abi/utils.ts +++ b/lib/abi/utils.ts @@ -101,7 +101,7 @@ function legacySignature(item: AbiItem): string | undefined { return `${item.name}(${inputs.map((i) => i.type).join(",")})`; } -/** Why a qualified key did not resolve to exactly one function. */ +/** Why a key did not resolve to exactly one function. */ export type AbiFunctionResolution = | { status: "found"; entry: AbiFunctionItem; canonicalKey: string } | { status: "not_found" } @@ -118,8 +118,8 @@ export type AbiFunctionResolution = * and then against the legacy raw spelling (`send(tuple,address)`), which older * saved workflows and external API callers still send. A legacy key resolves * when it identifies exactly one overload; it is reported as `ambiguous` only - * when two overloads share the same raw spelling, which is the one case where - * the choice was never recoverable from what was stored. + * when two overloads share the same raw spelling. Bare names are also ambiguous + * when they name distinct signatures; neither key records an overload choice. */ export function resolveAbiFunction( abi: AbiItem[], @@ -138,8 +138,13 @@ export function resolveAbiFunction( ); if (parenIdx === -1) { - // Plain names keep their long-standing first-match behaviour. - const entry = named[0]; + // Execution must not pick an arbitrary overload for a bare name. Repeated + // entries of the same signature still identify one function. + const distinct = distinctBySignature(named); + if (distinct.length > 1) { + return { status: "ambiguous", candidates: distinct }; + } + const entry = distinct[0]; return entry ? { status: "found", @@ -193,9 +198,9 @@ function distinctBySignature(entries: AbiFunctionItem[]): AbiFunctionItem[] { } /** - * Explain an ambiguous legacy key, naming the overloads to choose between. + * Explain an ambiguous key, naming the overloads to choose between. * - * The stored key is a raw-type signature that two overloads share, so which + * The stored key is a bare name or raw-type signature shared by overloads, so which * one the user picked was never recorded. Nothing can recover it -- the message * has to send them back to the function selector. */ @@ -227,6 +232,14 @@ export function findAbiFunction( abi: AbiItem[], key: string | undefined | null ): AbiFunctionItem | undefined { + // Preserve the UI helper's historical plain-name behaviour. Execution + // boundaries use resolveAbiFunction and report ambiguity explicitly. + if (key && !key.includes("(")) { + return abi.find( + (item): item is AbiFunctionItem => + item != null && item.type === "function" && item.name === key + ); + } const resolution = resolveAbiFunction(abi, key); return resolution.status === "found" ? resolution.entry : undefined; } diff --git a/plugins/web3/steps/query-transactions-core.ts b/plugins/web3/steps/query-transactions-core.ts index 26cf64044..9478cc335 100644 --- a/plugins/web3/steps/query-transactions-core.ts +++ b/plugins/web3/steps/query-transactions-core.ts @@ -3,6 +3,8 @@ import { getRpcPreferenceUserId } from "@/lib/workflow/executor/helpers"; import { eq } from "drizzle-orm"; import { ethers } from "ethers"; +import { describeAmbiguousKey, resolveAbiFunction } from "@/lib/abi/utils"; +import { ExecutionErrorType } from "@/lib/errors/execution-error-type"; import { db } from "@/lib/db"; import { explorerConfigs } from "@/lib/db/schema"; import { @@ -57,7 +59,11 @@ export type QueryTransactionsResult = contractAddressLink: string; error?: string; } - | (ReadDestinationFailure & { success: false; error: string }); + | (ReadDestinationFailure & { + success: false; + error: string; + errorClass?: ExecutionErrorType; + }); export type QueryTransactionsCoreInput = ReadFailOnErrorInput & { network: string; @@ -290,19 +296,36 @@ function validateInputs( return { success: false, error: abiResult.error }; } - const iface = new ethers.Interface(abiResult.parsed); - const functionFragment = iface.getFunction(abiFunction); - if (!functionFragment) { + const resolution = resolveAbiFunction(abiResult.parsed, abiFunction); + if (resolution.status === "ambiguous") { + return { + success: false, + error: describeAmbiguousKey(abiFunction, resolution.candidates), + }; + } + if (resolution.status !== "found") { return { success: false, error: `Function '${abiFunction}' not found in ABI`, }; } - return { - success: true, - data: { iface, functionFragment, chainId }, - }; + try { + const iface = new ethers.Interface(abiResult.parsed); + const functionFragment = iface.getFunction(resolution.canonicalKey); + if (!functionFragment) { + return { + success: false, + error: `Function '${abiFunction}' has no valid ABI fragment`, + }; + } + return { success: true, data: { iface, functionFragment, chainId } }; + } catch (error) { + return { + success: false, + error: `Invalid ABI function '${abiFunction}': ${getErrorMessage(error)}`, + }; + } } /** Data fields a softened query reports, so a soft failure never looks like an empty result set. */ @@ -329,7 +352,7 @@ async function queryTransactionsInner( ): Promise { const validation = validateInputs(input); if (!validation.success) { - return validation; + return { ...validation, errorClass: ExecutionErrorType.USER }; } const { iface, functionFragment, chainId } = validation.data; diff --git a/plugins/web3/steps/read-contract-core.ts b/plugins/web3/steps/read-contract-core.ts index 38c2410a1..8c32b6752 100644 --- a/plugins/web3/steps/read-contract-core.ts +++ b/plugins/web3/steps/read-contract-core.ts @@ -176,6 +176,28 @@ async function readContractInner( const functionAbi = resolution.entry; const abiFunctionKey = getAbiFunctionKey(parsedAbi, abiFunction, functionAbi); + // Fragment errors are deterministic user input errors, not provider failures. + // Validate before entering the adapter's RPC failover loop. + let contractInterface: ethers.Interface; + try { + contractInterface = new ethers.Interface(parsedAbi as ethers.InterfaceAbi); + if (!contractInterface.getFunction(abiFunctionKey)) { + throw new Error(`Function '${abiFunction}' has no valid ABI fragment`); + } + } catch (error) { + logUserError( + ErrorCategory.VALIDATION, + "[Read Contract] Invalid ABI function:", + error, + { plugin_name: "web3", action_name: "read-contract" } + ); + return { + success: false, + error: `Invalid ABI function '${abiFunction}': ${getErrorMessage(error)}`, + errorClass: ExecutionErrorType.USER, + }; + } + // Parse function arguments let args: unknown[] = []; if (functionArgs && functionArgs.trim() !== "") { @@ -267,10 +289,6 @@ async function readContractInner( }; } - const contractInterface = new ethers.Interface( - parsedAbi as ethers.InterfaceAbi - ); - const adapter = getChainAdapter(chainId); const isView = functionAbi.stateMutability === "view" || diff --git a/tests/fixtures/abi-tuple-shapes.ts b/tests/fixtures/abi-tuple-shapes.ts new file mode 100644 index 000000000..7f7e2803b --- /dev/null +++ b/tests/fixtures/abi-tuple-shapes.ts @@ -0,0 +1,56 @@ +import type { AbiItemComponent } from "@/lib/abi/utils"; + +const flat = [ + { name: "id", type: "uint32" }, + { name: "to", type: "bytes32" }, +]; +const nested = [ + { name: "inner", type: "tuple", components: flat }, + { name: "amount", type: "uint256" }, +]; + +export const TUPLE_SHAPES: { + label: string; + input: AbiItemComponent; + canonical: string; +}[] = [ + { + label: "flat tuple", + input: { name: "p", type: "tuple", components: flat }, + canonical: "(uint32,bytes32)", + }, + { + label: "nested tuple", + input: { name: "p", type: "tuple", components: nested }, + canonical: "((uint32,bytes32),uint256)", + }, + { + label: "tuple array", + input: { name: "p", type: "tuple[]", components: flat }, + canonical: "(uint32,bytes32)[]", + }, + { + label: "fixed tuple array", + input: { name: "p", type: "tuple[2]", components: flat }, + canonical: "(uint32,bytes32)[2]", + }, + { + label: "array inside tuple", + input: { + name: "p", + type: "tuple", + components: [{ name: "inner", type: "tuple[]", components: flat }], + }, + canonical: "((uint32,bytes32)[])", + }, + { + label: "multidimensional nested tuple array", + input: { name: "p", type: "tuple[][2]", components: nested }, + canonical: "((uint32,bytes32),uint256)[][2]", + }, + { + label: "empty tuple", + input: { name: "p", type: "tuple", components: [] }, + canonical: "()", + }, +]; diff --git a/tests/integration/query-transactions.test.ts b/tests/integration/query-transactions.test.ts index 44d711081..516a351a6 100644 --- a/tests/integration/query-transactions.test.ts +++ b/tests/integration/query-transactions.test.ts @@ -68,7 +68,7 @@ vi.mock("ethers", () => ({ inputs: Array<{ name: string }>; } | null { const fn = this.abi.find( - (e) => e.type === "function" && e.name === name + (e) => e.type === "function" && e.name === name.split("(")[0] ); if (!fn) { return null; diff --git a/tests/unit/abi-combine-abis.test.ts b/tests/unit/abi-combine-abis.test.ts index faa8f9eea..7d79838d4 100644 --- a/tests/unit/abi-combine-abis.test.ts +++ b/tests/unit/abi-combine-abis.test.ts @@ -147,8 +147,9 @@ describe("combineAbis", () => { expect(functions(merged)).toHaveLength(2); }); - it("keeps two distinct tuple overloads that both lack components", () => { - // Without components no real selector can be computed for either entry. + it("keeps two malformed tuple entries that both lack components", () => { + // Parameter names do not distinguish signatures. These are malformed + // entries, not proven distinct overloads; neither has a real selector. // A selector hashed from the literal "tuple" would be the same for both // and one would be dropped as a duplicate of the other. const first = { diff --git a/tests/unit/abi-function-inputs.test.ts b/tests/unit/abi-function-inputs.test.ts index c20d4223a..79c75d0a5 100644 --- a/tests/unit/abi-function-inputs.test.ts +++ b/tests/unit/abi-function-inputs.test.ts @@ -143,3 +143,21 @@ describe("isValidAbiInput", () => { expect(isValidAbiInput({ type: "tuple", components: {} })).toBe(false); }); }); + +it("rejects missing tuple components recursively but accepts an explicit empty tuple", () => { + for (const type of ["tuple", "tuple[]", "tuple[2]"]) { + expect(isValidAbiInput({ type })).toBe(false); + expect(isValidAbiInput({ type, components: [] })).toBe(true); + expect(isValidAbiInput({ type, components: [{ type: "tuple" }] })).toBe( + false + ); + expect( + resolveFunctionInputs( + JSON.stringify([ + { type: "function", name: "f", inputs: [{ name: "p", type }] }, + ]), + "f" + ).malformed + ).toBe(true); + } +}); diff --git a/tests/unit/abi-function-key.test.ts b/tests/unit/abi-function-key.test.ts index 401de1d4e..9698eb144 100644 --- a/tests/unit/abi-function-key.test.ts +++ b/tests/unit/abi-function-key.test.ts @@ -1,5 +1,6 @@ import { ethers } from "ethers"; import { describe, expect, it, vi } from "vitest"; +import { TUPLE_SHAPES } from "../fixtures/abi-tuple-shapes"; vi.mock("server-only", () => ({})); @@ -127,3 +128,27 @@ describe("resolve then encode", () => { ).not.toThrow(); }); }); + +describe("tuple shape keys against ethers", () => { + it.each(TUPLE_SHAPES)( + "resolves canonical and legacy $label keys", + ({ input, canonical }) => { + const abi: AbiItem[] = [ + { name: "f", type: "function", inputs: [input] }, + { + name: "f", + type: "function", + inputs: [{ name: "n", type: "uint256" }], + }, + ]; + const iface = new ethers.Interface(abi as ethers.InterfaceAbi); + for (const key of [`f(${canonical})`, `f(${input.type})`]) { + const canonicalKey = keyFor(abi, key); + expect(canonicalKey).toBe(`f(${canonical})`); + expect(iface.getFunction(canonicalKey)?.format("sighash")).toBe( + canonicalKey + ); + } + } + ); +}); diff --git a/tests/unit/abi-function-select.test.tsx b/tests/unit/abi-function-select.test.tsx new file mode 100644 index 000000000..5a4bd16a1 --- /dev/null +++ b/tests/unit/abi-function-select.test.tsx @@ -0,0 +1,249 @@ +// @vitest-environment jsdom +import { createStore } from "jotai"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/logging", () => ({ + ErrorCategory: { + VALIDATION: "validation", + WORKFLOW_ENGINE: "workflow_engine", + UNKNOWN: "unknown", + }, + logSystemError: vi.fn(), + logUserError: vi.fn(), +})); +vi.mock("@/lib/workflow/editor/auto-layout", () => ({ + computeAutoLayout: vi.fn(), +})); +vi.mock("@/lib/workflow/editor/template-helpers", () => ({ + buildExecutionLogsMap: vi.fn(() => ({})), +})); +vi.mock("server-only", () => ({})); +vi.mock("@/lib/api-client", () => ({ api: { workflow: { update: vi.fn() } } })); +vi.mock("@/components/address-book/save-address-bookmark", () => ({ + SaveAddressBookmark: () => null, +})); +vi.mock("@/components/ui/template-badge-input", () => ({ + TemplateBadgeInput: ({ value, id }: { value: string; id: string }) => ( + + ), +})); +vi.mock("@/components/ui/template-badge-textarea", () => ({ + TemplateBadgeTextarea: () => null, +})); +vi.mock("@/components/workflow/config/schema-builder", () => ({ + SchemaBuilder: () => null, +})); + +import { ActionConfigRenderer } from "@/components/workflow/config/action-config-renderer"; +import { api } from "@/lib/api-client"; +import { + cancelPendingAutosave, + currentWorkflowIdAtom, + nodesAtom, + updateNodeDataAtom, +} from "@/lib/workflow/store"; +import type { ActionConfigField } from "@/plugins/registry"; + +const fields: ActionConfigField[] = [ + { + key: "abiFunction", + label: "Function", + type: "abi-function-select", + functionFilter: "write", + }, + { key: "functionArgs", label: "Arguments", type: "abi-function-args" }, +]; +const tuple = { + type: "function", + name: "send", + stateMutability: "nonpayable", + inputs: [ + { + name: "params", + type: "tuple", + components: [ + { name: "id", type: "uint32" }, + { name: "to", type: "bytes32" }, + ], + }, + { name: "recipient", type: "address" }, + ], +}; +const scalar = { + type: "function", + name: "send", + stateMutability: "nonpayable", + inputs: [{ name: "amount", type: "uint256" }], +}; +const args = JSON.stringify([ + { id: "7", to: `0x${"11".repeat(32)}` }, + `0x${"22".repeat(20)}`, +]); +let container: HTMLDivElement; +let root: Root; +const onChange = vi.fn(); +let store: ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); + store = createStore(); + store.set(currentWorkflowIdAtom, "saved-legacy-workflow"); + Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + container = document.createElement("div"); + document.body.append(container); + root = createRoot(container); +}); +afterEach(async () => { + await act(() => root.unmount()); + container.remove(); + cancelPendingAutosave(); + vi.useRealTimers(); +}); +async function render( + abi: unknown[], + abiFunction: string, + functionArgs = "[]" +) { + const config = { abi: JSON.stringify(abi), abiFunction, functionArgs }; + store.set(nodesAtom, [ + { + id: "write", + type: "action", + position: { x: 0, y: 0 }, + data: { type: "action", label: "Saved write", config }, + }, + ]); + onChange.mockImplementation((key: string, value: unknown) => { + store.set(updateNodeDataAtom, { + id: "write", + data: { config: { ...config, [key]: value } }, + }); + }); + await act(async () => + root.render( + + ) + ); + return config; +} + +describe("saved ABI function selection", () => { + it("B1 keeps healthy functions beside a components-less tuple without inventing its selector", async () => { + const healthy = Array.from({ length: 40 }, (_, index) => ({ + ...scalar, + name: `good${index}`, + })); + await render( + [ + ...healthy, + { ...tuple, name: "broken", inputs: [{ name: "p", type: "tuple" }] }, + ], + "good0" + ); + expect(container.textContent).not.toContain("No functions found in ABI"); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "good0" + ); + for (const entry of healthy) { + await render( + [ + ...healthy, + { ...tuple, name: "broken", inputs: [{ name: "p", type: "tuple" }] }, + ], + entry.name + ); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + `${entry.name}(` + ); + expect(container.querySelector("[role=combobox] code")).not.toBeNull(); + } + await render( + [ + ...healthy, + { ...tuple, name: "broken", inputs: [{ name: "p", type: "tuple" }] }, + ], + "broken" + ); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "broken" + ); + expect(container.querySelector("[role=combobox] code")).toBeNull(); + }); + + it("B3 displays an unambiguous saved legacy key with populated arguments and no writes", async () => { + vi.useFakeTimers(); + const config = await render([tuple, scalar], "send(tuple,address)", args); + await act(() => vi.advanceTimersByTimeAsync(3500)); + expect(store.get(nodesAtom)[0].data.config).toEqual(config); + expect( + container.querySelector("#functionArgs-0-id")?.getAttribute("value") + ).toBe("7"); + expect(onChange).not.toHaveBeenCalled(); + expect(api.workflow.update).not.toHaveBeenCalled(); + expect(config.abiFunction).toBe("send(tuple,address)"); + // Positive control: the callback is wired to the real autosave atom. + // A hidden normalization write would therefore be observable here. + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "send(tuple params, address recipient)" + ); + }); + + it("observes a workflow save when the same update callback is invoked", async () => { + vi.useFakeTimers(); + vi.mocked(api.workflow.update).mockResolvedValue({} as never); + await render([tuple, scalar], "send(tuple,address)", args); + onChange("abiFunction", "send((uint32,bytes32),address)"); + await act(() => vi.advanceTimersByTimeAsync(3500)); + expect(api.workflow.update).toHaveBeenCalledTimes(1); + }); + + it("leaves an ambiguous legacy key unselected and does not write", async () => { + const second = { + ...tuple, + inputs: [ + { ...tuple.inputs[0], components: [{ name: "other", type: "bytes" }] }, + tuple.inputs[1], + ], + }; + await render([tuple, second], "send(tuple,address)", args); + expect( + container.querySelector("[role=combobox]")?.textContent + ).not.toContain("send("); + expect(onChange).not.toHaveBeenCalled(); + expect(api.workflow.update).not.toHaveBeenCalled(); + }); + + it("does not resolve ambiguity by hiding the read-only overload", async () => { + const readOnly = { + ...tuple, + stateMutability: "view", + inputs: [ + { ...tuple.inputs[0], components: [{ name: "other", type: "bytes" }] }, + tuple.inputs[1], + ], + }; + await render([tuple, readOnly], "send(tuple,address)", args); + expect( + container.querySelector("[role=combobox]")?.textContent + ).not.toContain("send("); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("retains canonical and plain-name selections", async () => { + await render([tuple, scalar], "send((uint32,bytes32),address)", args); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "send(tuple params" + ); + await render([tuple], "send", args); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "send(tuple params" + ); + expect(onChange).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/abi-utils.test.ts b/tests/unit/abi-utils.test.ts index 2a7521a26..3e4a1465e 100644 --- a/tests/unit/abi-utils.test.ts +++ b/tests/unit/abi-utils.test.ts @@ -1,5 +1,5 @@ +import { ethers } from "ethers"; import { describe, expect, it } from "vitest"; - import { type AbiItem, canonicalType, @@ -8,6 +8,7 @@ import { findAbiFunction, resolveAbiFunction, } from "@/lib/abi/utils"; +import { TUPLE_SHAPES } from "../fixtures/abi-tuple-shapes"; const SELECTOR_PATTERN = /^0x[\da-f]{8}$/; @@ -367,12 +368,9 @@ describe("resolveAbiFunction", () => { } }); - it("keeps first-match behaviour for a plain name", () => { - const result = resolveAbiFunction(OVERLOADED_ABI, "send"); - expect(result).toMatchObject({ status: "found" }); - if (result.status === "found") { - expect(result.entry.stateMutability).toBe("payable"); - } + it("reports a bare overloaded name as ambiguous while the UI helper stays total", () => { + expect(resolveAbiFunction(OVERLOADED_ABI, "send").status).toBe("ambiguous"); + expect(findAbiFunction(OVERLOADED_ABI, "send")).toBe(OVERLOADED_ABI[0]); }); it("reports an unknown key as not found", () => { @@ -552,3 +550,30 @@ describe("describeAmbiguousKey", () => { expect(message).toContain("permit(address,(address,uint256),bytes)"); }); }); + +describe("canonical tuple shapes against ethers", () => { + it.each(TUPLE_SHAPES)("$label", ({ input, canonical }) => { + expect(canonicalType(input)).toBe(canonical); + const fragment = ethers.FunctionFragment.from({ + type: "function", + name: "f", + inputs: [input], + }); + expect(fragment.format("sighash")).toBe(`f(${canonical})`); + expect(computeSelector("f", [input])).toBe(fragment.selector); + }); + + it("combines a tuple and a scalar in one selector", () => { + const inputs = [ + TUPLE_SHAPES[0].input, + { name: "recipient", type: "address" }, + ]; + const fragment = ethers.FunctionFragment.from({ + type: "function", + name: "send", + inputs, + }); + expect(fragment.format("sighash")).toBe("send((uint32,bytes32),address)"); + expect(computeSelector("send", inputs)).toBe(fragment.selector); + }); +}); diff --git a/tests/unit/contract-call-ambiguous-key.test.ts b/tests/unit/contract-call-ambiguous-key.test.ts index 1f1e426bd..bfec63738 100644 --- a/tests/unit/contract-call-ambiguous-key.test.ts +++ b/tests/unit/contract-call-ambiguous-key.test.ts @@ -142,29 +142,33 @@ beforeEach(() => { }); describe("contract-call with a legacy key two overloads share", () => { - it("returns 400 naming the signatures to choose from", async () => { - const response = await (POST as (req: Request) => Promise)( - post("permit(address,tuple,bytes)") - ); - const body = (await response.json()) as { error: string; field?: string }; - - expect(response.status).toBe(400); - expect(body.field).toBe("functionName"); - expect(body.error).toContain("matches 2 overloads"); - expect(body.error).toContain("permit(address,(address,uint160),bytes)"); - expect(body.error).toContain("permit(address,(address,uint256),bytes)"); - expect(body.error).not.toContain("not found in ABI"); - }); - - it("never reaches the read or write path", async () => { - await (POST as (req: Request) => Promise)( - post("permit(address,tuple,bytes)") - ); - - expect(mockReadContractCore).not.toHaveBeenCalled(); - expect(mockWriteContractCore).not.toHaveBeenCalled(); - expect(mockBeginIdempotentFromRequest).not.toHaveBeenCalled(); - }); + it.each(["permit", "permit(address,tuple,bytes)"])( + "returns 400 naming the signatures for %s", + async (key) => { + const response = await (POST as (req: Request) => Promise)( + post(key) + ); + const body = (await response.json()) as { error: string; field?: string }; + + expect(response.status).toBe(400); + expect(body.field).toBe("functionName"); + expect(body.error).toContain("matches 2 overloads"); + expect(body.error).toContain("permit(address,(address,uint160),bytes)"); + expect(body.error).toContain("permit(address,(address,uint256),bytes)"); + expect(body.error).not.toContain("not found in ABI"); + } + ); + + it.each(["permit", "permit(address,tuple,bytes)"])( + "never reaches execution for %s", + async (key) => { + await (POST as (req: Request) => Promise)(post(key)); + + expect(mockReadContractCore).not.toHaveBeenCalled(); + expect(mockWriteContractCore).not.toHaveBeenCalled(); + expect(mockBeginIdempotentFromRequest).not.toHaveBeenCalled(); + } + ); it("still reports a genuinely missing function as not found", async () => { const response = await (POST as (req: Request) => Promise)( diff --git a/tests/unit/execute-simulate.test.ts b/tests/unit/execute-simulate.test.ts index eef86bdb2..8f911dda8 100644 --- a/tests/unit/execute-simulate.test.ts +++ b/tests/unit/execute-simulate.test.ts @@ -156,6 +156,67 @@ function resetSpies(): void { } describe("simulateContractCall", () => { + it("B2 refuses a bare overloaded name before RPC", async () => { + resetSpies(); + executeWithFailover.mockResolvedValue([BigInt(45_000), "0x"]); + const abi = [ + { + type: "function", + name: "swap", + inputs: [{ name: "amount", type: "uint256" }], + outputs: [], + }, + { + type: "function", + name: "swap", + inputs: [ + { + name: "params", + type: "tuple", + components: [ + { name: "token", type: "address" }, + { name: "amount", type: "uint256" }, + ], + }, + ], + outputs: [], + }, + ]; + const result = await simulateContractCall({ + organizationId: "org_test", + network: "1", + contractAddress: CONTRACT_ADDRESS, + abi: JSON.stringify(abi), + functionName: "swap", + functionArgs: "[1]", + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("matches 2 overloads"); + } + expect(getRpcProvider).not.toHaveBeenCalled(); + expect(executeWithFailover).not.toHaveBeenCalled(); + }); + + it("B2 accepts duplicate entries of the same canonical signature", async () => { + resetSpies(); + executeWithFailover.mockResolvedValueOnce([BigInt(45_000), "0x"]); + const entry = JSON.parse(WRITE_ABI)[0]; + const result = await simulateContractCall({ + organizationId: "org_test", + network: "1", + contractAddress: CONTRACT_ADDRESS, + abi: JSON.stringify([ + entry, + { ...entry, inputs: [{ name: "renamed", type: "uint256" }] }, + ]), + functionName: "setValue", + functionArgs: "[1]", + }); + expect(result.success).toBe(true); + expect(executeWithFailover).toHaveBeenCalledTimes(1); + }); + // The contract-call route calls this function directly rather than going // through simulateTokenTransfer, so gating only the latter left // POST /api/execute/contract-call?simulate=true reporting a clean dry run diff --git a/tests/unit/gas-estimate-function-key.test.ts b/tests/unit/gas-estimate-function-key.test.ts new file mode 100644 index 000000000..01a15d5bd --- /dev/null +++ b/tests/unit/gas-estimate-function-key.test.ts @@ -0,0 +1,134 @@ +import { ethers } from "ethers"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("server-only", () => ({})); +vi.mock("@/lib/middleware/auth-helpers", () => ({ + resolveOrganizationId: vi.fn().mockResolvedValue({ + organizationId: "org-1", + authMethod: "oauth", + apiKeyId: null, + scope: "mcp:read", + }), +})); +vi.mock("@/lib/middleware/require-scope", () => ({ + requireScope: vi.fn().mockReturnValue(null), +})); +vi.mock("@/lib/web3/wallet-helpers", () => ({ + getOrganizationWalletAddress: vi + .fn() + .mockResolvedValue("0x1111111111111111111111111111111111111111"), +})); +vi.mock("@/lib/safe/signer-resolver", () => ({ + resolveSignerForNode: vi.fn(), + SIGNER_MODE: { EOA: "eoa" }, +})); +vi.mock("@/plugins/web3/steps/batch-write-contract-core", () => ({ + buildCallsWithMeta: vi.fn(), +})); +const estimateGas = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/rpc/provider-factory", () => ({ + getRpcProvider: vi.fn().mockResolvedValue({ + executeWithFailover: (fn: (provider: unknown) => unknown) => + fn({ estimateGas }), + }), +})); + +import { POST } from "@/app/api/gas/estimate/route"; + +const ADDRESS = "0x2222222222222222222222222222222222222222"; +const tuple = { + name: "send", + type: "function", + inputs: [ + { name: "p", type: "tuple", components: [{ name: "n", type: "uint256" }] }, + ], +}; +const scalar = { + name: "send", + type: "function", + inputs: [{ name: "n", type: "uint256" }], +}; + +function estimate( + abi: unknown[], + abiFunction: string, + args: unknown[] = [[7]] +) { + return POST( + new Request("http://localhost/api/gas/estimate", { + method: "POST", + body: JSON.stringify({ + chainId: 1, + actionSlug: "write-contract", + config: { + contractAddress: ADDRESS, + abi: JSON.stringify(abi), + abiFunction, + functionArgs: JSON.stringify(args), + }, + }), + }) + ); +} + +beforeEach(() => { + vi.clearAllMocks(); + estimateGas.mockResolvedValue(BigInt(45_000)); +}); + +describe("gas estimate function keys", () => { + it.each(["send(tuple)", "send((uint256))"])( + "N2 estimates %s using the actual ethers encoder", + async (key) => { + const response = await estimate([tuple, scalar], key); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ estimatedGas: "45000" }); + expect(estimateGas).toHaveBeenCalledWith( + expect.objectContaining({ + data: new ethers.Interface([tuple, scalar]).encodeFunctionData( + "send((uint256))", + [[7]] + ), + }) + ); + } + ); + it("preserves human-readable ABI entries accepted by ethers", async () => { + const response = await estimate( + ["function send((uint256 n) p)", scalar], + "send(tuple)" + ); + expect(response.status).toBe(200); + expect(estimateGas).toHaveBeenCalledTimes(1); + }); + + it("rejects ambiguous bare names without estimating an arbitrary overload", async () => { + const response = await estimate([tuple, scalar], "send", [7]); + expect(response.status).toBe(400); + expect(await response.json()).toMatchObject({ + error: expect.stringContaining("matches 2 overloads"), + }); + expect(estimateGas).not.toHaveBeenCalled(); + }); + it("does not confuse duplicate canonical entries with overloads", async () => { + const response = await estimate([tuple, tuple], "send"); + expect(response.status).toBe(200); + expect(estimateGas).toHaveBeenCalledTimes(1); + }); + it("distinguishes a malformed fragment from an absent function", async () => { + const malformed = await estimate( + [{ ...tuple, inputs: [{ name: "p", type: "tuple" }] }], + "send(tuple)" + ); + expect(malformed.status).toBe(400); + expect(await malformed.json()).toMatchObject({ + error: expect.stringContaining("Invalid ABI function"), + }); + const absent = await estimate([tuple], "missing"); + expect(absent.status).toBe(400); + expect(await absent.json()).toMatchObject({ + error: expect.stringContaining("not found in ABI"), + }); + expect(estimateGas).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/query-transactions-core.test.ts b/tests/unit/query-transactions-core.test.ts index 0c85425e3..4234b9875 100644 --- a/tests/unit/query-transactions-core.test.ts +++ b/tests/unit/query-transactions-core.test.ts @@ -1,3 +1,4 @@ +import { ethers } from "ethers"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("server-only", () => ({})); @@ -129,6 +130,84 @@ describe("queryTransactionsCore", () => { }); describe("validation", () => { + const tupleEntry = { + type: "function", + name: "send", + inputs: [ + { + name: "p", + type: "tuple", + components: [{ name: "n", type: "uint256" }], + }, + ], + }; + it("N1 decodes a saved legacy tuple key", async () => { + mockFindFirst.mockResolvedValue({ + explorerApiUrl: "https://api.etherscan.io/api", + explorerUrl: "https://etherscan.io", + explorerApiType: "etherscan", + chainId: 1, + chainType: "evm", + explorerAddressPath: "/address/{address}", + explorerTxPath: "/tx/{hash}", + }); + const iface = new ethers.Interface([tupleEntry]); + mockFetch.mockResolvedValue({ + ok: true, + json: async () => + makeEtherscanTxResponse([ + { hash: "0xtest", input: iface.encodeFunctionData("send", [[7]]) }, + ]), + }); + const result = await queryTransactionsCore({ + ...BASE_INPUT, + abi: JSON.stringify([tupleEntry]), + abiFunction: "send(tuple)", + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.matchCount).toBe(1); + expect(result.transactions?.[0].functionSignature).toBe( + "send((uint256))" + ); + } + }); + + it("N1 returns USER errors for malformed and ambiguous keys without RPC", async () => { + for (const [abi, key] of [ + [ + [{ ...tupleEntry, inputs: [{ name: "p", type: "tuple" }] }], + "send(tuple)", + ], + [ + [ + tupleEntry, + { + ...tupleEntry, + inputs: [ + { + name: "p", + type: "tuple", + components: [{ name: "a", type: "address" }], + }, + ], + }, + ], + "send(tuple)", + ], + [[tupleEntry], "send(("], + ] as const) { + const result = await queryTransactionsCore({ + ...BASE_INPUT, + abi: JSON.stringify(abi), + abiFunction: key, + }); + expect(result).toMatchObject({ success: false, errorClass: "user" }); + } + expect(mockExecuteWithFailover).not.toHaveBeenCalled(); + expect(mockFetch).not.toHaveBeenCalled(); + }); + it("returns error for invalid contract address", async () => { const result = await queryTransactionsCore({ ...BASE_INPUT, diff --git a/tests/unit/read-contract-core.test.ts b/tests/unit/read-contract-core.test.ts index edae3eb4d..fa08b94df 100644 --- a/tests/unit/read-contract-core.test.ts +++ b/tests/unit/read-contract-core.test.ts @@ -589,3 +589,29 @@ describe("read-contract-core - failOnError", () => { expect(result.errorClass).toBe("system"); }); }); + +describe("ABI fragment validation before RPC failover", () => { + it("N3 classifies a components-less legacy tuple as USER before provider creation", async () => { + vi.clearAllMocks(); + const result = await readContractCore({ + contractAddress: VALID_ADDRESS, + network: "ethereum", + abi: JSON.stringify([ + { + type: "function", + name: "broken", + inputs: [{ name: "p", type: "tuple" }], + }, + { type: "function", name: "broken", inputs: [] }, + ]), + abiFunction: "broken(tuple)", + _context: { organizationId: "org-test" }, + }); + expect(result).toMatchObject({ success: false, errorClass: "user" }); + if (!result.success) { + expect(result.error).toContain("Invalid ABI function"); + } + expect(mockGetRpcProvider).not.toHaveBeenCalled(); + expect(mockContractFunction).not.toHaveBeenCalled(); + }); +}); From 63b1c6e88e2e03980960844ca1e089d32c03b9a3 Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Wed, 9 Sep 2026 15:20:43 +0300 Subject: [PATCH 6/7] fix: #2330 qualify keys for hidden overloads and resolve saved keys in MCP calldata The function dropdown counted overloads only within the read or write filter, so a name overloaded across both emitted a bare key that every lookup resolves against the full ABI and reports as ambiguous. Count across the whole ABI so the stored key is qualified whenever it has to be. The MCP calldata path encoded with the raw stored key, so a saved legacy tuple key executed through the workflow engine but failed here. Resolve the key the same way the engine does and encode the canonical signature. --- .../config/action-config-renderer.tsx | 10 +- lib/mcp/calldata.ts | 31 ++++- tests/unit/abi-function-select.test.tsx | 59 ++++++++ tests/unit/mcp-calldata-function-key.test.ts | 127 ++++++++++++++++++ tests/unit/mcp-calldata.test.ts | 18 +-- 5 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 tests/unit/mcp-calldata-function-key.test.ts diff --git a/components/workflow/config/action-config-renderer.tsx b/components/workflow/config/action-config-renderer.tsx index 7840675f1..6388a3dbe 100644 --- a/components/workflow/config/action-config-renderer.tsx +++ b/components/workflow/config/action-config-renderer.tsx @@ -502,10 +502,14 @@ export function AbiFunctionSelectField({ const filtered = abi.filter(filterFn); - // Count how many times each function name appears to detect overloads + // Count overloads across the whole ABI, not the filtered list: a read + // overload hidden here still shares the name, and a bare key would be + // ambiguous to every lookup that resolves against the full ABI. const nameCounts = new Map(); - for (const func of filtered) { - nameCounts.set(func.name, (nameCounts.get(func.name) ?? 0) + 1); + for (const func of abi) { + if (func?.type === "function") { + nameCounts.set(func.name, (nameCounts.get(func.name) ?? 0) + 1); + } } return filtered.map((func) => { diff --git a/lib/mcp/calldata.ts b/lib/mcp/calldata.ts index 0bbc83170..14f5f655e 100644 --- a/lib/mcp/calldata.ts +++ b/lib/mcp/calldata.ts @@ -1,4 +1,9 @@ import { ethers } from "ethers"; +import { + type AbiItem, + describeAmbiguousKey, + resolveAbiFunction, +} from "@/lib/abi/utils"; import { MULTICALL3_ABI, MULTICALL3_ADDRESS } from "@/lib/contracts/multicall3"; import { BATCH_WRITE_CONTRACT_ACTION_TYPE, @@ -159,12 +164,32 @@ function generateSingleWriteCalldata( }; } - let parsedAbi: unknown[]; + let parsedAbi: unknown; try { - parsedAbi = JSON.parse(abi) as unknown[]; + parsedAbi = JSON.parse(abi); } catch { return { success: false, error: "Invalid ABI JSON in workflow node" }; } + if (!Array.isArray(parsedAbi)) { + return { success: false, error: "Invalid ABI JSON in workflow node" }; + } + + // The stored key may be a legacy raw spelling such as `send(tuple,address)`, + // which the workflow engine accepts but ethers cannot encode. Resolve it the + // same way the engine does and encode with the canonical signature. + const resolution = resolveAbiFunction(parsedAbi as AbiItem[], abiFunction); + if (resolution.status === "ambiguous") { + return { + success: false, + error: describeAmbiguousKey(abiFunction, resolution.candidates), + }; + } + if (resolution.status !== "found") { + return { + success: false, + error: `Function '${abiFunction}' not found in ABI`, + }; + } let resolvedArgs: unknown[] = []; if (typeof functionArgs === "string" && functionArgs) { @@ -191,7 +216,7 @@ function generateSingleWriteCalldata( let data: string; try { const iface = new ethers.Interface(parsedAbi as ethers.InterfaceAbi); - data = iface.encodeFunctionData(abiFunction, resolvedArgs); + data = iface.encodeFunctionData(resolution.canonicalKey, resolvedArgs); } catch (err) { return { success: false, diff --git a/tests/unit/abi-function-select.test.tsx b/tests/unit/abi-function-select.test.tsx index 5a4bd16a1..38144fb42 100644 --- a/tests/unit/abi-function-select.test.tsx +++ b/tests/unit/abi-function-select.test.tsx @@ -91,6 +91,13 @@ beforeEach(() => { store = createStore(); store.set(currentWorkflowIdAtom, "saved-legacy-workflow"); Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + // jsdom has no pointer capture or scrolling; Radix Select needs both to open. + Object.assign(HTMLElement.prototype, { + hasPointerCapture: () => false, + setPointerCapture: () => undefined, + releasePointerCapture: () => undefined, + scrollIntoView: () => undefined, + }); container = document.createElement("div"); document.body.append(container); root = createRoot(container); @@ -133,6 +140,35 @@ async function render( return config; } +async function choose(optionText: string) { + const trigger = container.querySelector("[role=combobox]") as HTMLElement; + await act(async () => { + trigger.dispatchEvent( + new PointerEvent("pointerdown", { + bubbles: true, + button: 0, + pointerType: "mouse", + }) + ); + }); + const option = Array.from(document.querySelectorAll("[role=option]")).find( + (o) => o.textContent?.includes(optionText) + ) as HTMLElement | undefined; + if (!option) { + throw new Error(`No option containing ${optionText}`); + } + await act(async () => { + option.dispatchEvent( + new PointerEvent("pointermove", { bubbles: true, pointerType: "mouse" }) + ); + }); + await act(async () => { + option.dispatchEvent( + new PointerEvent("pointerup", { bubbles: true, pointerType: "mouse" }) + ); + }); +} + describe("saved ABI function selection", () => { it("B1 keeps healthy functions beside a components-less tuple without inventing its selector", async () => { const healthy = Array.from({ length: 40 }, (_, index) => ({ @@ -235,6 +271,29 @@ describe("saved ABI function selection", () => { expect(onChange).not.toHaveBeenCalled(); }); + it("qualifies the key when the only other overload is hidden by the filter", async () => { + // One `send` is view, the other is a write. The write dropdown lists one + // `send`, but every lookup resolves against the whole ABI, where the name + // is overloaded: a bare key would be ambiguous the moment it was saved. + const readOnly = { ...scalar, stateMutability: "view" }; + await render([readOnly, tuple], ""); + await choose("send(tuple params"); + expect(onChange).toHaveBeenCalledWith( + "abiFunction", + "send((uint32,bytes32),address)" + ); + onChange.mockClear(); + + await render([readOnly, tuple], "send((uint32,bytes32),address)", args); + expect(container.querySelector("[role=combobox]")?.textContent).toContain( + "send(tuple params" + ); + expect( + container.querySelector("#functionArgs-0-id")?.getAttribute("value") + ).toBe("7"); + expect(onChange).not.toHaveBeenCalled(); + }); + it("retains canonical and plain-name selections", async () => { await render([tuple, scalar], "send((uint32,bytes32),address)", args); expect(container.querySelector("[role=combobox]")?.textContent).toContain( diff --git a/tests/unit/mcp-calldata-function-key.test.ts b/tests/unit/mcp-calldata-function-key.test.ts new file mode 100644 index 000000000..ea7c655d8 --- /dev/null +++ b/tests/unit/mcp-calldata-function-key.test.ts @@ -0,0 +1,127 @@ +import { ethers } from "ethers"; +import { describe, expect, it, vi } from "vitest"; + +// Mocked at the module boundary for the same reason as mcp-calldata.test.ts: +// the real module pulls in db and wallet helpers this encoding test does not +// need. ethers itself is real here, because the point is what it encodes. +vi.mock("@/plugins/web3/steps/batch-write-contract-core", () => ({ + buildCallsWithMeta: vi.fn(), +})); + +import { generateCalldataForWorkflow } from "@/lib/mcp/calldata"; + +const CONTRACT = "0x1111111111111111111111111111111111111111"; +const tuple = { + name: "send", + type: "function", + stateMutability: "nonpayable", + inputs: [ + { + name: "params", + type: "tuple", + components: [ + { name: "id", type: "uint32" }, + { name: "to", type: "bytes32" }, + ], + }, + { name: "recipient", type: "address" }, + ], + outputs: [], +}; +const scalar = { + name: "send", + type: "function", + stateMutability: "nonpayable", + inputs: [{ name: "amount", type: "uint256" }], + outputs: [], +}; +const tupleArgs = [[7, `0x${"11".repeat(32)}`], `0x${"22".repeat(20)}`]; + +function nodes(abi: unknown[], abiFunction: string, args: unknown[]) { + return [ + { + id: "write-1", + data: { + actionType: "web3/write-contract", + config: { + contractAddress: CONTRACT, + network: "base", + abi: JSON.stringify(abi), + abiFunction, + functionArgs: JSON.stringify(args), + ethValue: "", + }, + }, + }, + ]; +} + +describe("generateCalldataForWorkflow function keys", () => { + const expected = new ethers.Interface([tuple, scalar]).encodeFunctionData( + "send((uint32,bytes32),address)", + tupleArgs + ); + + it.each(["send(tuple,address)", "send((uint32,bytes32),address)"])( + "encodes the saved key %s the way the workflow engine resolves it", + (key) => { + const result = generateCalldataForWorkflow( + nodes([tuple, scalar], key, tupleArgs), + {} + ); + expect(result).toMatchObject({ success: true, to: CONTRACT }); + if (result.success) { + expect(result.data).toBe(expected); + } + } + ); + + it("still accepts a bare name that identifies one function", () => { + const result = generateCalldataForWorkflow( + nodes([tuple], "send", tupleArgs), + {} + ); + expect(result).toMatchObject({ success: true, data: expected }); + }); + + it("names the overloads instead of encoding an arbitrary one for a bare name", () => { + const result = generateCalldataForWorkflow( + nodes([tuple, scalar], "send", [1]), + {} + ); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("matches 2 overloads"); + expect(result.error).toContain("send((uint32,bytes32),address)"); + expect(result.error).toContain("send(uint256)"); + } + }); + + it("reports a missing function and a non-array ABI as such", () => { + const missing = generateCalldataForWorkflow(nodes([tuple], "burn", []), {}); + expect(missing).toMatchObject({ + success: false, + error: "Function 'burn' not found in ABI", + }); + const object = generateCalldataForWorkflow( + [ + { + id: "write-1", + data: { + actionType: "web3/write-contract", + config: { + contractAddress: CONTRACT, + abi: JSON.stringify({ not: "an array" }), + abiFunction: "send", + }, + }, + }, + ], + {} + ); + expect(object).toMatchObject({ + success: false, + error: "Invalid ABI JSON in workflow node", + }); + }); +}); diff --git a/tests/unit/mcp-calldata.test.ts b/tests/unit/mcp-calldata.test.ts index f90854be8..148862e66 100644 --- a/tests/unit/mcp-calldata.test.ts +++ b/tests/unit/mcp-calldata.test.ts @@ -136,14 +136,14 @@ describe("generateCalldataForWorkflow", () => { } }); - it("calls encodeFunctionData with correct ABI, function name, and args", () => { + it("calls encodeFunctionData with the canonical signature and args", () => { const nodes = [makeWriteNode()]; generateCalldataForWorkflow(nodes, {}); - expect(mockEncodeFunctionData).toHaveBeenCalledWith("transfer", [ - "0xRecipient", - "1000", - ]); + expect(mockEncodeFunctionData).toHaveBeenCalledWith( + "transfer(address,uint256)", + ["0xRecipient", "1000"] + ); }); it("resolves {{@trigger:Trigger.recipient}} template from triggerInputs", () => { @@ -154,10 +154,10 @@ describe("generateCalldataForWorkflow", () => { ]; generateCalldataForWorkflow(nodes, { recipient: "0xResolvedAddress" }); - expect(mockEncodeFunctionData).toHaveBeenCalledWith("transfer", [ - "0xResolvedAddress", - "500", - ]); + expect(mockEncodeFunctionData).toHaveBeenCalledWith( + "transfer(address,uint256)", + ["0xResolvedAddress", "500"] + ); }); it("converts ethValue '0.1' to wei string via parseEther", () => { From 0e17e270cf8edfab52daccf0b863392388aa1a74 Mon Sep 17 00:00:00 2001 From: Vastargazing Date: Wed, 9 Sep 2026 16:26:11 +0300 Subject: [PATCH 7/7] fix: #2330 keep accepting human-readable ABI entries where the key is now resolved Gas estimate and MCP calldata used to hand the stored ABI straight to ethers, which accepts human-readable fragments in the array and skips a string it cannot parse. Resolving the function key first made both paths see only object entries, and the gas estimate rejected the whole ABI over one bad string. Expand string entries into objects before resolving, and drop an unparsable one the way ethers does, so the healthy functions next to it keep working. --- app/api/gas/estimate/route.ts | 15 ++--- lib/abi/normalize.ts | 30 ++++++++++ lib/mcp/calldata.ts | 9 ++- tests/unit/abi-normalize.test.ts | 60 ++++++++++++++++++++ tests/unit/gas-estimate-function-key.test.ts | 32 +++++++++++ tests/unit/mcp-calldata-function-key.test.ts | 31 ++++++++++ 6 files changed, 164 insertions(+), 13 deletions(-) create mode 100644 lib/abi/normalize.ts create mode 100644 tests/unit/abi-normalize.test.ts diff --git a/app/api/gas/estimate/route.ts b/app/api/gas/estimate/route.ts index 52f20491d..414652e0c 100644 --- a/app/api/gas/estimate/route.ts +++ b/app/api/gas/estimate/route.ts @@ -1,5 +1,6 @@ import { ethers } from "ethers"; import { NextResponse } from "next/server"; +import { normalizeAbiEntries } from "@/lib/abi/normalize"; import { type AbiItem, describeAmbiguousKey, @@ -161,17 +162,9 @@ function estimateWriteContract( if (!Array.isArray(parsedAbi)) { return badRequest("ABI must be a JSON array"); } - // ethers also accepts JSON arrays of human-readable fragments. Normalize - // those entries for the shared resolver without discarding malformed objects. - try { - parsedAbi = parsedAbi.map((entry) => - typeof entry === "string" - ? JSON.parse(ethers.Fragment.from(entry).format("json")) - : entry - ); - } catch { - return badRequest("Invalid ABI fragment"); - } + // ethers also accepts human-readable fragments in the array; the resolver + // needs them as objects. + parsedAbi = normalizeAbiEntries(parsedAbi) as ethers.InterfaceAbi; const resolution = resolveAbiFunction( parsedAbi as AbiItem[], config.abiFunction diff --git a/lib/abi/normalize.ts b/lib/abi/normalize.ts new file mode 100644 index 000000000..2c41441f3 --- /dev/null +++ b/lib/abi/normalize.ts @@ -0,0 +1,30 @@ +import { ethers } from "ethers"; + +/** + * Expand human-readable ABI fragments such as + * `"function transfer(address to, uint256 amount)"` into the JSON objects the + * lookup helpers understand. ethers accepts both spellings in one array, so a + * caller that used to hand the array straight to ethers has to accept them + * too once it resolves the function key itself. + * + * Object entries pass through untouched, malformed ones included: lookup + * decides what to make of those. A string ethers cannot parse is dropped, + * which is what `new ethers.Interface` does with it (it warns and skips), so + * one broken entry keeps failing on its own instead of taking the healthy + * functions next to it down with it. + */ +export function normalizeAbiEntries(entries: unknown[]): unknown[] { + const normalized: unknown[] = []; + for (const entry of entries) { + if (typeof entry !== "string") { + normalized.push(entry); + continue; + } + try { + normalized.push(JSON.parse(ethers.Fragment.from(entry).format("json"))); + } catch { + // Skipped, as ethers would skip it. See above. + } + } + return normalized; +} diff --git a/lib/mcp/calldata.ts b/lib/mcp/calldata.ts index 14f5f655e..6f10b33a3 100644 --- a/lib/mcp/calldata.ts +++ b/lib/mcp/calldata.ts @@ -1,4 +1,5 @@ import { ethers } from "ethers"; +import { normalizeAbiEntries } from "@/lib/abi/normalize"; import { type AbiItem, describeAmbiguousKey, @@ -176,8 +177,12 @@ function generateSingleWriteCalldata( // The stored key may be a legacy raw spelling such as `send(tuple,address)`, // which the workflow engine accepts but ethers cannot encode. Resolve it the - // same way the engine does and encode with the canonical signature. - const resolution = resolveAbiFunction(parsedAbi as AbiItem[], abiFunction); + // same way the engine does and encode with the canonical signature. The ABI + // itself still goes to ethers as stored, human-readable entries included. + const resolution = resolveAbiFunction( + normalizeAbiEntries(parsedAbi) as AbiItem[], + abiFunction + ); if (resolution.status === "ambiguous") { return { success: false, diff --git a/tests/unit/abi-normalize.test.ts b/tests/unit/abi-normalize.test.ts new file mode 100644 index 000000000..7ba730730 --- /dev/null +++ b/tests/unit/abi-normalize.test.ts @@ -0,0 +1,60 @@ +import { ethers } from "ethers"; +import { describe, expect, it, vi } from "vitest"; +import { normalizeAbiEntries } from "@/lib/abi/normalize"; +import { type AbiItem, resolveAbiFunction } from "@/lib/abi/utils"; + +const HUMAN = "function transfer(address to, uint256 amount) returns (bool)"; +const OBJECT = { + type: "function", + name: "burn", + inputs: [{ name: "n", type: "uint256" }], + outputs: [], + stateMutability: "nonpayable", +}; +const MALFORMED = { type: "function", name: "odd", inputs: "nope" }; + +describe("normalizeAbiEntries", () => { + it("expands a human-readable fragment into the object ethers derives from it", () => { + const [entry] = normalizeAbiEntries([HUMAN]) as AbiItem[]; + expect(entry).toMatchObject({ + type: "function", + name: "transfer", + inputs: [ + { type: "address", name: "to" }, + { type: "uint256", name: "amount" }, + ], + }); + const resolution = resolveAbiFunction([entry], "transfer"); + expect(resolution).toMatchObject({ + status: "found", + canonicalKey: "transfer(address,uint256)", + }); + }); + + it("passes object entries through unchanged, malformed ones included", () => { + const normalized = normalizeAbiEntries([OBJECT, MALFORMED]); + expect(normalized[0]).toBe(OBJECT); + expect(normalized[1]).toBe(MALFORMED); + }); + + it("keeps order across mixed spellings", () => { + const names = (normalizeAbiEntries([HUMAN, OBJECT]) as AbiItem[]).map( + (e) => e.name + ); + expect(names).toEqual(["transfer", "burn"]); + }); + + it("drops a string ethers cannot parse and keeps its neighbours, as ethers does", () => { + const warn = vi.spyOn(console, "log").mockImplementation(() => undefined); + const abi = [HUMAN, "not a fragment at all", OBJECT]; + const normalized = normalizeAbiEntries(abi) as AbiItem[]; + expect(normalized.map((e) => e.name)).toEqual(["transfer", "burn"]); + // Same functions ethers itself keeps from the unnormalized array. + expect( + new ethers.Interface(abi as ethers.InterfaceAbi).fragments.map((f) => + f.format("sighash") + ) + ).toEqual(["transfer(address,uint256)", "burn(uint256)"]); + warn.mockRestore(); + }); +}); diff --git a/tests/unit/gas-estimate-function-key.test.ts b/tests/unit/gas-estimate-function-key.test.ts index 01a15d5bd..c0bd47fa5 100644 --- a/tests/unit/gas-estimate-function-key.test.ts +++ b/tests/unit/gas-estimate-function-key.test.ts @@ -102,6 +102,38 @@ describe("gas estimate function keys", () => { expect(estimateGas).toHaveBeenCalledTimes(1); }); + it.each([ + ["a human-readable ABI", ["function send((uint256 n) p)"]], + ["a mixed ABI", ["function send((uint256 n) p)", scalar]], + [ + "a function beside a string ethers cannot parse", + ["not a fragment at all", "function send((uint256 n) p)"], + ], + ])("estimates with %s", async (_label, abi) => { + const warn = vi.spyOn(console, "log").mockImplementation(() => undefined); + const response = await estimate(abi, "send(tuple)"); + expect(response.status).toBe(200); + expect(estimateGas).toHaveBeenCalledTimes(1); + expect(estimateGas).toHaveBeenCalledWith( + expect.objectContaining({ + data: new ethers.Interface([tuple]).encodeFunctionData( + "send((uint256))", + [[7]] + ), + }) + ); + warn.mockRestore(); + }); + + it("reports a function missing from a human-readable ABI as not found", async () => { + const response = await estimate(["function send((uint256 n) p)"], "burn"); + expect(response.status).toBe(400); + expect(await response.json()).toMatchObject({ + error: expect.stringContaining("not found in ABI"), + }); + expect(estimateGas).not.toHaveBeenCalled(); + }); + it("rejects ambiguous bare names without estimating an arbitrary overload", async () => { const response = await estimate([tuple, scalar], "send", [7]); expect(response.status).toBe(400); diff --git a/tests/unit/mcp-calldata-function-key.test.ts b/tests/unit/mcp-calldata-function-key.test.ts index ea7c655d8..08cd9d348 100644 --- a/tests/unit/mcp-calldata-function-key.test.ts +++ b/tests/unit/mcp-calldata-function-key.test.ts @@ -97,6 +97,37 @@ describe("generateCalldataForWorkflow function keys", () => { } }); + const humanTuple = + "function send((uint32 id, bytes32 to) params, address recipient)"; + + it.each([ + ["a human-readable ABI", [humanTuple]], + ["a mixed ABI", [humanTuple, scalar]], + [ + "a function beside a string ethers cannot parse", + ["not a fragment at all", humanTuple], + ], + ])("encodes a legacy tuple key against %s", (_label, abi) => { + const warn = vi.spyOn(console, "log").mockImplementation(() => undefined); + const result = generateCalldataForWorkflow( + nodes(abi, "send(tuple,address)", tupleArgs), + {} + ); + expect(result).toMatchObject({ success: true, data: expected }); + warn.mockRestore(); + }); + + it("reports a function missing from a human-readable ABI as not found", () => { + const result = generateCalldataForWorkflow( + nodes([humanTuple], "burn", []), + {} + ); + expect(result).toMatchObject({ + success: false, + error: "Function 'burn' not found in ABI", + }); + }); + it("reports a missing function and a non-array ABI as such", () => { const missing = generateCalldataForWorkflow(nodes([tuple], "burn", []), {}); expect(missing).toMatchObject({