fix(gh-actions): release PR requisted changes #284
Conversation
Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]> Signed-off-by: Andrew Fleming <[email protected]>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughEstablishes release infrastructure by introducing new CI workflows for version bumping and npm publishing, centralizing Compact compiler setup in a composite action, refactoring the build pipeline to a four-step process, and adding public package metadata to enable distribution while documenting the complete release workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub as GitHub Actions
participant Installer as Compact Installer
participant npm as npm Registry
rect rgb(200, 220, 240)
Note over User,GitHub: Prepare Release Phase
User->>GitHub: Push release branch (release-v*)
GitHub->>GitHub: prepare-release workflow triggers
GitHub->>GitHub: Extract version from branch name
GitHub->>GitHub: Update contracts/package.json
GitHub->>GitHub: Replace version strings in codebase
GitHub->>GitHub: Auto-commit changes
end
rect rgb(240, 220, 200)
Note over User,GitHub: Version Validation & Setup
User->>GitHub: Create release tag & publish
GitHub->>GitHub: release workflow triggers
GitHub->>GitHub: Build contracts
GitHub->>GitHub: Validate tag vs package.json version
end
rect rgb(220, 240, 200)
Note over GitHub,npm: Publishing Phase
GitHub->>GitHub: Pack tarball from contracts/dist
GitHub->>GitHub: Compute npm dist-tag (latest/beta/next)
GitHub->>GitHub: Verify tarball (no private field)
GitHub->>npm: Publish tarball with tag & provenance
npm-->>GitHub: Package published
GitHub->>User: Log success with install instructions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
compact/src/Builder.ts (1)
60-71: Consider making the file copy loop more robust for edge cases.The
while read fileloop could potentially break if file paths contain spaces, newlines, or other special characters. While.compactfiles likely follow naming conventions that avoid such characters, using a more robust pattern would align with shell best practices.Consider this more robust approach using
find -print0andwhile IFS= read -r -d '':{ cmd: ` - find src -type f -name "*.compact" ! -name "Mock*" ! -path "*/archive/*" | while read file; do + find src -type f -name "*.compact" ! -name "Mock*" ! -path "*/archive/*" -print0 | while IFS= read -r -d '' file; do # Remove src/ prefix from path rel_path="\${file#src/}" mkdir -p "dist/\$(dirname "\$rel_path")" cp "\$file" "dist/\$rel_path" done `,.github/actions/setup/action.yml (1)
110-134: Version parsing logic assumes specific output format.The version validation uses
head -n 1for compiler version andtail -n 1for language version, which assumes a specific output format fromcompactc --versionandcompactc --language-version. If the output format changes or multiple version numbers appear, this parsing could break or extract incorrect values.Consider documenting the expected output format or making the parsing more robust. For example:
run: | set -euo pipefail echo "🔍 Checking Compact compiler version..." # Expected format: "compactc 0.25.0" or similar COMPILER_OUTPUT=$(compactc --version) echo "Raw compiler output: $COMPILER_OUTPUT" COMPUTED_COMPILER_VERSION=$(echo "$COMPILER_OUTPUT" | grep -oP '\b0\.[0-9]+\.[0-9]+\b' | head -n 1) # ... rest of validationAdditionally, verify that the version commands produce predictable output:
#!/bin/bash # Verify compactc version output format echo "=== Testing version command outputs ===" echo "Checking if compactc is available..." # Note: This requires the Compact compiler to be installed # Skip if not available in the environment if command -v compactc &> /dev/null; then echo "Compiler version output:" compactc --version echo "" echo "Language version output:" compactc --language-version else echo "compactc not found in PATH (expected in sandbox)" fi.github/workflows/release.yml (2)
64-92: Optimize tarball I/O by extracting package.json once and reusing values.The workflow extracts
package.jsonfrom the tarball three times (lines 67–69, 90–91), when one extraction could be reused. Additionally, storing extracted values in$GITHUB_OUTPUTwould eliminate the need for redundant parsing.- name: Verify tarball integrity id: verify run: | echo "=== Verifying tarball contents ===" - PACKAGE_NAME=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json | jq -r .name) - PACKAGE_VERSION=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json | jq -r .version) - PRIVATE_FIELD=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json | jq -r '.private // "not found"') + PKG_JSON=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json) + PACKAGE_NAME=$(echo "$PKG_JSON" | jq -r .name) + PACKAGE_VERSION=$(echo "$PKG_JSON" | jq -r .version) + PRIVATE_FIELD=$(echo "$PKG_JSON" | jq -r '.private // "not found"') + + echo "package_name=$PACKAGE_NAME" >> $GITHUB_OUTPUT + echo "package_version=$PACKAGE_VERSION" >> $GITHUB_OUTPUT + echo "private_field=$PRIVATE_FIELD" >> $GITHUB_OUTPUT echo "📦 Package: $PACKAGE_NAME@$PACKAGE_VERSION" echo "🏷️ Tag: ${{ steps.pack.outputs.tag }}" echo "🔒 Private field: $PRIVATE_FIELD" # Ensure no private field if [ "$PRIVATE_FIELD" = "true" ]; then echo "❌ Tarball contains private: true - cannot publish" exit 1 fi - name: Log success run: | - PACKAGE_NAME=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json | jq -r .name) - PACKAGE_VERSION=$(tar xfO "${{ steps.pack.outputs.tarball }}" package/package.json | jq -r .version) - echo "✅ Successfully published $PACKAGE_NAME@$PACKAGE_VERSION to npm with tag ${{ steps.pack.outputs.tag }}" - echo "📦 Install with: npm install $PACKAGE_NAME@${{ steps.pack.outputs.tag }}" + echo "✅ Successfully published ${{ steps.verify.outputs.package_name }}@${{ steps.verify.outputs.package_version }} to npm with tag ${{ steps.pack.outputs.tag }}" + echo "📦 Install with: npm install ${{ steps.verify.outputs.package_name }}@${{ steps.pack.outputs.tag }}"
81-86: Remove redundant provenance configuration.The
--provenanceflag on line 84 andNPM_CONFIG_PROVENANCE: trueon line 86 are redundant. The environment variable alone is sufficient.- name: Publish to npm run: | # Publish the tarball with appropriate tag - npm publish "${{ steps.pack.outputs.tarball }}" --tag "${{ steps.pack.outputs.tag }}" --access public --provenance + npm publish "${{ steps.pack.outputs.tarball }}" --tag "${{ steps.pack.outputs.tag }}" --access public env: NPM_CONFIG_PROVENANCE: true.github/workflows/prepare-release.yml (1)
54-64: Consider restricting sed replacement to targeted version contexts.The global find-and-replace (lines 55–64) using sed will match the version string anywhere it appears in files, including unintended locations like commented examples, hardcoded test strings, or as part of larger version numbers. This could introduce subtle bugs or false positives.
Consider a more targeted approach for critical files (e.g., only updating version fields in JSON/YAML files, or using a more specific regex pattern):
- # Escape special characters for sed - ESCAPED_CURRENT=$(printf '%s' "$CURRENT_VERSION" | sed -e 's/[\/&]/\\&/g') - ESCAPED_NEW=$(printf '%s' "$NEW_VERSION" | sed -e 's/[\/&]/\\&/g') - - # Replace version in contracts/src/ - find ./contracts/src/ -type d -name '.*' -prune -o \ - -type f -exec sed -i "s#$ESCAPED_CURRENT#$ESCAPED_NEW#g" {} + - - # Replace version in docs/, excluding package-lock.json - find ./docs/ -type d -name '.*' -prune -o \ - -type f ! -name 'package-lock.json' -exec sed -i "s#$ESCAPED_CURRENT#$ESCAPED_NEW#g" {} + + # Escape special characters for sed + ESCAPED_CURRENT=$(printf '%s' "$CURRENT_VERSION" | sed -e 's/[\/&]/\\&/g') + ESCAPED_NEW=$(printf '%s' "$NEW_VERSION" | sed -e 's/[\/&]/\\&/g') + + # Replace version in contracts/src/ with a more targeted pattern (e.g., after "version") + find ./contracts/src/ -type d -name '.*' -prune -o \ + -type f -name "*.ts" -o -name "*.js" -exec sed -i "s/\(version['\"]?:\s*['\"]?\)$ESCAPED_CURRENT/\1$ESCAPED_NEW/g" {} + + + # Replace version in docs/ with targeted pattern + find ./docs/ -type d -name '.*' -prune -o \ + -type f ! -name 'package-lock.json' -name "*.md" -o -name "*.json" \ + -exec sed -i "s/\(version['\"]?:\s*['\"]?\)$ESCAPED_CURRENT/\1$ESCAPED_NEW/g" {} +Alternatively, use a dedicated version-management tool or scripts that parse and update structured files rather than global string replacement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/actions/setup/action.yml(3 hunks).github/workflows/checks.yml(1 hunks).github/workflows/codeql.yml(1 hunks).github/workflows/prepare-release.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test.yml(2 hunks)RELEASING.md(1 hunks)compact/src/Builder.ts(4 hunks)contracts/package.json(1 hunks)contracts/tsconfig.json(1 hunks)
🔇 Additional comments (18)
contracts/tsconfig.json (1)
2-5: LGTM! TypeScript compilation scope appropriately narrowed.The narrowed include pattern to
src/**/witnesses/**/*.tsand explicit exclusion ofsrc/archive/align well with the new four-step build pipeline, where TypeScript compilation (Step 2) focuses only on witness files while .compact files are handled separately (Step 3).RELEASING.md (2)
1-11: LGTM! Clear introduction and branch creation steps.The instructions properly account for both standard releases from
mainand hotfix scenarios, with clear examples.
31-34: LGTM! GitHub release creation step is clear.The instructions properly reference the releases page and explain the workflow trigger for npm publishing.
compact/src/Builder.ts (4)
16-40: LGTM! Documentation clearly reflects the new four-step build process.The updated class description and examples accurately document the new build pipeline and distribution structure.
47-51: LGTM! Clean step is safe and correct.The
rm -rf dist && mkdir -p distcommand properly cleans and recreates the distribution directory with appropriate error chaining.
167-169: LGTM! Output guard improves code quality.The conditional check
if (lines.length > 0)before printing colored output prevents empty sections and improves the build output readability.
109-110: LGTM! Build completion message enhances user experience.The green checkmark with "Build complete!" message provides clear feedback when all build steps finish successfully.
.github/actions/setup/action.yml (5)
1-25: LGTM! Input/output structure and version configuration are correct.The action properly defines the
skip-compactinput and version outputs, with environment variables set appropriately for downstream steps.
27-54: LGTM! Cache configuration is correct and efficient.The caching strategy properly includes Yarn, Turbo, and Compact compiler caches, with appropriate cache keys and conditional execution for the Compact cache based on the
skip-compactinput.
56-71: LGTM! Node.js and dependency setup follows best practices.Using
.nvmrcfor Node version,--immutablefor Yarn, and version-pinned Turbo installation with telemetry disabled are all appropriate configuration choices.
73-85: LGTM! Compact installation is secure and well-configured.The installation step uses appropriate security measures with
--proto '=https'and--tlsv1.2, proper error handling withset -euo pipefail, and conditional execution. The trust inCOMPACT_INSTALLER_URLfrom GitHub repository variables is reasonable for this context.
86-108: LGTM! Environment setup and outputs are correctly configured.The Compact environment setup properly exports variables, validates the installation, and provides outputs for downstream consumption, with appropriate error handling.
contracts/package.json (2)
1-22: LGTM! Package metadata is complete and professional.The public package metadata is well-structured with appropriate version (0.0.1), clear description, relevant keywords, and correct URLs for repository, documentation, and bug tracking. The MIT license aligns with OpenZeppelin's licensing standards.
23-34: Build script correctly delegates TypeScript compilation to compact-builder.Removing the explicit
tsccall from the build script is correct. The script output confirms thatcompact-buildernow handles TypeScript compilation in Builder.ts Step 2 withtsc --project tsconfig.build.json, and the requiredtsconfig.build.jsonconfiguration file exists atcontracts/tsconfig.build.json. The refactoring is complete and properly encapsulates the build process..github/workflows/codeql.yml (1)
25-26: LGTM! Environment variable properly configured.The
COMPACT_INSTALLER_URLis correctly sourced from repository variables and made available to the workflow's steps, consistent with the centralized setup approach used across other workflows..github/workflows/checks.yml (1)
17-18: LGTM! Environment variable properly configured.The
COMPACT_INSTALLER_URLfollows the same correct pattern as other workflows, making the installer URL available from repository variables..github/workflows/test.yml (2)
15-16: LGTM! Environment variable properly configured.The
COMPACT_INSTALLER_URLis correctly configured, consistent with the pattern used in other workflows.
27-28: LGTM! Simplified compilation step delegates setup to shared action.The streamlined compilation step using
turbo compactwith sequential concurrency (--concurrency=1) is appropriate, and delegating installation to the setup action reduces duplication across workflows.
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
emnul
left a comment
emnul
left a comment
There was a problem hiding this comment.
Let me know what you think about the changes:
- Updated references to Compact 0.26.0
- Updated actions to latest versions
- Updated runner to use pinned version
As well as the proposed change to prerelease.yml
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]> Signed-off-by: 0xisk <[email protected]>
Co-authored-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]> Signed-off-by: 0xisk <[email protected]>
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #212 #268
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
New Features
Documentation
Chores