feat: implement parametric E3 pricing [skip-line-limit]#1422
Open
hmzakhalid wants to merge 17 commits intomainfrom
Open
feat: implement parametric E3 pricing [skip-line-limit]#1422hmzakhalid wants to merge 17 commits intomainfrom
hmzakhalid wants to merge 17 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an on-chain PricingConfig to Enclave, dynamic fee computation in getE3Quote, owner-settable pricing via setPricingConfig, protocol treasury share extraction in reward distribution, committee threshold validations, tests, and deployment/script/config updates. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Enclave as Enclave (contract)
participant Pricing as PricingConfig (storage)
participant Treasury as ProtocolTreasury
participant Nodes as CommitteeNodes
Client->>Enclave: call getE3Quote(params)
Enclave->>Pricing: read PricingConfig
Pricing-->>Enclave: pricing fields
Enclave->>Enclave: compute fee (components + duration + margin)
Enclave-->>Client: fee (view)
Client->>Enclave: request(...) with USDC approval & transfer
Enclave->>Enclave: _distributeRewards(totalFee, committee)
Enclave->>Treasury: transfer protocolShare (protocolShareBps)
Enclave->>Nodes: distribute remainder among committee nodes
Nodes-->>Enclave: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/interfaces/IEnclave.sol`:
- Around line 79-92: The PricingConfig struct includes publicationPerByte but
getE3Quote never uses it; update the fee logic in getE3Quote to include a
per-byte publication charge by adding (pricing.publicationPerByte * outputSize)
to the publication fee (ensure getE3Quote has or accepts an outputSize/numBytes
parameter and use pricing.publicationBase + pricing.publicationPerByte *
outputSize), or if per-byte pricing is not intended, remove publicationPerByte
from PricingConfig and all initializations and deployments that set it;
reference the PricingConfig struct, its publicationPerByte field, and the
getE3Quote function when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0133057-ac67-4583-99ae-52a9d243fbe6
📒 Files selected for processing (8)
packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/test/Pricing/Pricing.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/enclave-contracts/contracts/Enclave.sol (1)
569-583: Skip CN reward transfer path when CN allocation is zero.Avoid external approve/distribute calls when
cnAmount == 0; it reduces unnecessary risk and call overhead.♻️ Suggested change
- paymentToken.forceApprove(address(bondingRegistry), cnAmount); - - bondingRegistry.distributeRewards(paymentToken, activeNodes, amounts); - - paymentToken.forceApprove(address(bondingRegistry), 0); + if (cnAmount > 0) { + paymentToken.forceApprove(address(bondingRegistry), cnAmount); + bondingRegistry.distributeRewards(paymentToken, activeNodes, amounts); + paymentToken.forceApprove(address(bondingRegistry), 0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/contracts/Enclave.sol` around lines 569 - 583, The CN reward path should be skipped when cnAmount == 0 to avoid unnecessary approves/calls; in the function handling distribution, add a guard that if cnAmount == 0 you do not build the amounts array, call paymentToken.forceApprove(address(bondingRegistry), ...), or call bondingRegistry.distributeRewards(paymentToken, activeNodes, amounts). Locate the block that computes amount/amounts/distributed and the subsequent calls to paymentToken.forceApprove and bondingRegistry.distributeRewards and wrap them in a conditional (or early continue/return) so they run only when cnAmount > 0.packages/enclave-contracts/test/Pricing/Pricing.spec.ts (1)
331-342: Unused helper function.The
makeRequesthelper is defined but not used in any of the test cases. Tests at lines 597-598, 679-680, and 766-767 manually callapproveandrequestinstead.Consider either using this helper in the tests for consistency or removing it to reduce dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/Pricing/Pricing.spec.ts` around lines 331 - 342, The helper makeRequest is unused; either remove it or refactor tests to use it for consistency: replace the manual sequences of calling enclave.getE3Quote/ usdcToken.approve(...) and enclave.request(...) in the tests that manually perform approve+request with a single call to makeRequest(enclave, usdcToken, requestParams, signer) (ensure signer is passed when tests connect contracts) so fee retrieval and approval are centralized via makeRequest; alternatively, delete the makeRequest function if you prefer to keep the explicit approve/request flows (remove references to makeRequest and its imports like Enclave.getE3Quote and MockUSDC if orphaned).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 1043-1053: The setPricingConfig function currently allows a
PricingConfig with protocolShareBps > 0 and protocolTreasury == address(0),
which causes protocol rewards to be skipped; update the validation in
setPricingConfig to reject such configs by adding a require that when
config.protocolShareBps > 0 then config.protocolTreasury != address(0) before
assigning _pricingConfig and emitting PricingConfigUpdated; reference the
PricingConfig struct fields protocolShareBps and protocolTreasury and the
setPricingConfig function/_pricingConfig assignment to locate where to add this
check.
- Around line 1071-1075: The getE3Quote function currently doesn't validate that
the provided committee size is configured in committeeThresholds; add the same
guard used in request() by loading uint32[2] memory threshold =
committeeThresholds[committeeSize] (or using requestParams.committeeSize if that
variable is in scope) and require that threshold[1] != 0 (or both elements
non-zero) with a clear revert string like "invalid committee size" before using
threshold to compute n and m; this mirrors request() behavior and prevents
unconfigured sizes from being accepted.
---
Nitpick comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 569-583: The CN reward path should be skipped when cnAmount == 0
to avoid unnecessary approves/calls; in the function handling distribution, add
a guard that if cnAmount == 0 you do not build the amounts array, call
paymentToken.forceApprove(address(bondingRegistry), ...), or call
bondingRegistry.distributeRewards(paymentToken, activeNodes, amounts). Locate
the block that computes amount/amounts/distributed and the subsequent calls to
paymentToken.forceApprove and bondingRegistry.distributeRewards and wrap them in
a conditional (or early continue/return) so they run only when cnAmount > 0.
In `@packages/enclave-contracts/test/Pricing/Pricing.spec.ts`:
- Around line 331-342: The helper makeRequest is unused; either remove it or
refactor tests to use it for consistency: replace the manual sequences of
calling enclave.getE3Quote/ usdcToken.approve(...) and enclave.request(...) in
the tests that manually perform approve+request with a single call to
makeRequest(enclave, usdcToken, requestParams, signer) (ensure signer is passed
when tests connect contracts) so fee retrieval and approval are centralized via
makeRequest; alternatively, delete the makeRequest function if you prefer to
keep the explicit approve/request flows (remove references to makeRequest and
its imports like Enclave.getE3Quote and MockUSDC if orphaned).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbaabff4-e0dd-4bf1-98d5-5cafe3138c02
📒 Files selected for processing (11)
packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/token/EnclaveTicketToken.sol/EnclaveTicketToken.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/test/Pricing/Pricing.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
- packages/enclave-contracts/contracts/interfaces/IEnclave.sol
- packages/enclave-contracts/scripts/deployEnclave.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/enclave-contracts/artifacts/contracts/verifier/ThresholdPkAggregationVerifier.sol/ZKTranscriptLib.json`:
- Around line 393-394: Update the artifact metadata so inputSourceName matches
the actual source: replace the incorrect "inputSourceName":
"project/contracts/verifier/DkgPkVerifier.sol" with
"project/contracts/verifier/ThresholdPkAggregationVerifier.sol" in the
ZKTranscriptLib.json artifact (the one containing the ZKTranscriptLib entry) so
it aligns with the existing sourceName and other artifacts; ensure the change is
applied to the artifact JSON that references ThresholdPkAggregationVerifier.sol
to keep build metadata consistent.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 1066-1077: After loading threshold and deriving n and m in
getE3Quote/request paths, add runtime checks that enforce the stored values meet
current pricing-config minimums: require(n >= pc.minCommitteeSize && m >=
pc.minThreshold) (with appropriate error/revert like
CommitteeSizeTooSmall/ThresholdTooSmall) so previously-written thresholds cannot
bypass governance-updated minimums; locate the check right after "uint256 n =
uint256(threshold[1]); uint256 m = uint256(threshold[0]); PricingConfig memory
pc = _pricingConfig;" in the getE3Quote and request-related code paths and use
the existing PricingConfig fields (minCommitteeSize/minThreshold) and existing
error types or add clear revert errors if needed.
- Around line 1079-1083: The subtraction of requestParams.inputWindow[0] from
requestParams.inputWindow[1] can underflow; before computing duration, add an
explicit guard like require(requestParams.inputWindow[1] >=
requestParams.inputWindow[0], "InvalidInputWindow") (or revert with your
domain-specific error) to validate ordering of requestParams.inputWindow, then
compute duration using the existing expression that adds
_timeoutConfig.dkgWindow, _timeoutConfig.computeWindow and
_timeoutConfig.decryptionWindow; ensure the check is placed immediately before
the duration calculation that references requestParams.inputWindow and
_timeoutConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85cb6d50-c087-46f2-bd3d-81a4a9acf2de
📒 Files selected for processing (3)
packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/verifier/ThresholdPkAggregationVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/contracts/Enclave.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
...ntracts/artifacts/contracts/verifier/ThresholdPkAggregationVerifier.sol/ZKTranscriptLib.json
Outdated
Show resolved
Hide resolved
hmzakhalid
force-pushed
the
feat/e3-pricing
branch
from
58ad7b1 to
f5f3094
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 565-579: The protocol split is using live _pricingConfig values
during payout, allowing owner changes between request() and
publishPlaintextOutput() to redirect fees; mirror the existing _e3FeeTokens
snapshot approach by adding snapshot fields (e.g., protocolShareBps and
protocolTreasury) to the Request struct when request() is created and set them
from _pricingConfig at that time, then change the payout logic that currently
reads _pricingConfig.protocolShareBps and _pricingConfig.protocolTreasury to use
the stored request.protocolShareBps and request.protocolTreasury (and keep the
existing protocolAmount calculation and transfer steps). Ensure any related
codepaths that assume _pricingConfig are updated to use the request-level
snapshot to preserve token-rotation safety.
- Around line 1137-1151: The code currently hard-codes proofsPerNode = 6 + 2 *
(n - 1) * 2 which embeds a "2-moduli" BFV assumption; update the calculation to
derive the moduli count from validated params instead of the magic 2 (or remove
proof-count surcharges from the quote path). Specifically, replace the literal 2
with a parameter (e.g., use pc.dkgModuliCount or compute dkgModuliCount from the
E3 params) when computing proofsPerNode, or move proofsPerNode-related
surcharges out of the quote calculation that uses keyGenFixedPerNode,
keyGenPerEncryptionProof, coordinationPerPair and verificationPerProof so the
quote uses only the validated n/m/duration/publication inputs; ensure
proofsPerNode is computed from the actual DKG modulus count and any L_dkg factor
available in the parameter set and validate that parameter before using it.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 236-252: The pricing config currently hardcodes protocolTreasury
to ownerAddress in the call to enclave.setPricingConfig, which routes fees to
the deployer; change this to accept a configurable treasury parameter (e.g.,
pass a treasuryAddress variable or CLI/env value) and use that when calling
setPricingConfig instead of ownerAddress; update any function signatures or
deployment argument parsing that constructs the config so the treasury can be
supplied (reference enclave.setPricingConfig, protocolTreasury, and
ownerAddress) and ensure a sensible default or validation if the treasury is
missing.
In `@packages/enclave-contracts/test/Pricing/Pricing.spec.ts`:
- Around line 524-529: The assertions compare bigint-returned uint256 fields to
JS numbers (e.g., expect(pc.marginBps).to.equal(0)), which fails under ethers
v6; update tests to use bigint literals for expected values (e.g., 0n, 50000n)
wherever getPricingConfig() or other contract calls return bigint
fields—specifically change assertions in the "allows setting margin to 0" test
that reference getPricingConfig()/pc.marginBps and update all assertions in the
"Default pricing parameters" test to use bigint literals matching the contract
defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3209954e-1fce-43b7-a260-1caaf51b8012
📒 Files selected for processing (15)
examples/CRISP/enclave.config.yamlexamples/CRISP/packages/crisp-contracts/deployed_contracts.jsonexamples/CRISP/server/.env.examplepackages/enclave-contracts/artifacts/contracts/Enclave.sol/Enclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.jsonpackages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.jsonpackages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.jsonpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/contracts/interfaces/IEnclave.solpackages/enclave-contracts/scripts/deployAndSave/mockPkVerifier.tspackages/enclave-contracts/scripts/deployEnclave.tspackages/enclave-contracts/scripts/deployMocks.tspackages/enclave-contracts/test/Pricing/Pricing.spec.ts
✅ Files skipped from review due to trivial changes (6)
- packages/enclave-contracts/scripts/deployAndSave/mockPkVerifier.ts
- packages/enclave-contracts/scripts/deployMocks.ts
- packages/enclave-contracts/artifacts/contracts/interfaces/IBondingRegistry.sol/IBondingRegistry.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ICiphernodeRegistry.sol/ICiphernodeRegistry.json
- packages/enclave-contracts/artifacts/contracts/registry/CiphernodeRegistryOwnable.sol/CiphernodeRegistryOwnable.json
- packages/enclave-contracts/artifacts/contracts/interfaces/ISlashingManager.sol/ISlashingManager.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/enclave-contracts/artifacts/contracts/interfaces/IEnclave.sol/IEnclave.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/enclave-contracts/contracts/Enclave.sol (1)
1145-1159:⚠️ Potential issue | 🟠 MajorThe quote formula still charges non-spec proof surcharges.
The PR objective defines
getE3Quote()in terms ofn,m,duration, coordination, availability, decryption, publication, and margin. Lines 1145-1159 add proof-count and verification costs with a hard-coded2-moduli assumption, so on-chain fees will not match the published pricing formula.🧮 Minimal fix to align the quote path with the stated formula
- // ZK proof count per node: 6 fixed + 2 × (N-1) × L_dkg scaling - // TODO: get dkgModuliCount from E3 params instead of hardcoding - uint256 proofsPerNode = 6 + 2 * (n - 1) * 2; - - // Key generation cost: fixed per-node + per-proof (quadratic in n) + // Key generation cost (linear in n) uint256 baseFee = pc.keyGenFixedPerNode * n; - baseFee += pc.keyGenPerEncryptionProof * n * proofsPerNode; // Key generation coordination cost (quadratic in n) if (n > 1) { baseFee += (pc.coordinationPerPair * (n * (n - 1))) / 2; } - // Proof verification cost: each node verifies all others' proofs (quadratic) - baseFee += pc.verificationPerProof * n * proofsPerNode; - // Availability cost (linear in n × duration) baseFee += pc.availabilityPerNodePerSec * n * duration;
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 573-586: The protocol treasury split is being skipped on the early
return when activeLength == 0; compute and apply the protocol share using
_e3ProtocolShareBps[e3Id] and _e3ProtocolTreasury[e3Id] (calculate
protocolAmount and call paymentToken.safeTransfer to _protocolTreasury if >0)
before the zero-active-member refund path/return so the treasury still receives
its configured share even when all committee members are expelled; ensure you
still subtract protocolAmount from the remaining refund logic (cnAmount =
totalAmount - protocolAmount).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79faf048-d2c4-44fd-a3b3-f404257fa76c
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackages/enclave-contracts/contracts/Enclave.solpackages/enclave-contracts/scripts/deployEnclave.ts
hmzakhalid
force-pushed
the
feat/e3-pricing
branch
from
44ccd26 to
8089a4a
Compare
ctrlc03
reviewed
Mar 26, 2026
|
|
||
| /// @inheritdoc IEnclave | ||
| function setPricingConfig(PricingConfig calldata config) public onlyOwner { | ||
| require(config.marginBps <= BPS_BASE, "Margin exceeds 100%"); |
Collaborator
There was a problem hiding this comment.
should we use custom errors to be consistent?
|
|
||
| uint256 cnAmount = totalAmount - protocolAmount; | ||
|
|
||
| if (activeLength == 0) { |
Collaborator
There was a problem hiding this comment.
should we do this before we pay the protocol? as in, should requesters be made whole if all nodes were malicious?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
E3 fees should be a function of committee size ($c^1$ ), decryption threshold ($c^2$ ), time availability ($\hat{t}$ ), and publication costs. The critical insight is that coordination costs in a flat committee topology scale quadratically with $O\binom{n}{2}$ pairwise interactions, so the fee formula must capture this or large committees become economically unsustainable for ciphernodes.
How the pricing works
The fee formula in
getE3Quote():Where:
threshold[1])threshold[0])inputWindow[1] - inputWindow[0] + dkgWindow + computeWindow + decryptionWindowOn successful completion,
_distributeRewards()now splits the fee:protocolShareBps(e.g. 20%) → protocol treasurySummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores