fix(sdk): Allow calculateAtomId to accept string | Hex for better DX

[jeremie-olivier]

Problem

The calculateAtomId function currently has a restrictive type signature that only accepts Hex, but this creates a poor developer experience where documentation and expected usage patterns don't match the actual types.

Evidence of the Issue

1. Documentation mismatch: Official docs show examples using strings directly:

   const termId = calculateAtomId("completed");

2. Developer pain point: Real user experiencing this DX issue in Discord:

   "it is not the returned value that is the issue, it the value passed into the calculateAtomId. on the docs the code is const termId = calculateAtomId("completed"); but when i do the same thing in code, i get a lint error that an 0xstring(address) is what should be passed instead of a regular string"

   Discord thread

3. Comprehensive testing: Demo repository proving that both string and hex inputs work identically at runtime but TypeScript types are restrictive.

Solution

Update the type signature from:

function calculateAtomId(atomData: Hex): string

To:

function calculateAtomId(atomData: string | Hex): string

Implementation Details

• Type safe: Uses explicit conversion with stringToHex() rather than type assertions
• Performance optimized: Only converts strings when needed via typeof check
• Backward compatible: Existing hex usage continues to work unchanged
• Clear intent: Comments explain the conversion logic

export function calculateAtomId(atomData: string | Hex) {
  const salt = keccak256(toHex('ATOM_SALT'))
  // Convert to hex if it's a string, ensuring safe input to keccak256
  const hexData = typeof atomData === 'string' ? stringToHex(atomData) : atomData
  return keccak256(
    encodePacked(['bytes32', 'bytes'], [salt, keccak256(hexData)]),
  )
}

Benefits

• ✅ Types match documentation - No more confusion between docs and reality
• ✅ Better DX - Developers can use strings directly as shown in docs
• ✅ Backward compatible - Existing code with hex inputs continues to work
• ✅ Type safe - No dangerous type assertions, explicit conversion only
• ✅ Performance - Only converts when needed

Changes

• packages/sdk/src/utils/calculate-atom-id.ts: Updated type signature and implementation
• packages/protocol/tests/helpers/calculate.ts: Updated for consistency

Testing

• ✅ All existing tests pass
• ✅ Build succeeds
• ✅ Linting passes
• ✅ Type checking passes
• ✅ External demo validates both input formats work identically

This change aligns the SDK with its documentation and significantly improves developer experience while maintaining full backward compatibility and type safety.

[claude]

Code Review — fix(sdk): Allow calculateAtomId to accept string | Hex for better DX

Overview

This PR widens the calculateAtomId type signature from Hex to string | Hex to align the SDK with its documentation and improve DX. The motivation is solid and well-documented. However, there is a critical runtime bug that makes the implementation incorrect and actually breaks backward compatibility despite the PR's claims.

---

🚨 Critical Bug: typeof check is always true

File: packages/sdk/src/utils/calculate-atom-id.ts (and the identical helper)

const hexData =
  typeof atomData === 'string' ? stringToHex(atomData) : atomData

Hex in viem is the branded type `0x${string}` — it is a JavaScript string at runtime. TypeScript types are erased after compilation. This means typeof atomData === 'string' is always true, regardless of whether the caller passed a plain string or a hex-prefixed string.

Consequence: every input — including previously-valid Hex values like "0xdeadbeef" — goes through stringToHex(). This double-encodes hex strings, producing an entirely different hash from what the old code produced:

// Old code
calculateAtomId("0xdeadbeef" as Hex)
  → keccak256("0xdeadbeef")   ✅

// New code
calculateAtomId("0xdeadbeef")
  → keccak256(stringToHex("0xdeadbeef"))   // 0x307864656164626565 — wrong ❌

The hash changes silently — no error is thrown, but atom IDs computed by callers who were already passing Hex values now yield different results, corrupting on-chain lookups.

---

Suggested Fix

Distinguish plain strings from already-hex-encoded values by checking the 0x prefix:

export function calculateAtomId(atomData: string | Hex) {
  const salt = keccak256(toHex('ATOM_SALT'))
  const hexData = atomData.startsWith('0x')
    ? (atomData as Hex)
    : stringToHex(atomData)
  return keccak256(
    encodePacked(['bytes32', 'bytes'], [salt, keccak256(hexData)]),
  )
}

This correctly passes through existing hex inputs unchanged and only converts plain strings. The trade-off worth documenting: a literal plain string that happens to start with "0x" (unlikely but possible) will be treated as already-encoded hex.

Alternatively, if the intent is purely "users pass human-readable strings", simply always call stringToHex and drop the conditional entirely — then update the type to just string:

export function calculateAtomId(atomData: string) {
  const salt = keccak256(toHex('ATOM_SALT'))
  return keccak256(
    encodePacked(['bytes32', 'bytes'], [salt, keccak256(stringToHex(atomData))]),
  )
}

This would be a breaking change for hex callers, but it is honest and unambiguous.

---

Other Issues

No new tests added
The PR description says "All existing tests pass" but does not add a test for the new string input path. At minimum, a test case like:

it('produces the same hash for string and hex-encoded input', () => {
  const plain = calculateAtomId('completed')
  const hex   = calculateAtomId(stringToHex('completed'))
  expect(plain).toBe(hex)
})

…would have caught the bug described above.

Code duplication between packages/sdk and packages/protocol/tests/helpers
The test helper mirrors the SDK implementation. If the protocol helper is used only for test fixture parity, duplicating the bug there compounds the problem. If the helpers must stay in sync, consider importing from the SDK rather than reimplementing.

Misleading comment

// Convert to hex if it's a string, ensuring safe input to keccak256

The comment implies the branch is conditional, but as explained above it is always taken. Once fixed this comment should be updated to reflect the actual branching logic.

---

Summary

	Status
Motivation / DX intent	✅ Valid and well-argued
Type signature change	✅ Correct direction
Runtime implementation	❌ Always invokes stringToHex, including for existing Hex inputs
Backward compatibility	❌ Silently changes hashes for existing hex callers
New tests	❌ Missing

The fix is one line away from being correct — please update the typeof guard to a startsWith('0x') check (or decide on the "strings only" approach) and add a regression test before merging.

[claude]

Code Review

Overview

This PR fixes a real DX pain point: calculateAtomId only accepted Hex but the docs showed plain string usage, causing confusing TypeScript lint errors. The fix widens the type to string | Hex and uses isHex() to decide whether to call stringToHex() before hashing.

---

What's Good

• Correct implementation: isHex(atomData) is the right runtime discriminator. A Hex is a subtype of string, so a typeof check alone would always be 'string' and would incorrectly double-encode valid hex inputs — isHex() avoids that trap.
• Both files updated consistently: packages/sdk/src/utils/calculate-atom-id.ts and packages/protocol/tests/helpers/calculate.ts apply the same logic.
• Backward compatible: Existing callers passing a Hex value hit the isHex() branch and nothing changes for them.
• No type assertions: Explicit conversion with stringToHex() is safer than a cast.

---

Issues / Suggestions

1. PR description contains a subtly buggy code snippet

The description shows:

const hexData = typeof atomData === 'string' ? stringToHex(atomData) : atomData

Since Hex (\0x${string}`) is a subtype of string, typeof atomData === 'string'is **always true** — hex strings would be double-encoded. The actual diff correctly usesisHex()`. The description should be updated to avoid misleading future readers / contributors.

2. No new tests for plain-string input

The PR description says "all existing tests pass" but doesn't add tests for the new behaviour — i.e. that calculateAtomId("completed") and calculateAtomId(stringToHex("completed")) produce the same result. A test like this would lock in the semantics and prevent accidental regression:

it('produces the same id for a string and its hex equivalent', () => {
  expect(calculateAtomId('completed')).toBe(
    calculateAtomId(stringToHex('completed')),
  )
})

3. Missing changeset

This is a user-visible behaviour change in packages/sdk. A changeset file (via pnpm changeset) should accompany it so the release process can bump the SDK version and generate a changelog entry.

4. Minor: string | Hex simplifies to string

Because Hex (\0x${string}`) is assignable to string, the union string | Hexis equivalent tostring` at the type level. Keeping both is fine as documentation intent (signals "hex strings are explicitly supported"), but it's worth knowing it provides no additional type narrowing.

---

Summary

The core implementation is correct and addresses a legitimate DX gap. The main asks before merging are:

1. Add at least one unit test covering plain-string input.
2. Add a changeset for the SDK package.
3. (Optional but nice) Fix the typeof snippet in the PR description so it matches the actual implementation.

Overall a clean, well-motivated fix — just needs tests and a changeset.