versioned interface implementation with strict ERC-165 validation

[0xJem]

Summary

This PR implements strict ERC-165 interface validation for submodule installation. Building on the price feed improvements in the base branch, it ensures all submodules properly implement the ISubmodule interface before installation.

Changes

Core Architecture

• src/Submodules.sol: Enhanced submodule base contract

  • Submodule now implements ISubmodule and IVersioned
  • Added supportsInterface() function with ERC-165 support
  • Added Module_SubmoduleInterfaceNotImplemented error

• src/interfaces/ISubmodule.sol: New interface (MIT-licensed)

  • Extends IVersioned
  • Defines PARENT(), SUBKEYCODE(), INIT() functions
  • Provides type safety and interface compliance checking

Submodule Interface Validation

• Strict ERC-165 requirement in _validateSubmodule(): Installation fails if:
  1. The contract doesn't implement supportsInterface (staticcall fails)
  2. The contract returns false for ISubmodule.interfaceId
  3. The contract returns empty data

Code Simplification

• Removed unnecessary override from PriceSubmodule.supportsInterface()
• Super delegation: All concrete PRICE submodules now use super.supportsInterface()

Test Coverage

• New test file: SubmoduleInstallationTest.t.sol
  • Valid submodule installation succeeds
  • Submodule returning false for ISubmodule fails
  • Submodule without supportsInterface fails
• New mocks: MockInvalidSubmodule, MockSubmoduleNoERC165

Summary by CodeRabbit

• New Features

  • Added a formal submodule interface and ERC‑165 validation to ensure only compatible submodules can be installed or upgraded.

• Bug Fixes

  • Installation/upgrade now rejects submodules that don't advertise required interfaces, preventing invalid deployments.

• Tests

  • Added tests and mocks covering successful installs and failure cases for invalid or non‑ERC‑165 submodules; updated snapshot.

• Chores

  • Simplified version-return code and import/path cleanups across price feed modules; adjusted compiler profile settings.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an ISubmodule interface and enforces ERC‑165 support for submodules during install/upgrade via staticcall; Submodule base updated to implement IVersioned and ERC‑165; PRICE submodules updated (VERSION refactor, some supportsInterface changes); new test mocks and a test suite validate installation failures; one snapshot numeric value updated.

Changes

Cohort / File(s)	Summary
Submodule Core & Interface src/Submodules.sol, src/interfaces/ISubmodule.sol	New ISubmodule interface; Submodule abstract now implements IVersioned/ISubmodule and advertises ERC‑165 via supportsInterface. Installation/upgrade now staticcalls supportsInterface(ISubmodule) and reverts with Module_SubmoduleInterfaceNotImplemented(address) if unsupported.
PRICE Module src/modules/PRICE/PRICE.v2.sol	Adds VERSION() returning (2,0).
PRICE Feed & Strategy Submodules src/modules/PRICE/submodules/feeds/..., src/modules/PRICE/submodules/strategies/SimplePriceFeedStrategy.sol	Import path reordering and consolidation; VERSION() implementations simplified to return tuples directly; some files had supportsInterface removed or relocated (notably PythPriceFeeds).
Test Mocks src/test/mocks/MockInvalidSubmodule.sol, src/test/mocks/MockSubmoduleNoERC165.sol	Adds mocks that intentionally fail ERC‑165/ISubmodule validation (one returns false for ISubmodule, one omits ERC‑165).
Tests src/test/modules/PRICE.v2/SubmoduleInstallation.t.sol	Adds tests covering successful installation and expected reverts for submodules that fail ERC‑165/ISubmodule validation.
Build Config & Scripts foundry.toml, package.json	Adjusts optimizer runs profile from 5000→4000 for a specific compile target and updates related npm script.
Snapshot snapshots/DepositRedemptionVaultFinishRedemptionTest.json	Updated numeric snapshot: "redeem" changed from "201080" to "203580".

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Module
    participant Submodule
    participant ERC165 as IERC165

    User->>Module: installSubmodule(submoduleAddr)
    Module->>Module: _validateSubmodule(submoduleAddr)
    Module->>Submodule: staticcall supportsInterface(ISubmodule)
    Submodule->>ERC165: evaluate interfaceId
    alt supports ISubmodule
        ERC165-->>Submodule: true
        Submodule-->>Module: true
        Module->>Module: proceed with installation
        Module-->>User: installation succeeded
    else does not support
        ERC165-->>Submodule: false
        Submodule-->>Module: false
        Module-->>User: revert Module_SubmoduleInterfaceNotImplemented(submoduleAddr)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Release: CD Parameter Update and Limit Orders #194: Adds/versioning and ERC‑165 support changes that overlap with ISubmodule/IVersioned and ERC‑165 validation logic.
• Convertible Deposits #29: Touches the same snapshot DepositRedemptionVaultFinishRedemptionTest.json and is directly related to the numeric change.
• Price Feed Resilience: Add support for Pyth oracles #193: Related changes around PRICE PythPriceFeeds and ERC‑165/IVersioned behavior affecting the same submodule areas.

Poem

🐇 I hopped through code to check each badge,

I tapped interfaces, avoiding a fidget.
Submodules wear their badges true,
No sly surprises slip on through.
🥕 Hooray — installs now pass the widget!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR objectives include implementing strict ERC-165 validation, versioning support, and submodule architecture changes. However, the linked issue #39 states 'It doesn't contain any contract updates,' which contradicts the substantial contract changes (Submodules.sol, ISubmodule.sol, multiple PRICE submodules, test mocks) present in this PR.	Clarify whether the linked issue accurately describes this PR's scope. If #39 should not include contract updates, this PR should be linked to different issues (e.g., #27 or #37) that describe the versioning and ERC-165 validation features.
Out of Scope Changes check	❓ Inconclusive	Most changes align with the stated objectives of ERC-165 validation and versioning implementation. However, the foundry.toml and package.json changes (reducing OlympusPrice.v1_2.sol optimizer runs from 5000 to 4000) appear unrelated to the versioning/ERC-165 validation work.	Confirm whether the optimizer configuration changes for OlympusPrice.v1_2.sol are necessary for this PR or should be addressed separately in a build optimization PR.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'versioned interface implementation with strict ERC-165 validation' clearly and specifically describes the main architectural changes in the PR, which align with the primary objectives of implementing ERC-165 validation and versioning support.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Submodules.sol`:
- Around line 190-200: The supportsInterface staticcall result handling on
address(newSubmodule_) must guard against short/malformed return data before
abi.decode; after the staticcall check (success and data.length != 0) also
require data.length >= 32 and treat anything shorter as a validation failure,
then only abi.decode(data, (bool)); on failure (success false, data.length < 32,
or decoded bool false) revert with
Module_SubmoduleInterfaceNotImplemented(address(newSubmodule_)) so callers
always get the intended custom error instead of an abi.decode panic.

🧹 Nitpick comments (2)

src/test/mocks/MockInvalidSubmodule.sol (1)

21-25: Keep ERC-165 semantics intact in the mock.

Right now the mock claims support for all interface IDs except ISubmodule, which is non‑ERC165‑compliant and could mask future interface checks. Consider returning false for ISubmodule but delegating to super for everything else.

♻️ Proposed refinement

-    function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
-        // Always return false for ISubmodule interface ID, making this contract fail validation
-        return interfaceId != type(ISubmodule).interfaceId;
-    }
+    function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
+        if (interfaceId == type(ISubmodule).interfaceId) return false;
+        return super.supportsInterface(interfaceId);
+    }

src/modules/PRICE/submodules/feeds/PythPriceFeeds.sol (1)

432-437: Consider removing the redundant supportsInterface override.

This override only delegates to super.supportsInterface(interfaceId) without adding any logic. Solidity's inheritance will automatically invoke the parent's implementation, making this override unnecessary. This pattern is repeated across all PRICE submodules (BalancerPoolTokenPrice, ChainlinkPriceFeeds, ERC4626Price, UniswapV2PoolTokenPrice, UniswapV3Price, SimplePriceFeedStrategy, and PythPriceFeeds), suggesting it could be removed from all of them for consistency.

If there's a specific reason for keeping this explicit override (e.g., documentation purposes, ABI clarity, or ensuring the function is included in the contract's interface), please disregard this suggestion.