feat: expose generator hash in REST /api/orders/by-owner response (COW-993) by lgahdl · Pull Request #82 · bleu/cow-programmatic-orders-api

lgahdl · 2026-06-01T18:40:48Z

Grant Review Finding

[F15] REST /api/orders/by-owner response missing on-chain canonical hash field — MINOR (API ergonomics) (COW-993)

/api/orders/by-owner/{owner} response embeds a generator object without hash. hash = keccak256(abi.encode(handler, salt, staticInput)) is the on-chain canonical identifier used by ComposableCow.singleOrders(owner, hash) and remove(owner, hash). An integrator who has the hash from an on-chain transaction can't look it up via REST — they're forced into GraphQL. hash is already indexed in the schema (hashIdx on schema/tables.ts:84) — this is a response-shape addition only.

Summary

Adds the hash field to the generator object in the /api/orders/by-owner/{owner} REST response. hash = keccak256(abi.encode(handler, salt, staticInput)) is the on-chain canonical identifier used by ComposableCow.singleOrders(owner, hash) and remove(owner, hash). Without it, integrators who have a hash from an on-chain transaction were forced to use GraphQL to look it up.

The field is already stored and indexed in the DB (hashIdx on schema/tables.ts:84) — this is a response-shape addition only, no schema migration required.

Changes

src/api/schemas/orders-by-owner.ts — add hash: z.string() to GeneratorSummary with an OpenAPI description
src/api/endpoints/orders-by-owner.ts — include hash in the generator .select() projection

How to Test

pnpm typecheck — no errors
pnpm test — all tests pass
After COW-995 is merged, the .todo("includes hash in generator object (COW-993)") test in tests/api/orders-by-owner.test.ts can be promoted to an active assertion

Checklist

Tests pass locally
Linting passes
Documentation updated (if needed)
Breaking changes documented (if any)

Breaking Changes

None — additive field to an existing response object.

Related Issues

Closes COW-993

…W-993) hash = keccak256(abi.encode(handler, salt, staticInput)) is the on-chain canonical identifier used by ComposableCow.singleOrders() and remove(). It was already indexed in the schema but missing from the REST response, forcing integrators to use GraphQL to look up an order by hash. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

linear-code · 2026-06-01T18:40:53Z

COW-993

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

lgahdl · 2026-06-02T19:47:50Z

The change looks correct — hash is selected from the DB and surfaced through the Zod schema in one consistent pass.

One small wording note on the description string:

"On-chain canonical identifier: keccak256(abi.encode(handler, salt, staticInput)). Used by ComposableCow.singleOrders(owner, hash) and remove(owner, hash)."

The ABI encoding isn't quite right. The ConditionalOrder library computes the hash as keccak256(abi.encode(handler, salt, staticInput)) where those three fields are packed as a struct, but the contract function is ComposableCow.hash(IConditionalOrder.ConditionalOrderParams) — so the description would be more precise as:

keccak256(abi.encode(ConditionalOrderParams { handler, salt, staticInput })) — the value returned by ComposableCow.hash(params) and used as the key in singleOrders(owner, hash) and remove(owner, hash).

Minor, but since this is a public API surface the description is what consumers will rely on. Otherwise the PR is fine.

lgahdl · 2026-06-02T19:56:57Z

Review: Expose generator hash (COW-993)

Hash description accuracy

The Zod .describe() text says keccak256(abi.encode(handler, salt, staticInput)). Looking at composableCow.ts:110-124, the actual computation is keccak256(encodeAbiParameters([{ type: "tuple", components: [handler, salt, staticInput] }], [{ handler, salt, staticInput }])) — i.e. the inputs are wrapped in a tuple. The description omits the tuple wrapping. The functional meaning is the same (it is still the canonical composable-order hash used by ComposableCoW), but strictly the encoding is abi.encode((handler, salt, staticInput)) not abi.encode(handler, salt, staticInput) — the former is a single tuple argument, the latter is three separate arguments. Recommend updating the description to keccak256(abi.encode((handler, salt, staticInput))) (with the inner parens indicating the tuple) to match the on-chain Solidity ABI exactly.

Response type consistency

The DB column is t.hex().notNull(), so the field is always present. z.string() (non-nullable) is correct — no type inconsistency.

No test changes

The PR correctly notes that the .todo("includes hash in generator object (COW-993)") test in orders-by-owner.test.ts will be promoted after this merges. The active schema tests (added in PR #79) already cover hash being required, non-string rejection, and correct .description text. No gaps.

DB query correctness

schema.conditionalOrderGenerator.hash is a real column (hashIdx at line 85 of schema/tables.ts). The .select() projection addition is correct.

Minor nit on the description wording aside, this PR is clean.

…COW-993) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

yvesfracari · 2026-06-03T17:58:12Z

+  it("fails parse when hash is missing", () => {
+    const { hash: _omitted, ...withoutHash } = validGenerator;
+    const result = GeneratorSummary.safeParse(withoutHash);
+    expect(result.success).toBe(false);
+    if (!result.success) {
+      const paths = result.error.issues.map((i) => i.path.join("."));
+      expect(paths).toContain("hash");
+    }
+  });


is it possible has be missing? isn't it required on the schema?

Kept this one — safeParse accepts unknown, so TypeScript has no protection against missing fields at runtime; the test is the only guard here.

It is a really direct test of the schema, but I am ok on leaving if you think that would be better as well

yvesfracari · 2026-06-03T17:58:36Z

+    }
+  });
+
+  it("fails parse when hash is not a string (number supplied)", () => {


do we need test for it? why ts compiler wouldn't catch it

Agreed, removed.

yvesfracari · 2026-06-03T18:02:59Z

+    }
+  });
+
+  it("hash field carries the correct describe() text", () => {


why do you think this test is useful? If we change the description on the future we would need to update this test as well. Not sure how it would help this app to be easier to maintain

Agreed, removed.

yvesfracari · 2026-06-03T18:04:22Z

+  it("ownerAddressType accepts null (regression guard for unchanged field)", () => {
+    const result = GeneratorSummary.safeParse({ ...validGenerator, ownerAddressType: null });
+    expect(result.success).toBe(true);
+    if (result.success) {
+      expect(result.data.ownerAddressType).toBeNull();
+    }
+  });
+
+  it("ownerAddressType accepts the enum value 'cowshed_proxy'", () => {
+    const result = GeneratorSummary.safeParse({
+      ...validGenerator,
+      ownerAddressType: "cowshed_proxy",
+    });
+    expect(result.success).toBe(true);
+    if (result.success) {
+      expect(result.data.ownerAddressType).toBe("cowshed_proxy");
+    }
+  });
+
+  it("ownerAddressType accepts the enum value 'flash_loan_helper'", () => {


This three tests are also things that TS types should catch and we shouldn't need tests for it

Removed the enum value tests, kept the null one since ownerAddressType is nullable and null is the common case for non-proxy generators.

…w.hash(params) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Remove tests that duplicate TypeScript's static coverage (type check, describe text, enum values). Keep only the two runtime regression guards that safeParse-unknown cannot catch at compile time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

yvesfracari · 2026-06-05T13:11:53Z

+  it("fails parse when hash is missing", () => {
+    const { hash: _omitted, ...withoutHash } = validGenerator;
+    const result = GeneratorSummary.safeParse(withoutHash);
+    expect(result.success).toBe(false);
+    if (!result.success) {
+      const paths = result.error.issues.map((i) => i.path.join("."));
+      expect(paths).toContain("hash");
+    }
+  });


It is a really direct test of the schema, but I am ok on leaving if you think that would be better as well

Combines develop's ordersByOwnerHandler integration tests (mocked db.select) with HEAD's GeneratorSummary and OrdersByOwnerResponse schema regression guards. The "returns enriched orders" test gains an explicit gen["hash"] assertion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test: add GeneratorSummary schema tests including hash field (COW-993)

8804df3

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

lgahdl changed the base branch from main to develop June 2, 2026 13:52

lgahdl mentioned this pull request Jun 2, 2026

test: add pnpm test to CI and expand test coverage (COW-995) #79

Merged

4 tasks

lgahdl and others added 2 commits June 2, 2026 17:05

fix: correct hash field describe() to use tuple abi.encode notation (…

4594e5f

…COW-993) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix: add non-null assertion on orders[0] in test to fix TS2532

bb80e89

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

$yvesfracari$

yvesfracari reviewed Jun 3, 2026

View reviewed changes

lgahdl and others added 2 commits June 4, 2026 10:47

fix: improve hash field description accuracy — reference ComposableCo…

a418d7b

…w.hash(params) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

lgahdl requested a review from yvesfracari June 4, 2026 21:58

$yvesfracari$

yvesfracari approved these changes Jun 5, 2026

View reviewed changes

lgahdl merged commit 3d100ee into develop Jun 8, 2026
4 checks passed

Conversation

lgahdl commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Grant Review Finding

Summary

Changes

How to Test

Checklist

Breaking Changes

Related Issues

Uh oh!

linear-code Bot commented Jun 1, 2026

Uh oh!

lgahdl commented Jun 2, 2026

Uh oh!

lgahdl commented Jun 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lgahdl commented Jun 1, 2026 •

edited

Loading