Key rotation improvements

[corrideat]

This relates to okTurtles/group-income#2988

When debugging that issue, I found that sometimes key rotations for foreign keys (which should happen automatically) don't occur. This PR enhances how foreign key rotations are handled by:

1. Correctly evicting keys from pendingKeyRotations if a more recent key with the same name is rotated. This prevents lingering keys.
2. When a key rotation needs to be performed, use the most recent key by the same name instead of the literal key ID in pendingKeyRotations, which addresses key rotations.

---

This PR also adds foreignContractIDs to key shares state, which is used to know whom a key was shared with. That didn't exist before, but it now does and it works in non-adversarial settings. (†)

† The adversarial setting would be claiming that a key is shared with a contract but sending undecryptable or wrong information. (edited)

---

[taoeffect]

Prelim machine review, ran it through two:

Opus 4.5

Details

Critical Bug: Missing Null Check

In the chelonia/private/deleteOrRotateRevokedKeys function, there's a potential TypeError when accessing a key that may not exist in activeForeignKeyIds:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc

If pkrKey.foreignKey doesn't have a corresponding active key (e.g., because all keys with that foreign key have been revoked), activeKeyId will be undefined, making key undefined, and key.id will throw a TypeError.

Suggested fix:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!activeKeyId) return acc  // No active key exists for this foreign key
const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc

Potential Issue: Null Safety in deleteKeyHelper

The helper function assumes state._volatile and state._volatile.pendingKeyRevocations exist:

for (const id of allIdsForNames[name]) {
  if (has(state._volatile!.pendingKeyRevocations, id)) {
    delete state._volatile!.pendingKeyRevocations![id]
  }
}

While the call sites currently initialize these properties beforehand, the function would be more robust with a guard:

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  const allIdsForNames = Object.values(state._vm.authorizedKeys)
    .reduce<Record<string, string[]>>((acc, { id, name }) => {
      if (!acc[name]) {
        acc[name] = [id]
      } else {
        acc[name].push(id)
      }
      return acc
    }, Object.create(null))
  for (const keyId of keyIds) {
    const name = state._vm.authorizedKeys[keyId].name
    // Clear pending revocations for all keys with the same name
    // to handle key rotation scenarios where multiple keys exist
    for (const id of allIdsForNames[name]) {
      if (state._volatile?.pendingKeyRevocations && has(state._volatile.pendingKeyRevocations, id)) {
        delete state._volatile.pendingKeyRevocations[id]
      }
    }

    state._vm.authorizedKeys[keyId]._notAfterHeight = height
  }
}

Improvement: Type Annotation for Reduce Accumulator

The reduce accumulator in deleteKeyHelper lacks explicit typing:

.reduce((acc, { id, name }) => {
  // ...
}, Object.create(null))

Suggested improvement:

.reduce<Record<string, string[]>>((acc, { id, name }) => {
  // ...
}, Object.create(null))

Behavioral Change Clarification

The new deleteKeyHelper clears pending revocations for all keys with the same name, not just the specific key being deleted. The comment explains this is "to handle key rotation scenarios where multiple keys exist."

This is a significant behavioral change from the previous code which only cleared the specific keyId. Please confirm this broadened scope is intentional and won't cause unintended side effects when multiple keys share the same name but should be handled independently.

Gemini 3 Pro

Details

There are two critical bugs introduced in the chelonia/private/deleteOrRotateRevokedKeys selector. One causes a runtime crash (DoS), and the other causes logical errors where keys may fail to update if multiple local keys reference the same foreign key.

1. Critical: Runtime Crash (DoS) in deleteOrRotateRevokedKeys

In the reduce function within deleteOrRotateRevokedKeys, the code attempts to resolve an active key ID from a foreign key string. If the foreign key is not found in activeForeignKeyIds (which can happen if the key was deleted locally, or due to the overwrite bug detailed in point #2), activeKeyId becomes undefined.

Subsequently, accessing contractState._vm.authorizedKeys[undefined] returns undefined. Trying to access .id on that undefined result throws a TypeError, crashing the message processing.

Problematic Lines:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc

Recommended Fix:
You must check if activeKeyId (and consequently key) exists before accessing properties on it.

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
// FIX: Check if activeKeyId exists
if (!activeKeyId) return acc

const key = contractState._vm.authorizedKeys[activeKeyId]
// FIX: Ensure key lookup succeeded (sanity check)
if (!key) return acc 

if (affectedKeyIds.has(key.id)) return acc

2. Critical: Logic Error in activeForeignKeyIds Map Construction

The construction of activeForeignKeyIds uses Object.fromEntries with the foreign key URL as the key. If a contract has multiple local keys pointing to the same foreign key URL (e.g., different permissions or ring levels for the same foreign identity), Object.fromEntries will overwrite previous entries. Only the last processed key will be stored.

This results in the reduce loop failing to update/rotate the keys that were overwritten in the map.

Problematic Lines:

const activeForeignKeyIds = Object.fromEntries(
  Object.values(contractState._vm.authorizedKeys)
    .filter(({ foreignKey, _notAfterHeight }) =>
      foreignKey != null && _notAfterHeight == null
    )
    .map(({ foreignKey, id }) => [foreignKey!, id])
)

Recommended Fix:
Since deleteOrRotateRevokedKeys is trying to find the current active key that corresponds to a revoked key, and multiple keys might share a foreign key string, this logic is brittle.

However, to fix the immediate overwrite issue without refactoring the entire logic, you should map the Foreign Key string to an Array of IDs, and iterate over that array in the reducer.

Revised Code Suggestion (Fixing #1 and #2):

// 1. Create a map of ForeignKey -> Array<LocalKeyID>
const activeForeignKeyIds = Object.values(contractState._vm.authorizedKeys)
  .filter(({ foreignKey, _notAfterHeight }) =>
    foreignKey != null && _notAfterHeight == null
  )
  .reduce((acc, { foreignKey, id }) => {
    const fk = foreignKey!
    if (!acc[fk]) acc[fk] = []
    acc[fk].push(id)
    return acc
  }, Object.create(null) as Record<string, string[]>)

// ... inside the reducer ...

const [, keyUpdateSigningKeyId, keyUpdateArgs] = keysToUpdate.reduce(
  (acc, keyId) => {
    const pkrKey = contractState._vm?.authorizedKeys?.[keyId]
    if (!pkrKey || !pkrKey.foreignKey) return acc
    
    // 2. Retrieve the list of active IDs for this foreign key
    const activeKeyIds = activeForeignKeyIds[pkrKey.foreignKey]
    if (!activeKeyIds || activeKeyIds.length === 0) return acc

    // 3. Iterate over all potential active keys (handling the 1-to-many case)
    let currentAcc = acc
    
    for (const activeKeyId of activeKeyIds) {
      const key = contractState._vm.authorizedKeys[activeKeyId]
      if (!key) continue // Safety check

      // Prevent processing the same key twice
      if (affectedKeyIds.has(key.id)) continue

      // ... existing logic regarding ringLevel and finding signingKeyId ...
      // Note: You will need to adapt the accumulator logic here to handle 
      // potentially adding multiple updates in one pass of the reducer, 
      // or ensure the logic selects the specific correct key based on `name`.
      
      // OPTIMIZATION: Usually, a key rotation replaces a key with the SAME name.
      // It is safer to filter by name here:
      if (key.name !== pkrKey.name) continue

      // ... proceed with existing logic ...
    }
    return currentAcc
  },
  // ... initial value ...
)

3. Improvement: Type Safety in deleteKeyHelper

In deleteKeyHelper, you access state._volatile! using the non-null assertion operator. While the callers (OP_KEY_DEL, OP_KEY_UPDATE) currently ensure _volatile exists, relying on the caller for null safety in a helper function can lead to future bugs if the helper is reused elsewhere.

Problematic Lines:

if (has(state._volatile!.pendingKeyRevocations, id)) {
  delete state._volatile!.pendingKeyRevocations![id]
}

Improvement:
Use optional chaining.

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  // ... existing grouping logic ...

  for (const keyId of keyIds) {
    const key = state._vm.authorizedKeys[keyId]
    if (!key) continue // Safety check in case keyId is invalid

    const name = key.name
    
    // Safe access
    if (state._volatile?.pendingKeyRevocations) {
      for (const id of allIdsForNames[name] || []) {
        if (has(state._volatile.pendingKeyRevocations, id)) {
           delete state._volatile.pendingKeyRevocations[id]
        }
      }
    }

    state._vm.authorizedKeys[keyId]._notAfterHeight = height
  }
}

4. Logic Check: deleteKeyHelper grouping

In deleteKeyHelper:

const allIdsForNames = Object.values(state._vm.authorizedKeys)
// ...
for (const id of allIdsForNames[name]) {

If keyIds contains a key ID that does not exist in state._vm.authorizedKeys, the line const name = state._vm.authorizedKeys[keyId].name will throw.

While OP_KEY_DEL filters keys before calling this helper, OP_KEY_UPDATE passes keysToDelete derived from validateKeyUpdatePermissions. If that validation function returns an ID that isn't in authorizedKeys (unlikely, but possible during race conditions or state desyncs), this helper will crash.

Suggestion: Add a simple existence check inside the loop in deleteKeyHelper (as shown in the code block in point #3 above).

[corrideat]

This issue:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
// [suggested] FIX: Check if activeKeyId exists
// ...
const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc

Isn't really possible due to how activeForeignKeyIds is constructed.

Critical: Logic Error in activeForeignKeyIds Map Construction

It definitely is weird that a contract might have a foreign key defined as a key multiple time, and doing this will break authorizedKeys, which is keyed by key ID. Since things already break when this is attempted, then perhaps we shouldn't worry too much about it.

Potential Issue: Null Safety in deleteKeyHelper

While the call sites currently initialize these properties beforehand

Dismissing adding a guard since it's redundant (a DRY argument can be made for moving the _volatile logic, but, personally, I prefer it as it currently is since it's clear and explicit.

Type Annotation for Reduce Accumulator

This would IMHO be more verbose and confusing than the current implicit approach.

Improvement: Type Safety in deleteKeyHelper

[...] While the callers [...] currently ensure _volatile exists [...]

Dismissing

Logic Check: deleteKeyHelper grouping

This check is redundant because all keys are supposed to exist, and their non-existence means that there's a bug elsewhere.

[corrideat]

I've been testing these changes, and there still is one race condition that needs to be handled. The situation is more or less as follows:

Setup:

A contract with a foreign key that's out of sync (but that has been rotated at least once) and syncing from scratch.

Race condition:

1. Chelonia notices that the key needs to be rotated at pendingKeyRotations is set
2. Chelonia notices that the key has been updated and clears pendingKeyRotations. This step may be wrong if the key still is out of sync.

Solution:

pendingKeyRotations should not be cleared if the key still is out of sync. Alternatively, pendingKeyRotations can be cleared, but it must be set again.

(pendingKeyRotations is a dictionary; set and clear truly mean the corresponding key in the dictionary)

[taoeffect]

Ran latest changes through Opus 4.5:

Details

I've analyzed the changes and found several issues worth addressing:

1. Potential Runtime Error - Missing Null Check

In chelonia/private/deleteOrRotateRevokedKeys, if there's no active foreign key for a given foreignKey URL, the code will throw when accessing key.id:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc  // Will throw if key is undefined

Suggested fix:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!activeKeyId) return acc  // Add this check
const key = contractState._vm.authorizedKeys[activeKeyId]
if (!key || affectedKeyIds.has(key.id)) return acc

2. Same Issue in Key Deletion Loop

The same pattern exists in the keysToDelete.reduce block:

const [, keyDelSigningKeyId, keyIdsToDelete] = keysToDelete.reduce(
  (acc, pkrKeyId) => {
    const pkrKey = contractState._vm?.authorizedKeys?.[pkrKeyId]
    if (!pkrKey || !pkrKey.foreignKey) return acc
    const keyId = activeForeignKeyIds[pkrKey.foreignKey]
    if (affectedKeyIds.has(keyId)) return acc  // keyId could be undefined
    // ... later uses keyId without null check

Suggested fix:

const keyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!keyId || affectedKeyIds.has(keyId)) return acc

3. Good Improvement - reduce to some

The change from reduce with || to some is a good improvement - it's more idiomatic and short-circuits:

// Old:
!keys.reduce((acc, [, id]) => {
  return acc || has(externalContractState._vm.authorizedKeys, id)
}, false)

// New:
!keys.some(([, id]) => {
  return has(externalContractState._vm.authorizedKeys, id)
})

This change is correct and improves readability.

4. Inconsistent Formatting in Error Handler

In the OP_KEY_UPDATE handler, the console.error call has unusual indentation:

internalSideEffectStack?.push(() => {
  sbp('chelonia/private/queueEvent', contractID, [
    'chelonia/private/deleteOrRotateRevokedKeys',
    contractID
  ]).catch((e: unknown) => {
    console.error(
`Error at deleteOrRotateRevokedKeys for contractID ${contractID} at OP_KEY_UPDATE with ${hash}`,
e
    )
  })
})

Suggested fix:

internalSideEffectStack?.push(() => {
  sbp('chelonia/private/queueEvent', contractID, [
    'chelonia/private/deleteOrRotateRevokedKeys',
    contractID
  ]).catch((e: unknown) => {
    console.error(
      `Error at deleteOrRotateRevokedKeys for contractID ${contractID} at OP_KEY_UPDATE with ${hash}`,
      e
    )
  })
})

5. Type Safety Improvement for deleteKeyHelper

The deleteKeyHelper function uses type assertions with ! that could mask issues. Consider adding explicit runtime checks or better typing:

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  const allIdsForNames = Object.values(state._vm.authorizedKeys)
    .reduce((acc, { id, name }) => {
      // ...
    }, Object.create(null) as Record<string, string[]>)  // Add explicit type
  
  for (const keyId of keyIds) {
    const keyEntry = state._vm.authorizedKeys[keyId]
    if (!keyEntry) continue  // Defensive check
    const name = keyEntry.name
    // ...

Question

In deleteOrRotateRevokedKeys, the change from oldKeyId: keyId to oldKeyId: key.id changes which key ID is passed to chelonia/out/keyUpdate. Could you confirm this is intentional? The variable naming suggests oldKeyId should be the key being replaced, but now it's being set to the active key's ID rather than the pendingKeyRevocations entry's ID.

And Gemini 3:

Details

Here is a review of the provided changes to `internals.ts`.

There are critical stability bugs introduced in the deleteOrRotateRevokedKeys function that will cause the application to crash if a local key has been deleted but is still referenced in pendingKeyRevocations. There are also safety issues in the new helper function.

1. Critical Bug: Crash in deleteOrRotateRevokedKeys (Undefined Access)

In deleteOrRotateRevokedKeys, you create a map activeForeignKeyIds of currently active keys. However, inside the reduce loops for both keysToUpdate and keysToDelete, you attempt to access this map. If the local key corresponding to the foreign key has already been deleted (and thus is not in activeForeignKeyIds), the code will crash when attempting to access properties of undefined.

Problematic Lines:

Lines 2216-2217 (in keysToUpdate reduce):

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
const key = contractState._vm.authorizedKeys[activeKeyId] // CRASH: activeKeyId is undefined
if (affectedKeyIds.has(key.id)) return acc // CRASH: key is undefined

Lines 2331-2332 (in keysToDelete reduce):

const keyId = activeForeignKeyIds[pkrKey.foreignKey]
if (affectedKeyIds.has(keyId)) return acc // CRASH: keyId is undefined (Set.has allows undefined, but next lines fail)
// ...
const ringLevel = Math.min(
  currentRingLevel,
  contractState._vm?.authorizedKeys?.[keyId]?.ringLevel ?? Number.POSITIVE_INFINITY // Safe due to optional chaining, but logic is broken
)
if (ringLevel >= currentRingLevel) {
  affectedKeyIds.add(keyId) // Adds 'undefined' to set
  currentKeyIds.push(keyId) // Pushes 'undefined' to array -> Invalid OP_KEY_DEL

Fix:
You must check if activeKeyId exists before proceeding. If it does not exist, it implies the local key is already gone, so no rotation/deletion is needed for that specific entry.

// Inside keysToUpdate reduce
const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!activeKeyId) return acc // Fix: Skip if no active local key found

const key = contractState._vm.authorizedKeys[activeKeyId]
if (affectedKeyIds.has(key.id)) return acc

// Inside keysToDelete reduce
const keyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!keyId) return acc // Fix: Skip if no active local key found

if (affectedKeyIds.has(keyId)) return acc

2. Potential Bug: Unsafe Property Access in deleteKeyHelper

The new deleteKeyHelper function assumes state._volatile exists and has pendingKeyRevocations. While the call sites currently appear to initialize _volatile before calling this helper, relying on the caller for deep property existence in a helper function is brittle and can lead to runtime errors during future refactors.

Problematic Lines:
Lines 300-302:

if (has(state._volatile!.pendingKeyRevocations, id)) {
  delete state._volatile!.pendingKeyRevocations![id]
}

Fix:
Use optional chaining to ensure safety.

// In deleteKeyHelper
if (has(state._volatile?.pendingKeyRevocations, id)) {
  delete state._volatile!.pendingKeyRevocations![id]
}

3. Logic Issue: activeForeignKeyIds Collision

In deleteOrRotateRevokedKeys, you create a map of foreign keys to local IDs.

Problematic Lines:
Lines 2187-2191:

const activeForeignKeyIds = Object.fromEntries(
  Object.values(contractState._vm.authorizedKeys)
    .filter(({ foreignKey, _notAfterHeight }) =>
      foreignKey != null && _notAfterHeight == null
    )
    .map(({ foreignKey, id }) => [foreignKey!, id])
)

If the contract state is messy or complex and contains two active local keys pointing to the same foreignKey URL, Object.fromEntries will overwrite the previous entry. This results in only one of the local keys being rotated/deleted, leaving the other one in an inconsistent state.

Fix:
While a full fix requires mapping to an array of IDs, a simple guard to ensure we don't accidentally ignore keys is to iterate the keys directly in the reducer instead of using a lookup map, or accept that this edge case requires a more complex data structure. Given the complexity, simply adding a comment about this limitation or ensuring your application logic enforces a 1-to-1 mapping is recommended.

4. Code Simplification & Cleanup

The deleteKeyHelper builds a map of allIdsForNames by iterating over all authorized keys every time it is called. If deleteKeyHelper is called with a single key (common case), iterating the entire key set is inefficient ($O(N)$), though likely negligible for small contracts.

However, the logic inside deleteOrRotateRevokedKeys is heavily duplicated between the "Update" and "Delete" sections.

Refactoring Suggestion:
You can consolidate the logic for finding the activeKeyId and checking ringLevel.

Revised deleteOrRotateRevokedKeys (incorporating fixes):

'chelonia/private/deleteOrRotateRevokedKeys': function (
  this: CheloniaContext,
  contractID: string
) {
  const rootState = sbp(this.config.stateSelector) as ChelRootState
  const contractState = rootState[contractID]
  const pendingKeyRevocations = contractState?._volatile?.pendingKeyRevocations

  if (!pendingKeyRevocations || Object.keys(pendingKeyRevocations).length === 0) return

  // Map foreignKey -> active local key ID
  const activeForeignKeyIds = Object.fromEntries(
    Object.values(contractState._vm.authorizedKeys)
      .filter(({ foreignKey, _notAfterHeight }) =>
        foreignKey != null && _notAfterHeight == null
      )
      .map(({ foreignKey, id }) => [foreignKey!, id])
  )

  const affectedKeyIds = new Set<string>()

  // --- Helper to resolve active key from pending revocation ---
  const resolveActiveKey = (pkrKeyId: string) => {
    const pkrKey = contractState._vm?.authorizedKeys?.[pkrKeyId]
    if (!pkrKey || !pkrKey.foreignKey) return null
    
    const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
    if (!activeKeyId || affectedKeyIds.has(activeKeyId)) return null
    
    return { 
      activeKeyId, 
      activeKey: contractState._vm.authorizedKeys[activeKeyId],
      foreignKeyUrl: pkrKey.foreignKey
    }
  }

  // 1. Handle Rotations
  const keysToUpdate = Object.entries(pendingKeyRevocations)
    .filter(([, v]) => v === true)
    .map(([id]) => id)

  const [, keyUpdateSigningKeyId, keyUpdateArgs] = keysToUpdate.reduce(
    (acc, keyId) => {
      const resolved = resolveActiveKey(keyId)
      if (!resolved) return acc
      const { activeKeyId, activeKey, foreignKeyUrl } = resolved

      const fkUrl = new URL(foreignKeyUrl)
      const foreignContractID = fkUrl.pathname
      const foreignKeyName = fkUrl.searchParams.get('keyName')
      if (!foreignKeyName) throw new Error('Missing foreign key name')
      
      const foreignState = rootState[foreignContractID]
      if (!foreignState) return acc
      
      const fKeyId = findKeyIdByName(foreignState, foreignKeyName)
      
      // Handle cases where key is missing or already rotated in foreign state
      if (!fKeyId) {
        if (pendingKeyRevocations[keyId] === true) {
          this.config.reactiveSet(pendingKeyRevocations, keyId, 'del')
        }
        return acc
      } else if (fKeyId === activeKey.id) {
        this.config.reactiveDel(pendingKeyRevocations, keyId)
        return acc
      }

      const [currentRingLevel, currentSigningKeyId, currentKeyArgs] = acc
      const ringLevel = Math.min(currentRingLevel, activeKey.ringLevel ?? Number.POSITIVE_INFINITY)
      
      // Check if we can use current signer or need a new one
      if (ringLevel >= currentRingLevel) {
        affectedKeyIds.add(activeKeyId)
        currentKeyArgs.push({
          name: activeKey.name,
          oldKeyId: activeKeyId,
          id: fKeyId,
          data: foreignState._vm.authorizedKeys[fKeyId].data
        })
        return [currentRingLevel, currentSigningKeyId, currentKeyArgs]
      } else if (Number.isFinite(ringLevel)) {
        const signingKeyId = findSuitableSecretKeyId(
          contractState,
          [SPMessage.OP_KEY_UPDATE],
          ['sig'],
          ringLevel
        )
        if (signingKeyId) {
          affectedKeyIds.add(activeKeyId)
          currentKeyArgs.push({
            name: activeKey.name,
            oldKeyId: activeKeyId,
            id: fKeyId,
            data: foreignState._vm.authorizedKeys[fKeyId].data
          })
          return [ringLevel, signingKeyId, currentKeyArgs]
        }
      }
      return acc
    },
    [Number.POSITIVE_INFINITY, '', [] as any[]]
  )

  if (keyUpdateArgs.length !== 0) {
     // ... send OP_KEY_UPDATE ...
     sbp('chelonia/out/keyUpdate', {
        contractID,
        contractName: contractState._vm.type,
        data: keyUpdateArgs,
        signingKeyId: keyUpdateSigningKeyId
      }).catch((e: unknown) => console.error(`[chelonia] Error sending OP_KEY_UPDATE`, e))
  }

  // 2. Handle Deletions
  const keysToDelete = Object.entries(pendingKeyRevocations)
    .filter(([, v]) => v === 'del')
    .map(([id]) => id)

  const [, keyDelSigningKeyId, keyIdsToDelete] = keysToDelete.reduce(
    (acc, pkrKeyId) => {
      const resolved = resolveActiveKey(pkrKeyId)
      if (!resolved) return acc
      const { activeKeyId, activeKey } = resolved

      const [currentRingLevel, currentSigningKeyId, currentKeyIds] = acc
      const ringLevel = Math.min(
        currentRingLevel,
        activeKey.ringLevel ?? Number.POSITIVE_INFINITY
      )

      if (ringLevel >= currentRingLevel) {
        affectedKeyIds.add(activeKeyId)
        currentKeyIds.push(activeKeyId)
        return [currentRingLevel, currentSigningKeyId, currentKeyIds]
      } else if (Number.isFinite(ringLevel)) {
        const signingKeyId = findSuitableSecretKeyId(
          contractState,
          [SPMessage.OP_KEY_DEL],
          ['sig'],
          ringLevel
        )
        if (signingKeyId) {
          affectedKeyIds.add(activeKeyId)
          currentKeyIds.push(activeKeyId)
          return [ringLevel, signingKeyId, currentKeyIds]
        }
      }
      return acc
    },
    [Number.POSITIVE_INFINITY, '', [] as string[]]
  )

  if (keyIdsToDelete.length !== 0) {
      // ... send OP_KEY_DEL ...
      sbp('chelonia/out/keyDel', {
        contractID,
        contractName: contractState._vm.type,
        data: keyIdsToDelete,
        signingKeyId: keyDelSigningKeyId
      }).catch((e: unknown) => console.error(`[chelonia] Error sending OP_KEY_DEL`, e))
  }
},

GLM 4.7:

Details

I've reviewed the changes to the `internals.ts` file, focusing on bugs, security issues, and simplification opportunities. Here's my detailed feedback:

Critical Security Issues

1. Missing Validation in deleteKeyHelper

Location: Line 275-294 (defined in the changes)

Issue: The function doesn't validate that the key IDs exist before accessing them, which could lead to runtime errors or potential security exploits.

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  const allIdsForNames = Object.values(state._vm.authorizedKeys)
    .reduce((acc, { id, name }) => {
      if (!acc[name]) {
        acc[name] = [id]
      } else {
        acc[name].push(id)
      }
      return acc
    }, Object.create(null))
  for (const keyId of keyIds) {
    const name = state._vm.authorizedKeys[keyId].name  // BUG: No validation
    // Clear pending revocations for all keys with the same name
    // to handle key rotation scenarios where multiple keys exist
    for (const id of allIdsForNames[name]) {
      if (has(state._volatile!.pendingKeyRevocations, id)) {
        delete state._volatile!.pendingKeyRevocations![id]
      }
    }

    state._vm.authorizedKeys[keyId]._notAfterHeight = height  // BUG: No validation
  }
}

Suggested Fix:

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  const allIdsForNames = Object.values(state._vm.authorizedKeys)
    .reduce((acc, { id, name }) => {
      if (!acc[name]) {
        acc[name] = [id]
      } else {
        acc[name].push(id)
      }
      return acc
    }, Object.create(null))
  
  for (const keyId of keyIds) {
    // Validate that the key exists
    if (!has(state._vm.authorizedKeys, keyId)) {
      console.warn('Attempted to delete non-existent key', { keyId })
      continue
    }
    
    const name = state._vm.authorizedKeys[keyId].name
    // Clear pending revocations for all keys with the same name
    // to handle key rotation scenarios where multiple keys exist
    for (const id of allIdsForNames[name]) {
      if (has(state._volatile!.pendingKeyRevocations, id)) {
        delete state._volatile!.pendingKeyRevocations![id]
      }
    }

    state._vm.authorizedKeys[keyId]._notAfterHeight = height
  }
}

2. Unhandled URL Parsing Error

Location: Lines 1731-1732 in the updated code (SPMessage.OP_KEY_UPDATE)

Issue: The code attempts to parse key.foreignKey as a URL without proper error handling, which could throw an exception if the foreign key string is malformed.

const fkUrl = new URL(key.foreignKey!)
const foreignContractID = fkUrl.pathname

Suggested Fix:

let fkUrl: URL
try {
  fkUrl = new URL(key.foreignKey!)
} catch (e) {
  console.error('Invalid foreign key URL format', { foreignKey: key.foreignKey })
  continue // Skip this key if the URL is invalid
}
const foreignContractID = fkUrl.pathname

Potential Bugs

3. Duplicate Foreign Keys Handling

Location: Lines 2208-2214 in chelonia/private/deleteOrRotateRevokedKeys

Issue: When creating the activeForeignKeyIds map, if multiple keys have the same foreignKey value, only the last one will be kept due to Object.fromEntries behavior. This might lead to incorrect key handling.

const activeForeignKeyIds = Object.fromEntries(
  Object.values(contractState._vm.authorizedKeys)
    .filter(({ foreignKey, _notAfterHeight }) =>
      foreignKey != null && _notAfterHeight == null
    )
    .map(({ foreignKey, id }) => [foreignKey!, id])  // Potential data loss with duplicates
)

Suggested Fix: Add validation to ensure uniqueness or use a more explicit approach

// Option 1: Check for duplicates and log warnings
const activeForeignKeyIds = Object.fromEntries(
  Object.values(contractState._vm.authorizedKeys)
    .filter(({ foreignKey, _notAfterHeight }) =>
      foreignKey != null && _notAfterHeight == null
    )
    .map(({ foreignKey, id }) => {
      if (foreignKey && Object.keys(contractState._vm.authorizedKeys).filter(k => 
        contractState._vm.authorizedKeys[k].foreignKey === foreignKey && k !== id
      ).length > 0) {
        console.warn('Duplicate foreign key detected. This may cause unexpected behavior.', { foreignKey })
      }
      return [foreignKey!, id]
    })
)

4. Foreign Contract State Not Safely Accessed

Location: Lines 1736-1748 in SPMessage.OP_KEY_UPDATE

Issue: The code assumes foreignState exists after calling sbp('chelonia/contract/state', foreignContractID). If the contract hasn't been synced or doesn't exist, this could lead to issues.

const foreignState = sbp('chelonia/contract/state', foreignContractID)
if (foreignState) {  // Good that they check for existence
  const fKeyId = findKeyIdByName(foreignState, foreignKeyName)
  // ...
}

Potential Fix: The check is already there, but consider adding more defensive handling for cases where the contract might not be synced.

Improvements & Simplifications

1. Reduce Code Duplication

Location: Lines 275-294

Observation: The deleteKeyHelper function is good but could be improved to reduce code duplication.

Suggestion: Extract the name-based pending revocation clearing into a separate helper function:

const clearPendingKeyRevocationsByName = (state: ChelContractState, name: string) => {
  const allIdsForName = Object.values(state._vm.authorizedKeys)
    .filter(key => key.name === name)
    .map(key => key.id)
    
  for (const id of allIdsForName) {
    if (has(state._volatile!.pendingKeyRevocations, id)) {
      delete state._volatile!.pendingKeyRevocations![id]
    }
  }
}

const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  for (const keyId of keyIds) {
    // Validate that the key exists
    if (!has(state._vm.authorizedKeys, keyId)) {
      console.warn('Attempted to delete non-existent key', { keyId })
      continue
    }
    
    const name = state._vm.authorizedKeys[keyId].name
    clearPendingKeyRevocationsByName(state, name)
    state._vm.authorizedKeys[keyId]._notAfterHeight = height
  }
}

2. Simplify Key Rotation Logic

Location: Lines 1698-1765 in SPMessage.OP_KEY_UPDATE

Observation: The foreign key handling logic is quite complex and could benefit from being extracted into a separate function.

Suggestion:

const handleForeignKeys = (
  state: ChelContractState,
  updatedKeys: SPKey[],
  height: number,
  internalSideEffectStack?: (({state, message}: {state: ChelContractState, message: SPMessage}) => void)[]
) => {
  let canMirrorOperationsUpToRingLevel = NaN
  let hasOutOfSyncKeys = false
  
  for (const key of updatedKeys) {
    if (key.foreignKey == null) continue
    
    // Check ring level limits
    if (!(key.ringLevel >= canMirrorOperationsUpToRingLevel)) {
      const signingKey = findSuitableSecretKeyId(
        state,
        [SPMessage.OP_KEY_DEL],
        ['sig'],
        key.ringLevel
      )
      if (signingKey) {
        canMirrorOperationsUpToRingLevel = key.ringLevel
      }
    }
    
    // Handle foreign key state checking
    let fkUrl: URL
    try {
      fkUrl = new URL(key.foreignKey!)
    } catch (e) {
      console.error('Invalid foreign key URL format', { foreignKey: key.foreignKey })
      continue
    }
    
    const foreignContractID = fkUrl.pathname
    const foreignKeyName = fkUrl.searchParams.get('keyName')
    
    if (!foreignKeyName) {
      console.warn('Missing foreign key name', { foreignKey: key.foreignKey })
      continue
    }
    
    const foreignState = sbp('chelonia/contract/state', foreignContractID)
    if (!foreignState) continue
    
    const fKeyId = findKeyIdByName(foreignState, foreignKeyName)
    if (!fKeyId) {
      // Key was deleted; mark it for deletion
      state._volatile!.pendingKeyRevocations![key.id] = 'del'
      hasOutOfSyncKeys = true
    } else if (fKeyId !== key.id) {
      // Key still needs to be rotated
      state._volatile!.pendingKeyRevocations![key.id] = true
      hasOutOfSyncKeys = true
    }
  }
  
  return { canMirrorOperationsUpToRingLevel, hasOutOfSyncKeys }
}

Then in the main OP_KEY_UPDATE handler:

// ... initialization code
deleteKeyHelper(state, height, keysToDelete)

// Handle foreign keys
const { canMirrorOperationsUpToRingLevel, hasOutOfSyncKeys } = handleForeignKeys(
  state, updatedKeys, height, internalSideEffectStack
)

// ... rest of the code with canMirrorOperationsUpToRingLevel and hasOutOfSyncKeys

3. Add JSDoc to Helper Functions

Location: Lines 275-294

Observation: The new deleteKeyHelper function would benefit from documentation explaining its purpose and parameters.

Suggestion:

/**
 * Helper function to delete keys from the state and clear related pending revocations.
 * Handles key rotation scenarios by clearing pending revocations for all keys with the same name.
 * 
 * @param state - The contract state to modify
 * @param height - The height at which the keys should be marked as deleted
 * @param keyIds - Array of key IDs to delete
 */
const deleteKeyHelper = (state: ChelContractState, height: number, keyIds: string[]) => {
  // ... implementation
}

Questions

1. Are there any tests covering the new deleteKeyHelper function, particularly the edge cases where invalid key IDs are passed?

2. For the foreign key URL handling, is there a defined format that key.foreignKey should follow? It would be good to document or validate this format.

3. In SPMessage.OP_KEY_UPDATE, what happens if the foreign contract state is not synced yet? Is there a mechanism to ensure it's synced before processing?

These changes overall improve the code organization by extracting common functionality into helper functions, but the security concerns about validation should be addressed before merging.

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

I'll analyze this PR for bugs, security issues, and potential improvements. Let me start by examining some key areas.## Current State

• Task: Code review of a large PR diff for a JavaScript/TypeScript project (Chelonia - a contract/message handling library)
• PR Description: Fixes automatic foreign key rotations, ensures correct eviction of keys from pendingKeyRotations, prioritizes most recent key by name, and adds foreignContractIDs to key shares state
• Progress: Partially complete - initial context gathering done, code review in progress
• Incomplete: Full analysis of all changes, identifying all bugs/security issues, providing complete feedback

Files & Changes

The PR modifies multiple files:

File	Changes Summary
src/SPMessage.ts	New type definitions for V1/V2 key request formats, updated message parsing logic
src/chelonia.ts	New withRetained function, updated keyRequest logic, setPendingKeyRevocation changes
src/db.ts	Added saveMessageMetadata config check
src/internals.ts	New deleteKeyHelper function, updated hook naming (_postOpHook), key rotation logic changes
src/presets.ts	Added saveMessageMetadata: true to SERVER preset
src/signedData.ts	New RawSignedData type, updated SignedDataContext type

Key areas examined so far:

• src/internals.ts:295-330 - New deleteKeyHelper function
• src/internals.ts:1790-1825 - OP_KEY_UPDATE handler changes
• src/utils.ts:46-52 - findKeyIdByName function (returns FIRST matching key via .find())

Technical Context

• Framework: SBP (Selector-Based Programming) pattern with @sbp/sbp library
• Crypto: @chelonia/crypto for key operations
• Utilities: turtledash library for has, cloneDeep, etc.
• Pattern: Message processing with operations (OP_KEY_ADD, OP_KEY_DEL, OP_KEY_UPDATE, etc.)

Key behaviors discovered:

• has(undefined, key) returns false (doesn't throw) - safe
• findKeyIdByName uses .find() returning FIRST match, not most recent by name
• Hook naming changed from .../hook/${opType} to .../_postOpHook/${opType}
• saveMessageMetadata config gates storing _private_hidx= entries

Strategy & Approach

1. Used agent tool to understand key function behaviors
2. Examined findKeyIdByName to understand key selection logic
3. Verified hook naming patterns for backward compatibility
4. Checked has() function behavior with undefined arguments
5. Reviewed deleteKeyHelper implementation

Potential concerns identified but not yet documented as issues:

• findKeyIdByName returns first key with matching name, not most recent - may conflict with PR goal of "prioritizing most recent key by name"
• Hook naming change (/hook/ → /_postOpHook/) could break existing contracts

Exact Next Steps

1. Complete analysis of deleteKeyHelper for edge cases (empty arrays, missing keys)
2. Review deleteOrRotateRevokedKeys changes around lines 2306-2450 for the key rotation logic fix
3. Examine the activeForeignKeyIds mapping logic for correctness
4. Check respondToKeyRequest for race condition handling around the processing flag
5. Verify all places where pendingKeyRevocations is read/written for consistency
6. Review foreignContractIDs tracking in OP_KEY_SHARE handler
7. Check for proper validation of skipInviteAccounting between request and response
8. Output complete code review with numbered issues, ratings, and checkboxesI'll continue the code review from where it was interrupted. Let me examine the remaining areas of concern.## Current State

• Task: Complete code review of a large PR diff for Chelonia, a JavaScript/TypeScript contract/message handling library using SBP (Selector-Based Programming) pattern
• PR Description: "Fixes an issue where automatic foreign key rotations failed to occur. It ensures correct eviction of keys from pendingKeyRotations and prioritizes the most recent key by name. Additionally, it adds foreignContractIDs to key shares state to track key recipients."
• Progress: Initial context gathering done, partial code review in progress - I have examined the main changed files but have not yet output the formal review with numbered issues
• Incomplete: Full analysis of all changes, identifying all bugs/security issues, providing complete feedback with numbered issues, ratings, and checkboxes

Files & Changes

Modified files in the PR:

File	Key Changes
src/SPMessage.ts:70-127	New type definitions: ProtoSPOpKeyRequestV1, ProtoSPOpKeyRequestInnerV2, SPOpKeyRequestV1, SPOpKeyRequestV2, SPOpKeyRequest, ProtoSPOpKeyRequestSeenV1, SPOpKeyRequestSeenV1, ProtoSPOpKeyRequestSeenInnerV2, SPOpKeyRequestSeenInnerV2, SPOpKeyRequestSeenV2, SPOpKeyRequestSeen
src/SPMessage.ts:396-436	Updated OP_KEY_REQUEST message parsing logic to handle V1 vs V2 formats
src/SPMessage.ts:471-500	Updated OP_KEY_REQUEST_SEEN message parsing logic for V1/V2
src/chelonia.ts:225-232	New params in ChelKeyRequestParams: request, keyRequestResponseId, skipInviteAccounting
src/chelonia.ts:312	Added saveMessageMetadata: false to default config
src/chelonia.ts:406	Added `rootState.secretKeys = rootState.secretKeys
src/chelonia.ts:485	Added this.config.reactiveSet(rootState, 'secretKeys', Object.create(null)) in reset
src/chelonia.ts:732-779	Updated setPendingKeyRevocation - now accepts contractIDOrState (string or state), added optional keyIds parameter
src/chelonia.ts:755-770	New withRetained helper function
src/chelonia.ts:2014-2140	Major rewrite of 'chelonia/out/keyRequest' - now uses withRetained, supports keyRequestResponseId for reusing keys, V2 format with skipInviteAccounting
src/db.ts:126-130	Added saveMessageMetadata config check in setEntryMeta
src/internals.ts:295-330	New deleteKeyHelper function - deletes keys and clears pending revocations for all keys with same name
src/internals.ts:637	Added name: contractName to manifestToContract mapping
src/internals.ts:990-1028	Updated 'chelonia/private/operationHook' - hook naming changed from /hook/ to /_postOpHook/, now supports atomic operations
src/internals.ts:1212-1245	Updated OP_KEY_SHARE handler - adds foreignContractIDs tracking to sharedKeyIds
src/internals.ts:1467-1590	Updated OP_KEY_REQUEST handler - handles V1/V2 formats, skipInviteAccounting, stores additional metadata (7-element array vs 4)
src/internals.ts:1593-1650	Updated OP_KEY_REQUEST_SEEN handler - handles V1/V2 formats, extracts innerData
src/internals.ts:1697-1720	Updated OP_KEY_DEL handler - now uses deleteKeyHelper
src/internals.ts:1790-1875	Updated OP_KEY_UPDATE handler - uses deleteKeyHelper, adds foreign key sync check, triggers deleteOrRotateRevokedKeys side effect
src/internals.ts:2138-2179	Updated watchForeignKeys and syncContractAndWatchKeys - changed .reduce() to .some() for cleaner logic
src/internals.ts:2311-2478	Updated deleteOrRotateRevokedKeys - added activeForeignKeyIds map, changed from using pendingKeyRevocations keys directly to looking up active foreign keys, added affectedKeyIds Set for deduplication
src/internals.ts:2520-2815	Major rewrite of respondToKeyRequest - supports custom request parameter, calls _responseOptionsForKeyRequest hook, validates skipInviteAccounting match, adds processing guard flag
src/presets.ts:20	Added saveMessageMetadata: true to SERVER preset
src/signedData.ts:19-25	New RawSignedData<T> and SignedDataContext<T> types, updated SignedData interface to use RawSignedData
src/signedData.ts:58-60	Updated signData return type to RawSignedData<U>

Technical Context

• Framework: SBP (Selector-Based Programming) with @sbp/sbp library
• Crypto: @chelonia/crypto for key operations (EDWARDS25519SHA512BATCH, CURVE25519XSALSA20POLY1305, etc.)
• Utilities: turtledash library for has, cloneDeep, difference, etc.
• Pattern: Message processing with operations (OP_KEY_ADD, OP_KEY_DEL, OP_KEY_UPDATE, OP_KEY_REQUEST, OP_KEY_SHARE, etc.)
• Style: StandardJS formatting (no semicolons)

Key function behaviors verified:

• has(undefined, key) returns false (doesn't throw) - safe to use
• findKeyIdByName (src/utils.ts:46-52) uses .find() returning FIRST match, not most recent by name

Strategy & Approach

1. Read through the entire diff to understand the scope of changes
2. Identify key areas: type definitions, message parsing, key rotation logic, hook naming
3. Examine specific functions for correctness: deleteKeyHelper, deleteOrRotateRevokedKeys, respondToKeyRequest
4. Check for edge cases, race conditions, and backward compatibility issues

Potential concerns identified so far (not yet formalized into issues):

1. findKeyIdByName returns first matching key - PR claims to "prioritize most recent key by name" but findKeyIdByName at src/utils.ts:46-52 uses .find() which returns the FIRST matching key, not most recent. This may be a bug or the PR description may be misleading.

2. Hook naming breaking change - Hook path changed from .../hook/${opType} to .../_postOpHook/${opType} at src/internals.ts:1002. Existing contracts using the old hook name will silently stop working (hook just won't be found).

3. Race condition in respondToKeyRequest - The processing flag check at line 2531 (has(entry, 'processing')) happens BEFORE Object.defineProperty(entry, 'processing', ...) at line 2590, but there's a timing gap between the initial check and setting the flag inside the .then() callback.

4. skipInviteAccounting validation mismatch - The check at line 2720 (!!requestedSkipInviteAccounting !== !!skipInviteAccounting) validates that request and response agree on this flag, but the error results in throwing rather than graceful handling.

Exact Next Steps

1. Formalize all identified issues with:

   • Sequential numbering
   • Rating (🔴/🟡/⚪️)
   • Checkboxes ([ ] Addressed, [ ] Dismissed)
   • Specific line references
   • Code improvement suggestions

2. Complete remaining analysis:

   • Verify deleteKeyHelper for edge cases (empty arrays, missing keys) - lines 295-330
   • Check activeForeignKeyIds mapping logic for correctness - lines 2321-2328
   • Verify foreignContractIDs tracking in OP_KEY_SHARE handler - lines 1212-1245
   • Check for proper validation of skipInviteAccounting between request and response
   • Look for any other race conditions or edge cases

3. Output the complete code review in the format requested by the userI'll complete the code review by examining key areas and formalizing all identified issues. Let me first verify some of the concerns I identified.

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/review_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (opencode)
• Model: glm-5

Click to expand review

I'll analyze these changes for bugs, security issues, and potential improvements. Let me start by examining the codebase for context on how these functions are used.
Let me examine more specific parts of the changes to understand the full context:
Let me check the remaining parts of the code for context:
Now I have enough context to provide a comprehensive review. Let me compile my findings.

Code Review

Issue 1: 🔴 skipInviteAccounting mismatch in default request path

• Addressed
• Dismissed

Location: src/internals.ts:2607-2688

Problem: In respondToKeyRequest, when request == null || request === '*' (the default case), skipInviteAccounting is never set, leaving it as undefined. However, the validation check at line 2720 compares requestedSkipInviteAccounting with skipInviteAccounting. If a request came with skipInviteAccounting: true but uses the default '*' request path, this will incorrectly throw a "Mismatched skipInviteAccounting" error.

if (request == null || request === '*') {
  // ... expiry check ...
  keyIds = Object.entries(contractState._vm.authorizedKeys)
    .filter(([, key]) => !!key.meta?.private?.shareable)
    .map(([kId]) => kId)
  // BUG: skipInviteAccounting is never set here
} else if (manifestHash) {
  // ... hook call ...
  skipInviteAccounting = result.skipInviteAccounting
}

// Later...
if (!!requestedSkipInviteAccounting !== !!skipInviteAccounting) {
  // This will fail if requestedSkipInviteAccounting=true and we took the '*' path
  throw new Error('Mismatched skipInviteAccounting')
}

Suggested fix:

if (request == null || request === '*') {
  if (contractState._vm?.invites?.[inviteId]?.expires != null) {
    if (contractState._vm.invites[inviteId].expires < Date.now()) {
      console.error(
        '[respondToKeyRequest] Ignoring OP_KEY_REQUEST because it expired at ' +
        contractState._vm.invites[inviteId].expires +
        ': ' +
        originatingContractID
      )
      return
    }
  }

  keyIds = Object.entries(contractState._vm.authorizedKeys)
    .filter(([, key]) => !!key.meta?.private?.shareable)
    .map(([kId]) => kId)
  // Set skipInviteAccounting to false for default case, or validate that
  // requestedSkipInviteAccounting is false/undefined
  skipInviteAccounting = false
}

---

Issue 2: 🟡 Stale pendingKeyRevocations entries not cleaned up

• Addressed
• Dismissed

Location: src/internals.ts:2343-2344

Problem: In deleteOrRotateRevokedKeys, when there's no active key with the same foreign key URI (!activeKeyId), the function just returns the accumulator without cleaning up the stale entry in pendingKeyRevocations. This could leave orphaned entries that will never be processed.

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!activeKeyId) return acc  // BUG: stale entry in pendingKeyRevocations not removed

Suggested fix:

const activeKeyId = activeForeignKeyIds[pkrKey.foreignKey]
if (!activeKeyId) {
  // No active key exists - clean up the stale pending revocation entry
  this.config.reactiveDel(pendingKeyRevocations, keyId)
  return acc
}

---

Issue 3: 🟡 Race condition in respondToKeyRequest processing guard

• Addressed
• Dismissed

Location: src/internals.ts:2584-2590

Problem: The check for has(entry, 'processing') and the subsequent setting of the processing property are not atomic. In a concurrent environment (e.g., multiple tabs or workers), two executions could both pass the check before either sets the flag, leading to duplicate responses.

if (has(entry, 'processing')) return
// Race condition window here
Object.defineProperty(entry, 'processing', { configurable: true, value: true })

Suggested fix: Use a synchronous check-and-set pattern or use a separate tracking mechanism:

// Use a Map outside the entry for tracking
if (!processingKeyRequests) processingKeyRequests = new Set()
if (processingKeyRequests.has(hash)) return
processingKeyRequests.add(hash)

try {
  // ... rest of processing ...
} finally {
  processingKeyRequests.delete(hash)
}

Or alternatively, move the check and set closer together and ensure they run synchronously without any async gaps before them.

---

Issue 4: 🟡 Missing cleanup of pendingKeyRevocations when key already rotated

• Addressed
• Dismissed

Location: src/internals.ts:2355-2360

Problem: When a key has been deleted in the foreign contract (!fKeyId), the code correctly marks it for deletion in pendingKeyRevocations. However, if activeKeyId refers to a different key than keyId (which it often will due to key rotation), marking keyId for deletion won't actually delete the active key. The function should delete activeKeyId instead.

if (!fKeyId) {
  // Key was deleted; mark it for deletion
  if (pendingKeyRevocations[keyId] === true) {
    this.config.reactiveSet(pendingKeyRevocations, keyId, 'del')
  }
  return acc
}

The keyId here is the pending revocation entry key, which may be an old rotated key. The actual key to delete is activeKeyId.

Suggested fix:

if (!fKeyId) {
  // Key was deleted in foreign contract; mark active key for deletion
  if (pendingKeyRevocations[keyId] === true) {
    this.config.reactiveDel(pendingKeyRevocations, keyId)
    this.config.reactiveSet(pendingKeyRevocations, activeKeyId, 'del')
  }
  return acc
}

---

Issue 5: ⚪ Silent skip when keyIds filter doesn't match in setPendingKeyRevocation

• Addressed
• Dismissed

Location: src/chelonia.ts:757-758

Problem: When the optional keyIds parameter is provided and the key found by name is not in the list, it's silently skipped. This could lead to confusion during debugging.

if (keyIds && !keyIds.includes(keyId)) continue

Suggested improvement: Add a debug/warn log for visibility:

if (keyIds && !keyIds.includes(keyId)) {
  console.debug('[setPendingKeyRevocation] Key excluded by keyIds filter', {
    contractID: contractID ?? '(unknown)',
    name,
    keyId
  })
  continue
}

---

Issue 6: ⚪ Potential confusion in deleteKeyHelper clearing all pending revocations by name

• Addressed
• Dismissed

Location: src/internals.ts:319-323

Problem: The deleteKeyHelper function clears ALL pending revocations for keys sharing the same name, not just the specific key being deleted. While this is likely intentional for key rotation scenarios, it could lead to unexpected behavior if there are edge cases where different keys with the same name should have independent pending operations.

for (const id of allIdsForNames[name]) {
  if (has(state._volatile!.pendingKeyRevocations, id)) {
    delete state._volatile!.pendingKeyRevocations![id]
  }
}

Suggested improvement: Add a comment to clarify the intentional behavior:

// Clear pending revocations for ALL keys with the same name.
// This is intentional for key rotation scenarios: when a key rotation
// completes (old key deleted or new key updated), we clear all pending
// operations for that key name to avoid duplicate or stale operations.
for (const id of allIdsForNames[name]) {
  if (has(state._volatile!.pendingKeyRevocations, id)) {
    delete state._volatile!.pendingKeyRevocations![id]
  }
}

---

Issue 7: ⚪ deleteKeyHelper called before setting _notAfterHeight

• Addressed
• Dismissed

Location: src/internals.ts:1686 and src/internals.ts:1686-1688

Problem: In the OP_KEY_DEL handler, deleteKeyHelper is called, which sets _notAfterHeight. But the code was previously doing this directly. Now the helper does it, but there's still a loop afterward that accesses key.foreignKey on keys that have already been marked as deleted. This is functionally fine since we're just reading, but the ordering could be clearer.

deleteKeyHelper(state, height, keyIds)
keyIds.forEach((keyId) => {
  const key = state._vm.authorizedKeys[keyId]
  // key now has _notAfterHeight set, but we still read foreignKey

Suggested improvement: This is working correctly but consider adding a comment or reordering to make the intent clearer.

---

Summary

The most critical issue is Issue 1 - the skipInviteAccounting validation mismatch, which could cause valid key requests to fail unexpectedly. The medium-priority issues (2-4) relate to edge cases in the key rotation/cleanup logic that could leave stale state or miss cleanup opportunities. The low-priority issues (5-7) are code quality and documentation improvements.

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

I'll review this PR for bugs, security issues, and improvements. Let me first examine some key areas in more detail.

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast (*oops, I forgot that crush has a bug that limits the GLM-5 context window to half its size. Fixed! re-running)

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

Based on my review, here are the issues I've identified:

---

Issue 1: skipInviteAccounting mismatch for wildcard requests

Rating: 🔴 High importance, High confidence

Location: src/internals.ts:2607-2622 and src/internals.ts:2720-2723

• Addressed
• Dismissed

Problem: When request === '*' (wildcard request for all shareable keys), the skipInviteAccounting variable is never set and remains undefined. However, if the requester sent skipInviteAccounting: true in their OP_KEY_REQUEST, the check at line 2720 will fail:

// src/internals.ts:2607-2622
if (request == null || request === '*') {
  // ... expiration check ...
  // skipInviteAccounting is NEVER set here!
  keyIds = Object.entries(contractState._vm.authorizedKeys)
    .filter(([, key]) => !!key.meta?.private?.shareable)
    .map(([kId]) => kId)
}

// src/internals.ts:2720-2723
if (!!requestedSkipInviteAccounting !== !!skipInviteAccounting) {
  console.error(`Error at respondToKeyRequest: mismatched result for skipInviteAccounting...`)
  throw new Error('Mismatched skipInviteAccounting')
}

If requestedSkipInviteAccounting === true and request === '*', then:

• !!true !== !!undefined = true !== false = true
• This throws an error even though the request is valid

Suggested fix:

// src/internals.ts:2607-2622
if (request == null || request === '*') {
  if (contractState._vm?.invites?.[inviteId]?.expires != null) {
    if (contractState._vm.invites[inviteId].expires < Date.now()) {
      console.error(
        '[respondToKeyRequest] Ignoring OP_KEY_REQUEST because it expired at ' +
        contractState._vm.invites[inviteId].expires +
        ': ' +
        originatingContractID
      )
      return
    }
  }

  keyIds = Object.entries(contractState._vm.authorizedKeys)
    .filter(([, key]) => !!key.meta?.private?.shareable)
    .map(([kId]) => kId)
  // For wildcard requests, pass through the requested skipInviteAccounting value
  skipInviteAccounting = requestedSkipInviteAccounting
}

---

Issue 2: Indentation inconsistencies in operationHook

Rating: 🟡 Medium importance, High confidence

Location: src/internals.ts:1001-1016

• Addressed
• Dismissed

Problem: Several lines in the new callHook function have inconsistent indentation that doesn't follow the project's 2-space standard:

// src/internals.ts:1001-1016
const callHook = (op: string, atomic?: boolean) => {
  const hook = `${manifestHash}/${contractName}/_postOpHook/${op}`
  // Check if a hook is defined
  if (sbp('sbp/selectors/fn', hook)) {
  // And call it                         <- Missing indentation
    try {
    // Note: Errors here...              <- Missing indentation
      sbp(hook, { contractID, message, state, atomic })
    } catch (e) {
      console.error(
      `[${hook}] hook error...`          <- Missing indentation
      e
      )
    }
  }
}

Suggested fix:

const callHook = (op: string, atomic?: boolean) => {
  const hook = `${manifestHash}/${contractName}/_postOpHook/${op}`
  // Check if a hook is defined
  if (sbp('sbp/selectors/fn', hook)) {
    // And call it
    try {
      // Note: Errors here should not stop processing, since running these
      // hooks is optional (for example, they aren't run on the server)
      sbp(hook, { contractID, message, state, atomic })
    } catch (e) {
      console.error(
        `[${hook}] hook error for message ${message.hash()} on contract ${contractID}:`,
        e
      )
    }
  }
}

---

Issue 3: Potential double-hook call for OP_ATOMIC operations

Rating: 🟡 Medium importance, High confidence

Location: src/internals.ts:1018-1027

• Addressed
• Dismissed

Problem: For OP_ATOMIC messages, hooks are called twice:

1. Once for each distinct operation type within the atomic batch (with atomic: true)
2. Once again for OP_ATOMIC itself (with atomic: undefined)

// src/internals.ts:1018-1027
if (message.opType() === SPMessage.OP_ATOMIC) {
  const opsSet = new Set()
  for (const [op] of message.opValue() as SPOpAtomic) {
    // Only call hook once per opcode
    if (opsSet.has(op)) continue
    opsSet.add(op)
    callHook(op, true)  // First call with atomic=true
  }
}
callHook(message.opType())  // Second call with atomic=undefined (even for OP_ATOMIC)

This could confuse hook implementers who might not expect two calls for the same atomic operation. When the message type is OP_ATOMIC, the final callHook will call callHook(SPMessage.OP_ATOMIC) with atomic=undefined, which is inconsistent with the inner calls that use atomic=true.

Suggested fix:

if (message.opType() === SPMessage.OP_ATOMIC) {
  const opsSet = new Set()
  for (const [op] of message.opValue() as SPOpAtomic) {
    // Only call hook once per opcode
    if (opsSet.has(op)) continue
    opsSet.add(op)
    callHook(op, true)
  }
  // Call OP_ATOMIC hook with consistent atomic flag
  callHook(SPMessage.OP_ATOMIC, true)
} else {
  callHook(message.opType())
}

---

Issue 4: deleteKeyHelper clears pending revocations for all same-named keys

Rating: ⚪️ Low importance, Medium confidence

Location: src/internals.ts:295-330

• Addressed
• Dismissed

Problem: The deleteKeyHelper function clears pending revocations for ALL keys with the same name as any key being deleted, not just the specific keys being deleted:

// src/internals.ts:320-326
// Clear pending revocations for all keys with the same name
// to handle key rotation scenarios where multiple keys exist
for (const id of allIdsForNames[name]) {
  if (has(state._volatile!.pendingKeyRevocations, id)) {
    delete state._volatile!.pendingKeyRevocations![id]
  }
}

While the comment indicates this is intentional for key rotation scenarios, it could have unintended effects if:

1. There are multiple pending revocations for different keys with the same name at different rotation cycles
2. Only one of those keys is being processed

This is likely a minor issue since in practice, key names are typically unique for active keys, but the behavior could be surprising.

Suggested improvement: Document this behavior more clearly in the function's JSDoc, or only clear the pending revocation for the specific key being deleted:

// Option 1: Better documentation
/**
 * Helper function to delete keys from the state and clear related pending revocations.
 * Handles key rotation scenarios by clearing pending revocations for ALL keys with the same name,
 * since key rotation creates new keys with the same name and old ones become stale.
 * ...
 */

// Option 2: Only clear for the specific key (more conservative)
if (has(state._volatile!.pendingKeyRevocations, keyId)) {
  delete state._volatile!.pendingKeyRevocations![keyId]
}

---

Issue 5: Typo in comment

Rating: ⚪️ Low importance, High confidence

Location: src/internals.ts:1546

• Addressed
• Dismissed

Problem: Minor typo in comment: "porocessing" should be "processing"

// src/internals.ts:1546
// If skipping porocessing or if the message is outgoing, there isn't

Suggested fix:

// If skipping processing or if the message is outgoing, there isn't

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

Based on my thorough review of the changes, here are the issues I've identified:

---

Issue 1: Breaking change in operation hook naming convention

🔴 High importance & high confidence

• Addressed
• Dismissed

Location: src/internals.ts:959

The hook naming convention has changed from /hook/ to /_postOpHook/:

Before (main branch):

const hook = `${manifestHash}/${contractName}/hook/${message.opType()}`
sbp(hook, { contractID, message, state })

After (this PR):

const hook = `${manifestHash}/${contractName}/_postOpHook/${op}`
sbp(hook, { contractID, message, state, atomic })

This is a breaking change for any existing contracts that have defined hooks using the old naming convention. Contracts will need to rename their hook selectors from {manifest}/{contractName}/hook/{opType} to {manifest}/{contractName}/_postOpHook/{opType}.

Additionally, the hook signature has changed to include an atomic parameter, and hooks may now be called multiple times for OP_ATOMIC operations (once per op-type with atomic: true, then once more for the atomic operation itself).

Suggestion: If backwards compatibility is needed, consider supporting both hook naming conventions temporarily, or document this as a breaking change clearly.

---

Issue 2: Type assertion inconsistency in SPMessage.ts

🟡 Medium importance & high confidence

• Addressed
• Dismissed

Location: src/SPMessage.ts:395-403

return maybeEncryptedIncomingData<ProtoSPOpKeyRequestV1 | SPOpKeyRequestV2>(
  contractID,
  state,
  message as ProtoSPOpKeyRequestV1,  // <-- Cast as V1 but type allows V1 | V2

The generic type parameter is ProtoSPOpKeyRequestV1 | SPOpKeyRequestV2 but the message is cast only as ProtoSPOpKeyRequestV1. While the runtime logic correctly handles both V1 and V2 formats inside the callback based on the presence of innerData, the type assertion is misleading and could cause confusion for code maintainers.

Suggestion:

return maybeEncryptedIncomingData<ProtoSPOpKeyRequestV1 | SPOpKeyRequestV2>(
  contractID,
  state,
  message as ProtoSPOpKeyRequestV1 | SPOpKeyRequestV2,

---

Issue 3: Potential issue with activeForeignKeyIds map overwriting

🟡 Medium importance & high confidence

• Addressed
• Dismissed

Location: src/internals.ts:2284-2290

const activeForeignKeyIds = Object.fromEntries(
  Object.values(contractState._vm.authorizedKeys)
    .filter(({ foreignKey, _notAfterHeight }) =>
      foreignKey != null && _notAfterHeight == null
    )
    .map(({ foreignKey, id }) => [foreignKey!, id])
)

If multiple active keys have the same foreignKey URI, only the last one (by iteration order) will be kept in the map. While this may be intentional (only one key should be active for a given foreign key at a time), there's no warning or validation if multiple keys with the same foreign key exist.

Suggestion: Add a check to warn if duplicate foreign keys are detected:

const activeForeignKeyIds: Record<string, string> = {}
for (const { foreignKey, id, _notAfterHeight } of Object.values(contractState._vm.authorizedKeys)) {
  if (foreignKey != null && _notAfterHeight == null) {
    if (activeForeignKeyIds[foreignKey]) {
      console.warn(`[deleteOrRotateRevokedKeys] Multiple active keys for foreignKey: ${foreignKey}`)
    }
    activeForeignKeyIds[foreignKey] = id
  }
}

---

Issue 4: Inconsistent ring level fallback value

⚪️ Lower importance & high confidence

• Addressed
• Dismissed

Location: src/internals.ts:2330, 2400

The code uses Number.MAX_SAFE_INTEGER as the fallback for key.ringLevel:

const ringLevel = Math.min(currentRingLevel, key.ringLevel ?? Number.MAX_SAFE_INTEGER)

But the initial accumulator value uses Number.POSITIVE_INFINITY:

[
  Number.POSITIVE_INFINITY,
  '',
  [] as { name: string; oldKeyId: string; id: string; data: string }[]
]

While this is functionally equivalent (both are larger than any realistic ring level), using consistent values would make the code clearer.

Suggestion: Use Number.POSITIVE_INFINITY consistently in both places, or use Number.MAX_SAFE_INTEGER consistently.

---

Issue 5: Missing cleanup of stale entries in pendingKeyRevocations

🟡 Medium importance & high confidence

• Addressed
• Dismissed

Location: src/internals.ts:2301-2327

When iterating over keysToUpdate (from pendingKeyRevocations), if pkrKey (the key from pending revocations) has been deleted (no longer exists in authorizedKeys), the code silently returns and continues:

const pkrKey = contractState._vm?.authorizedKeys?.[keyId]
if (!pkrKey || !pkrKey.foreignKey) return acc

However, the stale entry in pendingKeyRevocations is never cleaned up. Over time, this could lead to accumulation of orphaned entries.

Suggestion: Clean up the stale entry:

const pkrKey = contractState._vm?.authorizedKeys?.[keyId]
if (!pkrKey) {
  // Key was deleted, clean up the stale entry
  this.config.reactiveDel(pendingKeyRevocations, keyId)
  return acc
}
if (!pkrKey.foreignKey) {
  this.config.reactiveDel(pendingKeyRevocations, keyId)
  return acc
}

---

Issue 6: Potential race condition in respondToKeyRequest acknowledged but mitigated

⚪️ Lower importance & high confidence

• Addressed
• Dismissed

Location: src/internals.ts:2550-2561

// Guard to prevent responding to this request multiple times
// Note: there's a small time window where the previous check and this
// could pass. This is for brevity (avoiding the same check multiple times)
if (has(entry, 'processing')) return
if (!contractState?._vm?.pendingKeyshares?.[hash]) {
  return
}
Object.defineProperty(entry, 'processing', { configurable: true, value: true })

The comment acknowledges a race condition. Since JavaScript is single-threaded within synchronous code blocks, the checks and Object.defineProperty are effectively atomic within the Promise callback. However, if the entry object is shared across multiple contexts (e.g., if state is proxied), there might be edge cases.

The current implementation is adequate but could be improved for clarity by combining the checks.

---

Issue 7: Good security addition - skipInviteAccounting validation

✅ Positive observation

Location: src/internals.ts:2693-2696

if (!!requestedSkipInviteAccounting !== !!skipInviteAccounting) {
  console.error(`Error at respondToKeyRequest: mismatched result for skipInviteAccounting...`)
  throw new Error('Mismatched skipInviteAccounting')
}

This is a good security measure that ensures the responder cannot bypass invite accounting when the requester expected it to be enforced.

---

Summary

The main concerns are:

1. Breaking change in hook naming (/hook/ → /_postOpHook/) - contracts will need updates
2. Type inconsistency that could cause confusion
3. Potential for stale data accumulation in pendingKeyRevocations
4. Multiple active foreign keys warning would be helpful for debugging

---

Review generated using glm-5 via Z.AI. Comment /review to re-run.

[taoeffect]

/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}}

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: google/gemini-3.1-pro-preview

Click to expand review

I have thoroughly reviewed the supplied changes and have identified a couple of issues ranging from high to medium severity. Please review my feedback below.

1. 🔴 keysToDelete.reduce causes a bug when ringLevel is undefined due to Number.POSITIVE_INFINITY

• Addressed
• Dismissed

In src/internals.ts within the chelonia/private/deleteOrRotateRevokedKeys logic, you accurately fixed the missing fallback bug for keysToUpdate.reduce by replacing Number.POSITIVE_INFINITY with Number.MAX_SAFE_INTEGER. However, this fix was missed for the newly implemented keysToDelete.reduce operation immediately below it.

If the current target key being deleted does not have a defined ringLevel, Number.POSITIVE_INFINITY is used. This fails the subsequent Number.isFinite(ringLevel) check completely bypassing the findSuitableSecretKeyId hook, and results in attempting to dispatch an OP_KEY_DEL op with a signingKeyId equal to an empty string ('').

You can easily fix this by synchronizing the boundary behaviors:

        const keyId = activeForeignKeyIds[pkrKey.foreignKey]
        if (!keyId || affectedKeyIds.has(keyId)) return acc
        const [currentRingLevel, currentSigningKeyId, currentKeyIds] = acc
        const ringLevel = Math.min(
          currentRingLevel,
-          contractState._vm?.authorizedKeys?.[keyId]?.ringLevel ?? Number.POSITIVE_INFINITY
+          contractState._vm?.authorizedKeys?.[keyId]?.ringLevel ?? Number.MAX_SAFE_INTEGER
        )

2. 🟡 Unbounded array growth for sharedKeyId.foreignContractIDs array due to lack of deduplication

• Addressed
• Dismissed

Inside src/internals.ts handling for OP_KEY_SHARE, newly discovered foreignContractIDs are pushed onto the sharedKeyId.foreignContractIDs array without evaluating if the identifier has already been recorded.

If overlapping key request cycles process or if event timelines repeatedly broadcast the share op, the foreignContractIDs array will bloat with duplicate identifiers representing the exact same pairing. It is recommended to perform a cursory existence/deduplication check before pushing to preserve the compactness of the specific _vm state.

            } else if (v.foreignContractID) {
              if (!sharedKeyId.foreignContractIDs) {
                sharedKeyId.foreignContractIDs = [[v.foreignContractID, height]]
-             } else {
+             } else if (!sharedKeyId.foreignContractIDs.some(([id]) => id === v.foreignContractID)) {
                sharedKeyId.foreignContractIDs.push([v.foreignContractID, height])
              }
            }

(Note: As an alternative to ignoring duplicates entirely, if you strictly need access to the most recent height, you might instead find the matching entry and explicitly replace the logged height.)

---

Review generated using google/gemini-3.1-pro-preview via OpenRouter. Comment /review to re-run.

[taoeffect]

/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}}

(Sorry @corrideat - there was a small error in the "helpful context" run step of the previous job that i want to see if it's still there)

jq: error: Could not open file pr_data.json: No such file or directory

EDIT: looks like it's been fixed!

---

AI review started.

[taoeffect]

/crush_fast {{openrouter, google/gemini-3.1-pro-preview, google/gemini-3-flash-preview}} - cancelling the previous run since a commit was pushed

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: google/gemini-3.1-pro-preview

Click to expand review

Overall, the changes look extremely solid! The logic around foreign key tracking (foreignContractIDs), deferred updating inside deleteOrRotateRevokedKeys, and the V1/V2 key request decoupling is thoroughly constructed inside the SPMessage state machine.

I found a couple of relatively minor observations, particularly one code pattern that might inhibit UI reactivity:

1. Array index assignment could break Vue 2 reactivity

🟡 Medium Importance

• Addressed
• Dismissed

In src/internals.ts, when an already tracked foreign contract is found in sharedKeyId.foreignContractIDs, its 'last share height' is populated by assigning height directly to the tuple array index:

// src/internals.ts:1194-1196
                const tuple = sharedKeyId
                  .foreignContractIDs.find(([id]) => id === v.foreignContractID)
                if (tuple) {
                  tuple[2] = height
                } else {
                  sharedKeyId.foreignContractIDs.push([v.foreignContractID, height])

Details:
Direct index assignment on arrays (tuple[2] = ...) fails to trigger reactivity interceptions inside Vue 2. If this nested component happens to be a tracked reactive structure feeding the UI, data will fail to update. Since you are trying to assign or overwrite the 3rd element inside that nested array, utilizing .splice(2, 1, height) will natively guarantee reactive propagation, alongside your safe usage of .push() directly below it.

Suggestion:

                const tuple = sharedKeyId
                  .foreignContractIDs.find(([id]) => id === v.foreignContractID)
                if (tuple) {
                  tuple.splice(2, 1, height)
                } else {

---

2. Missing undefined check for state resolution

⚪️ Low Importance

• Addressed
• Dismissed

In src/chelonia.ts, setPendingKeyRevocation resolves the pointer contractIDOrState immediately to state but makes the hard assumption that the query successfully located a valid proxy/object in rootState:

// src/chelonia.ts:751-758
    let state: ChelContractState
    let contractID: string | undefined
    if (typeof contractIDOrState === 'string') {
      const rootState = sbp(this.config.stateSelector)
      contractID = contractIDOrState
      state = rootState[contractIDOrState]
    } else {
      state = contractIDOrState
    }

    if (!state._volatile) this.config.reactiveSet(state, '_volatile', Object.create(null))

Details:
If a disconnected frontend, an outdated event handler, or an invalid parameter string causes the contractID string lookup failure (meaning rootState[contractIDOrState] resolves to undefined), the immediate property access check of if (!state._volatile) will throw: TypeError: Cannot read properties of undefined (reading '_volatile').

Suggestion:
Bail gracefully if the contract state target isn't mapped:

    } else {
      state = contractIDOrState
    }

    if (!state) return

    if (!state._volatile) this.config.reactiveSet(state, '_volatile', Object.create(null))

---

Review generated using google/gemini-3.1-pro-preview via OpenRouter. Comment /review to re-run.

[taoeffect]

Amazing work @corrideat !!