fix(sdk): correct validateConditionContract preflight is a no-op + docs/CONDITIONS.md uses stale 3-arg signature

[sophiej-story]

Closes #95.

Fixes the two related bugs in #95: stale 3-arg condition signature in docs/CONDITIONS.md, and a validateConditionContract preflight that silently accepts any contract with bytecode (including ones that don't implement the function).

Summary of Changes

• docs/CONDITIONS.md — every signature (Expected Interface block, parameter table, narrative, and all four example contracts: OpenCondition, OwnerCondition, TokenGateCondition, MerkleCondition) now uses the canonical 4-arg form (uint32 uuid, bytes accessAuxData, bytes conditionData, address caller). Matches the ICDRReadCondition / ICDRWriteCondition interfaces in piplabs/story and the deployed LicenseReadCondition / OwnerWriteCondition on Aeneid.
• packages/sdk/src/uploader.ts — validateConditionContract now:
  • probes with the 4-arg ABI (correct selector — 0x8db3eb17 for read, 0x5645dbbf for write);
  • tightens the catch to require cause.raw !== "0x" on ContractFunctionRevertedError, so an empty-data dispatcher fallback revert (selector miss) is correctly classified invalid.

Notes on the implementation vs. the snippet in #95

Two empirical findings during implementation that the issue snippet doesn't account for — flagging here:

1. Dummy args need non-empty bytes, not "0x". The issue suggests args: [0, "0x", "0x", zeroAddr]. Probed against the canonical LicenseReadCondition on Aeneid, that produces a ContractFunctionRevertedError with cause.raw === "0x" — because the body's abi.decode(conditionData, (address, address)) reverts on truncated input via a low-level revert(0,0). That's indistinguishable from a selector miss. This PR passes 256-byte zero buffers for each bytes arg so the body decodes successfully and either returns a value or reverts with non-empty payload, keeping the empty-raw signal specific to dispatcher fallback. The issue's "known caveat" about bare revert() applies more broadly than written.

2. cause.raw is the populated field; cause.data is undefined. The issue's snippet uses cause.raw ?? cause.data. On the viem version in this repo, cause.data is undefined for ContractFunctionRevertedError — cause.raw is the only relevant field. Implementation just keys off cause.raw.

Test coverage

Three unit tests added in packages/sdk/__tests__/uploader.test.ts:

• allocate rejects when condition contract does not implement the check function — mocks cause.raw === "0x" (the dispatcher-fallback signal). This is the case the buggy preflight silently let through.
• allocate accepts when condition function body reverts with non-empty data — mocks a real Error("demo") revert payload (0x08c379a0...). Function exists → preflight passes.
• allocate preflight probes condition contracts with the 4-arg signature — deep-equals the ABI inputs to pin the canonical 4-arg shape and order. Catches any drift back to a 3-arg ABI or a permuted 4-arg form that would yield a different selector.

Existing tests preserved:

• All 9 prior __tests__/uploader.test.ts tests still pass (the file is now 12 tests).
• Integration tests asserting EOA rejection by default (uploader.test.ts "default policy rejects EOA…", security.test.ts SEC-06) continue to behave identically: the EOA path produces ContractFunctionZeroDataError, which falls through the new catch's name check to throw InvalidConditionContractError.
• Integration tests for the skipConditionValidation: true escape hatch are upstream of the preflight and unaffected.
• Suite total: 186 passed, 23 skipped (was 183 / 23).

Known limitation

A condition contract whose function body uses a bare revert() (no message, no custom error) produces empty revert data and would still be mis-classified as selector miss. Real contracts virtually always use require("...") or a custom error; flagging here per #95's note.

[jinn-agent]

The core fix is sound: the 4-arg ABI update matches the deployed interfaces, the sentinel-probe design correctly distinguishes real implementations from catch-all fallbacks, and the two-phase logic is well-covered by the new unit tests. A few edge-case observations below.

One logical gap worth noting: when the real-selector simulateContract call succeeds (no exception), the code still executes sentinelProbeCleanlyMisses. For a legitimate, well-behaved condition contract that happens to return true even for dummy args (zero uuid, 256-byte zero buffers), this adds a second RPC round-trip on every allocate. That is intentional per the PR description (guarding against catch-all fallbacks), but it means every valid allocate now pays two eth_call RTTs instead of one. Not a bug, but worth knowing for latency-sensitive callers.

No other high-confidence issues were found. The bytecode, ABI inputs, selector values, and error-classification branches all appear correct.

---

Review iteration 6 · Commit 0cf2cee · 2026-05-27T06:44:15Z

[yingyangxu2026]

This is tighter than the old implementation, but it still treats "non-empty revert data" as proof that checkReadCondition / checkWriteCondition exists. A contract that does not implement either function but has a fallback that reverts with Error(string) or a custom error will also produce non-empty revert data here and be accepted as valid. In other words, this no longer admits every bytecode address, but it still admits unrelated contracts whose fallback reverts with payload. Since the point of the preflight is to catch misconfigured condition addresses before allocate, this remains a real false-positive path and would benefit from a regression test at minimum.

[sophiej-story]

addressed in a7e698f — added a sentinel-selector probe so contracts with payload-reverting fallbacks (Diamond Diamond_FunctionDoesNotExist(), explicit revert-string fallbacks) are now rejected with a new reason: "ambiguous-fallback" instead of silently passing. Regression tests added.

[sophiej-story]

Re: review findings — iteration 1 follow-up (a7e698f):

Both findings now addressed:

• yingyangxu2026 (fallback-reverting-with-payload accepted as valid): added a sentinel-selector probe — a fabricated selector no condition contract implements. When the real call reverts with payload, the sentinel result distinguishes function-body revert (sentinel reverts empty / zero-data → accept) from a catch-all fallback (sentinel also non-empty, or returns OK → reject). Rejection now surfaces a new public reason: "ambiguous-fallback" on InvalidConditionContractError; the message points users at skipConditionValidation: true for the residual false-negative class (Diamond proxies with the facet, deliberate payload-reverting fallbacks).
• jinn (cause.raw !== "0x" silently true when raw undefined): tightened to also require typeof cause.raw === "string".

New regression tests added accordingly: undefined raw → selector-miss (the original #95 shape), ContractFunctionZeroDataError → selector-miss, real-payload + sentinel-payload → ambiguous-fallback, real-payload + sentinel-OK → ambiguous-fallback, and a check on the public reason field in errors.test.ts.

[yingyangxu2026]

I think there is still one important false-negative path left in validateConditionContract:

simulateContract() returns immediately as soon as the real-selector probe succeeds, so the new sentinel probe never runs for contracts whose fallback accepts arbitrary selectors and returns a value. In that shape, a contract that does not implement checkReadCondition / checkWriteCondition can still be accepted if its fallback swallows the call instead of reverting.

That means the new logic now covers:

• selector miss -> empty revert / zero-data
• payload-reverting fallback -> ambiguous-fallback

but it still does not cover:

• swallow-all fallback that returns success

I think the sentinel probe needs to run for the success path as well, not only for the ContractFunctionRevertedError + non-empty raw path. Otherwise this preflight is still bypassable by a permissive fallback dispatcher.

I also do not see a regression test for this case right now — the new tests cover the variant where the real selector reverts and the sentinel returns OK, but not the more dangerous variant where the real-selector call itself returns successfully via fallback.