You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The signing screen labeled a contract invocation's arguments by indexing the JSON-Schema required array positionally. required is not the argument list — Spec.jsonSchema() omits Option<T> parameters from it — so an optional parameter in any non-trailing position shifted every later label onto the wrong value, and a function whose parameters are all optional emitted no required at all, throwing into the app-level ErrorBoundary and replacing the entire signing view.
Labels now come from properties.args.properties, the complete ordered parameter list. That order survives the whole path — Spec.jsonSchema() fills properties in a single pass over the function's inputs, and JSON.stringify/JSON.parse both preserve insertion order for keys that aren't integer-like. Where that breaks down the derivation returns null and rows render unlabelled exactly as they do today, rather than shifting a name onto the wrong value.
What's in this PR
popup/helpers/soroban.ts — new getContractFnArgNames. Reads properties, and returns null on a length mismatch against the actual arguments or on integer-like keys that Object.keys hoists and sorts.
Operations/KeyVal/index.tsx — consumes the helper. argNames becomes string[] | null, and the optional index is what fixes the all-optional crash. Also drops a late spec response so it can't label a different invocation than the one it was issued for.
AuthEntry/index.tsx — comment only. Auth entries still skip spec lookup entirely; require_auth_for_args can substitute an arbitrary argument list under the same contract and function name at the same arity, so the length check doesn't protect them (Incorrect details annotated on contract auths #2196).
e2e-tests/contractArgLabels.test.ts — new spec covering the non-trailing Option, unavailable spec, all-optional, and auth-entry cases. No e2e previously asserted ParameterKey.
e2e-tests/helpers/stubs.ts — stubContractSpecDefinitions, taking a whole definitions map. The existing stubContractSpec only models a single-arg transfer, so it can't label a multi-arg list.
Test plan
yarn test:ci — 1916 passing
yarn test:e2e contractArgLabels — 4/4 against a fresh extension/build
Manual: pubnet contract with a non-trailing Option; labels align, all-optional renders instead of hitting the ErrorBoundary
CI green
Followups
/contract-spec could return an explicit ordered array derived from inputs(), so order is carried rather than inferred from object key order. Today the path is all JS end to end (jsonSchema() → Fastify JSON.stringify → the extension's response.json()), so key order is preserved incidentally; an ordered array would make it explicit and survive a backend rewrite in another language.
… `required`
The signing screen indexed the JSON-Schema `required` array positionally
against an invocation's arguments. `required` is not the argument list:
`Spec.jsonSchema()` omits `Option<T>` parameters from it, so an optional
in any non-trailing position shifted every subsequent label onto the wrong
value, and a function whose parameters are all optional emitted no
`required` at all, throwing into the app-level ErrorBoundary and replacing
the entire signing view.
Derive labels from `properties.args.properties`, the complete ordered
parameter list, via a new `getContractFnArgNames` helper. Object key order
is not guaranteed by JSON Schema and these names come from author-controlled
wasm metadata, so the helper fails closed and returns null when the derived
list cannot be trusted: on a length mismatch against the actual arguments,
on integer-like keys that `Object.keys` reorders, and when `required` is not
a subsequence of the derived names, which catches a re-serializer that sorted
them. An unnamed row renders blank, as before; no row is ever mislabelled.
Also guard the in-flight spec fetch so a late response cannot label a
different invocation than the one it was issued for.
Auth entries keep suppressing spec lookup entirely. Their arguments need not
be the function's declared parameters -- `require_auth_for_args` substitutes
an arbitrary list under the same contract and function name, at the same
arity -- so the length check does not protect them. Document that at both
the branch and the call site; it was reconstructible only from #2244 -> #2196.
Qualify spec-derived parameter names in the sign flow: the spec is
author-controlled wasm metadata that nothing validates against the
implementation, so a name is the contract's claim, not a verified fact.
Per design, the note belongs to the section rather than the card, so
hoist the spec lookup into the component that renders the "Parameters"
heading. It resolves the names once and hands them to the rows below,
which no longer fetch the spec themselves.
`required` is not an order witness. It is emitted in declaration order but
omits every `Option<T>`, so a subsequence match against the derived names
passes for reorderings it was meant to catch: `(z?, a, b)` alphabetized to
`[a, b, z]` still leaves `required = [a, b]`, and an all-optional function
emits no `required` at all. The check promised a fail-closed invariant it
never delivered, and it could not fire regardless — `Spec.jsonSchema()`
fills `properties` in one pass over the function's inputs, the backend
serializes that with `JSON.stringify`, and the extension reads it back with
`response.json()`, all of which preserve insertion order for keys that are
not integer-like.
Arity and integer-like keys remain the guards. The doc comment now says
what the code does and names the followup — an explicit ordered array from
`/contract-spec` — instead of claiming an order proof.
Two parameter rows can share a React key, so a label can move to the wrong row
Low severity — not a merge blocker. No incorrect label today, but the failure mode it opens is the one this PR exists to close.
TL;DR: Parameter rows are keyed by the argument's serialized value, so any two arguments that happen to be equal get the same key. That is not hypothetical — this PR's own test contract passes the same contract address twice, so six rows render under five distinct keys. React warns about it, and once keys collide, a later re-render is free to reuse the wrong row, which is exactly how a name ends up next to someone else's value. Keying by position instead makes rows unambiguous.
Steps to reproduce:
Sign any contract invocation that passes the same value in two argument slots — gauge_schedule_reward(router, distributor, gauge, …) from the tests already does, and a self-transfer (transfer(from, to, amount) with from == to) does it in the wild.
Open the transaction details pane.
React logs Encountered two children with the same key for the parameters list.
Detailed explanation (for agents)
Root cause: the row key is derived purely from the argument's value, with nothing positional in it:
arg.toXdr("base64") is a pure function of the ScVal, so two structurally equal arguments serialize identically. Argument position is what distinguishes these rows — it is the whole basis of the labelling this PR fixes — but position is exactly what the key throws away.
Deterministic repro — the fixture arg list from Operations.test.tsx and contractArgLabels.test.ts, run against @stellar/stellar-sdk 17:
[0] router key=AAAAEgAAAAE2Pqo4Z4QfutD07YjHeeT+ZuVqJHDcmMDsnAc9BcexAw==
[1] distributor key=AAAAEgAAAAAAAAAA2DBND5eKkuBaaqaVSvS7dqoQkItWTc4mERXCu4JTS2U=
[2] gauge key=AAAAEgAAAAE2Pqo4Z4QfutD07YjHeeT+ZuVqJHDcmMDsnAc9BcexAw==
[3] start_at key=AAAABQAAAABoTuGA
[4] duration key=AAAABQAAAAAACTqA
[5] tps key=AAAACgAAAAAAAAAAAAAAAAAAACo=
COLLISION: index 0 (router) and index 2 (gauge) share a key
unique keys: 5 / 6
router and gauge are both new Address(CONTRACT).toScVal() in both fixtures, so the collision is already present in the tests added here.
Initial mount still paints rows in order, which is why the new assertions pass. The exposure is on update: this list re-renders when argNames flips from null to resolved, and with duplicate keys React's reconciliation is free to match a child to the wrong previous element. That is a label-to-value misalignment — the same class of defect the PR fixes on the spec side.
Suggested fix: key by position, which is stable and unique, and reserve the value for content. The sibling port already does exactly this — freighter-mobile uses `arg-${contextKey}-${index}-${xdrString}`, so adopting the same shape here also removes a cross-platform divergence:
A refetch drops the labels without going back into the loading state
Low severity — not a merge blocker. Defensive only: I could not construct a user path that reaches it on the signing screen today.
TL;DR: The loading flag is raised once when the hook first mounts and is never raised again, but the effect that fetches the spec always clears the resolved names up front. So if that effect ever runs a second time, the parameter rows drop to unlabelled and the spec note disappears, with no spinner to indicate a refetch is in flight — and if the second fetch fails, the rows stay unlabelled for good, where the previous code would have kept showing the names it already had. Raising the flag alongside the clear keeps the two in step.
Detailed explanation (for agents)
Root cause:isLoading is only ever initialized true and subsequently set false; nothing re-raises it.
The effect opens by unconditionally discarding the names — correct on its own terms, since a stale name must never outlive the invocation it was resolved for — but it does not pair that with a return to the loading state:
// A resolved fetch must never label a different invocation than the one it
// was issued for, so drop the names up front and ignore a response that
// arrives after the inputs moved on.
letisCurrent=true;
setArgNames(null);
On the second and later runs, the pair (isLoading: false, argNames: null) is indistinguishable from "lookup finished and produced nothing", so the consumer renders unlabelled rows and suppresses ContractSpecNote instead of showing the loader. If that refetch then throws, the catch sets isLoading false and leaves argNames at null — permanently unlabelled, a regression against the pre-PR behaviour of retaining the previous names.
Reachability — why this is defensive and not a live bug. The effect deps are [contractId, fnName, networkDetails, isAuthEntry, argCount]:
contractId / fnName / argCount all derive from the parsed transaction, which is fixed for the popup's lifetime. The operations list is keyed by index, so a changing list would feed new props into a surviving instance — but the list never changes while mounted.
networkDetails comes from redux and is stable; the signing popup offers no way to switch networks.
So it is unreachable today and the fix is about not leaving the trap armed for a future caller that does re-run the effect.
Suggested fix: keep the flag and the names in step — raise one wherever the other is cleared.
spec can be narrowed past Record<string, any>, which also removes the cast
Suggestion — not a merge blocker. Type-level only; no behaviour change.
TL;DR: The helper already declares a type for the innermost part of the spec it reads, but the parameter itself stays fully untyped, so reaching that inner type needs a cast to get there. Declaring the two intermediate levels as well lets the optional chaining resolve on its own and the cast goes away. Worth doing here because the muxed-address helper already describes this same payload with its own hand-written copy of the shape — so there are two independent descriptions of one response, and a shared named type collapses both.
but the parameter is Record<string, any>, so spec?.definitions?.[fnName]?.properties?.args is any at every step, and the as is needed purely to land back on the declared type:
Suggested change — declare the two intermediate levels, then the chain resolves natively:
/** The slice of `Spec.jsonSchema()` output the wallet actually reads. */exportinterfaceContractFnArgsSchema{properties?: Record<string,unknown>;required?: string[];}exportinterfaceContractFnDefinition{properties?: {args?: ContractFnArgsSchema};}exportinterfaceContractSpecSchema{definitions?: Record<string,ContractFnDefinition|undefined>;}exportconstgetContractFnArgNames=(spec: ContractSpecSchema|undefined,fnName: string,argCount: number,): string[]|null=>{constnames=Object.keys(spec?.definitions?.[fnName]?.properties?.args?.properties??{},);if(names.length!==argCount){returnnull;}if(names.some((name)=>INTEGER_LIKE_KEY.test(name))){returnnull;}returnnames;};
Verified with tsc --strict: compiles clean, no any, no assertion. required is included so the same type covers the muxed-address call site.
Why not JSONSchema7. It is the provenance-correct type — the SDK declares jsonSchema(funcName?: string): JSONSchema7 — but it costs more than it returns here:
@types/json-schema (7.0.15) is only present transitively via the SDK; it is not in extension/package.json, so importing it directly means relying on an undeclared dependency.
JSONSchema7Definition = JSONSchema7 | boolean, and both definitions and properties are maps of it, so the chain does not compile as written:
error TS2339: Property 'properties' does not exist on type 'JSONSchema7Definition'.
Property 'properties' does not exist on type 'false'.
Getting it to compile needs a typeof d === "object" narrowing threaded at each level — I confirmed that version does typecheck, but it is two extra guard calls to model a boolean arm /contract-spec will never send.
Scope note. This is documentation, not validation — spec is untrusted JSON and an interface is a compile-time claim. The existing runtime guards already cover that independently: a non-object payload yields either integer-like keys ("abc" → ["0","1","2"], rejected by INTEGER_LIKE_KEY) or no keys at all (caught by the arity check), so no runtime validator is needed.
Optional follow-up (wider blast radius, probably not this PR). The any originates upstream:
Rows were keyed by `arg.toXdr("base64")`, so two arguments holding the
same value shared a key -- a self-transfer, or the `router`/`gauge` pair
in our own fixture. On the re-render that resolves argument names, React
is then free to match a row to the wrong previous element, putting a
label next to someone else's value.
`isLoading` was raised once at mount and never again, while the effect
cleared `argNames` on every run. A second run therefore left the pair
`(false, null)` -- indistinguishable from a finished lookup that found
nothing -- so the parameter rows dropped to unlabelled and the spec note
vanished with no loader to show a refetch was in flight.
Reachable, if narrowly: `grantAccess` dispatches `saveSettingsAction`
fire-and-forget, which writes a fresh `networkDetails` object with
identical values, and in sidebar mode the same React tree carries from
the grant into signing.
Collapsing the flag and the names into one `SpecLookup` value makes the
inconsistent pair unrepresentable: clearing the names is a return to the
loading state. The hook's return shape is unchanged, so no consumer moves.
… to it
`getContractFnArgNames` took `Record<string, any>`, so every step of
`spec?.definitions?.[fnName]?.properties?.args` was `any` and an assertion
was needed just to land back on the declared leaf type. Naming the two
intermediate levels lets the chain resolve on its own and the cast goes away.
`checkIsMuxedSupported` described the same payload with its own inline copy
of the shape, `required` included, so it now reads the shared type instead.
This route is installed on the original extension page, but openSignTransactionPopup creates a different page. The only route for that popup is installed after its page event, so the contract-spec request can start first and reach the real endpoint, making this failure-path test race or observe a successful spec. Register the abort on context here, before opening the popup, so it applies to every subsequently created page.
Use supplied argument lookup without falling back to internal state
When this component is controlled by OperationParametersSection, resolvedArgNames can intentionally be null and its loading state is already authoritative. Falling back to ownSpec with ?? and OR-ing its loading flag makes the unused internal hook affect rendering anyway. In particular, RenderOpArgsByType is declared inside Operations, so a parent rerender remounts this component with ownSpec.status === "loading"; even if the parent already has resolved names, the argument rows flash back to the loader until the no-ID effect completes. Select either the supplied lookup or the internal lookup based on whether resolvedArgNames was provided.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
piyalbasu
left a comment
Summary
The signing screen labeled a contract invocation's arguments by indexing the JSON-Schema
requiredarray positionally.requiredis not the argument list —Spec.jsonSchema()omitsOption<T>parameters from it — so an optional parameter in any non-trailing position shifted every later label onto the wrong value, and a function whose parameters are all optional emitted norequiredat all, throwing into the app-levelErrorBoundaryand replacing the entire signing view.Labels now come from
properties.args.properties, the complete ordered parameter list. That order survives the whole path —Spec.jsonSchema()fillspropertiesin a single pass over the function's inputs, andJSON.stringify/JSON.parseboth preserve insertion order for keys that aren't integer-like. Where that breaks down the derivation returnsnulland rows render unlabelled exactly as they do today, rather than shifting a name onto the wrong value.What's in this PR
popup/helpers/soroban.ts— newgetContractFnArgNames. Readsproperties, and returnsnullon a length mismatch against the actual arguments or on integer-like keys thatObject.keyshoists and sorts.Operations/KeyVal/index.tsx— consumes the helper.argNamesbecomesstring[] | null, and the optional index is what fixes the all-optional crash. Also drops a late spec response so it can't label a different invocation than the one it was issued for.AuthEntry/index.tsx— comment only. Auth entries still skip spec lookup entirely;require_auth_for_argscan substitute an arbitrary argument list under the same contract and function name at the same arity, so the length check doesn't protect them (Incorrect details annotated on contract auths #2196).e2e-tests/contractArgLabels.test.ts— new spec covering the non-trailingOption, unavailable spec, all-optional, and auth-entry cases. No e2e previously assertedParameterKey.e2e-tests/helpers/stubs.ts—stubContractSpecDefinitions, taking a wholedefinitionsmap. The existingstubContractSpeconly models a single-argtransfer, so it can't label a multi-arg list.Test plan
yarn test:ci— 1916 passingyarn test:e2e contractArgLabels— 4/4 against a freshextension/buildOption; labels align, all-optional renders instead of hitting theErrorBoundaryFollowups
/contract-speccould return an explicit ordered array derived frominputs(), so order is carried rather than inferred from object key order. Today the path is all JS end to end (jsonSchema()→ FastifyJSON.stringify→ the extension'sresponse.json()), so key order is preserved incidentally; an ordered array would make it explicit and survive a backend rewrite in another language.