diff --git a/.cursor/rules/solidity-styles.mdc b/.cursor/rules/solidity-styles.mdc index f3292277ecb8e..d7f4dddcafe6b 100644 --- a/.cursor/rules/solidity-styles.mdc +++ b/.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 diff --git a/ops/ai-eng/graphite/rules.md b/ops/ai-eng/graphite/rules.md index 4fe2a0a676192..b41e72651b6bd 100644 --- a/ops/ai-eng/graphite/rules.md +++ b/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. diff --git a/packages/contracts-bedrock/test/setup/FeatureFlags.sol b/packages/contracts-bedrock/test/setup/FeatureFlags.sol index 015e8613801c9..300bfea09d982 100644 --- a/packages/contracts-bedrock/test/setup/FeatureFlags.sol +++ b/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,6 +25,9 @@ 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 { @@ -31,6 +35,7 @@ abstract contract FeatureFlags { } /// @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,7 +96,7 @@ 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")); } } @@ -80,7 +104,7 @@ abstract contract FeatureFlags { /// @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")); } } @@ -88,7 +112,7 @@ abstract contract FeatureFlags { /// @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")); } } @@ -96,7 +120,7 @@ abstract contract FeatureFlags { /// @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")); } } }