chore: remove unused keyshare code [skip-line-limit]#1199
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes legacy plaintext aggregation and keyshare implementations, replacing them with threshold-based alternatives, updates repository factories, simplifies CiphernodeBuilder by removing legacy options, removes legacy integration tests and CI execution, and adds new P2P event-forwarding tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
hmzakhalid
changed the title
hmzakhalid
changed the title
|
i need this for #1197 |
|
i would like a quick look from @ryardley before merging this in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
examples/CRISP/server/.env.example (1)
13-13: Use a clear placeholder for CRON_API_KEY.Even in
.env.example, a realistic-looking token can be mistaken for a real secret. Consider using a placeholder like"CHANGE_ME"to reduce confusion.💡 Suggested tweak
-CRON_API_KEY=1234567890 +CRON_API_KEY="CHANGE_ME"packages/enclave-contracts/scripts/deployAndSave/enclave.ts (1)
65-83: IncludetimeoutConfigin the reuse guard comparison.
Right now, changingtimeoutConfigstill reuses the previous deployment, leaving the proxy initialized with stale timeout settings while persisting the new config.🛠️ Suggested fix
const preDeployedArgs = readDeploymentArgs("Enclave", chain); + const timeoutConfigJson = timeoutConfig + ? JSON.stringify(timeoutConfig) + : undefined; if ( !params || !owner || !maxDuration || !registry || !bondingRegistry || !e3RefundManager || !feeToken || !timeoutConfig || (preDeployedArgs?.constructorArgs?.owner === owner && preDeployedArgs?.constructorArgs?.maxDuration === maxDuration && preDeployedArgs?.constructorArgs?.registry === registry && preDeployedArgs?.constructorArgs?.bondingRegistry === bondingRegistry && preDeployedArgs?.constructorArgs?.e3RefundManager === e3RefundManager && preDeployedArgs?.constructorArgs?.feeToken === feeToken && + preDeployedArgs?.constructorArgs?.timeoutConfig === timeoutConfigJson && areArraysEqual( preDeployedArgs?.constructorArgs?.params as string[], params, )) ) { ... } storeDeploymentArgs( { constructorArgs: { owner, registry, bondingRegistry, e3RefundManager, feeToken, maxDuration, - timeoutConfig: JSON.stringify(timeoutConfig), + timeoutConfig: timeoutConfigJson, params, }, ... }, "Enclave", chain, );Also applies to: 122-133
examples/CRISP/server/src/cli/commands.rs (2)
108-176: Recomputeinput_deadlineafter refreshingcurrent_timestamp.Line 113 computes
input_deadlineonce, but Line 152 refreshescurrent_timestamp/start_windowbefore the request while reusing the old deadline. If approval takes time, the deadline can become stale (or even earlier than the refreshed start window), which can cause immediate expiry or a revert. Consider recalculatinginput_deadlinejust beforerequest_e3(and re-quoting if the fee depends on the deadline).🛠️ Suggested fix (keep deadline aligned with refreshed timestamp)
- let input_deadline = U256::from(current_timestamp) + U256::from(CONFIG.e3_duration); + let mut input_deadline = U256::from(current_timestamp) + U256::from(CONFIG.e3_duration); @@ - current_timestamp = get_current_timestamp().await?; + current_timestamp = get_current_timestamp().await?; start_window = [ U256::from(current_timestamp), U256::from(current_timestamp + CONFIG.e3_window_size as u64), ]; + input_deadline = U256::from(current_timestamp) + U256::from(CONFIG.e3_duration);
256-264: Use the CRISP program address when constructingCRISPContract.Line 256 uses
CONFIG.enclave_addresswhen creatingCRISPContract. Elsewhere (e.g., CRISP contract factory usage) the CRISP program address is used. IfCRISPContracttargets the program contract, this will sendpublish_inputto the wrong address.🛠️ Suggested fix
- let contract = CRISPContract::new( - &CONFIG.http_rpc_url, - &CONFIG.private_key, - &CONFIG.enclave_address, - ) + let contract = CRISPContract::new( + &CONFIG.http_rpc_url, + &CONFIG.private_key, + &CONFIG.e3_program_address, + )templates/default/server/index.ts (1)
140-150: Potential deadlock:def.resolve()not called when session already exists.If an
E3_ACTIVATEDevent is received for ane3Idthat already has a session (e.g., duplicate events or re-activation scenarios),def.resolve()is never called because it's inside theif (!e3Sessions.has(sessionKey))block. This will causehandleInputPublishedEventto hang indefinitely onawait getActivationDefer(e3Id).promise(line 174).🐛 Proposed fix
if (!e3Sessions.has(sessionKey)) { e3Sessions.set(sessionKey, { e3Id, e3ProgramParams: e3.e3ProgramParams, expiration, inputs: [], isProcessing: false, isCompleted: false, }) - def.resolve() } + def.resolve()
🤖 Fix all issues with AI agents
In `@crates/aggregator/src/committee_finalizer.rs`:
- Around line 90-95: Update the info! log message to reference the renamed field
by replacing "Submission deadline already passed, finalizing with buffer" with
wording that uses "Committee deadline" (e.g., "Committee deadline already
passed, finalizing with buffer") in the info! call that includes e3_id =
%e3_id_for_async, committee_deadline, current_timestamp in
committee_finalizer.rs so the message matches the committee_deadline field.
In `@examples/CRISP/server/.env.example`:
- Around line 16-18: Reorder the dotenv keys so they satisfy dotenv-linter's
alphabetical ordering by moving E3_PROGRAM_ADDRESS above ENCLAVE_ADDRESS; update
the block containing ENCLAVE_ADDRESS, CIPHERNODE_REGISTRY_ADDRESS, and
E3_PROGRAM_ADDRESS so the three entries are alphabetically ordered
(E3_PROGRAM_ADDRESS, CIPHERNODE_REGISTRY_ADDRESS, ENCLAVE_ADDRESS) preserving
the existing values and comment on E3_PROGRAM_ADDRESS.
In `@packages/enclave-contracts/contracts/E3RefundManager.sol`:
- Around line 261-271: The honest-node check in claimHonestNodeReward uses an
O(n) loop over _honestNodes[e3Id] (isHonest variable) and the per-node payout
calculation amount = dist.honestNodeAmount / dist.honestNodeCount can leave
unclaimable remainder; change storage to include a mapping
_isHonestNode[e3Id][address] set during calculateRefund (alongside pushing into
_honestNodes) and replace the loop/require with
require(_isHonestNode[e3Id][msg.sender], NotHonestNode(e3Id, msg.sender)) in
claimHonestNodeReward, and adjust the payout logic to avoid dust—e.g., compute
baseShare and track claimed count or assign the remainder to the last claimant
using dist.honestNodeAmount, dist.honestNodeCount and a claimed counter so total
distributed equals honestNodeAmount.
In `@packages/enclave-contracts/contracts/Enclave.sol`:
- Around line 667-681: Replace the string-require checks in processE3Failure
with custom errors consistent with the contract's pattern: declare new errors
(e.g., E3NotFailed(uint256 e3Id) and E3NoPayment(uint256 e3Id)) near the other
E3 errors, then replace require(stage == E3Stage.Failed, "E3 not failed") with a
check that reverts E3NotFailed(e3Id) and replace require(payment > 0, "No
payment to refund") with a check that reverts E3NoPayment(e3Id); keep the
existing e3Payments clearing, transfer, calculateRefund call, and the
E3FailureProcessed emit unchanged.
In `@packages/enclave-contracts/contracts/interfaces/IE3.sol`:
- Around line 19-22: Update the docstring for the `@param` duration in the IE3
interface to fix the typo "commitee" → "committee"; locate the comment near the
duration parameter declaration in IE3.sol (interface IE3 and the `@param` duration
line) and correct the word so the comment reads "Duration of committee duties".
In `@packages/enclave-contracts/contracts/interfaces/IE3RefundManager.sol`:
- Around line 109-112: Add a `@dev` note to the IE3RefundManager interface for the
function routeSlashedFunds stating that it can only be called by the Enclave
contract (the caller used by the slashing mechanism), so readers know the
implementation enforces access via onlyEnclave; update the comment block above
function routeSlashedFunds in IE3RefundManager.sol to explicitly mention this
caller restriction.
In `@packages/enclave-contracts/contracts/registry/BondingRegistry.sol`:
- Around line 87-88: The new state variable numActiveOperators can be zero after
upgrade and cause underflow when _updateOperatorStatus decrements; add an
explicit migration/initializer to set the correct current active operator count
(or add an admin-only setter) and call it as part of the upgrade path before any
code that can decrement the counter runs. Specifically, provide a protected
function (e.g., initializeNumActiveOperators or setNumActiveOperators) guarded
by onlyOwner/initializer semantics that computes or accepts the correct active
count and assigns numActiveOperators, mark it to be callable only once (or only
by the upgrade controller), and ensure any calls to _updateOperatorStatus occur
only after this seeding step (also update any upgrade scripts/tests to invoke
the migration).
In `@packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol`:
- Around line 348-378: The change to return bool is a breaking interface;
restore the original revert behavior in finalizeCommittee by removing the
"returns (bool success)" from the function signature, move the c.finalized =
true; assignment to after the threshold check, and replace the "if
(!thresholdMet) { ... return false; }" block with a require(thresholdMet,
InsufficientCommitteeMembers()); (or existing appropriate custom error), so the
function reverts when the committee threshold is not met and only finalizes (set
c.finalized, set c.committee, emit CommitteeFinalized, call
enclave.onCommitteeFinalized) when thresholdMet is true.
In `@packages/enclave-contracts/scripts/deployEnclave.ts`:
- Around line 151-163: The call to enclave.setE3RefundManager(...) only waits
for the transaction to be broadcast in ethers v6; capture the
TransactionResponse returned by enclave.setE3RefundManager and call await
tx.wait() before proceeding (i.e., await the mining confirmation). In the same
vein, verify that deployAndSaveE3RefundManager returns a fully-mined contract or
add awaiting there; but at minimum change the sequence around
enclave.setE3RefundManager to store the tx response and await tx.wait() to
ensure the state change is mined before moving on.
In `@packages/enclave-contracts/tasks/enclave.ts`:
- Around line 53-58: The `duration` option added via .addOption has a mismatched
description ("default: 3 days") and defaultValue (86400, which is 1 day); update
the code in the addOption for "duration" to make them consistent—either change
defaultValue to 86400 * 3 (259200) to match "3 days" or change the description
to "default: 1 day" (or equivalent) to match defaultValue; locate the .addOption
call for "duration" to apply the fix.
In `@packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts`:
- Around line 401-421: The test name "reverts if committee has already been
requested for given e3Id" is outdated; either rename the it(...) description to
reflect that it verifies the first request succeeds and rootAt(0) equals root
(update the string to something like "sets rootAt(0) after first request"), or
restore a duplicate-request assertion by invoking the same request twice (use
makeRequest to perform the first request, then call the contract path that would
create a second request for the same e3Id and assert it reverts) and keep the
original title; locate the test block using the it(...) string and the helper
makeRequest plus registry.rootAt/registry.root symbols to apply the change.
🧹 Nitpick comments (22)
examples/CRISP/packages/crisp-contracts/contracts/Mocks/MockEnclave.sol (2)
9-9: Unused import.
IEnclaveis imported but not used in this contract. Consider removing it to keep the code clean.🧹 Proposed fix
-import { IEnclave } from "@enclave-e3/contracts/contracts/interfaces/IEnclave.sol";
50-70: Consider returning the stored E3 directly.
getE3()reconstructs an E3 struct with mostly hardcoded values while only readingduration,expiration, andinputDeadlinefrom storage. For consistency and simpler test scaffolding, consider returning the stored E3 directly.♻️ Proposed simplification
function getE3(uint256 e3Id) external view returns (E3 memory) { - return - E3({ - seed: 0, - threshold: [uint32(1), uint32(2)], - requestBlock: 0, - startWindow: [uint256(0), uint256(0)], - duration: e3s[e3Id].duration, - expiration: e3s[e3Id].expiration, - inputDeadline: e3s[e3Id].inputDeadline, - encryptionSchemeId: bytes32(0), - e3Program: IE3Program(address(0)), - e3ProgramParams: bytes(""), - customParams: bytes(""), - decryptionVerifier: IDecryptionVerifier(address(0)), - committeePublicKey: committeePublicKey, - ciphertextOutput: bytes32(0), - plaintextOutput: plaintextOutput, - requester: address(0) - }); + E3 memory e3 = e3s[e3Id]; + e3.committeePublicKey = committeePublicKey; + e3.plaintextOutput = plaintextOutput; + return e3; }examples/CRISP/server/src/server/models.rs (1)
230-245: Minor field ordering inconsistency.In
E3Crisp, theinput_deadlinefield is placed at the end of the struct, whereas inE3StateLiteandE3, it's grouped with other timing fields (start_time,duration,expiration). Consider moving it for consistency.♻️ Suggested reordering for consistency
pub struct E3Crisp { pub emojis: [String; 2], pub has_voted: Vec<String>, pub start_time: u64, + pub input_deadline: u64, pub status: String, pub votes_option_1: String, pub votes_option_2: String, pub token_holder_hashes: Vec<String>, pub eligible_addresses: Vec<TokenHolder>, pub token_address: String, pub balance_threshold: String, pub ciphertext_inputs: Vec<(Vec<u8>, u64)>, pub requester: String, - pub input_deadline: u64, }examples/CRISP/crates/evm_helpers/src/lib.rs (1)
100-116: LGTM! Minor style nit.The new
publish_inputmethod correctly wraps the contract call. One minor observation: line 101 uses//for the comment while line 84 uses///for doc comments. Consider using///for consistency.Also,
data.into()on line 109 appears redundant sincedatais alreadyBytesand the generated binding expectsBytes. You could passdatadirectly.♻️ Optional style fix
- // publish an input to the CRISPProgram contract + /// Publish an input to the CRISPProgram contract pub async fn publish_input( &self, e3_id: U256, data: Bytes, ) -> Result<TransactionReceipt> { let contract = CRISPProgram::new(self.contract_address, self.provider.as_ref()); let receipt = contract - .publishInput(e3_id, data.into()) + .publishInput(e3_id, data) .send() .await? .get_receipt() .await?; Ok(receipt) }crates/tests/tests/integration.rs (1)
456-547: LGTM! Well-structured P2P event forwarding test.This test properly validates that:
- Network-destined events (
PlaintextAggregated) are forwarded to the network- Local-only events (
CiphernodeSelected) are NOT broadcast to the network- Forwarded events from network loopback are NOT re-published to the bus (deduplication)
Minor typo on line 472: "Pas" → "Pass".
📝 Typo fix
- // Pas cmd and event channels to NetEventTranslator + // Pass cmd and event channels to NetEventTranslatorpackages/enclave-sdk/src/contract-client.ts (1)
103-147: ClarifyinputDeadlinesemantics for SDK callers.This is a public SDK parameter now; a short param note helps prevent misuse (e.g., wrong units or meaning) and avoids contract reverts.
📝 Suggested doc tweak
/** * Request a new E3 computation * request(address filter, uint32[2] threshold, uint256[2] startWindow, uint256 inputDeadline, uint256 duration, IE3Program e3Program, bytes e3ProgramParams, bytes computeProviderParams, bytes customParams) + * `@param` inputDeadline Deadline for input publication (document expected units) */examples/CRISP/packages/crisp-contracts/deployed_contracts.json (1)
135-308: Confirm new deployment blobs aren’t secrets and theundefinedenv key is intentional.Gitleaks flagged several new hex blobs (initData) as “generic API keys.” They look like deterministic on-chain data, but please confirm no real secrets were introduced; if false positives, consider allowlisting this file/patterns to keep CI clean. Also sanity-check that the
undefinedenvironment key is expected (vs. a missing env var in deployment scripts).packages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.ts (1)
307-348: Avoid potential bigint/number mixing inmakeRequesttimestamps.If
networkHelpers.time.latest()returnsbigint(recent Hardhat helpers),currentTime + 100will throw. Using bigint literals keeps this safe and aligned with uint256 inputs.🔧 Safer bigint arithmetic
- const currentTime = await networkHelpers.time.latest(); + const currentTime = BigInt(await networkHelpers.time.latest()); const requestParams = { threshold: [2, 2] as [number, number], - startWindow: [currentTime, currentTime + 100] as [number, number], + startWindow: [currentTime, currentTime + 100n] as [bigint, bigint], duration: 60 * 60 * 24 * 30, // 30 days e3Program: await mockE3Program.mockE3Program.getAddress(), e3ProgramParams: encodedE3ProgramParams, - inputDeadline: currentTime + 500, + inputDeadline: currentTime + 500n, computeProviderParams: abiCoder.encode(packages/enclave-contracts/contracts/interfaces/IE3Program.sol (1)
40-45: ConsidercalldataforpublishInputto avoid extra copying.For external functions,
bytes calldatais the usual pattern; if you adopt it, update implementations/mocks accordingly.♻️ Suggested signature tweak
- function publishInput(uint256 e3Id, bytes memory data) external; + function publishInput(uint256 e3Id, bytes calldata data) external;examples/CRISP/server/src/server/routes/rounds.rs (1)
211-217: Use a single timestamp to avoid subtle skew.
CapturingUtc::now()once keepsstart_windowandinput_deadlinealigned.Proposed refactor
- let start_window: [U256; 2] = [ - U256::from(Utc::now().timestamp()), - U256::from(Utc::now().timestamp() + CONFIG.e3_window_size as i64), - ]; - let input_deadline = U256::from(Utc::now().timestamp()) + U256::from(CONFIG.e3_duration); + let now = Utc::now().timestamp(); + let start_window: [U256; 2] = [ + U256::from(now), + U256::from(now + CONFIG.e3_window_size as i64), + ]; + let input_deadline = U256::from(now) + U256::from(CONFIG.e3_duration);packages/enclave-contracts/scripts/deployAndSave/e3RefundManager.ts (2)
42-60: Clarify the reuse condition logic.The condition
!owner || !enclave || !treasury || (preDeployedArgs?.constructorArgs... matches)combines two distinct scenarios:
- Missing parameters → reuse existing deployment
- Parameters match stored values → reuse existing deployment
This is likely intentional (reuse when args not provided OR when they match), but the logic is subtle. Consider adding a comment to clarify intent, or restructuring for readability.
🔧 Suggested clarification
+ // Reuse existing deployment if: + // 1. Required parameters are not provided (caller wants to use existing) + // 2. Provided parameters match existing deployment if ( !owner || !enclave || !treasury || (preDeployedArgs?.constructorArgs?.owner === owner && preDeployedArgs?.constructorArgs?.enclave === enclave && preDeployedArgs?.constructorArgs?.treasury === treasury) ) {
70-71: Block number captured before proxy deployment.The
blockNumberis captured after the implementation deployment but before the proxy deployment. Since the proxy is the actual contract users interact with, consider capturing the block number after proxy deployment for more accurate tracking.🔧 Suggested fix
- const blockNumber = await ethers.provider.getBlockNumber(); const e3RefundManagerAddress = await e3RefundManager.getAddress(); const initData = e3RefundManagerFactory.interface.encodeFunctionData( "initialize", [owner, enclave, treasury], ); const ProxyCF = await ethers.getContractFactory( "TransparentUpgradeableProxy", ); const proxy = await ProxyCF.deploy(e3RefundManagerAddress, owner, initData); await proxy.waitForDeployment(); const proxyAddress = await proxy.getAddress(); + const blockNumber = await ethers.provider.getBlockNumber(); + const proxyAdminAddress = await getProxyAdmin(ethers.provider, proxyAddress);packages/enclave-contracts/contracts/E3RefundManager.sol (3)
72-95: Simplify ownership initialization.The pattern
__Ownable_init(msg.sender)followed by conditionaltransferOwnership(_owner)can be simplified to directly initialize with the owner.🔧 Suggested simplification
function initialize( address _owner, address _enclave, address _treasury ) public initializer { - __Ownable_init(msg.sender); + __Ownable_init(_owner); require(_enclave != address(0), "Invalid enclave"); require(_treasury != address(0), "Invalid treasury"); enclave = IEnclave(_enclave); feeToken = enclave.feeToken(); bondingRegistry = enclave.bondingRegistry(); treasury = _treasury; _workAllocation = WorkValueAllocation({ committeeFormationBps: 1000, dkgBps: 3000, decryptionBps: 5500, protocolBps: 500 }); - - if (_owner != owner()) transferOwnership(_owner); }
133-137: Consider gas optimization for storing honest nodes.The loop pushes each honest node individually, which incurs storage write costs per iteration. For large committees, this could be expensive. Consider if this is acceptable given expected committee sizes.
275-283: Consider using safeIncreaseAllowance instead of approve.Using
approveafter previous approvals can be problematic if there's a leftover allowance (front-running attack vector). WhilesafeApprovefrom SafeERC20 reverts if allowance is non-zero,safeIncreaseAllowanceor resetting to 0 first would be safer.However, since this immediately calls
distributeRewardswhich should consume the full allowance, the risk is minimal in practice.🔧 Alternative using forceApprove
- feeToken.approve(address(bondingRegistry), amount); + feeToken.forceApprove(address(bondingRegistry), amount);Note:
forceApprove(available in newer OpenZeppelin) handles non-standard ERC20s better.packages/enclave-contracts/contracts/interfaces/IEnclave.sol (2)
208-232: Duplicate "Structs" section header.There's a section header for "Structs" at lines 51-55, and another at lines 208-212. The
E3RequestParamsstruct at line 223 should likely be grouped with the other structs defined earlier, or the duplicate header should be removed.🔧 Suggested fix
- //////////////////////////////////////////////////////////// - // // - // Structs // - // // - //////////////////////////////////////////////////////////// - /// `@notice` This struct contains the parameters to submit a request to Enclave.Move
E3RequestParamsto the structs section near line 57-74, or remove the duplicate header.
379-383: Consider usingFailureReasonenum type directly for better type safety.The function accepts
uint8 reasonwhich is cast toFailureReasonenum. While this is a common cross-contract pattern in Solidity, acceptingFailureReasondirectly would provide compile-time type safety. The current pattern relies on callers correctly mapping valid enum values (0-13), as evidenced in the codebase where callers document the mapping inline (e.g.,onE3Failed(e3Id, 2); // FailureReason.InsufficientCommitteeMembers). The trade-off favorsuint8if external contracts with different ABI expectations need to call this function.packages/enclave-contracts/contracts/registry/CiphernodeRegistryOwnable.sol (2)
363-372: Magic number for failure reason should use a named constant.The hardcoded
2passed toenclave.onE3Failed(e3Id, 2)corresponds toFailureReason.InsufficientCommitteeMembers. This is fragile if the enum order changes.🔧 Suggested fix
Consider defining a constant or using a more explicit pattern:
+ // FailureReason.InsufficientCommitteeMembers = 2 + uint8 private constant FAILURE_INSUFFICIENT_COMMITTEE = 2; + if (!thresholdMet) { c.failed = true; emit CommitteeFormationFailed( e3Id, c.topNodes.length, c.threshold[1] ); - enclave.onE3Failed(e3Id, 2); // FailureReason.InsufficientCommitteeMembers + enclave.onE3Failed(e3Id, FAILURE_INSUFFICIENT_COMMITTEE); return false; }Alternatively, import the
IEnclaveinterface and cast the enum value explicitly.
531-535: Potential off-by-one withc.requestBlock - 1.Using
c.requestBlock - 1for the snapshot block assumes thatrequestBlockis always > 0. While this should always be true in practice (blocks start from genesis), consider adding a defensive check or documenting this assumption.The existing TODO comment on line 529-530 acknowledges this concern.
packages/enclave-contracts/contracts/Enclave.sol (2)
211-218: Consider using a custom error instead of string literal.The modifier uses a string literal
"Only CiphernodeRegistry"which is inconsistent with the rest of the contract that uses custom errors. This increases gas cost and reduces consistency.🔧 Suggested fix
Add a custom error and use it in the modifier:
+ /// `@notice` Caller is not the CiphernodeRegistry + error OnlyCiphernodeRegistry(); + /// `@notice` Restricts function to CiphernodeRegistry contract only modifier onlyCiphernodeRegistry() { require( msg.sender == address(ciphernodeRegistry), - "Only CiphernodeRegistry" + OnlyCiphernodeRegistry() ); _; }
651-662:setE3RefundManageruses string literal for error.Inconsistent with other setters that use custom errors. Also, unlike other setters, this doesn't check if the new value equals the current value.
🔧 Suggested fix
+ /// `@notice` Thrown when attempting to set an invalid E3 refund manager address. + /// `@param` e3RefundManager The invalid refund manager address. + error InvalidE3RefundManager(IE3RefundManager e3RefundManager); + function setE3RefundManager( IE3RefundManager _e3RefundManager ) public onlyOwner { require( - address(_e3RefundManager) != address(0), - "Invalid E3RefundManager address" + address(_e3RefundManager) != address(0) && + _e3RefundManager != e3RefundManager, + InvalidE3RefundManager(_e3RefundManager) ); e3RefundManager = _e3RefundManager; emit E3RefundManagerSet(address(_e3RefundManager)); }packages/enclave-contracts/contracts/interfaces/IE3RefundManager.sol (1)
20-26:WorkValueAllocationbasis points should be documented to sum to a specific total.The struct uses basis points (bps) where 10000 = 100%. The natspec should clarify whether these four fields should sum to exactly 10000 bps, or if they represent independent allocations. Based on the field names, it appears
protocolBpsis separate from the work stages (committeeFormation, dkg, decryption).📝 Suggested documentation improvement
/// `@notice` Work value allocation in basis points (10000 = 100%) + /// `@dev` committeeFormationBps + dkgBps + decryptionBps should equal (10000 - protocolBps) + /// `@dev` protocolBps represents the protocol's cut taken before work allocation struct WorkValueAllocation { uint16 committeeFormationBps; uint16 dkgBps; uint16 decryptionBps; uint16 protocolBps; }
hmzakhalid
force-pushed
the
chore/remove-keyshare-code
branch
from
cf664b1 to
62e3085
Compare
hmzakhalid
force-pushed
the
chore/remove-keyshare-code
branch
from
62e3085 to
67586d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/tests/tests/integration.rs`:
- Around line 448-536: The test test_p2p_actor_forwards_events_to_network is
racy because the spawned tokio task reading cmd_rx may not process GossipPublish
before the assertions run; add a short timeout-based wait after publishing and
before the assert_eq on msgs to ensure msgs contains the two expected entries.
Locate the spawned loop that pushes into msgs (msgs_loop) and, prior to the
first assert_eq, poll or await with tokio::time::timeout (or a small retry loop
with tokio::time::sleep) until msgs.lock().await.len() == 2 (fail the test if
the timeout elapses) so the assertions are deterministic. Ensure the timeout is
short (e.g., 100-500ms) to keep tests fast.
4 participants
Summary by CodeRabbit
Deprecations
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.