fix(signing): label contract args from the spec's parameter list (#1011)

* fix(signing): label contract args from the spec's parameter list

  Argument names came from the JSON-Schema `required` array, which is not
  the parameter list -- `Spec.jsonSchema()` omits `Option<T>` params from
  it. A non-trailing optional shifted every later label onto the wrong
  value, and an all-optional function emitted no `required` at all, so
  indexing it threw and took down the signing view.

  Names now come from `properties.args.properties`, the complete ordered
  list. On an arity mismatch or integer-like keys the derivation returns
  null and rows render unlabelled rather than mislabelled.
  list. On an arity mismatch or integer-like keys the derivation returns
  null and rows render unlabelled rather than mislabelled.

  Also adds a note qualifying spec-derived names as the contract's own
  claim, drops late spec responses so they can't label a different
  invocation, and skips the lookup for auth entries entirely --
  `require_auth_for_args` can substitute an arbitrary same-arity list, so
  the length check doesn't protect them (stellar/freighter#2196).

* docs(soroban): correct the contract-spec docstring's parameter list

    The `getContractSpecs` docstring still told the reader to take parameter
    names from `properties.args.required` -- the exact mistake this branch
    fixes -- and its two examples verified only because neither sample
    function has an `Option<T>` parameter.

    Prose and both examples now point at `properties.args.properties` and
    say why `required` is not the parameter list.

* refactor(soroban): type the contract spec instead of casting into it

    `getContractFnArgNames` took `Record<string, any>` and cast its way back
    to the one declared leaf type, and `checkContractSupportsMuxed` carried a
    second, inline copy of the same shape. Declaring the intermediate levels
    lets the optional chain resolve on its own, so both casts go away and one
    named type describes the response. Narrowing `getContractSpecs` to match
    drops the last `any` in that region, and the eslint-disable with it.

    Also corrects the `getContractSpecs` docstring, which still pointed at
    `properties.args.required` -- the mistake this branch fixes. Its two
    examples verified only because neither sample function has an `Option<T>`
    parameter.

* fix(signing): drop the unreachable auth-entry spec note

    The note was rendered under `showHeader && argNames?.length`, but every
    call site that shows the heading is one where names are never resolved --
    auth entries suppress the lookup, the rest pass no contract id -- and the
    one site with names owns its heading and renders the note itself. The
    branch could not fire, leaving its `authorizations.contractSpecNote` key
    dead in `en` and `pt`.

    Removes the branch, the `translationKey` prop that existed only to serve
    it, and both locale entries.

* test(signing): pin the auth-entry spec-label suppression

    Suppressing spec-derived labels on auth entries is the one behaviour this
    branch changes rather than ports, and nothing asserted it -- dropping
    `isAuthEntry` from either call site, or the guard from the hook, would
    have regressed silently.

    Covers both call sites: the spec mock resolves names that would label the
    rows, and each test asserts it is never called and the rows stay bare.
    Verified by mutation -- all three removals fail these tests.

* fix(signing): re-raise the spec lookup's loading flag on a refetch

  The effect drops the resolved parameter names on every run, but the
  loading flag was only ever lowered. A second run left
  (isLoading: false, argNames: null), which reads as "the lookup finished
  and produced nothing" -- unlabelled rows and no spec note, with no
  spinner while the refetch was in flight. Raise the flag alongside the
  clear so the two stay in step.

  Not reachable from the signing screen today: every effect dep is fixed
  for the screen's lifetime, and networkDetails is a module-level
  constant, so this disarms the trap rather than fixing a live bug.

  The clear itself stays unconditional -- a name resolved for one
  invocation must never outlive it -- and the second test pins that, so
  a failed refetch settles on unlabelled rows instead of stale names.

* Update contract spec note translation key

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix(signing): key the contract-spec lookup to its invocation

  The lookup dropped its names inside a passive effect, so a changed
  invocation committed one render carrying the previous one's argNames
  with isLoading already false -- new values wearing old parameter names.
  Store the result under the key it was fetched for and compare that key
  during render, which closes the window and collapses two useStates into
  one.

  That window was unreachable only because SignTransactionOperationDetails
  latched its operations into a ref, a workaround for
  useSignTransactionDetails re-parsing the XDR every render. Memoize the
  parse and drop the latch, so the sheet also follows a changed
  transaction instead of pinning the first one.

  Also move contractSpecNote to the operations namespace its only caller
  reads, which was rendering the raw key to users.

* fix(signing): drop resolved arg names for an unresolved invocation

  `isAuthEntry` gated the spec fetch but not the read of its result, and it
  is not part of the invocation key. A hook instance that resolved names and
  then flipped to an auth entry at the same contract, function and arity kept
  returning them -- spec labels on args that need not be the function's
  declared parameters. No call site can reach that today; every one passes a
  constant. Gating the read on `shouldResolve` states the invariant instead of
  relying on that.

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: aristidesstaffieri

__tests__/components/screens/SignTransactionDetails/components/KeyVal.test.tsx

@@ -0,0 +1,204 @@
+import { renderHook, waitFor } from "@testing-library/react-native";
+import { useContractArgNames } from "components/screens/SignTransactionDetails/components/KeyVal";
+import { ContractSpecSchema } from "helpers/soroban";
+import { getContractSpecs } from "services/backend";
+
+jest.mock("ducks/auth", () => ({
+  useAuthenticationStore: () => ({ network: "PUBLIC" }),
+}));
+
+jest.mock("services/backend", () => ({
+  getContractSpecs: jest.fn(),
+}));
+
+const CONTRACT_A = "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4";
+const CONTRACT_B = "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC";
+
+const specFor = (fnName: string, argNames: string[]): ContractSpecSchema => ({
+  definitions: {
+    [fnName]: {
+      properties: {
+        args: {
+          properties: Object.fromEntries(argNames.map((name) => [name, {}])),
+          required: argNames,
+        },
+      },
+    },
+  },
+});
+
+const TRANSFER_SPEC = specFor("transfer", ["from", "to"]);
+
+describe("useContractArgNames", () => {
+  const getContractSpecsMock = getContractSpecs as jest.MockedFunction<
+    typeof getContractSpecs
+  >;
+
+  beforeEach(() => {
+    getContractSpecsMock.mockReset();
+  });
+
+  // The names and the loading flag are reset together at the top of the
+  // effect. If only the names were dropped, `(isLoading: false, argNames:
+  // null)` would be indistinguishable from "the lookup finished and produced
+  // nothing", so the rows would render unlabelled -- and the spec note would
+  // disappear -- while a refetch was still in flight.
+  it("returns to the loading state when the invocation changes", async () => {
+    getContractSpecsMock
+      .mockResolvedValueOnce(TRANSFER_SPEC)
+      // Stays pending, so the in-flight state is observable.
+      .mockImplementationOnce(() => new Promise<never>(() => {}));
+
+    const { result, rerender } = renderHook(
+      (props: { contractId: string; fnName: string; argCount: number }) =>
+        useContractArgNames(props),
+      {
+        initialProps: {
+          contractId: CONTRACT_A,
+          fnName: "transfer",
+          argCount: 2,
+        },
+      },
+    );
+
+    await waitFor(() => expect(result.current.isLoading).toBe(false));
+    expect(result.current.argNames).toEqual(["from", "to"]);
+
+    rerender({ contractId: CONTRACT_B, fnName: "swap", argCount: 2 });
+
+    expect(result.current.isLoading).toBe(true);
+    expect(result.current.argNames).toBeNull();
+  });
+
+  // Dropping the names up front is deliberate: a name resolved for one
+  // invocation must never outlive it. So a failed refetch settles on
+  // unlabelled rows rather than resurrecting the names it already had.
+  it("does not fall back to the previous names when a refetch fails", async () => {
+    getContractSpecsMock
+      .mockResolvedValueOnce(TRANSFER_SPEC)
+      .mockRejectedValueOnce(new Error("no spec"));
+
+    const { result, rerender } = renderHook(
+      (props: { contractId: string; fnName: string; argCount: number }) =>
+        useContractArgNames(props),
+      {
+        initialProps: {
+          contractId: CONTRACT_A,
+          fnName: "transfer",
+          argCount: 2,
+        },
+      },
+    );
+
+    await waitFor(() =>
+      expect(result.current.argNames).toEqual(["from", "to"]),
+    );
+
+    rerender({ contractId: CONTRACT_B, fnName: "swap", argCount: 2 });
+
+    await waitFor(() => expect(result.current.isLoading).toBe(false));
+    expect(result.current.argNames).toBeNull();
+  });
+
+  // The guarantee is about the render itself, not about the state after
+  // effects have flushed. Resetting inside the effect would still satisfy the
+  // assertions above -- `rerender` flushes effects before they run -- while
+  // committing one frame that pairs the new invocation's values with the
+  // previous one's names. So record what every render actually saw.
+  it("never renders the previous invocation's names against a new one", async () => {
+    getContractSpecsMock
+      .mockResolvedValueOnce(TRANSFER_SPEC)
+      // Stays pending, so any stale frame has time to be observed.
+      .mockImplementationOnce(() => new Promise<never>(() => {}));
+
+    const renders: Array<{ argNames: string[] | null; isLoading: boolean }> =
+      [];
+
+    const { result, rerender } = renderHook(
+      (props: { contractId: string; fnName: string; argCount: number }) => {
+        const current = useContractArgNames(props);
+        renders.push({ ...current });
+        return current;
+      },
+      {
+        initialProps: {
+          contractId: CONTRACT_A,
+          fnName: "transfer",
+          argCount: 2,
+        },
+      },
+    );
+
+    await waitFor(() =>
+      expect(result.current.argNames).toEqual(["from", "to"]),
+    );
+
+    renders.length = 0;
+    rerender({ contractId: CONTRACT_B, fnName: "swap", argCount: 2 });
+
+    expect(renders.length).toBeGreaterThan(0);
+    renders.forEach((render) => {
+      expect(render.argNames).toBeNull();
+      expect(render.isLoading).toBe(true);
+    });
+  });
+
+  // An auth entry resolves nothing, so it has no in-flight state to report.
+  // Reporting one anyway would flash a spinner over rows that are always
+  // going to render unlabelled.
+  it("never reports loading for an invocation it will not resolve", () => {
+    const { result } = renderHook(() =>
+      useContractArgNames({
+        contractId: CONTRACT_A,
+        fnName: "transfer",
+        argCount: 2,
+        isAuthEntry: true,
+      }),
+    );
+
+    expect(result.current.isLoading).toBe(false);
+    expect(result.current.argNames).toBeNull();
+    expect(getContractSpecsMock).not.toHaveBeenCalled();
+  });
+
+  // `isAuthEntry` is not part of the invocation key, so names resolved for the
+  // operation view would survive a switch to an auth entry at the same
+  // contract, function and arity -- spec labels on args that need not be the
+  // function's declared parameters, which is what the flag exists to prevent.
+  it("drops resolved names when the same invocation becomes an auth entry", async () => {
+    getContractSpecsMock.mockResolvedValueOnce(TRANSFER_SPEC);
+
+    const { result, rerender } = renderHook(
+      (props: {
+        contractId: string;
+        fnName: string;
+        argCount: number;
+        isAuthEntry: boolean;
+      }) => useContractArgNames(props),
+      {
+        initialProps: {
+          contractId: CONTRACT_A,
+          fnName: "transfer",
+          argCount: 2,
+          isAuthEntry: false,
+        },
+      },
+    );
+
+    await waitFor(() =>
+      expect(result.current.argNames).toEqual(["from", "to"]),
+    );
+
+    rerender({
+      contractId: CONTRACT_A,
+      fnName: "transfer",
+      argCount: 2,
+      isAuthEntry: true,
+    });
+
+    expect(result.current.argNames).toBeNull();
+    // No spinner either: the rows are always going to render unlabelled.
+    expect(result.current.isLoading).toBe(false);
+    expect(getContractSpecsMock).toHaveBeenCalledTimes(1);
+  });
+});

__tests__/components/screens/SignTransactionDetails/components/Operations.test.tsx

@@ -5,6 +5,7 @@ import {
   Address,
   Asset,
   BASE_FEE,
+  nativeToScVal,
   Networks,
   Operation,
   OperationRecord,
@@ -16,6 +17,7 @@ import { render } from "@testing-library/react-native";
 import Operations from "components/screens/SignTransactionDetails/components/Operations";
 import { truncateAddress } from "helpers/stellar";
 import React from "react";
+import { getContractSpecs } from "services/backend";
 
 // Render i18n keys verbatim so assertions target the value rows, not labels.
 jest.mock("react-i18next", () => ({
@@ -52,6 +54,11 @@ jest.mock("services/blockaid/api", () => ({
   scanToken: jest.fn().mockResolvedValue(undefined),
 }));
 
+// The contract spec is the only source of parameter names; stub it per test.
+jest.mock("services/backend", () => ({
+  getContractSpecs: jest.fn().mockRejectedValue(new Error("no spec")),
+}));
+
 // Use the real number/asset formatting.
 jest.mock("helpers/formatAmount", () =>
   jest.requireActual("helpers/formatAmount"),
@@ -529,3 +536,162 @@ describe("SignTransactionDetails > Operations: hash-based signer keys", () => {
     ).toBeTruthy();
   });
 });
+
+describe("SignTransactionDetails > Operations: contract argument labels", () => {
+  const CONTRACT = "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4";
+  // gauge_schedule_reward(router, distributor, gauge, start_at: Option<u64>,
+  // duration, tps) as `Spec.jsonSchema()` emits it: `required` omits the
+  // Option, so indexing it positionally slid every later label up one row.
+  const GAUGE_SPEC = {
+    definitions: {
+      gauge_schedule_reward: {
+        properties: {
+          args: {
+            properties: {
+              router: {},
+              distributor: {},
+              gauge: {},
+              start_at: {},
+              duration: {},
+              tps: {},
+            },
+            required: ["router", "distributor", "gauge", "duration", "tps"],
+          },
+        },
+      },
+    },
+  };
+  const ALL_OPTIONAL_SPEC = {
+    definitions: {
+      maybe: {
+        properties: { args: { properties: { first: {}, second: {} } } },
+      },
+    },
+  };
+
+  const invokeContract = (fnName: string, args: xdr.ScVal[]) =>
+    operationsFor(
+      Operation.invokeContractFunction({
+        contract: CONTRACT,
+        function: fnName,
+        args,
+      }),
+    );
+
+  const getContractSpecsMock = getContractSpecs as jest.MockedFunction<
+    typeof getContractSpecs
+  >;
+
+  beforeEach(() => {
+    getContractSpecsMock.mockReset();
+  });
+
+  it("keeps every label on its own value when a middle parameter is optional", async () => {
+    const START_AT = "1750000000";
+    const DURATION = "604800";
+    const TPS = "42";
+    getContractSpecsMock.mockResolvedValue(GAUGE_SPEC);
+
+    const { findAllByTestId, getAllByTestId, getByTestId } = render(
+      <Operations
+        operations={invokeContract("gauge_schedule_reward", [
+          new Address(CONTRACT).toScVal(),
+          new Address(SOURCE).toScVal(),
+          new Address(CONTRACT).toScVal(),
+          nativeToScVal(BigInt(START_AT), { type: "u64" }),
+          nativeToScVal(BigInt(DURATION), { type: "u64" }),
+          nativeToScVal(BigInt(TPS), { type: "i128" }),
+        ])}
+      />,
+    );
+
+    await findAllByTestId("ParameterKey", {}, FIND);
+    const keys = getAllByTestId("ParameterKey").map(
+      (node) => node.props.children,
+    );
+    const values = getAllByTestId("ParameterValue").map(
+      (node) => node.props.children,
+    );
+
+    expect(keys).toEqual([
+      "router",
+      "distributor",
+      "gauge",
+      "start_at",
+      "duration",
+      "tps",
+    ]);
+    // The timestamp must sit under start_at, not under duration.
+    expect(values[3]).toBe(START_AT);
+    expect(values[4]).toBe(DURATION);
+    expect(values[5]).toBe(TPS);
+
+    // Names came from the spec, so they are qualified as the contract's own
+    // claim rather than presented as verified.
+    expect(getByTestId("ContractSpecNote")).toBeTruthy();
+  });
+
+  it("labels a function whose parameters are all optional instead of throwing", async () => {
+    // `required` is absent entirely here -- reading it positionally threw and
+    // took the whole signing view down with it.
+    getContractSpecsMock.mockResolvedValue(ALL_OPTIONAL_SPEC);
+
+    const { findAllByTestId, getAllByTestId } = render(
+      <Operations
+        operations={invokeContract("maybe", [
+          nativeToScVal(1, { type: "u32" }),
+          nativeToScVal(2, { type: "u32" }),
+        ])}
+      />,
+    );
+
+    await findAllByTestId("ParameterKey", {}, FIND);
+    expect(
+      getAllByTestId("ParameterKey").map((node) => node.props.children),
+    ).toEqual(["first", "second"]);
+  });
+
+  it("renders rows unlabelled, and no spec note, when the spec is unavailable", async () => {
+    getContractSpecsMock.mockRejectedValue(new Error("no spec"));
+
+    const { findAllByTestId, getAllByTestId, queryByTestId } = render(
+      <Operations
+        operations={invokeContract("transfer", [
+          new Address(SOURCE).toScVal(),
+          new Address(CONTRACT).toScVal(),
+          nativeToScVal(BigInt(100), { type: "i128" }),
+        ])}
+      />,
+    );
+
+    await findAllByTestId("ParameterValue", {}, FIND);
+    const keys = getAllByTestId("ParameterKey");
+
+    expect(keys).toHaveLength(3);
+    // No spec means no trustworthy names, so rows render unlabelled rather
+    // than borrowing a label from somewhere else.
+    keys.forEach((node) => expect(node.props.children).toBeUndefined());
+    // Nothing was labelled, so there is no claim to disclaim.
+    expect(queryByTestId("ContractSpecNote")).toBeNull();
+  });
+
+  it("renders rows unlabelled when the spec names a different number of parameters", async () => {
+    getContractSpecsMock.mockResolvedValue(GAUGE_SPEC);
+
+    const { findAllByTestId, getAllByTestId, queryByTestId } = render(
+      <Operations
+        operations={invokeContract("gauge_schedule_reward", [
+          new Address(CONTRACT).toScVal(),
+          new Address(SOURCE).toScVal(),
+        ])}
+      />,
+    );
+
+    await findAllByTestId("ParameterValue", {}, FIND);
+
+    getAllByTestId("ParameterKey").forEach((node) =>
+      expect(node.props.children).toBeUndefined(),
+    );
+    expect(queryByTestId("ContractSpecNote")).toBeNull();
+  });
+});