feat(workflows): add reusable contracts deployment workflow - #56
feat(workflows): add reusable contracts deployment workflow#56chriszhao1988 wants to merge 16 commits into
Conversation
Adds a new GitHub Actions workflow for compiling and deploying smart contracts across multiple networks with configurable inputs and secrets.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable GitHub Actions workflow to deploy contracts (workflow_call) with S3 state restore/backup, ABI uploads, AWS Secrets Manager address updates, and DingTalk notifications; also adds a composite DingTalk action and a .gitignore entry. ChangesReusable Deploy Contracts
Sequence DiagramsequenceDiagram
autonumber
actor Caller as Workflow Caller
participant GH as GitHub Actions
participant Runner as Self-hosted Runner
participant Repo as Repository
participant Build as Build System (npm)
participant S3 as AWS S3
participant SM as AWS Secrets Manager
participant DT as DingTalk API
Caller->>GH: Trigger reusable workflow (action, target, network, aws_region, secrets)
rect rgba(200,220,240,0.5)
GH->>Runner: Start deploy job
Runner->>Repo: Checkout code
Runner->>Build: Setup Node 20, install deps, compile contracts
end
rect rgba(220,240,200,0.5)
Runner->>Build: Resolve and run npm script (action:target:network)
Runner->>S3: Restore deployments state (deployments/${network}.json)
end
rect rgba(240,220,200,0.5)
Runner->>Build: Collect ABI JSONs from artifacts/contracts
Runner->>S3: Upload ABIs to network-specific latest and dated prefixes
Runner->>SM: Transform and write addresses-only secret
end
rect rgba(240,200,220,0.5)
Runner->>S3: Upload finalized deployments state
Runner->>DT: Send DingTalk notification via composite action
DT-->>Runner: Webhook response
end
GH-->>Caller: Workflow complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
50-66: npm cache is ineffective; remove cache configuration or skip lockfile deletion.The workflow configures npm caching on line 54 with the package-lock.json dependency path, but immediately deletes that lockfile on line 59. This makes the cache setup pointless since the cache key won't match on subsequent runs.
If the intent is to ensure a fresh, clean install on each runner (likely for OS-specific binary compatibility), omit the cache configuration. If caching is desired, keep the lockfile intact.
Option 1: Remove the cache configuration if fresh installs are required:
- name: Setup Node uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} - cache: npm - cache-dependency-path: package-lock.json - name: Remove lockfile and node_modulesOption 2: If caching is desired, skip the lockfile deletion:
- name: Setup Node uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} cache: npm cache-dependency-path: package-lock.json - - name: Remove lockfile and node_modules - run: | - rm -f package-lock.json - rm -rf node_modules - name: Install dependencies (fresh lock on runner OS)
68-87: Consider extracting script validation to a separate helper script.The inline Node.js code (lines 77–85) works correctly and safely validates the script exists, but it's somewhat verbose for a single lookup. If script validation becomes more complex or is reused elsewhere, extract it to a
.github/scripts/validate-script.jsfile for better maintainability.If you wish to extract this, the pattern would look like:
SCRIPT_ENV="${ACTION}:${TARGET}:${NETWORK}" node .github/scripts/validate-script.jsAnd
.github/scripts/validate-script.js:const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); const script = process.env.SCRIPT_ENV; if (!pkg.scripts[script]) { console.error(`Script "${script}" not found in package.json. Add it before rerunning.`); process.exit(1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reuseable-contracts-deploy.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
89-120: Network environment mapping is well-structured.The network-to-RPC and secret-mapping logic (lines 94–105) is clear and follows a good fallback pattern: try network-specific secret, then fall back to the generic
DEPLOYER_PRIVATE_KEY. The validation steps (lines 107–115) properly guard against missing RPC endpoints or private keys, with informative error messages that guide users to configure missing variables or secrets.
1-125: Overall workflow structure is sound.The workflow demonstrates good separation of concerns: checkout → setup → compile → validate → prepare environment → deploy. Error handling is comprehensive (unsupported networks, missing RPC/keys). Using
printfinstead ofechofor writing toGITHUB_ENV(line 119) is a best practice to avoid trailing newline issues. GitHub Actions will automatically mask secret values in logs, so the GITHUB_ENV approach is safe and idiomatic.Minor observational note: The file name is
reuseable-contracts-deploy.yml(note the spelling "reuseable"). If this is intentional (matching an existing naming convention), it's fine. Otherwise, consider correcting toreusable-contracts-deploy.ymlfor standard English spelling.
…d ABI upload to S3 - Replace verbose input/secrets definitions with concise inline syntax - Remove network-specific RPC/private key logic; use direct secret injection - Add jq installation, ABI JSON filtering, and S3 upload step for contract ABIs - Set default S3 bucket and AWS region in environment variables
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
10-11: Use English for comments in workflow files.Lines 10–11 contain Chinese-language comments, which may reduce accessibility and consistency with the rest of the English codebase.
Replace with English comments:
- rpc: { required: true } # 调用方传入选定网络的 RPC - private_key: { required: true } # 调用方传入选定网络的私钥 + rpc: { required: true } + private_key: { required: true }
63-71: Improve error messaging in the npm script validation step.When the resolved script is not found in
package.json, the error message only indicates the missing script but does not list available scripts. This makes debugging harder for callers.Enhance the Node.js script to list available scripts on validation failure:
SCRIPT_ENV="$SCRIPT" node - <<'NODE' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); const script = process.env.SCRIPT_ENV; if (!pkg.scripts[script]) { console.error(`Script "${script}" not found in package.json. Add it before rerunning.`); + console.error(`Available scripts matching the pattern: ${Object.keys(pkg.scripts).filter(s => s.includes(process.env.SCRIPT_ENV.split(':')[0])).join(', ') || 'none'}`); process.exit(1); } NODE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reuseable-contracts-deploy.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/reuseable-contracts-deploy.yml
135-135: property "s3_owner_access_key_id" is not defined in object type {actions_runner_debug: string; actions_step_debug: string; github_token: string; private_key: string; rpc: string}
(expression)
136-136: property "s3_owner_secret_access_key" is not defined in object type {actions_runner_debug: string; actions_step_debug: string; github_token: string; private_key: string; rpc: string}
(expression)
🔇 Additional comments (1)
.github/workflows/reuseable-contracts-deploy.yml (1)
75-92: Clarify network environment variable resolution strategy.The AI-generated summary describes network-specific fallback logic (e.g., dev, dev2, testnet, mainnet, ethereum networks with fallback to
DEPLOYER_PRIVATE_KEY), but the current implementation (lines 75–92) simply validates and passes through the provided secrets without any network-aware mapping or fallback logic.Verify whether the simplified validation logic is intentional or if network-specific fallback logic should be implemented. If fallbacks are desired, the caller's workflow is responsible for selecting the correct secret—this is a valid design, but it should be clearly documented in comments or in a README.
If network-specific resolution should be built into this workflow, consider implementing a mapping step that resolves the network input to the appropriate secret names before accessing them.
| secrets: | ||
| rpc: { required: true } # 调用方传入选定网络的 RPC | ||
| private_key: { required: true } # 调用方传入选定网络的私钥 |
There was a problem hiding this comment.
Declare missing S3 secret inputs for the workflow call.
The workflow references secrets.S3_OWNER_ACCESS_KEY_ID and secrets.S3_OWNER_SECRET_ACCESS_KEY (lines 135-136), but these secrets are not declared in the workflow_call secrets: section. This will cause the workflow to fail at runtime with "secrets not available" errors.
Declare the missing secrets in the workflow_call section:
on:
workflow_call:
inputs:
action: { required: true, type: string }
target: { required: true, type: string }
network: { required: true, type: string }
secrets:
rpc: { required: true }
private_key: { required: true }
+ s3_owner_access_key_id: { required: true }
+ s3_owner_secret_access_key: { required: true }Then update lines 135–136 to reference the declared secrets:
- AWS_ACCESS_KEY_ID: ${{ secrets.S3_OWNER_ACCESS_KEY_ID }}
- AWS_SECRET_ACCESS_KEY: ${{ secrets.S3_OWNER_SECRET_ACCESS_KEY }}
+ AWS_ACCESS_KEY_ID: ${{ secrets.s3_owner_access_key_id }}
+ AWS_SECRET_ACCESS_KEY: ${{ secrets.s3_owner_secret_access_key }}(GitHub Actions secret names follow snake_case convention.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| secrets: | |
| rpc: { required: true } # 调用方传入选定网络的 RPC | |
| private_key: { required: true } # 调用方传入选定网络的私钥 | |
| secrets: | |
| rpc: { required: true } # 调用方传入选定网络的 RPC | |
| private_key: { required: true } # 调用方传入选定网络的私钥 | |
| s3_owner_access_key_id: { required: true } | |
| s3_owner_secret_access_key: { required: true } |
🤖 Prompt for AI Agents
In .github/workflows/reuseable-contracts-deploy.yml around lines 9–11 and later
lines 135–136, the workflow_call secrets section declares rpc and private_key
but omits the S3 secrets referenced later, causing "secrets not available"
errors; add declarations for s3_owner_access_key_id and
s3_owner_secret_access_key to the workflow_call secrets list (marked required)
and update the references on lines 135–136 to use those snake_case secret names
(s3_owner_access_key_id and s3_owner_secret_access_key) so the workflow can
access the S3 credentials at runtime.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| cache: npm | ||
| cache-dependency-path: package-lock.json | ||
|
|
||
| - name: Remove lockfile and node_modules | ||
| run: | | ||
| rm -f package-lock.json | ||
| rm -rf node_modules | ||
|
|
||
| - name: Install dependencies (fresh lock on runner OS) | ||
| run: npm install --no-audit --no-fund |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd .github/workflows && wc -l reuseable-contracts-deploy.ymlRepository: treasurenetprotocol/reusable-workflows
Length of output: 116
🏁 Script executed:
cd .github/workflows && cat -n reuseable-contracts-deploy.ymlRepository: treasurenetprotocol/reusable-workflows
Length of output: 5601
🏁 Script executed:
cd .github/workflows && git log --oneline -20 reuseable-contracts-deploy.yml 2>/dev/null || echo "No git history available"Repository: treasurenetprotocol/reusable-workflows
Length of output: 469
🏁 Script executed:
cd .github/workflows && find . -name "*.yml" -o -name "*.yaml" | head -20Repository: treasurenetprotocol/reusable-workflows
Length of output: 605
🏁 Script executed:
grep -l "cache: npm" .github/workflows/*.yml | head -5Repository: treasurenetprotocol/reusable-workflows
Length of output: 130
🏁 Script executed:
grep -A 10 "cache: npm" .github/workflows/*.yml | grep -E "(cache: npm|Remove lockfile|rm -f package-lock)" | head -20Repository: treasurenetprotocol/reusable-workflows
Length of output: 330
🏁 Script executed:
# Check if there's any documentation in the repo about this decision
find . -name "README*" -o -name "*.md" | xargs grep -l "lockfile\|cache" 2>/dev/null | head -5Repository: treasurenetprotocol/reusable-workflows
Length of output: 64
🏁 Script executed:
# Verify npm cache behavior documentation - check if there are any inline comments we missed
sed -n '36,50p' .github/workflows/reuseable-contracts-deploy.yml | cat -nRepository: treasurenetprotocol/reusable-workflows
Length of output: 617
Remove the contradictory lockfile deletion, or document its necessity.
The workflow configures npm caching (lines 40–41) but immediately deletes the lockfile (line 45) before reinstalling, which defeats the cache mechanism. On every run, npm must resolve the entire dependency tree instead of reusing the cached lockfile, significantly impacting performance, especially on self-hosted runners.
Either:
- Remove lines 43–46 to allow npm to use the cached lockfile and
package-lock.jsonfrom the repository:
- - name: Remove lockfile and node_modules
- run: |
- rm -f package-lock.json
- rm -rf node_modules
-
- - name: Install dependencies (fresh lock on runner OS)
+ - name: Install dependencies
run: npm install --no-audit --no-fund- Or, if OS-specific consistency requires regenerating the lockfile, remove the cache configuration entirely and add an explanatory comment so future maintainers understand the trade-off.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cache: npm | |
| cache-dependency-path: package-lock.json | |
| - name: Remove lockfile and node_modules | |
| run: | | |
| rm -f package-lock.json | |
| rm -rf node_modules | |
| - name: Install dependencies (fresh lock on runner OS) | |
| run: npm install --no-audit --no-fund | |
| cache: npm | |
| cache-dependency-path: package-lock.json | |
| - name: Install dependencies | |
| run: npm install --no-audit --no-fund |
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-deploy.yml lines 40 to 49: the workflow
enables npm caching but then deletes package-lock.json and node_modules before
reinstalling, which defeats the cache; either remove the rm -f package-lock.json
and rm -rf node_modules steps so the cached package-lock.json and node_modules
can be reused, or if you truly need to regenerate the lockfile for OS-specific
reasons, remove the npm cache configuration and add a clear comment explaining
why the lockfile is intentionally deleted and why caching is disabled to justify
the performance trade-off.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…d support - Added optional aws_region input to configure AWS region dynamically - Added sm_secret_id secret to store contract addresses in Secrets Manager - Updated AWS_REGION env to use inputs.aws_region fallback - Added step to extract and upload latest contract addresses to Secrets Manager
…s to multi-line YAML format
… and dynamic secret ID Replace S3_OWNER credentials with CONTRACT_DEPLOYER credentials for AWS access. Update Secrets Manager secret ID to use dynamic network-based path.
…ts with generic access key inputs - Updated workflow inputs to use `access_key_id` and `secret_access_key` instead of `sm_secret_id` - Updated AWS environment variables in S3 upload and Secrets Manager steps to reference new secret names
…ents - Restore deployments state from S3 before deployment - Upload updated deployments state to S3 after deployment
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/reuseable-contracts-deploy.yml (1)
56-65: Remove the lockfile deletion to preserve npm cache benefits.The workflow enables npm caching (lines 56–57) but immediately deletes
package-lock.jsonandnode_modules(lines 59–62) before reinstalling. This defeats the cache entirely—on every run, npm must resolve the full dependency tree instead of reusing the cached lockfile, significantly degrading performance on self-hosted runners.Remove lines 59–62 to let npm use the cached lockfile:
- name: Setup Node uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} cache: npm cache-dependency-path: package-lock.json - - name: Remove lockfile and node_modules - run: | - rm -f package-lock.json - rm -rf node_modules - - - name: Install dependencies (fresh lock on runner OS) + - name: Install dependencies run: npm install --no-audit --no-fundAlternatively, if OS-specific lockfile regeneration is required, remove the cache configuration entirely and add a comment explaining the performance trade-off.
🧹 Nitpick comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
35-35: Simplify the default region ternary expression.The expression
inputs.aws_region && inputs.aws_region || 'us-west-1'is functionally correct but unnecessarily verbose. Sinceaws_regionis optional, the simpler form covers all cases:- AWS_REGION: ${{ inputs.aws_region && inputs.aws_region || 'us-west-1' }} + AWS_REGION: ${{ inputs.aws_region || 'us-west-1' }}
38-38: Remove redundant fallback in concurrency group.The concurrency group uses
inputs.network || 'manual', butnetworkis a required input (line 12–14), so it will never be empty or undefined. The fallback is unreachable:- group: deploy-${{ github.workflow }}-${{ github.ref }}-${{ inputs.network || 'manual' }} + group: deploy-${{ github.workflow }}-${{ github.ref }}-${{ inputs.network }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reuseable-contracts-deploy.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
18-26: Secrets properly declared and referenced.The workflow correctly declares
access_key_idandsecret_access_keyin theworkflow_callsecrets section, and references them with the proper snake_case convention throughout (lines 112, 170, 184, 214). This addresses the earlier critical issue flagged in the review.
197-200: Verify deployment file structure matches the jq filter expectations.The jq transformation (lines 197–200) assumes the deployments file has an
entriesarray where the last entry containsnetwork,generatedAt, andcontractsfields with anaddresssubfield. While defensive checks exist (lines 202–205), verify that this structure is guaranteed by the upstream npm deployment script, or add inline comments documenting the expected schema to aid future maintainers.
- Changed deployment state path from /deployments/{NETWORK}.json to /{NETWORK}/state.json
- Flattened ABI JSON files before upload to avoid nested S3 paths
- Added timestamped and latest ABI uploads under /{NETWORK}/abis/
…ontract deployments Add DingTalk webhook integration to notify on deployment status, including action, target, network, and deployed contracts summary.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
60-66: 🟠 Remove contradictory lockfile deletion to restore npm cache effectiveness.The workflow enables npm caching (lines 57–58) but immediately deletes
package-lock.jsonandnode_modules(lines 60–63) before reinstalling. This forces npm to resolve the entire dependency tree on every run, defeating the cache and significantly degrading performance—especially problematic on self-hosted runners.This issue was flagged in prior review and remains unaddressed.
Remove the lockfile deletion steps to allow the cached lockfile to be reused:
- - name: Remove lockfile and node_modules - run: | - rm -f package-lock.json - rm -rf node_modules - - - name: Install dependencies (fresh lock on runner OS) + - name: Install dependencies run: npm install --no-audit --no-fundIf OS-specific consistency truly requires regenerating the lockfile, remove the cache configuration entirely and document this trade-off with an explanatory comment.
35-35: Fix invalid GitHub Actions expression syntax.The conditional logic on line 35 uses incorrect syntax:
inputs.aws_region && inputs.aws_region || 'us-west-1'mixes boolean operators (&&) with string coercion (||), which is not valid in GitHub Actions expression context.Simplify to use only the
||operator:- AWS_REGION: ${{ inputs.aws_region && inputs.aws_region || 'us-west-1' }} + AWS_REGION: ${{ inputs.aws_region || 'us-west-1' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reuseable-contracts-deploy.yml(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
.github/workflows/reuseable-contracts-deploy.yml
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| NODE_VERSION: 20 | ||
| S3_BUCKET: tn-deployment-contract-abis | ||
| AWS_REGION: ${{ inputs.aws_region && inputs.aws_region || 'us-west-1' }} | ||
| DINGTALK_ACCESS_TOKEN: 2d2baaef4ce067ebbeb9c3f552957fdb6de73a9a0b748b6dbca5a66aec0f6bac |
There was a problem hiding this comment.
🔴 Remove hardcoded DINGTALK_ACCESS_TOKEN from source code.
The access token is hardcoded in plain text in the workflow, creating a critical security vulnerability. Secrets and API tokens must never be stored in source control.
Move this to a GitHub secret and reference it:
- DINGTALK_ACCESS_TOKEN: 2d2baaef4ce067ebbeb9c3f552957fdb6de73a9a0b748b6dbca5a66aec0f6bac
+ DINGTALK_ACCESS_TOKEN: ${{ secrets.dingtalk_access_token }}Then add the secret declaration to the workflow_call.secrets section:
secrets:
rpc:
required: true
private_key:
required: true
access_key_id:
required: true
secret_access_key:
required: true
+ dingtalk_access_token:
+ required: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DINGTALK_ACCESS_TOKEN: 2d2baaef4ce067ebbeb9c3f552957fdb6de73a9a0b748b6dbca5a66aec0f6bac | |
| DINGTALK_ACCESS_TOKEN: ${{ secrets.dingtalk_access_token }} |
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-deploy.yml around line 36 contains a
hardcoded DINGTALK_ACCESS_TOKEN which must be removed; delete the literal token,
add a workflow input by declaring the secret under workflow_call.secrets (e.g.,
DINGTALK_ACCESS_TOKEN) and then reference it in the workflow env using the
GitHub secrets expression (secrets.DINGTALK_ACCESS_TOKEN); also add the token to
the repository or organization Secrets in Settings so the workflow can access it
at runtime.
Split DingTalk notification logic from deploy workflow into a dedicated composite action to improve reusability and maintainability. The workflow now builds the message in one step and delegates sending to the new action.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
36-36: 🔴 Remove hardcoded DINGTALK_ACCESS_TOKEN from source code.The access token is hardcoded in plain text in the workflow, creating a critical security vulnerability. Secrets and API tokens must never be stored in source control.
Apply this diff to move the token to GitHub Secrets:
secrets: rpc: required: true private_key: required: true access_key_id: required: true secret_access_key: required: true + dingtalk_access_token: + required: true permissions: contents: read env: NODE_VERSION: 20 S3_BUCKET: tn-deployment-contract-abis AWS_REGION: ${{ inputs.aws_region && inputs.aws_region || 'us-west-1' }} - DINGTALK_ACCESS_TOKEN: 2d2baaef4ce067ebbeb9c3f552957fdb6de73a9a0b748b6dbca5a66aec0f6bac + DINGTALK_ACCESS_TOKEN: ${{ secrets.dingtalk_access_token }}Then add the token to your repository Secrets in GitHub Settings.
57-66: Remove lockfile deletion or disable npm caching.The workflow enables npm caching (lines 57–58) but immediately deletes the lockfile (lines 60–66) before reinstalling. This defeats the cache mechanism entirely. On every run, npm must resolve the entire dependency tree instead of reusing the cached lockfile, significantly impacting performance—especially on self-hosted runners.
Option 1 (preferred): Remove the lockfile deletion and let npm reuse the cache:
- name: Setup Node uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} cache: npm cache-dependency-path: package-lock.json - - name: Remove lockfile and node_modules - run: | - rm -f package-lock.json - rm -rf node_modules - - - name: Install dependencies (fresh lock on runner OS) + - name: Install dependencies run: npm install --no-audit --no-fundOption 2: If OS-specific consistency is required, remove the cache and document the trade-off:
uses: actions/setup-node@v4 with: node-version: ${{ env.NODE_VERSION }} - cache: npm - cache-dependency-path: package-lock.json - name: Remove lockfile and node_modules + # Note: Intentionally regenerating lockfile for OS-specific consistency. + # This disables npm caching; cache is traded for deterministic builds. run: | rm -f package-lock.json rm -rf node_modulesChoose Option 1 unless there is a specific reason to regenerate the lockfile for the self-hosted runner OS.
🧹 Nitpick comments (3)
.github/workflows/reuseable-contracts-deploy.yml (3)
206-206: Consider parameterizing the hardcoded Secret ID.The Secret ID is hardcoded as
{network}/treasurenet-tnservices-dataprovider(line 206). If this needs to differ per network or use case, it should be passed as a workflow input rather than hardcoded.If this is intentional and always follows the
{network}/treasurenet-tnservices-dataproviderpattern, add an inline comment to clarify:- name: Update contract addresses in Secrets Manager env: AWS_ACCESS_KEY_ID: ${{ secrets.access_key_id }} AWS_SECRET_ACCESS_KEY: ${{ secrets.secret_access_key }} AWS_DEFAULT_REGION: ${{ env.AWS_REGION }} NETWORK: ${{ inputs.network }} + # Secret ID follows the pattern: {network}/treasurenet-tnservices-dataprovider SECRET_ID: ${{ inputs.network }}/treasurenet-tnservices-dataproviderAlternatively, if flexibility is needed, add a
secret_id_suffixinput or environment variable.
158-176: Validate ABI structure validation is overly strict; ensure empty containers are handled.The ABI collection step validates that
has("abi") and (.abi | type=="array")(line 158), but does not explicitly handle the case whereabiis an empty array. While the step succeeds in such cases, the later check on line 173 (wc -l) counts files, not ABI entries. If all ABIs are empty arrays, the step will succeed but produce no useful output to Secrets Manager.Consider adding a secondary validation to warn if all collected ABIs are empty:
# Flatten ABI filenames to avoid nested paths in S3 find "$OUT" -type f -name '*.json' ! -name '*.dbg.json' -print0 \ | while IFS= read -r -d '' file; do base="$(basename "$file")" cp "$file" "$FLAT/$base" done + ABI_COUNT=$(find "$OUT" -type f | wc -l) + echo "Collected $ABI_COUNT ABI JSON files" - if [ "$(find "$OUT" -type f | wc -l)" -eq 0 ]; then + if [ "$ABI_COUNT" -eq 0 ]; then echo "No ABI JSON files found to upload." >&2 exit 1 fiThis makes it explicit how many ABIs were collected, improving observability.
247-272: Add timeout and retry logic to DingTalk notification for resilience.The DingTalk notification step runs with
if: ${{ always() }}to ensure visibility of deployments, which is good. However, if the notification action fails (e.g., network issue, DingTalk API outage), the entire workflow fails despite successful contract deployment.Consider wrapping the notification in a continue-on-error block to prevent notification failures from blocking deployment success:
- name: Notify DingTalk if: ${{ always() }} + continue-on-error: true uses: ./.github/actions/dingtalk-notify with: access_token: ${{ env.DINGTALK_ACCESS_TOKEN }} content: ${{ steps.ding_msg.outputs.content }}This allows deployments to complete even if the notification service is temporarily unavailable, while still logging the failure for visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/dingtalk-notify/action.yml(1 hunks).github/workflows/reuseable-contracts-deploy.yml(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
.github/workflows/reuseable-contracts-deploy.yml
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
.github/workflows/reuseable-contracts-deploy.yml (1)
215-219: Verify consistency in contract address extraction logic.Line 215 extracts from
(.entries | last)(the most recent entry), but line 259 in the DingTalk message builder reads(.entries[0].contracts // {})(the first entry). This inconsistency could cause the notification to display stale contract addresses while Secrets Manager is updated with the latest ones.Confirm whether the intent is to:
- Use the latest entry (most recent deployment)
- Use the first entry (oldest/initial deployment)
Then align line 259 to match. If the intent is to show the latest contracts in the notification, change line 259:
- ADDR_SUMMARY=$(jq -r '(.entries[0].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE") + ADDR_SUMMARY=$(jq -r '(.entries[-1].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE")Or use
lastfor consistency with line 215:- ADDR_SUMMARY=$(jq -r '(.entries[0].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE") + ADDR_SUMMARY=$(jq -r '(.entries | last).contracts | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE")
| curl -sS -X POST "$WEBHOOK" \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"msgtype\":\"text\",\"text\":{\"content\":\"${CONTENT}\"}}" |
There was a problem hiding this comment.
Add HTTP response validation and timeout to curl request.
The curl command does not validate the HTTP response status code. If the DingTalk API returns a non-2xx response (e.g., 4xx, 5xx), the step will still succeed silently, making failures invisible.
Apply this diff to add response validation and timeout:
WEBHOOK="https://oapi.dingtalk.com/robot/send?access_token=${ACCESS_TOKEN}"
- curl -sS -X POST "$WEBHOOK" \
+ curl -sSf --max-time 30 -X POST "$WEBHOOK" \
-H 'Content-Type: application/json' \
-d "{\"msgtype\":\"text\",\"text\":{\"content\":\"${CONTENT}\"}}"The -f flag causes curl to fail on HTTP errors, and --max-time 30 prevents indefinite hangs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl -sS -X POST "$WEBHOOK" \ | |
| -H 'Content-Type: application/json' \ | |
| -d "{\"msgtype\":\"text\",\"text\":{\"content\":\"${CONTENT}\"}}" | |
| curl -sSf --max-time 30 -X POST "$WEBHOOK" \ | |
| -H 'Content-Type: application/json' \ | |
| -d "{\"msgtype\":\"text\",\"text\":{\"content\":\"${CONTENT}\"}}" |
🤖 Prompt for AI Agents
In .github/actions/dingtalk-notify/action.yml around lines 21 to 23, the curl
invocation doesn't fail on HTTP error responses or enforce a timeout; update the
command to include the -f flag (fail on HTTP errors) and a timeout such as
--max-time 30 so the step fails on non-2xx responses and cannot hang
indefinitely, preserving the same headers and payload.
Adds a new GitHub Actions workflow to run Hardhat scripts with configurable network and secrets.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/reuseable-contracts-run-script.yml (1)
50-68: Environment variable setup is secure, but consider explicit secret masking.The validation and environment variable export are implemented correctly. The use of
printfat line 67 safely handles special characters in the private key. Secrets are properly validated before export.GitHub Actions will auto-mask
PRIVATE_KEYin logs, but for extra clarity, you could consider explicitly addingadd-maskto mark these as secrets.Consider adding explicit masking for extra defensive logging:
- name: Prepare network env shell: bash run: | RPC="${{ secrets.rpc }}" KEY="${{ secrets.private_key }}" if [ -z "$RPC" ]; then echo "RPC is not provided." >&2 exit 1 fi if [ -z "$KEY" ]; then echo "Private key is not provided." >&2 exit 1 fi echo "RPC=$RPC" >> "$GITHUB_ENV" echo "NETWORK=${{ inputs.network }}" >> "$GITHUB_ENV" printf 'PRIVATE_KEY=%s\n' "$KEY" >> "$GITHUB_ENV" + + echo "::add-mask::$RPC" + echo "::add-mask::$KEY"
| script_path: | ||
| required: true | ||
| type: string | ||
| network: | ||
| required: true | ||
| type: string | ||
| secrets: | ||
| rpc: | ||
| required: true | ||
| private_key: | ||
| required: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the relevant file with line numbers to verify the snippet and context
sed -n '1,140p' .github/workflows/reuseable-contracts-run-script.ymlRepository: treasurenetprotocol/reusable-workflows
Length of output: 1834
Validate and sanitize workflow inputs (script_path, network) and quote them when used.
Reject or normalize values containing path traversal, absolute paths, whitespace, or shell metacharacters before exporting to GITHUB_ENV; whitelist allowed network names. In the "Prepare network env" and "Run hardhat script" steps, avoid unvalidated interpolation and invoke the command with quoted arguments, e.g.:
npx hardhat run --network "${{ env.NETWORK }}" "${{ inputs.script_path }}"
| - name: Remove lockfile and node_modules | ||
| run: | | ||
| rm -f package-lock.json | ||
| rm -rf node_modules | ||
|
|
||
| - name: Install dependencies (fresh lock on runner OS) | ||
| run: npm install --no-audit --no-fund |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Removing package-lock.json defeats caching and reproducibility.
Lines 43–45 delete the lockfile and node_modules, which contradicts the npm caching configured at line 39. This prevents:
- Cache reuse across runs
- Deterministic dependency resolution
- Consistent build behavior
If the intent is to ensure fresh dependencies on the self-hosted runner due to OS-specific binaries, consider either:
- Using a cache key that includes the runner OS (e.g.,
${{ runner.os }}) to avoid cross-platform contamination. - Keeping the lockfile but using
npm ciinstead ofnpm installfor deterministic installs.
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-run-script.yml lines 42–48: the workflow
deletes package-lock.json and node_modules which breaks caching and
deterministic installs; instead stop removing the lockfile and node_modules,
keep package-lock.json in place and replace the install step with a
deterministic command (npm ci) or, if OS-specific binaries are the concern,
include runner.os in the cache key so caches aren’t shared across different OSs
(e.g., add ${ { runner.os } } to the cache key) to preserve reproducible
installs and cache reuse.
| - name: Run hardhat script | ||
| env: | ||
| RPC: ${{ env.RPC }} | ||
| PRIVATE_KEY: ${{ env.PRIVATE_KEY }} | ||
| run: npx hardhat run --network ${{ env.NETWORK }} ${{ inputs.script_path }} |
There was a problem hiding this comment.
Quote inputs in hardhat command to prevent shell injection.
The script_path and network inputs are directly interpolated without quoting at line 73. If either input contains special characters or shell metacharacters (e.g., ;, |, $, backticks), command injection is possible.
Example attack vector: script_path = ../scripts/evil.js; malicious_command
Apply this diff to safely quote the variables:
- name: Run hardhat script
env:
RPC: ${{ env.RPC }}
PRIVATE_KEY: ${{ env.PRIVATE_KEY }}
- run: npx hardhat run --network ${{ env.NETWORK }} ${{ inputs.script_path }}
+ run: npx hardhat run --network "${{ env.NETWORK }}" "${{ inputs.script_path }}"This ensures that the values are treated as literal strings, preventing injection even if they contain special characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run hardhat script | |
| env: | |
| RPC: ${{ env.RPC }} | |
| PRIVATE_KEY: ${{ env.PRIVATE_KEY }} | |
| run: npx hardhat run --network ${{ env.NETWORK }} ${{ inputs.script_path }} | |
| - name: Run hardhat script | |
| env: | |
| RPC: ${{ env.RPC }} | |
| PRIVATE_KEY: ${{ env.PRIVATE_KEY }} | |
| run: npx hardhat run --network "${{ env.NETWORK }}" "${{ inputs.script_path }}" |
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-run-script.yml lines 69-73: the hardhat
run command interpolates unquoted inputs which allows shell injection via
special characters in script_path or network; wrap both interpolations in quotes
so the shell treats them as literal arguments (quote the network and the
script_path expansions) to prevent injection and ensure values with spaces or
metacharacters are passed safely.
Remove deprecated reusable-contracts-run-script.yml workflow and update dingtalk-notify action to use remote path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
36-36: 🔴 Critical: Hardcoded DingTalk token must be removed.The access token remains hardcoded in plain text. This critical security vulnerability was previously flagged and must be addressed before merging.
Move to GitHub secrets as previously suggested, and add
dingtalk_access_tokento the workflow_call secrets declaration (lines 18-26).As per coding guidelines, secrets must never be stored in source control.
57-66: Cache configuration contradicts lockfile deletion.The workflow enables npm caching (lines 57-58) but immediately deletes
package-lock.jsonandnode_modules(lines 60-63) before reinstalling. This defeats the cache mechanism and was previously flagged.Either remove the deletion steps to leverage caching, or remove the cache configuration and document why OS-specific lockfile regeneration is required.
🧹 Nitpick comments (2)
.github/workflows/reuseable-contracts-deploy.yml (2)
206-206: Consider making secret ID configurable.The secret ID path is partially hardcoded with
treasurenet-tnservices-dataprovider. If different targets or projects require different secret paths, consider adding asecret_idinput for flexibility.💡 Optional enhancement
Add to workflow_call inputs:
aws_region: required: false type: string + secret_id_suffix: + required: false + type: string + default: treasurenet-tnservices-dataproviderThen update line 206:
- SECRET_ID: ${{ inputs.network }}/treasurenet-tnservices-dataprovider + SECRET_ID: ${{ inputs.network }}/${{ inputs.secret_id_suffix }}
167-172: Filename collision in ABI flattening is a potential edge case; consider optional safeguards.The flattening logic (lines 167-172) copies all ABI files to a single directory using only the basename. If contracts across different directories share the same filename, later copies will silently overwrite earlier ones. Since this is a reusable workflow, the actual risk depends on how calling repositories structure their contracts.
If your project uses consistent, unique contract filenames across directories, this is not a concern. If there's a possibility of duplicates, consider appending a parent directory name or hash to the flattened filename for safety.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/reuseable-contracts-deploy.yml
🧰 Additional context used
🪛 Gitleaks (8.30.0)
.github/workflows/reuseable-contracts-deploy.yml
[high] 36-36: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
.github/workflows/reuseable-contracts-deploy.yml (3)
18-26: S3 secrets properly declared.The secrets
access_key_idandsecret_access_keyare correctly declared and consistently referenced throughout the workflow (lines 113-114, 180-181, 202-203, 232-233). This addresses the previously flagged issue about missing S3 secret declarations.
48-48: Appropriate timeout configured.The 60-minute timeout is reasonable for contract compilation, deployment, and S3/Secrets Manager operations, providing protection against hanging workflows.
186-198: Well-designed ABI versioning strategy.Uploading ABIs to both
latest/and date-stamped paths provides a clean versioning strategy: consumers can reference the latest ABIs while maintaining historical snapshots for auditing or rollback.
| FILE="deployments/${NETWORK}.json" | ||
| ADDR_SUMMARY="No deployments file" | ||
| if [ -f "$FILE" ]; then | ||
| ADDR_SUMMARY=$(jq -r '(.entries[0].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE") |
There was a problem hiding this comment.
Inconsistent deployment entry indexing.
Line 259 uses entries[0] (first entry) while line 215 uses entries | last (last entry) for the same deployments file. The DingTalk notification should show the latest deployment, not the first.
🔎 Proposed fix
if [ -f "$FILE" ]; then
- ADDR_SUMMARY=$(jq -r '(.entries[0].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE")
+ ADDR_SUMMARY=$(jq -r '(.entries | last | .contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE")
if [ -z "$ADDR_SUMMARY" ]; then
ADDR_SUMMARY="No contracts recorded"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ADDR_SUMMARY=$(jq -r '(.entries[0].contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE") | |
| if [ -f "$FILE" ]; then | |
| ADDR_SUMMARY=$(jq -r '(.entries | last | .contracts // {}) | to_entries | map("\(.key)=\(.value.address)") | join(", ")' "$FILE") | |
| if [ -z "$ADDR_SUMMARY" ]; then | |
| ADDR_SUMMARY="No contracts recorded" | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-deploy.yml around line 259: the script
extracts contract addresses using '(.entries[0].contracts // {})' which picks
the first deployment entry while the notification earlier uses 'entries | last';
update this line to use the last entry instead (e.g., '(.entries | last |
.contracts // {})') so the ADDR_SUMMARY reflects the latest deployment; ensure
the jq expression mirrors the format used at line 215 and still handles missing
contracts via the fallback.
|
|
||
| - name: Notify DingTalk | ||
| if: ${{ always() }} | ||
| uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@feature/add-contracts-deploy-DEV-749 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the workflow file exists and examine the content around line 269
ls -la .github/workflows/ | grep -i deployRepository: treasurenetprotocol/reusable-workflows
Length of output: 417
🏁 Script executed:
# Read the specific workflow file to verify the exact content
if [ -f ".github/workflows/reuseable-contracts-deploy.yml" ]; then
wc -l .github/workflows/reuseable-contracts-deploy.yml
sed -n '265,273p' .github/workflows/reuseable-contracts-deploy.yml
fiRepository: treasurenetprotocol/reusable-workflows
Length of output: 499
🏁 Script executed:
# Check all uses statements in this workflow to understand the pattern
if [ -f ".github/workflows/reuseable-contracts-deploy.yml" ]; then
grep -n "uses:" .github/workflows/reuseable-contracts-deploy.yml
fiRepository: treasurenetprotocol/reusable-workflows
Length of output: 283
🏁 Script executed:
# Check the git branch to understand the PR context
git branch -a 2>/dev/null | head -20
git log --oneline -5 2>/dev/nullRepository: treasurenetprotocol/reusable-workflows
Length of output: 390
🏁 Script executed:
# Check the full context of the dingtalk-notify usage and surrounding code
sed -n '260,272p' .github/workflows/reuseable-contracts-deploy.yml | cat -nRepository: treasurenetprotocol/reusable-workflows
Length of output: 781
🏁 Script executed:
# Check if there are any other dingtalk-notify usages in other workflows
grep -r "dingtalk-notify" .github/workflows/Repository: treasurenetprotocol/reusable-workflows
Length of output: 252
🏁 Script executed:
# Look at the git history to understand if this is intentional
git log --oneline --all -20 | grep -i "dingtalk\|action"Repository: treasurenetprotocol/reusable-workflows
Length of output: 276
🏁 Script executed:
# Check if the feature branch actually exists in the reusable-workflows repo
# and what the dingtalk-notify action looks like
if [ -d ".github/actions/dingtalk-notify" ]; then
ls -la .github/actions/dingtalk-notify
if [ -f ".github/actions/dingtalk-notify/action.yml" ]; then
cat .github/actions/dingtalk-notify/action.yml
fi
fiRepository: treasurenetprotocol/reusable-workflows
Length of output: 948
Update action reference to stable version.
The DingTalk notification action references the feature branch @feature/add-contracts-deploy-DEV-749. This branch reference is unstable and will break the workflow once the branch is deleted. Update to use a stable tag, the main branch, or a specific commit SHA.
- uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@feature/add-contracts-deploy-DEV-749
+ uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@mainOr use a specific commit SHA for immutability:
- uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@feature/add-contracts-deploy-DEV-749
+ uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@<commit-sha>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@feature/add-contracts-deploy-DEV-749 | |
| uses: treasurenetprotocol/reusable-workflows/.github/actions/dingtalk-notify@main |
🤖 Prompt for AI Agents
.github/workflows/reuseable-contracts-deploy.yml around line 269: the workflow
references a feature branch for the DingTalk action
("@feature/add-contracts-deploy-DEV-749"), which is unstable and will break if
the branch is removed; update the action reference to a stable identifier—either
a released tag (preferred), the main branch, or a specific commit SHA—for
example replace the branch suffix with a version tag (e.g. @v1.2.3) or a commit
SHA to make the workflow immutable and reliable.
…overwrite-INF-119 fix: stop contract secret overwrite INF-119
…-secret-path-contracts-INF-120 INF-120: remove legacy contract state secret path
2 participants
Adds a new GitHub Actions workflow for compiling and deploying smart contracts across multiple networks with configurable inputs and secrets.
Summary by CodeRabbit
New Features
Chores