KeyPurpose is documented and used as a bitmask (e.g. KeyDescriptor.usages / DerivedKeySpec.usages), but the enum values here are sequential (ENCRYPT=0, DECRYPT=1, …). With a bitmask, each purpose must be a distinct power-of-two flag (and typically 0 means "no purposes"), otherwise OR-ing values will lose information (e.g. ENCRYPT|DECRYPT collapses to 1). Consider either (a) changing KeyPurpose values to bit flags (1<<n) and updating docs, or (b) changing the APIs to use KeyPurpose[] (or a dedicated KeyUsageFlags enum) instead of int bitmasks.

This docstring says ICryptoOperation is returned by IKeyVaultController.begin(), but IKeyVaultController does not define a begin() method. Please update the comment to reference only existing APIs (or add the missing begin() method if that is the intended design).

deriveIntoVault() docs reference config.keyAlias, but CryptoConfig does not have a keyAlias field. This is confusing for callers and makes the source-key selection rules unclear. Please update the documentation to match the actual API (it looks like sourceKeyAlias is the authoritative parameter) and remove/replace the keyAlias reference.

registerEventListener() documentation lists callbacks onDeviceSuspending/onDeviceResumed, but IKeyVaultEventListener does not define these methods. Please either add the missing callbacks to IKeyVaultEventListener or update the @post text to only mention callbacks that actually exist.

IKeyVaultEventListener is documented as covering "power state transitions", but there are no callbacks for suspend/resume (only state/key lifecycle events). This is misleading for API consumers; either add explicit power-state callbacks or tighten the description to match the current method set.

metadata.yaml lists a dependency on "Shared enums from KeyVault HAL", but the AIDL sources in this module do not import from com.rdk.hal.keyvault (KeyVault imports CryptoEngine types, not the other way around). Please correct/remove this dependency entry to avoid misleading dependency tracking.

The interface documentation says attaching an ICryptoEngineController enables crypto operations on vault-managed keys, but IKeyVaultController currently exposes no methods to invoke encrypt/decrypt/sign/verify using a vault key alias (only key lifecycle + deriveIntoVault). As written, non-extractable keys cannot be used for crypto at all via this HAL. Consider adding explicit operation entry points on IKeyVaultController (e.g. beginWithKeyAlias()/encryptWithKeyAlias()/signWithKeyAlias()) so callers can use vault-resident keys without exporting raw material.

EngineCapabilities.keySizes is a flat int[] which loses the association between supported algorithms and their valid key sizes (e.g. AES 128/192/256 vs RSA 2048/3072/4096). This makes capability-driven client validation ambiguous. Consider modelling key sizes per algorithm (e.g. an array of parcelables like {Algorithm algorithm; int[] sizes;}) or otherwise documenting that keySizes is a union and how callers should interpret it.

This module CMakeLists does not forward the root-level AIDL_GEN_DIR / AIDL_BIN variables into compile_aidl(). Most other modules build a COMPILE_AIDL_ARGV list and pass TARGET_DIRECTORY ${AIDL_GEN_DIR} (and AIDL_BIN when set) so generated sources go under gen/ rather than the module source tree (e.g. avbuffer/current/CMakeLists.txt:56-67). As-is, building with the repo-root CMake will generate output into cryptoengine/current/, which is likely unintended and can dirty the working tree.

This module CMakeLists does not forward the root-level AIDL_GEN_DIR / AIDL_BIN variables into compile_aidl(). In most existing modules, TARGET_DIRECTORY ${AIDL_GEN_DIR} is passed so generated output lands in gen// (see avbuffer/current/CMakeLists.txt:56-67). As written, a root-level build will generate output into keyvault/current/ instead, which is likely unintended and can dirty the working tree.

The computeDigest() documentation lists SHA-1 as a supported digest option, but the Digest enum does not include SHA_1. Please either add SHA_1 to Digest (and update HFP/capabilities accordingly) or update the docs to reflect the actual supported values.

The related-page link points to ../../keyvault/current/document.md, but this repository uses key_vault/current/key_vault.md (and MkDocs sources are under docs/). As written, this link is likely broken; please update it to the correct published documentation location.

This document states the engine supports SHA-1, but the Digest enum and HFP don’t include SHA-1. Please either add SHA-1 support to the API/HFP or update this list to match the actual supported digests.

This section says EngineCapabilities “reflects these settings”, including padding modes, EC curves, KDFs, and per-algorithm key sizes, but EngineCapabilities.aidl currently only exposes algorithms/blockModes/digests and a flat keySizes array. Either extend EngineCapabilities to include these advertised capability dimensions (and allow per-algorithm key size reporting) or adjust the documentation/HFP expectations to avoid a mismatch.

EngineCapabilities is missing several capability dimensions that are declared in the CryptoEngine HFP and referenced in docs (e.g., padding modes, EC curves, key derivations, and per-algorithm key sizes). As-is, callers can’t reliably introspect what the engine supports. Consider adding explicit fields for these, or clarifying that they are not discoverable at runtime.

MkDocs is configured with docs_dir: docs, so markdown files under key_vault/current/ won’t be published unless there’s an external copy/sync step. If this is intended to be part of the public HAL documentation, consider moving it under docs/halif/key_vault/current/ and adding it to mkdocs.yml nav (or document the generation path).

MkDocs is configured with docs_dir: docs, so markdown files under crypto_engine/current/ won’t be published unless there’s an external copy/sync step. If this is intended to be part of the public HAL documentation, consider moving it under docs/halif/crypto_engine/current/ and adding it to mkdocs.yml nav (or document the generation path).

This CMakeLists pulls CryptoEngine AIDL files into SRC (Algorithm.aidl, Digest.aidl, etc.). Other modules with cross-module imports (e.g. avbuffer/current/CMakeLists.txt:39-52) rely on INCLUDE_DIRECTORY to resolve imports without recompiling dependency AIDL sources. Consider removing the external AIDL files from SRC and keeping only the include path to crypto_engine/current to avoid duplicate generated outputs and reduce coupling.

The interface description states “A KeyVault is a configured crypto engine instance” and that each vault has its own attached crypto engine. This conflicts with the design described in key_vault.md/PR description where the vault is a keystore and callers attach an ICryptoEngineController per session. Please update this overview text to match the intended separation of concerns.

The generateKeyPair() documentation says “The public key bytes are also returned directly”, but the method signature returns only KeyDescriptor[]. If callers must export the public key via exportKey(publicAlias) (as shown in the markdown doc), please update this comment to avoid implying that raw public key bytes are returned here.