feat(abip): improve skip message for tests skipped by features

.cursor/rules/solidity-styles.mdc

@@ -12,7 +12,8 @@ Applies to Solidity files.
 
 - NatSpec documentation comments must use the triple-slash `///` style
 - Use `//` for regular inline comments that are not NatSpec
-- Always use `@notice` instead of `@dev`
+- Use `@notice` for documenting what a function/contract does (external-facing documentation)
+- Use `@dev` for internal developer notes, reminders, or invariants (e.g., "when updating this, also update X")
 - Use a line-length of 100 characters
 - Custom tags:
   - `@custom:proxied`: Add to a contract whenever it's meant to live behind a proxy

ops/ai-eng/graphite/rules.md

@@ -25,6 +25,13 @@ If the PR modifies `OPContractsManagerV2.sol` and changes the `version` constant
 
 This section applies to Solidity files ONLY.
 
+### @dev Comments
+
+- Pay close attention to `@dev` natspec comments in the codebase
+- These comments often contain important invariants, requirements, or reminders for developers
+- When reviewing changes to a function, check if there are `@dev` comments that specify conditions or actions that must be taken when modifying that code
+- Flag violations of instructions in `@dev` comments (e.g., "when updating this function, also update X")
+
 ### Style Guide
 
 - Follow the style guide found at `.cursor/rules/solidity-styles.mdc` in the root of this repository.

packages/contracts-bedrock/test/setup/FeatureFlags.sol

@@ -7,6 +7,7 @@ import { Vm } from "forge-std/Vm.sol";
 
 // Libraries
 import { DevFeatures } from "src/libraries/DevFeatures.sol";
+import { Features } from "src/libraries/Features.sol";
 import { Config } from "scripts/libraries/Config.sol";
 
 // Interfaces
@@ -24,13 +25,17 @@ abstract contract FeatureFlags {
     /// @notice The address of the SystemConfig contract.
     ISystemConfig internal sysCfg;
 
+    /// @notice Thrown when an unknown feature is provided.
+    error FeatureFlags_UnknownFeature(bytes32);
+
     /// @notice Sets the address of the SystemConfig contract.
     /// @param _sysCfg The address of the SystemConfig contract.
     function setSystemConfig(ISystemConfig _sysCfg) public {
         sysCfg = _sysCfg;
     }
 
     /// @notice Resolves the development feature bitmap.
+    /// @dev When updating this function, make sure to also update the getFeatureName function.
     function resolveFeaturesFromEnv() public {
         if (Config.devFeatureInterop()) {
             console.log("Setup: DEV_FEATURE__OPTIMISM_PORTAL_INTEROP is enabled");
@@ -42,6 +47,25 @@ abstract contract FeatureFlags {
         }
     }
 
+    /// @notice Returns the string name of a feature.
+    /// @param _feature The feature to get the name of.
+    /// @return The name of the feature.
+    function getFeatureName(bytes32 _feature) public pure returns (string memory) {
+        if (_feature == DevFeatures.OPTIMISM_PORTAL_INTEROP) {
+            return "DEV_FEATURE__OPTIMISM_PORTAL_INTEROP";
+        } else if (_feature == DevFeatures.OPCM_V2) {
+            return "DEV_FEATURE__OPCM_V2";
+        } else if (_feature == Features.CUSTOM_GAS_TOKEN) {
+            return "SYS_FEATURE__CUSTOM_GAS_TOKEN";
+        } else if (_feature == Features.ETH_LOCKBOX) {
+            return "SYS_FEATURE__ETH_LOCKBOX";
+        } else {
+            // NOTE: We error out here so that developers remember to actually name their features
+            //       above. Solidity doesn't have anything like reflection that could do this.
+            revert FeatureFlags_UnknownFeature(_feature);
+        }
+    }
+
     /// @notice Enables a feature.
     /// @param _feature The feature to set.
     function setDevFeatureEnabled(bytes32 _feature) public {
@@ -72,31 +96,31 @@ abstract contract FeatureFlags {
     /// @param _feature The feature to check.
     function skipIfSysFeatureEnabled(bytes32 _feature) public {
         if (isSysFeatureEnabled(_feature)) {
-            vm.skip(true);
+            vm.skip(true, string.concat("Skipping test because ", getFeatureName(_feature), " is enabled"));
         }
     }
 
     /// @notice Skips tests when the provided system feature is disabled.
     /// @param _feature The feature to check.
     function skipIfSysFeatureDisabled(bytes32 _feature) public {
         if (!isSysFeatureEnabled(_feature)) {
-            vm.skip(true);
+            vm.skip(true, string.concat("Skipping test because ", getFeatureName(_feature), " is disabled"));
         }
     }
 
     /// @notice Skips tests when the provided development feature is enabled.
     /// @param _feature The feature to check.
     function skipIfDevFeatureEnabled(bytes32 _feature) public {
         if (isDevFeatureEnabled(_feature)) {
-            vm.skip(true);
+            vm.skip(true, string.concat("Skipping test because ", getFeatureName(_feature), " is enabled"));
         }
     }
 
     /// @notice Skips tests when the provided development feature is disabled.
     /// @param _feature The feature to check.
     function skipIfDevFeatureDisabled(bytes32 _feature) public {
         if (!isDevFeatureEnabled(_feature)) {
-            vm.skip(true);
+            vm.skip(true, string.concat("Skipping test because ", getFeatureName(_feature), " is disabled"));
         }
     }
 }