Skip to content

feat: expose generator hash in REST /api/orders/by-owner response (COW-993)#82

Open
lgahdl wants to merge 4 commits into
developfrom
luizhatem/cow-993-f15-rest-apiordersby-owner-response-missing-on-chain
Open

feat: expose generator hash in REST /api/orders/by-owner response (COW-993)#82
lgahdl wants to merge 4 commits into
developfrom
luizhatem/cow-993-f15-rest-apiordersby-owner-response-missing-on-chain

Conversation

@lgahdl
Copy link
Copy Markdown
Contributor

@lgahdl lgahdl commented Jun 1, 2026

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

  1. pnpm typecheck — no errors
  2. pnpm test — all tests pass
  3. 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
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

COW-993

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lgahdl lgahdl changed the base branch from main to develop June 2, 2026 13:52
@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

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
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

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.

lgahdl and others added 2 commits June 2, 2026 17:05
…COW-993)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +28 to +36
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");
}
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
});

it("fails parse when hash is not a string (number supplied)", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
});

it("hash field carries the correct describe() text", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +55 to +74
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'", () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants