diff --git a/.github/scripts/check-diff-changeset.sh b/.github/scripts/check-diff-changeset.sh new file mode 100755 index 00000000000..c55b02b62db --- /dev/null +++ b/.github/scripts/check-diff-changeset.sh @@ -0,0 +1,102 @@ +#!/bin/bash +# Analyzes a diff between two directories and validates changeset requirements +# Usage: check-diff-changeset.sh +# analysis-type: bytecode | storage +# +# For bytecode: any change requires patch+ +# For storage: additions require minor+, removals require major + +set -euo pipefail + +ANALYSIS_TYPE=${1:-} +BASE_DIR=${2:-} +HEAD_DIR=${3:-} + +if [ -z "$ANALYSIS_TYPE" ] || [ -z "$BASE_DIR" ] || [ -z "$HEAD_DIR" ]; then + echo "Usage: check-diff-changeset.sh " + exit 1 +fi + +# Verify directories exist +if [ ! -d "$BASE_DIR" ]; then + echo "ERROR: Base directory does not exist: $BASE_DIR" + exit 1 +fi +if [ ! -d "$HEAD_DIR" ]; then + echo "ERROR: Head directory does not exist: $HEAD_DIR" + exit 1 +fi + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# Generate diff +DIFF_OUTPUT=$(diff --unified --recursive "$BASE_DIR" "$HEAD_DIR" || true) + +if [ -z "$DIFF_OUTPUT" ]; then + echo "No $ANALYSIS_TYPE changes detected." + exit 0 +fi + +echo "Detected $ANALYSIS_TYPE changes:" +echo "$DIFF_OUTPUT" +echo "" + +# Classify changes +HAS_REMOVALS=false +HAS_ADDITIONS=false + +# Check for removed lines in diff (lines starting with '-' but not '---') +if echo "$DIFF_OUTPUT" | grep -E '^-[^-]' >/dev/null; then + HAS_REMOVALS=true +fi +# Check for added lines in diff (lines starting with '+' but not '+++') +if echo "$DIFF_OUTPUT" | grep -E '^\+[^+]' >/dev/null; then + HAS_ADDITIONS=true +fi +# Check for files only in base (removed files) - "Only in " +if echo "$DIFF_OUTPUT" | grep -E "^Only in ${BASE_DIR}" >/dev/null; then + HAS_REMOVALS=true +fi +# Check for files only in head (added files) - "Only in " +if echo "$DIFF_OUTPUT" | grep -E "^Only in ${HEAD_DIR}" >/dev/null; then + HAS_ADDITIONS=true +fi + +# Determine required level based on analysis type and change classification +case "$ANALYSIS_TYPE" in +bytecode) + # Any bytecode change requires patch + REQUIRED_LEVEL="patch" + CHANGE_DESC="Bytecode changes" + ;; +storage) + if [ "$HAS_REMOVALS" = true ]; then + REQUIRED_LEVEL="major" + CHANGE_DESC="Storage layout removals (breaking change)" + elif [ "$HAS_ADDITIONS" = true ]; then + REQUIRED_LEVEL="minor" + CHANGE_DESC="Storage layout additions" + else + echo "No significant storage changes detected." + exit 0 + fi + ;; +*) + echo "Unknown analysis type: $ANALYSIS_TYPE" + exit 1 + ;; +esac + +echo "$CHANGE_DESC detected." +echo "" + +# Check for adequate changeset +if "$SCRIPT_DIR/check-solidity-changeset.sh" "$REQUIRED_LEVEL"; then + echo "" + echo "$CHANGE_DESC are permitted with the existing changeset." + exit 0 +else + echo "" + echo "ERROR: $CHANGE_DESC require a changeset for @hyperlane-xyz/core with at least a '$REQUIRED_LEVEL' bump." + exit 1 +fi diff --git a/.github/scripts/check-solidity-changeset.sh b/.github/scripts/check-solidity-changeset.sh new file mode 100755 index 00000000000..1b3d2da4985 --- /dev/null +++ b/.github/scripts/check-solidity-changeset.sh @@ -0,0 +1,44 @@ +#!/bin/bash +# Checks if @hyperlane-xyz/core has a changeset at or above the required level +# Usage: check-solidity-changeset.sh +# Levels: patch < minor < major +# Exit 0 if adequate changeset exists, exit 1 otherwise + +set -euo pipefail + +REQUIRED_LEVEL=${1:-} +PACKAGE="@hyperlane-xyz/core" + +if [ -z "$REQUIRED_LEVEL" ]; then + echo "Usage: check-solidity-changeset.sh " + exit 1 +fi + +# Get changeset status as JSON +# Note: changeset status --output requires a path relative to repo root +STATUS_FILE=".changeset-status-$$.json" +trap "rm -f $STATUS_FILE" EXIT +# changeset status exits non-zero when there are pending changesets, so ignore exit code +pnpm changeset status --output "$STATUS_FILE" 2>/dev/null || true + +# Extract bump type for the package (only if it has explicit changesets, not transitive) +FOUND_LEVEL=$(jq -r --arg pkg "$PACKAGE" '.releases[] | select(.name == $pkg and (.changesets | length > 0)) | .type' "$STATUS_FILE") + +# Map levels to numbers +level_to_num() { + case "$1" in + patch) echo 1 ;; minor) echo 2 ;; major) echo 3 ;; *) echo 0 ;; + esac +} + +REQUIRED_NUM=$(level_to_num "$REQUIRED_LEVEL") +FOUND_NUM=$(level_to_num "$FOUND_LEVEL") + +if [ "$FOUND_NUM" -ge "$REQUIRED_NUM" ]; then + echo "Found $PACKAGE changeset with '$FOUND_LEVEL' bump (required: $REQUIRED_LEVEL or higher)" + exit 0 +else + echo "No adequate changeset for $PACKAGE (found: ${FOUND_LEVEL:-none}, required: $REQUIRED_LEVEL or higher)" + echo "Run 'pnpm changeset' and select '$PACKAGE' with a '$REQUIRED_LEVEL' (or higher) bump." + exit 1 +fi diff --git a/.github/workflows/bytecode-analysis.yml b/.github/workflows/bytecode-analysis.yml index a89c7dfa8b8..18af32ec46e 100644 --- a/.github/workflows/bytecode-analysis.yml +++ b/.github/workflows/bytecode-analysis.yml @@ -65,18 +65,5 @@ jobs: run: pnpm -C solidity run bytecode base-bytecode # Compare outputs - - name: Compare outputs (fail on any changes) - run: | - DIFF_OUTPUT=$(diff --unified --recursive solidity/base-bytecode solidity/HEAD-bytecode || true) - - if [ -z "$DIFF_OUTPUT" ]; then - echo "No bytecode changes detected." - exit 0 - fi - - echo "Detected bytecode changes:" - echo "$DIFF_OUTPUT" - echo "" - echo "ERROR: Bytecode changes detected. This job fails if there are any bytecode changes." - echo "If these changes are expected, please review them carefully." - exit 1 + - name: Compare outputs + run: .github/scripts/check-diff-changeset.sh bytecode solidity/base-bytecode solidity/HEAD-bytecode diff --git a/.github/workflows/interface-analysis.yml b/.github/workflows/interface-analysis.yml index e4c24c1e19f..df370a9002e 100644 --- a/.github/workflows/interface-analysis.yml +++ b/.github/workflows/interface-analysis.yml @@ -62,6 +62,44 @@ jobs: - name: Run command on target branch run: pnpm -C solidity interface base-interface - # Compare outputs (only fail on function removals) - - name: Compare outputs (fail on function removals) - run: pnpm -C solidity interface test-interface base-interface HEAD-interface + # Compare outputs and check for appropriate changeset + - name: Compare outputs + run: | + set +e + pnpm -C solidity interface test-interface base-interface HEAD-interface + EXIT_CODE=$? + set -e + + if [ "$EXIT_CODE" -eq 0 ]; then + echo "No interface changes detected." + exit 0 + elif [ "$EXIT_CODE" -eq 1 ]; then + # Removals detected - require major changeset + echo "" + if .github/scripts/check-solidity-changeset.sh major; then + echo "" + echo "Interface removals are permitted with the existing changeset." + exit 0 + else + echo "" + echo "ERROR: Interface removals (breaking changes) require a changeset for @hyperlane-xyz/core with a 'major' bump." + exit 1 + fi + elif [ "$EXIT_CODE" -eq 2 ]; then + # Additions only - require minor changeset + echo "" + if .github/scripts/check-solidity-changeset.sh minor; then + echo "" + echo "Interface additions are permitted with the existing changeset." + exit 0 + else + echo "" + echo "ERROR: Interface additions require a changeset for @hyperlane-xyz/core with at least a 'minor' bump." + exit 1 + fi + else + # Unexpected exit code - treat as error + echo "" + echo "ERROR: Interface check script failed with unexpected exit code: $EXIT_CODE" + exit 1 + fi diff --git a/.github/workflows/storage-analysis.yml b/.github/workflows/storage-analysis.yml index 2bbaab42bb0..9dc54ff350a 100644 --- a/.github/workflows/storage-analysis.yml +++ b/.github/workflows/storage-analysis.yml @@ -16,7 +16,8 @@ on: jobs: diff-check: runs-on: ubuntu-latest - + # Skip on changeset version PRs, as storage layout is expected to change with package version bumps + if: github.head_ref != 'changeset-release/main' steps: # Checkout the PR branch - name: Checkout PR branch @@ -65,14 +66,5 @@ jobs: run: pnpm -C solidity storage base-storage # Compare outputs - - name: Compare outputs (fail on removals only) - run: | - DIFF_OUTPUT=$(diff --unified solidity/base-storage solidity/HEAD-storage || true) - echo "$DIFF_OUTPUT" - # Fail only if there are removal lines in diff hunks (lines starting with '-' but not '---') - if echo "$DIFF_OUTPUT" | grep -E '^-([^-])' >/dev/null; then - echo "Detected storage removals in diff. Failing job." - exit 1 - else - echo "No storage removals detected." - fi + - name: Compare outputs + run: .github/scripts/check-diff-changeset.sh storage solidity/base-storage solidity/HEAD-storage diff --git a/solidity/README.md b/solidity/README.md index dd75da5faf0..e7f767305c2 100644 --- a/solidity/README.md +++ b/solidity/README.md @@ -30,6 +30,26 @@ pnpm test Some forge tests may generate fixtures. This allows the [SDK](https://github.com/hyperlane-xyz/hyperlane-monorepo/tree/main/typescript/sdk) tests to leverage forge fuzzing. These are git ignored and should not be committed. +## Contributing + +When modifying Solidity contracts, CI checks will validate that appropriate changesets are included based on the type of change: + +| Analysis | Change Type | Required Changeset | +| ------------- | ---------------------------------------- | ------------------ | +| **Bytecode** | Any change | `patch` or higher | +| **Interface** | Addition (new functions, events, errors) | `minor` or higher | +| **Interface** | Removal or modification | `major` | +| **Storage** | Addition (new storage slots) | `minor` or higher | +| **Storage** | Removal | `major` | + +To add a changeset, run: + +```bash +pnpm changeset +``` + +Select `@hyperlane-xyz/core` and choose the appropriate bump level based on your changes. + ## License Apache 2.0 diff --git a/solidity/interface.sh b/solidity/interface.sh index d3e5806e817..3152e98f32b 100755 --- a/solidity/interface.sh +++ b/solidity/interface.sh @@ -102,29 +102,37 @@ if [ "$1" = "test-interface" ]; then base_file="$BASE_DIR/$contract_name-abi.json" if [ ! -f "$base_file" ]; then - echo "INFO: New contract added: $contract_name" + ADDED_ITEMS="$ADDED_ITEMS\n New contract: $contract_name" fi done # Report results - if [ -n "$ADDED_ITEMS" ]; then - echo "" - echo "ABI entries added (non-breaking):" - echo -e "$ADDED_ITEMS" - fi - + # Exit codes: 0 = no changes, 1 = removals (breaking), 2 = additions only (non-breaking) if [ "$HAS_REMOVALS" = true ]; then + if [ -n "$ADDED_ITEMS" ]; then + echo "" + echo "ABI entries added:" + echo -e "$ADDED_ITEMS" + fi echo "" echo "ERROR: ABI entries removed or changed (breaking change):" echo -e "$REMOVED_ITEMS" echo "" echo "This PR removes or modifies entries in contract ABIs, which is a breaking change." - echo "If this is intentional, please review carefully." exit 1 fi + if [ -n "$ADDED_ITEMS" ]; then + echo "" + echo "ABI entries added:" + echo -e "$ADDED_ITEMS" + echo "" + echo "Interface additions detected (non-breaking)." + exit 2 + fi + echo "" - echo "No breaking interface changes detected." + echo "No interface changes detected." exit 0 fi diff --git a/solidity/test/interface.test.ts b/solidity/test/interface.test.ts index 0be28c75a7e..c643fc547fa 100644 --- a/solidity/test/interface.test.ts +++ b/solidity/test/interface.test.ts @@ -160,7 +160,12 @@ const BASE_CONTRACT = buildContract(BASE_CONFIG); // Contract variants for testing different breaking changes const CONTRACT_VARIANTS: Record< string, - { contract: string; shouldFail: boolean; expectedMatch: string } + { + contract: string; + shouldFail: boolean; + expectedMatch: string; + expectedExitCode?: number; + } > = { function_removed: { contract: buildContract({ @@ -247,7 +252,7 @@ const CONTRACT_VARIANTS: Record< no_changes: { contract: BASE_CONTRACT, shouldFail: false, - expectedMatch: 'No breaking interface changes', + expectedMatch: 'No interface changes detected', }, additions_only: { @@ -261,7 +266,8 @@ const CONTRACT_VARIANTS: Record< ], }), shouldFail: false, - expectedMatch: 'No breaking interface changes', + expectedExitCode: 2, // Additions return exit code 2 + expectedMatch: 'Interface additions detected', }, }; @@ -350,9 +356,14 @@ for (const [testName, testCase] of Object.entries(CONTRACT_VARIANTS)) { const result = runInterfaceCheck(); // Verify result - const exitCodeCorrect = testCase.shouldFail - ? result.exitCode === 1 - : result.exitCode === 0; + // Use expectedExitCode if specified, otherwise derive from shouldFail + const expectedExitCode = + testCase.expectedExitCode !== undefined + ? testCase.expectedExitCode + : testCase.shouldFail + ? 1 + : 0; + const exitCodeCorrect = result.exitCode === expectedExitCode; const outputCorrect = result.output.includes(testCase.expectedMatch); if (exitCodeCorrect && outputCorrect) { @@ -360,7 +371,7 @@ for (const [testName, testCase] of Object.entries(CONTRACT_VARIANTS)) { passed++; } else { console.log(`❌ ${testName}`); - console.log(` Expected exit code: ${testCase.shouldFail ? 1 : 0}`); + console.log(` Expected exit code: ${expectedExitCode}`); console.log(` Actual exit code: ${result.exitCode}`); console.log(` Expected match: "${testCase.expectedMatch}"`); console.log(` Output contains match: ${outputCorrect}`);