Add storage layout diff checks (#2991)

.changeset/smooth-swans-wave.md

@@ -0,0 +1,6 @@
+---
+'@hyperlane-xyz/sdk': patch
+'@hyperlane-xyz/core': patch
+---
+
+Rename StaticProtocolFee hook to ProtocolFee for clarity

.github/workflows/storage-analysis.yml

@@ -0,0 +1,66 @@
+name: Check Storage Layout Changes
+
+on:
+  pull_request:
+    branches: [main]
+    paths:
+      - 'solidity/**'
+  workflow_dispatch:
+
+jobs:
+  diff-check:
+    runs-on: ubuntu-latest
+
+    steps:
+      # Checkout the PR branch
+      - name: Checkout PR branch
+        uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+          submodules: recursive
+
+      - uses: actions/setup-node@v3
+        with:
+          node-version: 18
+
+      - name: yarn-cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            **/node_modules
+            .yarn
+          key: ${{ runner.os }}-yarn-cache-${{ hashFiles('./yarn.lock') }}
+
+      - name: yarn-install
+        run: yarn install
+
+      - name: foundry-install
+        uses: onbjerg/foundry-toolchain@v1
+
+      # Run the command on PR branch
+      - name: Run command on PR branch
+        run: yarn workspace @hyperlane-xyz/core storage HEAD-storage
+
+      # Checkout the target branch (base)
+      - name: Checkout target branch (base) contracts
+        env:
+          BASE_REF: ${{ github.event.pull_request.base.sha }}
+        run: |
+          git fetch origin $BASE_REF
+          git checkout $BASE_REF -- solidity/contracts
+
+      # Run the command on the target branch
+      - name: Run command on target branch
+        run: yarn workspace @hyperlane-xyz/core storage base-storage
+
+      # Compare outputs
+      - name: Compare outputs
+        run: diff --unified solidity/base-storage solidity/HEAD-storage > layout.diff
+
+      - name: Comment PR with layout diff
+        uses: yorhodes/actions-comment-pull-request@v2.4.6
+        with:
+          header: Storage Layout Diff
+          filePath: layout.diff
+          mdLanguage: diff
+          comment_tag: storagelayoutdiff

solidity/.gitignore

@@ -5,6 +5,7 @@ types/
 dist/
 coverage/
 coverage.json
+storage/
 .env
 *lcov.info
 # Foundry

solidity/contracts/hooks/ProtocolFee.sol

@@ -24,10 +24,10 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
 /**
- * @title StaticProtocolFee
+ * @title ProtocolFee
  * @notice Collects a static protocol fee from the sender.
  */
-contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {
+contract ProtocolFee is AbstractPostDispatchHook, Ownable {
     using StandardHookMetadata for bytes;
     using Address for address payable;
     using Message for bytes;
@@ -97,7 +97,7 @@ contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {
     ) internal override {
         require(
             msg.value >= protocolFee,
-            "StaticProtocolFee: insufficient protocol fee"
+            "ProtocolFee: insufficient protocol fee"
         );
 
         uint256 refund = msg.value - protocolFee;
@@ -123,7 +123,7 @@ contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {
     function _setProtocolFee(uint256 _protocolFee) internal {
         require(
             _protocolFee <= MAX_PROTOCOL_FEE,
-            "StaticProtocolFee: exceeds max protocol fee"
+            "ProtocolFee: exceeds max protocol fee"
         );
         protocolFee = _protocolFee;
     }
@@ -133,10 +133,7 @@ contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {
      * @param _beneficiary The new beneficiary.
      */
     function _setBeneficiary(address _beneficiary) internal {
-        require(
+        require(_beneficiary != address(0), "ProtocolFee: invalid beneficiary");
-            _beneficiary != address(0),
-            "StaticProtocolFee: invalid beneficiary"
-        );
         beneficiary = _beneficiary;
     }
 }

solidity/package.json

@@ -51,6 +51,7 @@
     "coverage": "./coverage.sh",
     "docs": "forge doc",
     "flatten": "./flatten.sh",
+    "storage": "./storage.sh",
     "prettier": "prettier --write ./contracts ./test",
     "test": "hardhat test && forge test -vvv",
     "gas": "forge snapshot",

solidity/storage.sh

@@ -0,0 +1,21 @@
+#!/bin/bash
+OUTPUT_PATH=${1:-storage}
+EXCLUDE="test|mock|interfaces|libs|upgrade|README|Abstract|Static"
+
+IFS=$'\n'
+CONTRACT_FILES=($(find ./contracts -type f))
+unset IFS
+
+echo "Generating layouts in $OUTPUT_PATH"
+mkdir -p $OUTPUT_PATH
+
+for file in "${CONTRACT_FILES[@]}";
+do
+    if [[ $file =~ .*($EXCLUDE).* ]]; then
+        continue
+    fi
+
+    contract=$(basename "$file" .sol)
+    echo "Generating storage layout of $contract"
+    forge inspect "$contract" storage --pretty > "$OUTPUT_PATH/$contract.md"
+done

solidity/test/hooks/ProtocolFee.t.sol

@@ -7,12 +7,12 @@ import {MessageUtils} from "../isms/IsmTestUtils.sol";
 import {StandardHookMetadata} from "../../contracts/hooks/libs/StandardHookMetadata.sol";
 import {IPostDispatchHook} from "../../contracts/interfaces/hooks/IPostDispatchHook.sol";
 
-import {StaticProtocolFee} from "../../contracts/hooks/StaticProtocolFee.sol";
+import {ProtocolFee} from "../../contracts/hooks/ProtocolFee.sol";
 
-contract StaticProtocolFeeTest is Test {
+contract ProtocolFeeTest is Test {
     using TypeCasts for address;
 
-    StaticProtocolFee internal fees;
+    ProtocolFee internal fees;
 
     address internal alice = address(0x1); // alice the user
     address internal bob = address(0x2); // bob the beneficiary
@@ -27,7 +27,7 @@ contract StaticProtocolFeeTest is Test {
     bytes internal testMessage;
 
     function setUp() public {
-        fees = new StaticProtocolFee(MAX_FEE, FEE, bob, address(this));
+        fees = new ProtocolFee(MAX_FEE, FEE, bob, address(this));
 
         testMessage = _encodeTestMessage();
     }
@@ -63,7 +63,7 @@ contract StaticProtocolFeeTest is Test {
             10 * fees.MAX_PROTOCOL_FEE()
         );
 
-        vm.expectRevert("StaticProtocolFee: exceeds max protocol fee");
+        vm.expectRevert("ProtocolFee: exceeds max protocol fee");
         fees.setProtocolFee(fee);
 
         assertEq(fees.protocolFee(), FEE);
@@ -95,7 +95,7 @@ contract StaticProtocolFeeTest is Test {
         uint256 balanceBefore = alice.balance;
 
         vm.prank(alice);
-        vm.expectRevert("StaticProtocolFee: insufficient protocol fee");
+        vm.expectRevert("ProtocolFee: insufficient protocol fee");
         fees.postDispatch{value: feeSent}("", "");
 
         assertEq(alice.balance, balanceBefore);

typescript/sdk/src/hook/HyperlaneHookDeployer.ts

@@ -7,8 +7,8 @@ import {
   IL1CrossDomainMessenger__factory,
   OPStackHook,
   OPStackIsm,
+  ProtocolFee,
   StaticAggregationHook__factory,
-  StaticProtocolFee,
 } from '@hyperlane-xyz/core';
 import { Address, addressToBytes32 } from '@hyperlane-xyz/utils';
 
@@ -90,8 +90,8 @@ export class HyperlaneHookDeployer extends HyperlaneDeployer<
   async deployProtocolFee(
     chain: ChainName,
     config: ProtocolFeeHookConfig,
-  ): Promise<StaticProtocolFee> {
+  ): Promise<ProtocolFee> {
-    this.logger('Deploying StaticProtocolFeeHook for %s', chain);
+    this.logger('Deploying ProtocolFeeHook for %s', chain);
     return this.deployContract(chain, HookType.PROTOCOL_FEE, [
       config.maxProtocolFee,
       config.protocolFee,

typescript/sdk/src/hook/contracts.ts

@@ -4,15 +4,15 @@ import {
   InterchainGasPaymaster__factory,
   MerkleTreeHook__factory,
   OPStackHook__factory,
+  ProtocolFee__factory,
   StaticAggregationHook__factory,
-  StaticProtocolFee__factory,
 } from '@hyperlane-xyz/core';
 
 import { HookType } from './types';
 
 export const hookFactories = {
   [HookType.MERKLE_TREE]: new MerkleTreeHook__factory(),
-  [HookType.PROTOCOL_FEE]: new StaticProtocolFee__factory(),
+  [HookType.PROTOCOL_FEE]: new ProtocolFee__factory(),
   [HookType.INTERCHAIN_GAS_PAYMASTER]: new InterchainGasPaymaster__factory(), // unused
   [HookType.AGGREGATION]: new StaticAggregationHook__factory(), // unused
   [HookType.OP_STACK]: new OPStackHook__factory(),