SIG-744: remove unused RiskGate functions#74
Conversation
There was a problem hiding this comment.
Summary
Removes the trade-time _riskGate(...) delegatecalls from openPosition, openPositionFor, and increasePosition in SignalsCore, drops the corresponding no-op gateOpenPosition / gateIncreasePosition implementations and the never-implemented gateRequestWithdraw declaration from RiskModule / IRiskModule, and prunes the two RiskGateCallOrder tests that asserted only that the no-op gates did not revert. The surviving gateCreateMarket enforcement path is untouched. ABI/JSON regeneration and .gas-snapshot refresh are bundled in.
Cross-PR Context
Sibling PRs (SIG-744)
v1-whitepaperPR#13 (OPEN): SIG-744: align whitepaper with v1 RiskModule enforcement surfacev1-docsPR#23 (OPEN): SIG-744: remove trade gate hook narrative from risk docs- Impact map check: no
v1-subgraph/v1-sdk/v1-serverPRs needed.IRiskModuleis not referenced in any downstream repo (verified via grep acrossv1-subgraph,v1-sdk,v1-server,signals-app,v1-admin); the impact map atv1-skills/references/impact-map.mdlists no risk-module dependency.
Issues
No issues found.
Verifications performed
- Confirmed zero in-tree callers of the removed selectors via
grep -r 'gateOpenPosition|gateIncreasePosition|gateRequestWithdraw'(only the four files in this diff matched). - Confirmed
RiskModulecarries theonlyDelegatedmodifier (contracts/modules/RiskModule.sol:36-39), so any external direct call against the deployedRiskModuleaddress reverts withNotDelegated()regardless of which selectors exist. The interface shrinkage cannot break an off-chain integration. - Confirmed the removed gate bodies (
marketId; trader; quantity;etc.) had norequire/revert/ state mutation, so removing both the call sites and the implementations is behavior-preserving foropenPosition/openPositionFor/increasePosition. Theviewqualifier rules out lost events or storage writes. - Confirmed
_riskGatestill exists inSignalsCore.sol:773and is still wired togateCreateMarketat line 451, so the surviving market-creation enforcement path is unaffected. - Confirmed
nonReentrantordering is unchanged: the reentrancy critical section now wraps only the_delegate(tradeModule, ...)call, which is a tightening, not a loosening, of the invariant. - Confirmed
gateRequestWithdrawwas indeed never implemented inRiskModule(interface-only, never a concrete function), so its removal from the interface is a pure cleanup with no caller to break. - SignalsCore external function ABI surface in
abis/SignalsCore.jsonis preserved; only thebytecodefield changed, consistent with removing the three pre-delegate calls.
Suggestions
Gas snapshot churn is broader than the directive
The .gas-snapshot diff removes 434 entries and adds 311. The bulk of the disappearance is testFuzz_* and invariant_* lines (e.g. ClmsrInvariantTest:invariant_*, LazyMulSegmentTreeFuzzTest:*, FixedPointMathFuzz:*, FeeWaterfallLibFuzzTest:*). This is consistent with package.json's snapshot script (forge snapshot --no-match-test 'testFuzz|invariant_'), which excludes those by design, while the previous on-disk snapshot (last refreshed in PR #63) predates the exclusion. So the churn is harmless, but two follow-ups would make future SIG-744-style refactors easier to read:
- Consider a one-shot housekeeping commit on
mainthat regenerates.gas-snapshotstandalone, so future PRs only show the load-bearing deltas. - The
ApprovedOperatorTest:*entries appear added because that suite shipped after the last snapshot refresh (PR #69,test/foundry/integration/trade/ApprovedOperator.t.sol). Same housekeeping commit would absorb that drift too.
Status: Accepted. The PR description explicitly flags the broad churn and identifies the load-bearing deltas. No code change required for this PR.
Test file naming
test/foundry/integration/core/RiskGateCallOrder.t.sol retains the name and the contract RiskGateCallOrderTest but, post-PR, only covers the createMarket gate plus generic open/increase valid paths. The header comment was updated, but a future rename to something like RiskCreateMarketGate.t.sol (or moving the open/increase valid-path tests into an existing trade-suite) would more honestly reflect the file's scope.
Status: Accepted. Purely cosmetic, out of scope for a deletion-only directive.
Verdict
APPROVE: zero unresolved items. Directive cleanly executed across interface, implementation, call sites, and tests with no surviving stale gate surface. Sibling narrative PRs in v1-whitepaper and v1-docs cover the documentation side. Both suggestions above are accepted as out-of-scope cosmetic follow-ups that would not justify blocking this change.
Context
SIG-744.
RiskModule.gateOpenPositionandgateIncreasePositionwere no-op view functions that silenced their parameters with a// Exposure cap enforcement pending implementationcomment.IRiskModule.gateRequestWithdrawhad no implementation at all.SignalsCorestill issued a_riskGate(...)delegatecall into the two trade-time gates fromopenPosition,openPositionFor, andincreasePositionon every trade.Per-trade alpha enforcement was removed in commit
0908712("remove per-trade alpha enforcement, keep market-creation-only"). The placeholder gates added afterward for a future exposure-cap feature were never implemented and now confuse the audit narrative ("why call empty functions on every trade?") while burning ~700 gas per call on Citrea.Decisions
Deletion-first across interface, implementation, call sites, and tests in a single change so no layer keeps a stale gate surface. No replacement abstraction added:
gateCreateMarketis the only real risk gate, andSignalsCore._riskGatealready supports it.Surviving valid-path tests in
RiskGateCallOrder.t.sol(test_openPosition_succeeds_with_valid_params,test_increasePosition_updates_quantity) kept as-is; the two removed tests (test_openPosition_gate_no_revert,test_increasePosition_gate_no_revert) only asserted that no-op hooks did not revert. No new regression test added because the directive removes dead behavior rather than introducing a new path.Impact
SignalsCoreexternal ABI surface is unchanged. Only internal bytecode (regeneratedabis/SignalsCore.jsonandabis/SignalsCoreHarness.json) and the internalIRiskModuledelegatecall surface differ. No subgraph events touched, no SDK calculation helpers touched.The
IRiskModuleinterface is internal to v1-contract (used only bySignalsCoreforabi.encodeCallagainst the RiskModule delegate). No downstream repo (v1-subgraph,v1-sdk,v1-server) consumes those three signatures.Risk Areas
contracts/core/SignalsCore.solopenPosition / openPositionFor / increasePosition: trade entrypoint behavior with the pre-delegate_riskGatecall removed. Reviewer should confirm no other side effect (e.g. reentrancy ordering, event emission expectations) depended on the gate executing before_delegate(tradeModule, ...)..gas-snapshotchurn is broad becauseyarn snapshotregenerated the full file under current Foundry output; the load-bearing deltas are the trade entrypoints losing the delegatecall and the two removedRiskGateCallOrdertests.IRiskModuleABI shape change: any off-chain caller that hand-encodedgateOpenPosition/gateIncreasePosition/gateRequestWithdrawselectors against the deployed RiskModule address would now hit a non-existent function. Repo grep confirms zero in-tree callers; out-of-tree callers are unexpected because RiskModule is a delegatecall target rather than a directly invoked contract.