Skip to content

Add PrivacyHook#15

Merged
ariessa merged 1 commit into
erc-8183:mainfrom
PRXVT:privacy-hook
May 11, 2026
Merged

Add PrivacyHook#15
ariessa merged 1 commit into
erc-8183:mainfrom
PRXVT:privacy-hook

Conversation

@PRXVT
Copy link
Copy Markdown
Contributor

@PRXVT PRXVT commented Mar 23, 2026

Profile C hook. Providers submit deliverables as encrypted envelopes instead of plaintext. The hook validates
envelope structure on-chain and optionally runs a ZK proof over the encrypted contents.

Mechanism

Off-chain, the provider AES-256-GCM encrypts the deliverable and uploads the ciphertext to IPFS. Only the CID hits the
chain.

The AES key is ECDH-wrapped per recipient (client, evaluator, and so on) using secp256k1, so participants use the same
keys they already sign transactions with. Ephemeral keys are fresh per submission and discarded after.

setBudget.optParams carries the per-job privacy config: (zkVerifier, minWrappedKeys), or (zkVerifier, minWrappedKeys, numPublicInputs) when the circuit needs more than 2 public inputs. Config is immutable once set.

submit.optParams carries (cid, wrappedKeys, zkProof). The hook checks that the CID matches the submit's
deliverable hash, validates wrapped-key count and v1 format (94 bytes), and calls IZKVerifier.verify if one was
configured. Setting zkVerifier = address(0) at config skips ZK and enforces envelope shape only.

Key properties

  • No plaintext or ciphertext on-chain.
  • The hook never sees deliverable contents, only metadata.
  • Payment flow is unchanged. The hook is pure policy enforcement.

Audit status

Self-reviewed for hook auth, envelope validation, commitment handling, and reentrancy via a
malicious verifier. No findings. Unaudited by a third party.

Answers to earlier review questions

Participants hold their own secp256k1 keys, no registry or third-party service. Ephemeral keys are per-submission and
zeroized after use, so past deliverables stay safe even if a long-term key is later compromised. ZK verification is
fully optional (address(0) disables it).

@psmiratisu
Copy link
Copy Markdown
Contributor

Hey dropping some questions!

  1. Key Management & Service Availability
    The ECDH flow requires key pairs. Who generates and holds these?
    • Client's private key?
    • Agent's private key?
    • A service (ThoughtProof, etc.)?

Critical question: If the key holder service goes offline, can the deliverable still be decrypted? If not, this becomes a single point of failure for job completion.

  1. ThoughtProof Dependency (Optional Path)
    The README mentions "optional ThoughtProof opinion." To confirm:
    • Is opinionVerifier a setter that can remain address(0)?
    • If address(0), does the hook still encrypt/decrypt normally, just without quality opinion?
    • If not, what's the fallback behavior?

Request: Document the "no-opinion" mode explicitly. If the dependency isn't truly optional, we need to discuss interface extraction.

  1. Forward Secrecy
    ECDH-ES implies ephemeral keys. Clarify:
    • Are ephemeral keys generated per-job or reused?
    • If a long-term key is compromised, are past deliverables at risk?
    • Is there key rotation without invalidating historical on-chain data?

  2. Gas Economics
    decrypt() takes ciphertext as calldata. Have you benchmarked:
    • Gas cost for 1KB deliverable?
    • Gas cost for 10KB deliverable?
    • Is there a practical size limit for viability?

  3. Interface Neutrality (Standard Alignment)
    The hook logic is vendor-agnostic, but documentation emphasizes ThoughtProof. To align with open standard principles:
    Are we open to define IPrivacyVerifier interface:

interface IPrivacyVerifier {
    function encrypt(bytes memory plaintext, bytes memory recipientKey) external returns (bytes memory ciphertext);
    function decrypt(bytes memory ciphertext, bytes memory recipientKey) external returns (bytes memory plaintext);
}

Then PrivacyHook uses any IPrivacyVerifier implementation — ThoughtProof, Lit Protocol, decentralized KMS, etc.

@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented Mar 25, 2026

Hey Miratisu, thanks for the detailed questions. Really appreciate the thoroughness.

1. Key Management & Service Availability

No centralized service or middleman. Participants hold their own secp256k1 keys (same ones used to
sign transactions). Provider generates a fresh ephemeral key per submission and discards it after.
Public keys are recovered from tx signatures, no registry needed.

There's no key management service. Client holds their key, evaluator holds theirs. Nothing in between,
nothing to go offline.

2. ThoughtProof Dependency

I think there might be a mix-up here. This implementation doesn't have a ThoughtProof dependency. The
optional piece is zkVerifier, which defaults to address(0). The hook works fully without it:
encryption enforcement, envelope validation, commitment hashing, event emission. ZK verification is an
optional layer that can be enabled per job when on-chain proof of deliverable properties is needed.

IZKVerifier is generic. Any proof system that implements verify(bytes, bytes32[]) -> bool plugs
right in.

3. Forward Secrecy

Every submission uses a fresh ephemeral key pair. The provider generates it, wraps the AES key for
each recipient using ECDH with their public key, and the ephemeral private key is zeroized immediately
after. It never leaves memory, is never stored, and is never transmitted.

This means even if an attacker compromises the provider's infrastructure after the fact, they can't
recover the ephemeral key to decrypt past submissions. Each job's encryption is completely
independent.

For recipient key rotation, it's straightforward. A participant starts using a new key for new jobs.
Old on-chain data (commitments, wrapped keys) remains valid and unchanged. Old key still decrypts old
deliverables, new key handles new ones.

4. Gas Economics

Ciphertext never touches the chain. The on-chain footprint is just the CID hash (32B), wrapped keys
(94B each), and an optional ZK proof (256B). Measured ~87k gas without ZK, ~299k with Groth16. Same
cost whether the deliverable is 1KB or 1GB.

5. Interface Neutrality

Agreed on keeping things vendor-agnostic. On-chain encrypt/decrypt isn't practical gas-wise, which is
why encryption is handled entirely off-chain. The hook only enforces envelope structure. It doesn't
care which SDK or tooling produced it. The wrapped key format is an open 94-byte layout that anyone
can implement against, and IZKVerifier serves as the on-chain extension point.

Happy to adjust anything based on your feedback.

@psmiratisu
Copy link
Copy Markdown
Contributor

I think this is a good privacy-pattern hook and it is close. It feels like a real example hook rather than a bundled system. Before merge, could you please rebase on the current shared plumbing fixes, add also if it has been auditted and keep the PR description and docs template-like rather than product-style. Once that cleanup is in, I think this is one of the cleaner examples to merge.

@PRXVT

@PRXVT PRXVT force-pushed the privacy-hook branch 2 times, most recently from acf3fd6 to 30a94b0 Compare April 18, 2026 20:53
@jhamelin3315-collab
Copy link
Copy Markdown

I think this is a good privacy-pattern hook and it is close. It feels like a real example hook rather than a bundled system. Before merge, could you please rebase on the current shared plumbing fixes, add also if it has been auditted and keep the PR description and docs template-like rather than product-style. Once that cleanup is in, I think this is one of the cleaner examples to merge.

@PRXVT

@miratisu_ps

Thanks for the continued review and guidance on this PR.

As an observer following the ERC-8183 standardization effort, I have a couple of questions:

  1. Do you (or the Virtuals team) see the PrivacyHook being adopted or integrated within the Virtuals ecosystem for agent commerce jobs? Would it be something teams building on Virtuals might start using once merged?

  2. Is there any potential for a privacy-focused hook like this to become strongly recommended (or even default in certain contexts) for production agent commerce use cases, given the transparency concerns around deliverables?

  3. On a broader note, what’s the current expected timeline for ERC-8183 to move from Draft status toward Final? Are there any major blockers or target milestones the community should watch for?

Appreciate all the work going into making this a clean, composable standard. Looking forward to seeing how privacy extensions evolve alongside the core job primitive.

@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented Apr 24, 2026

Thanks @psmiratisu, appreciate the detailed review.

Addressed all three points:

  • Rebased onto current main. Picked up the BaseERC8183Hook rename and the new erc8183 submodule paths, and adapted PrivacyHook to the new callback signatures.
  • Added an audit status section to the PR description.
  • Revised the NatSpec and the README row to match the template tone of BiddingHook and FundTransferHook, and added a KEY PROPERTY line like the siblings have. Rewrote the PR description in the same tone.

Let me know if there's anything else you'd like changed.

@ariessa
Copy link
Copy Markdown
Collaborator

ariessa commented May 4, 2026

Hi @PRXVT,

  1. This PR is outdated. Can you pull current code from main branch into this PR?
  2. This hook doesn't support MultiHookRouter. If PrivacyHook supports MultiHookRouter, it can be used with n number of hooks where n is the max hooks per job. Can you make the hook support MultiHookRouter?

@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented May 4, 2026

Thanks @ariessa.

Both points addressed.

Rebase
Branch is on current main, including the IERC8183Hook rename and the recent security fixes.

MultiHookRouter support
PrivacyHook implements IERC8183HookMetadata.

  • requiredSelectors() returns an empty bytes4[], matching the BiddingHook and FundTransferHook convention.
  • supportsInterface advertises both IERC8183Hook and IERC8183HookMetadata, so MultiHookRouter._validateSubHook accepts it.
  • The existing PrivacyNotConfigured runtime revert covers misconfiguration when used through the router.

Copy link
Copy Markdown
Collaborator

@ariessa ariessa left a comment

Choose a reason for hiding this comment

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

The PR is more polished now. Please address all requested changes.

The blockers are all in the trust model rather than the code mechanics. As written, a provider can quietly weaken the privacy guarantees the hook advertises: race the client to lock the config, pick a verifier the hook does not actually vet, and exploit the empty requiredSelectors to brick jobs registered through MultiHookRouter. Together these turn "privacy-enforcing hook" into "envelope shape checker that the provider controls."

I've left inline comments with suggestion blocks where the fix is mechanical, and a description for the verifier whitelist (it needs Ownable + an admin setter, so it doesn't fit a one-line suggestion).

Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol
Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol
Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol
Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol Outdated
Comment thread contracts/hooks/PrivacyHook.sol
@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented May 5, 2026

Thanks @ariessa. All addressed.

The three things from your summary:

  • Race-the-client: only the client can configure now, provider gets Unauthorized.
  • Unvetted verifier: dropped the code.length check and added a trustedVerifiers whitelist behind Ownable,
    admin manages it via setTrustedVerifier.
  • Bricked-via-router: requiredSelectors returns [setBudget, submit] so the router catches missing pairings.

Smaller stuff:

  • Trust model section rewritten with your wording.
  • EncryptedSubmission emits wrappedKeyCount instead of the keys themselves.
  • Empty optParams reverts PrivacyNotConfigured instead of silently passing.
  • optParams length must be exactly 64 or 96 now, anything else hits InvalidOptParamsLength.
  • Added the shape-only note above the wrapped-key loop.
  • privacyConfigs cleared in both _postComplete and _postReject alongside envelopeCommitments.

Constructor takes (erc8183Contract_, owner_) now for Ownable.

Let me know if you want anything else tweaked.

@PRXVT PRXVT requested a review from ariessa May 5, 2026 21:19
Copy link
Copy Markdown
Collaborator

@ariessa ariessa left a comment

Choose a reason for hiding this comment

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

One last thing before this hook can be merged.

https://github.com/erc-8183/hook-contracts/pull/15/changes#r3193694787

Comment thread contracts/hooks/PrivacyHook.sol Outdated
@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented May 6, 2026

Thanks @ariessa.

Removed the _postComplete and _postReject overrides so envelopeCommitments and
privacyConfigs persist past terminal states. Both getters now return the real values after complete and reject.

Also rebased on current main and removed the zero-address check in the constructor since BaseERC8183Hook covers it.

@PRXVT PRXVT requested a review from ariessa May 6, 2026 10:06
@ariessa
Copy link
Copy Markdown
Collaborator

ariessa commented May 11, 2026

Hi @PRXVT,

These four items block merge. Approve once all four land.

A blocker here is one of: the hook does not behave as its own docs say (1), it silently narrows a public core flow without telling the caller (2), a typed field disagrees with the documented contract (3), or the code will not compile against current main and violates the hook style rules (4). Items in the non-blocking section at the bottom are tradeoffs you have already opted into or hardening that does not change shipped behavior. Record them, but they do not gate this PR.

  1. complete / reject are documented as clearing envelopeCommitments[jobId]. Remove the claim.

  2. _postSetBudget requires non-empty optParams on every call and reverts with ConfigAlreadySet after the first. Core allows multiple setBudget calls while Open and allows the provider to call it. This hook silently locks the budget at the first call and forbids provider-initiated setBudget. Decide whether that is intentional; if it is, document it; if it isn't, gate config-set on a sentinel rather than on optParams != "".

  3. cid is bytes32. Real IPFS CIDs do not fit (CIDv0 is 34 bytes, CIDv1 is variable). The doc's "submit an IPFS CID" wording and any circuit that binds cid to a real content identifier will not line up. Either change the field to bytes, or rewrite the doc to "32-byte content commitment, e.g. sha256(CID)" and update the ZK public-input wording to match.

  4. Rebase onto current main and bring the PR into compliance with the hook PR rules before re-requesting review. Two items are not met today.

    • Stale submodule reference. PrivacyHook.sol imports @erc8183/AgenticCommerce.sol and casts AgenticCommerce(erc8183Contract) on line 141. The submodule was bumped in a6de09a and the contract is now ERC8183 (@erc8183/ERC8183.sol). Once rebased, this hook will not compile until the import and the type cast are updated. The other in-tree hooks already carry the new name on main; only PrivacyHook ships with the stale reference.

    • Named imports required in hook files per CONTRIBUTING.md Code style (https://github.com/erc-8183/hook-contracts/blob/main/CONTRIBUTING.md#code-style, added in 4c90be3): import {Symbol} from "path", never whole-file import "path". PrivacyHook.sol uses plain imports for all four imports.

Non-blocking. None of these gate merge. Each one is either a tradeoff the design already takes on or a hardening step that does not change shipped behavior. Address in this PR if convenient, otherwise file follow-ups.

  • Owner can untrust a verifier but cannot invalidate it for in-flight jobs. Not a gate: per-job config is frozen at setBudget time by design, so this is the chosen immutability tradeoff. Flag in NatSpec so operators know remediation is bounded.

  • minWrappedKeys = 1 is permitted and defeats the privacy property. Not a gate: the floor is caller-configurable and the hook cannot infer recipient intent. Either raise the on-chain floor to 2 or document that 1 means "self-broadcast, no privacy".

  • _postSubmit decodes the standard tuple shape even when config.numPublicInputs > 2. Not a gate: abi.decode tolerates the trailing bytes32[] today and the emitted cid / wrappedKeys.length are correct; only the unused extra inputs are dropped. Fragile if the wire format ever changes.

  • IZKVerifier.verify is called via external, not staticcall; view on the interface is not enforced by Solidity. Not a gate: the trust model already concedes verifier trust to the hook owner. Switching to staticcall closes the gap between the interface's view declaration and the actual call.

@PRXVT
Copy link
Copy Markdown
Contributor Author

PRXVT commented May 11, 2026

Thanks @ariessa, took another pass and got all four blockers plus the non-blockers in.

Blockers:

  1. Removed the clearing claim from the docs. The complete/reject step in the flow just says "normal flow" now.
  2. _postSetBudget gates on the config.configured sentinel instead of optParams length. Empty optParams is a no-op
    once the job is configured, so multi-setBudget by either party works the way core intends. Kept
    first-call-must-include-config and client-only-initial-write, and documented both in the NatSpec as intentional
    (race-the-client is the reason for the latter).
  3. Rewrote the cid wording everywhere it mattered. It now reads as a "32-byte content commitment, e.g. sha256(CID)"
    in the @ notice, use case, flow, and trust model. The ZK public-input wording lines up with that now.
  4. Rebased on current main. All four imports are named, AgenticCommerce got renamed to ERC8183 in both the cast and
    the section comment.

Non-blockers:

  • NatSpec note that untrusting a verifier does not invalidate in-flight jobs.
  • NatSpec note that minWrappedKeys=1 means self-broadcast, no privacy.
  • _postSubmit branches on numPublicInputs now instead of leaning on abi.decode's tolerance for trailing fields.
  • IZKVerifier.verify goes through staticcall with abi.encodeCall.

Copy link
Copy Markdown
Collaborator

@ariessa ariessa left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Merging this PR as it passes all requirements 🎉

@ariessa ariessa merged commit 3abab14 into erc-8183:main May 11, 2026
douglasborthwick-crypto added a commit to douglasborthwick-crypto/hook-contracts that referenced this pull request May 14, 2026
… error rename

- WalletStateHook.sol: convert to named imports (`{BaseERC8183Hook}`, `{IERC8183HookMetadata}`) per PR erc-8183#45 hook PR rules.
- WalletStateHook.t.sol: update test_Constructor_RevertsOnZeroCore to expect `BaseERC8183Hook.InvalidERC8183Contract` (per upstream BaseERC8183Hook zero-core revert added in `0179241`).

Rebase resolves the README hook-table conflict from PR erc-8183#15 (PrivacyHook) by keeping both rows. forge test: 12/12 passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants