fix: #2330 expand tuples in overloaded ABI function keys - #2349
fix: #2330 expand tuples in overloaded ABI function keys#2349Vastargazing wants to merge 4 commits into
Conversation
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.
…he 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.
…ew 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.
About the
|
joelorzet
left a comment
joelorzet
left a comment
There was a problem hiding this comment.
The design is careful and the documentation on it is unusually good. resolveAbiFunction reporting ambiguous separately from not_found, distinctBySignature stopping repeated facet entries from looking like overloads, and findAbiFunction staying total so the UI helpers keep failing closed, are all the right calls.
One change, and one smaller one, left inline.
Worth adding to the description rather than fixing: a saved workflow whose key collapsed two overloads now returns an error instead of silently calling the first match. That is the correct behaviour and your body does say it, but it changes workflows that run today, so it should be called out as a breaking change rather than sitting inside a bullet list.
| * and a tuple[] becomes "(uint32,bytes32)[]" | ||
| */ | ||
| function canonicalType(input: AbiInput): string { | ||
| export function canonicalType(input: AbiInput): string { |
There was a problem hiding this comment.
This function can still return the literal string tuple, which is the exact spelling the PR exists to eliminate.
The guard below reads:
if (!(input.type.startsWith("tuple") && input.components)) {
return input.type;
}A tuple with no components fails that condition and falls through to return input.type. I ran it:
tuple, no components -> "tuple"
tuple[], no components -> "tuple[]"
tuple, components [] -> "()"
good tuple -> "(address,uint256)"
Three consequences, all in the new code:
canonicalSignaturecan producesend(tuple,address). InresolveAbiFunctionthat matches on the canonical branch, ahead of the legacy branch, and returnsstatus: "found"withcanonicalKey: "send(tuple,address)". The guarantee thatcanonicalKeyis the spelling ethers accepts does not hold for that entry, and encoding fails exactly as it does today.getAbiFunctionKeystores that same unencodable key, so the original bug is still reachable from a different input.computeSelectorhashessend(tuple,address)and returns a selector that is not the real on-chain one.getFunctionSelectortherefore returns a value rather thannull, so the entry is not excluded from deduplication the way its comment promises. Two distinct tuple overloads that both lackcomponentscollide on that wrong selector andcombineAbisdrops one, which is the failure that comment says this change prevents.
Your malformed-tuple tests cover components: [{ name: "a" }] and components: { a: "uint256" }. Both throw and are handled correctly. A tuple with components absent is the one shape that produces a wrong answer silently instead of throwing, and it is untested.
The fix is one line: treat a tuple-prefixed type without usable components the same as a missing type, and throw.
There was a problem hiding this comment.
you're right, i missed the case where components is absent (:
fixed in 294294f: canonicalType now throws for tuple-prefixed types unless components is an array
an empty components array still gives ()
this prevents the entry from matching through the canonical branch or producing a selector for deduplication. the legacy lookup fallback still works, but it cannot make a malformed ABI entry encodable
added regression tests for tuple and tuple[] without components, the legacy lookup fallback, and keeping both entries in combineAbis when neither has components
| components?: AbiItemComponent[]; | ||
| }>; | ||
| }): string | null { | ||
| if (abiItem.type !== "function" || !abiItem.name || !abiItem.inputs) { |
There was a problem hiding this comment.
!abiItem.inputs excludes an entry that omits inputs entirely. An empty array passes, so zero-argument functions are fine, but explorer-fetched ABIs sometimes omit the key rather than emitting [].
Such an entry returns null, drops out of deduplication, and can then appear twice in a merged Diamond ABI. Treating a missing inputs as [] covers it.
There was a problem hiding this comment.
fixed in 294294f
missing inputs now defaults to [], so both forms of a zero-argument function produce the same selector
added a test with inputs omitted in one facet and inputs: [] in another, checking that the merged ABI contains the function only once
joelorzet
added
the
changes-requested
…ling 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.
|
thanks for the review! addressed both inline comments in 294294f and added regression tests for the missing cases i've also added a separate Breaking change section to the description and updated the validation results after rerunning on Node 24.19.0 |
suisuss
left a comment
suisuss
left a comment
There was a problem hiding this comment.
What this changes
Four files, two of them source. lib/abi/utils.ts:25-33 splits canonicalType's single guard in two: a non-tuple returns input.type, and a tuple whose components is not an array now throws where it previously returned the literal string "tuple". lib/abi/combine-abis.ts:27,33 drops the !abiItem.inputs early return in favour of computeSelector(name, abiItem.inputs ?? []), so a function entry that omits inputs participates in selector dedup instead of bypassing it. Two test files gain two cases each.
The expansion itself is right, and I checked it rather than reading it: I ported canonicalType and computeSelector verbatim and diffed them against ethers v6 FunctionFragment.format("sighash") and .selector for a flat tuple, a nested tuple, tuple[], tuple[2], a tuple[] nested inside a tuple, tuple[][2] of nested tuples, the empty tuple and a tuple-plus-scalar. All eight match on both the string and the four-byte selector, and new Interface([entry]).getFunction(key) resolves in every case. The suffix slice is correct for arbitrary array dimensions because Solidity puts every dimension after the tuple. Legacy stored keys also still resolve on all five paths I traced, and getAbiFunctionKey upgrades a legacy spelling to the canonical one before ethers sees it.
Does it match the description
Undersells. The diff also changes what a components-less tuple does everywhere canonicalType is reached, and one of those call sites has no guard - see the first blocker. That is a behaviour change well outside "expand tuples in overloaded function keys", and it is a regression against both staging and the previously reviewed head.
Blocking
-
components/workflow/config/action-config-renderer.tsx:520,529- the new throw empties the entire function dropdown.isValidAbiInput(lib/abi/function-inputs.ts:28-30) returnstruewhencomponents === undefined, so for{name:"p", type:"tuple"}thecompletetest at:511is true,computeSelectorat:520andinputs.map(canonicalType)at:529both throw, and thecatchat:541returns[]. -> Paste an ABI with forty good functions and one hand-written{"type":"function","name":"f","inputs":[{"name":"p","type":"tuple"}]}and the selector renders "No functions found in ABI" for the whole ABI. Atc9ee6a18and onstagingall forty-one listed. -> Guard the two calls, or make the: inputTypesfallback reachable - the comment at:523-527says it is "the same condition that already suppresses the selector", and it is not, becausecompleteis true for this input. The two notions of validity have diverged and nothing makes them agree. -
lib/execute/simulate.ts:637- a bare function name against an overloaded ABI now silently encodes against the first overload.resolveAbiFunctionkeeps first-match for a bare name and hands backcanonicalKey = canonicalSignature(named[0]), whichsimulate.tsthen encodes with. Onstagingthis reachediface.encodeFunctionData("swap", args), which throwsambiguous function description. -> An API or MCP caller sendingswapfor a contract withswap(uint256)andswap((address,uint256))used to get a clean failure and now gets an arbitrary pick, with the decode at:703matching. -> The PR adds anambiguousstatus for a legacy qualified key matching more than one overload;parenIdx === -1 && named.length > 1is the case that most deserves it and is the one that got quieter. -
components/workflow/config/action-config-renderer.tsx:532,557- option values changed format, so an existing workflow's stored key no longer matches anySelectItem.stagingemittedsend(tuple,address); head emitssend((uint32,bytes32),address). The controlled<Select value={value}>falls back to the placeholder, whileAbiFunctionArgsFieldbelow it resolves the same legacy key throughfindAbiFunctionand renders the arguments fully populated. -> The user sees "Select a function" above a filled-in argument form on a workflow that still executes correctly. -> Normalise the stored key when the panel loads, or emit the legacy spelling as an additional matching value.
Mechanical - actionable as-is
-
plugins/web3/steps/query-transactions-core.ts:294-iface.getFunction(abiFunction)with notry, and neitherqueryTransactionsInner:330norqueryTransactionsCore:317wraps it. A legacy tuple key throws out of the step instead of returning aUSERerror. Pre-existing, but it is exactly the key format this change teaches the rest of the system to accept. -
app/api/gas/estimate/route.ts:176-contract.getFunction(config.abiFunction)fails a stored legacy key with"Function not found in ABI"while the same workflow executes fine, so the message names the wrong cause. -
lib/abi/function-key.ts:44-47- the comment says the malformed-key path "still fails, but at the encoder, naming the fragment". InreadContractthatgetFunctionsits insiderpcManager.executeWithFailover(lib/web3/chain-adapter/evm.ts:258), so a deterministicinvalid function fragmentis retried across every configured provider first, andread-contract-core.tslogs it underErrorCategory.NETWORK_RPC. -
tests/unit/abi-utils.test.ts:266-285-canonicalTypehas two positive cases, a flat tuple and a one-componenttuple[], andabi-function-key.test.tsuses only a flat two-field struct. Nested tuples,tuple[2], arrays of nested tuples and the empty tuple are all unpinned. I verified every one of them empirically and they are correct - the recursion and the suffix slice are precisely the two things that could regress, and neither has a test. -
tests/unit/abi-combine-abis.test.ts- the new case is titled "keeps two distinct tuple overloads", but the two entries differ only in parameter name, which is not part of any signature. They are two unencodable entries rather than distinguishable overloads. The behaviour is fine; the title and comment are not. -
One screenshot: an existing workflow whose stored
abiFunctionis a legacy tuple key, showing the function selector and the argument fields below it together. That state is the third blocker and only a render settles what it looks like.
Verdict
Changes requested - one components-less tuple anywhere in an ABI now blanks the entire function dropdown, and a bare overloaded name silently encodes against an arbitrary overload where it used to fail.
suisuss
added
the
requested-evidence
3 participants
Closes #2330
Selecting a tuple overload could store a key such as
permit(address,tuple,bytes): lookup accepted it, but ethers could not encode it. Different overloads could also produce the same stored key.This follows the accepted plan, including the Diamond ABI merge fix.
getAbiFunctionKeynow build canonical signatures with expanded tuple types.canonicalTypestays strict. Lookup catches canonicalisation errors per entry so malformed tuple components do not crash the config renderer or prevent lookup of healthy entries.lib/abi/combine-abis.tsso the merge itself could be tested.Bare-name lookup keeps its existing first-match behaviour. Saved keys such as
send(tuple,address)still work when they identify one overload. Keys that collapsed multiple overloads require the user to select the function again.Breaking change
Saved workflows with a legacy function key that matches multiple overloads, such as
permit(address,tuple,bytes), now return an error listing the full signatures instead of selecting the first match. Users need to re-select the function once to save its full signature.This only affects ambiguous legacy keys. Bare-name lookup, overloads without tuple parameters, and legacy keys that identify a single overload, such as
send(tuple,address), keep their existing behaviour.Tests
Tests cover canonical and legacy keys through encoding, decoded simulation results, duplicate ABI entries, malformed tuple components, and ambiguous-key errors before the API's read/write branch. Diamond merge tests cover overloads within one facet and across facets, genuine duplicates, ordering, and malformed entries.
Validation on
294294f, using Node 24.19.0 to match.node-version:pnpm discover-plugins && pnpm type-check: passed.pnpm check: no errors across 2168 files; one pre-existing warning about an oversized JSON fixture.