Feature/eng 229 cicd for rust services#377
Open
petarvujovic98 wants to merge 19 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis pull request introduces comprehensive infrastructure-as-code for GCP, GitHub Actions CI/CD workflows for building and deploying Docker images, unified Dockerfile for Rust services with multi-stage builds, GCP runtime infrastructure (VPC, compute instances, service accounts), secret management utilities for file-based and environment-variable configuration, and removes individual service Dockerfiles in favor of centralized configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/infrastructure-gcp.yml:
- Around line 37-42: The validation step named "Validate CI configuration"
currently echoes specific secret names when missing; change the failure messages
in that step (the shell commands under the run block) to use a generic error
message (e.g., "Missing required CI variable or secret") instead of revealing
which secret/variable is absent, or suppress echoing by exiting with a non-zero
status without printing secret identifiers; update the three test -n checks (for
GCP_PROJECT_ID, GCP_WORKLOAD_IDENTITY_PROVIDER, GCP_SERVICE_ACCOUNT_EMAIL) to
use the generic message or silent failure so logs do not disclose existence of
specific secrets.
- Around line 65-73: The "OpenTofu plan" step creates a tfplan file but never
exposes it to reviewers; add an upload step immediately after the "OpenTofu
plan" run to publish the tfplan as a workflow artifact (use
actions/upload-artifact@v4), point the upload path to the created tfplan, and
include a descriptive name such as tfplan-${{ github.event.pull_request.number
}} and retention-days (e.g., 7) so reviewers can download and inspect the exact
plan; target the artifact upload to the same tfplan produced by the OpenTofu
plan step.
- Around line 55-58: The workspace selection is inconsistent: the plan job uses
"tofu workspace select -or-create \"${TF_WORKSPACE}\"" while apply uses "tofu
workspace select || tofu workspace new"; update the apply job to use the same
"-or-create" form for consistency—replace the apply job's "tofu workspace select
|| tofu workspace new" with "tofu workspace select -or-create
\"${TF_WORKSPACE}\"" (ensure the TF_WORKSPACE variable is referenced the same
way as in the plan job).
In @.gitignore:
- Around line 28-34: Add a glob rule to also ignore JSON-format
Terraform/OpenTofu variable files by extending the existing terraform ignore
rules (which currently include "**/*.tfvars") to include "**/*.tfvars.json" so
that files like myvars.tfvars.json are prevented from being committed; update
the same .gitignore section that contains "**/*.tfvars" and ensure no negation
rule (e.g., "!**/terraform.tfvars.example") unintentionally re-includes
"*.tfvars.json".
In `@infrastructure/gcp/main.tf`:
- Line 79: The attribute condition currently allows any token from the
repository by using attribute_condition = "assertion.repository ==
'${var.github_repository}'"; update this to include a ref check using
assertion.ref (e.g. combine repository and branch checks like
assertion.repository == '${var.github_repository}' && (assertion.ref ==
'refs/heads/main' || assertion.ref == 'refs/heads/dev') or drive the allowed
refs from a variable) and mirror the same ref constraint in the IAM member
binding that references the OIDC token so both the attribute_condition and the
IAM binding include assertion.ref to restrict WIF to the desired branches.
In `@infrastructure/gcp/README.md`:
- Around line 46-49: The README's Terraform state backend bucket name is
incorrect; update the README entry in infrastructure/gcp/README.md to match the
actual bucket configured in infrastructure/gcp/backend.tf (replace
`templar-contract-mvp-tfstate` with `templar-tfstate`), and verify the prefix
(`templar/gcp`) matches as well so both files are consistent; alternatively, if
you intend the README value to be canonical, change the bucket name in
backend.tf to `templar-contract-mvp-tfstate` instead—pick one source of truth
and make both files match.
In `@infrastructure/gcp/runtime.tf`:
- Around line 91-99: The google_compute_subnetwork "runtime" resource needs
Private Google Access and VPC flow logs enabled: update the resource
(google_compute_subnetwork.runtime) to set private_ip_google_access = true and
enable_flow_logs = true (and add a log_config block if you want custom settings
such as aggregation_interval, flow_sampling, and metadata) so runtime workloads
can access Google APIs privately and produce flow logs for network forensics and
monitoring.
- Around line 30-32: The current approach builds relayer_env_file from
local.relayer_env_effective and injects it into metadata_startup_script, which
persists sensitive environment values into Terraform state and GCP instance
metadata; remove any secret values from var.relayer_env, var.accumulator_env,
var.market_monitor_env, and var.funding_bridge_env and stop rendering them into
relayer_env_file (and the other similar env_*_file locals referenced at lines
cited). Instead store secrets in Google Secret Manager and pass only secret
identifiers (or no secret data) in Terraform; modify the startup script
referenced by metadata_startup_script to retrieve secrets at boot using Secret
Manager access (gcloud/secretmanager API or workload identity/service account)
and set the environment variables at runtime, and ensure IAM/service account
bindings are provisioned so instances can read those secrets rather than
embedding secret values in Terraform state or instance metadata.
- Around line 306-310: The network_interface blocks for the accumulator and
market_monitor VM resources currently include access_config {} which assigns
external (public) IPs; remove the access_config {} stanza from the
network_interface in the google_compute_instance resources for accumulator and
market_monitor (e.g., resource names google_compute_instance.accumulator and
google_compute_instance.market_monitor) so those instances no longer get
external IPs; verify there are no other code paths relying on external IPs for
these resources and run terraform plan to confirm no unintended changes.
- Around line 185-231: Add an explicit metadata entry to block project-wide SSH
keys: for the google_compute_instance_template "relayer" (and likewise for the
accumulator, market_monitor, and funding_bridge instance resources) add a
metadata map containing "block-project-ssh-keys" = "true" (e.g., metadata = {
"block-project-ssh-keys" = "true" }) so project-level SSH keys are disabled
while preserving the existing metadata_startup_script/service configuration.
In `@infrastructure/gcp/templates/daemon-startup.sh.tftpl`:
- Around line 16-18: The generated env file /opt/templar/${container_name}.env
contains secrets and must be created with restrictive permissions; modify the
daemon-startup.sh.tftpl snippet that writes ${env_file} to
/opt/templar/${container_name}.env so the file is created with mode 0600 (for
example by setting umask 077 before creation or creating then immediately chmod
600) and only owned by the service user; ensure you still write the contents
from the ${env_file} template variable and reference the same ${container_name}
target.
- Line 2: The startup script currently enables xtrace via the shebang 'set -euxo
pipefail', which prints executed commands and can leak the OAuth access token
fetched from the metadata service and passed to docker login; remove the xtrace
flag by changing that invocation to not include -x (e.g., use 'set -euo
pipefail') or at minimum disable xtrace around the sensitive token retrieval and
docker login (use 'set +x' before fetching the token and 'set -x' afterward),
updating the line containing 'set -euxo pipefail' and around the token
fetch/docker login commands to eliminate printing of secrets.
- Line 20: The TOKEN fetch currently uses a plain curl with no timeouts or
retries; update the TOKEN assignment (the line that sets TOKEN from the curl
call) to add client-side timeouts and retry semantics: include --connect-timeout
and --max-time, enable retries with --retry and --retry-delay (or
--retry-backoff) and cap total retry duration with --retry-max-time (around
15s), preserve existing -fS and the Metadata-Flavor header, and keep the sed
extraction unchanged so the command fails fast instead of hanging during VM
boot.
In `@infrastructure/gcp/terraform.tfvars.example`:
- Around line 32-34: The example terraform vars expose relayer and funding
bridge to the entire internet; change the values for
relayer_allowed_source_ranges and funding_bridge_allowed_source_ranges from
"0.0.0.0/0" to a constrained test CIDR such as "203.0.113.10/32" (matching the
admin_source_ranges pattern) so the example is safe if copied into production.
In `@infrastructure/gcp/variables.tf`:
- Around line 243-247: The variable blocks that hold container environment maps
(e.g., the variable "relayer_env" and the other two env-map variables in the
same file) must be marked sensitive to avoid leaking secrets; update each
variable declaration (the map(string) defaults for container envs) to include
sensitive = true so Terraform hides their values in CLI/plan/log output while
preserving description, type, and default.
- Around line 175-179: The variable relayer_port lacks bounds validation; add a
Terraform validation block to variable "relayer_port" ensuring its value is
between 1 and 65535 (same guard used for funding_bridge_port) by adding a
validation { condition = ... error_message = ... } to the relayer_port variable
declaration so invalid port numbers are rejected at plan time.
- Around line 198-202: The variables relayer_allowed_source_ranges and
funding_bridge_allowed_source_ranges currently default to ["0.0.0.0/0"]; update
their variable blocks to use an empty list default (default = []) so they no
longer open ingress to the world by default, forcing callers to explicitly
supply allowed CIDRs for relayer_allowed_source_ranges and
funding_bridge_allowed_source_ranges when deploying.
In `@service/compose.yaml`:
- Around line 119-121: The funding-bridge service is using inconsistent ports:
the command runs "/app/bin/templar-funding-bridge" with "--port ${PORT:-3000}"
while the compose publish maps host ${PORT:-3000} to container 3001, which
collides with relayer's 3000:3000; fix by making the service use a single
consistent port variable and container port across the command, healthcheck and
ports mapping (e.g., pick either container port 3000 or 3001), update the
command argument ("--port ${PORT:-...}"), the published ports entry
(host:${PORT:-...}:containerPort), and the healthcheck/port checks to reference
the same chosen variable/port name so there is no host port collision with
relayer.
- Around line 123-124: The compose file currently passes private keys as CLI
args ("--near-treasury-key" and "--signer-key"), which exposes secrets in
process metadata; remove these flags from the service command and instead wire
the keys through Docker secrets (declare the secrets in the compose file, add
them under the service's "secrets:" block, and mount them as files) and update
the application start logic (the code that reads CLI args in the service
entrypoint/command) to read the private key from the secret file path (or from
an environment variable that points to that file) rather than from process
arguments; ensure you reference and change the exact flags "--near-treasury-key"
and "--signer-key" and the service command/entrypoint that supplies them.
- Line 1: Remove the leading blank line at the top of the compose.yaml file (the
initial empty line causing YAMLlint empty-lines error) so the file starts
immediately with the first YAML document/key; this resolves the `too many blank
lines` lint failure.
In `@service/Dockerfile`:
- Around line 40-51: Add a Docker HEALTHCHECK to the runtime stage so
orchestrators can detect degraded containers: after the runtime stage setup (the
block with "FROM debian:${DEBIAN_VERSION}-slim AS runtime", "WORKDIR /app",
"COPY --from=builder /app/bin/* /app/bin/", and before or after "USER
10001:10001") add a HEALTHCHECK instruction that periodically probes the service
(e.g., via curl or wget against the service HTTP health endpoint or by executing
the service's health binary in /app/bin/) with sensible interval, timeout and
retries so failures are reported to the container runtime.
- Line 18: The Dockerfile uses a broad COPY . . which sends the entire repo into
the build context; update the root .dockerignore to exclude sensitive and
cache-invalidating files by adding entries for .git/, .env, .env.local, *.key,
and *.pem (use trailing slash for .git/ and wildcard patterns for keys/pems) so
those files are not included in the context when the Dockerfile's COPY . . runs;
place the .dockerignore at the repository root and commit the updated file to
ensure builds and cache behave correctly.
- Line 7: Replace mutable image tags in the Dockerfile's FROM instructions with
immutable digests: locate the FROM line that references
lukemathwalker/cargo-chef:${CHEF_VERSION}-rust-${RUST_VERSION}-slim-${DEBIAN_VERSION}
and change it to use the corresponding image@sha256:<digest> for that exact tag,
and likewise pin the debian:bookworm-slim base image (the second FROM referenced
around the file) to debian@sha256:<digest>; ensure you determine and insert the
correct sha256 digests for both the cargo-chef image and the debian image that
match the tags currently used so builds are reproducible and supply-chain
secure.
- Line 28: Update the two cargo invocations in the Dockerfile to add the
--locked flag so builds use the committed Cargo.lock and are reproducible;
specifically, modify the existing "cargo chef cook --release --recipe-path
recipe.json" command and the other cargo command later in the file (the second
cargo invocation) to include "--locked" in their argument lists.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/build-service-image.yml.github/workflows/infrastructure-gcp.yml.gitignoreinfrastructure/README.mdinfrastructure/gcp/.terraform.lock.hclinfrastructure/gcp/README.mdinfrastructure/gcp/backend.tfinfrastructure/gcp/main.tfinfrastructure/gcp/outputs.tfinfrastructure/gcp/providers.tfinfrastructure/gcp/runtime.tfinfrastructure/gcp/templates/daemon-startup.sh.tftplinfrastructure/gcp/terraform.tfvars.exampleinfrastructure/gcp/variables.tfinfrastructure/gcp/versions.tfservice/Dockerfileservice/accumulator/accumulator.serviceservice/compose.yamlservice/funding-bridge/Dockerfileservice/funding-bridge/docker-compose.prod.ymlservice/liquidator/Dockerfileservice/liquidator/docker-compose.prod.ymlservice/market-monitor/Dockerfileservice/market-monitor/docker-compose.prod.ymlservice/relayer/Dockerfileservice/relayer/compose.prod.yaml
💤 Files with no reviewable changes (9)
- service/funding-bridge/docker-compose.prod.yml
- service/accumulator/accumulator.service
- service/market-monitor/Dockerfile
- service/liquidator/Dockerfile
- service/market-monitor/docker-compose.prod.yml
- service/relayer/compose.prod.yaml
- service/liquidator/docker-compose.prod.yml
- service/funding-bridge/Dockerfile
- service/relayer/Dockerfile
Comment on lines
+37
to
+42
| - name: Validate CI configuration | ||
| run: | | ||
| test -n "${{ vars.GCP_PROJECT_ID }}" || (echo "Missing repo variable: GCP_PROJECT_ID" && exit 1) | ||
| test -n "${{ secrets.GCP_WORKLOAD_IDENTITY_PROVIDER }}" || (echo "Missing secret: GCP_WORKLOAD_IDENTITY_PROVIDER" && exit 1) | ||
| test -n "${{ secrets.GCP_SERVICE_ACCOUNT_EMAIL }}" || (echo "Missing secret: GCP_SERVICE_ACCOUNT_EMAIL" && exit 1) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Secret validation may expose existence of secrets in logs.
The validation step will print "Missing secret: ..." to logs if a secret is missing. While this is useful for debugging, ensure the workflow logs are not publicly accessible, as this confirms which secrets exist or are missing.
Consider using a generic error message or ensuring repository visibility settings are appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infrastructure-gcp.yml around lines 37 - 42, The
validation step named "Validate CI configuration" currently echoes specific
secret names when missing; change the failure messages in that step (the shell
commands under the run block) to use a generic error message (e.g., "Missing
required CI variable or secret") instead of revealing which secret/variable is
absent, or suppress echoing by exiting with a non-zero status without printing
secret identifiers; update the three test -n checks (for GCP_PROJECT_ID,
GCP_WORKLOAD_IDENTITY_PROVIDER, GCP_SERVICE_ACCOUNT_EMAIL) to use the generic
message or silent failure so logs do not disclose existence of specific secrets.
Comment on lines
+55
to
+58
| - name: Select workspace | ||
| run: | | ||
| tofu workspace select -or-create "${TF_WORKSPACE}" | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent workspace selection syntax between jobs.
The plan job uses tofu workspace select -or-create (line 57), while the apply job uses tofu workspace select || tofu workspace new (line 108). Use the same syntax in both jobs for consistency and maintainability.
♻️ Suggested fix for apply job (line 108)
- name: Select workspace
run: |
- tofu workspace select "${TF_WORKSPACE}" || tofu workspace new "${TF_WORKSPACE}"
+ tofu workspace select -or-create "${TF_WORKSPACE}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infrastructure-gcp.yml around lines 55 - 58, The workspace
selection is inconsistent: the plan job uses "tofu workspace select -or-create
\"${TF_WORKSPACE}\"" while apply uses "tofu workspace select || tofu workspace
new"; update the apply job to use the same "-or-create" form for
consistency—replace the apply job's "tofu workspace select || tofu workspace
new" with "tofu workspace select -or-create \"${TF_WORKSPACE}\"" (ensure the
TF_WORKSPACE variable is referenced the same way as in the plan job).
Comment on lines
+65
to
+73
| - name: OpenTofu plan | ||
| env: | ||
| TF_VAR_project_id: ${{ vars.GCP_PROJECT_ID }} | ||
| TF_VAR_github_repository: ${{ github.repository }} | ||
| run: | | ||
| tofu plan \ | ||
| -input=false \ | ||
| -lock=false \ | ||
| -out=tfplan |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Plan artifact is not uploaded for review.
The plan is saved to tfplan but not uploaded as a workflow artifact. This means reviewers cannot inspect the exact plan that was generated. Consider uploading the plan artifact or posting a plan summary to the PR.
♻️ Suggested addition after the plan step
- name: Upload plan artifact
uses: actions/upload-artifact@v4
with:
name: tfplan-${{ github.event.pull_request.number }}
path: infrastructure/gcp/tfplan
retention-days: 7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infrastructure-gcp.yml around lines 65 - 73, The "OpenTofu
plan" step creates a tfplan file but never exposes it to reviewers; add an
upload step immediately after the "OpenTofu plan" run to publish the tfplan as a
workflow artifact (use actions/upload-artifact@v4), point the upload path to the
created tfplan, and include a descriptive name such as tfplan-${{
github.event.pull_request.number }} and retention-days (e.g., 7) so reviewers
can download and inspect the exact plan; target the artifact upload to the same
tfplan produced by the OpenTofu plan step.
Comment on lines
+28
to
+34
| # OpenTofu / Terraform | ||
| **/.terraform/ | ||
| **/*.tfstate | ||
| **/*.tfstate.* | ||
| **/crash.log | ||
| **/*.tfvars | ||
| !**/terraform.tfvars.example |
Contributor
There was a problem hiding this comment.
Ignore *.tfvars.json to prevent accidental secret commits.
Current rules ignore *.tfvars but not *.tfvars.json. Terraform/OpenTofu commonly uses JSON var files, and these often contain credentials/project secrets.
Suggested `.gitignore` hardening
# OpenTofu / Terraform
**/.terraform/
**/*.tfstate
**/*.tfstate.*
**/crash.log
**/*.tfvars
+**/*.tfvars.json
!**/terraform.tfvars.example📝 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.
Suggested change
| # OpenTofu / Terraform | |
| **/.terraform/ | |
| **/*.tfstate | |
| **/*.tfstate.* | |
| **/crash.log | |
| **/*.tfvars | |
| !**/terraform.tfvars.example | |
| # OpenTofu / Terraform | |
| **/.terraform/ | |
| **/*.tfstate | |
| **/*.tfstate.* | |
| **/crash.log | |
| **/*.tfvars | |
| **/*.tfvars.json | |
| !**/terraform.tfvars.example |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 28 - 34, Add a glob rule to also ignore JSON-format
Terraform/OpenTofu variable files by extending the existing terraform ignore
rules (which currently include "**/*.tfvars") to include "**/*.tfvars.json" so
that files like myvars.tfvars.json are prevented from being committed; update
the same .gitignore section that contains "**/*.tfvars" and ensure no negation
rule (e.g., "!**/terraform.tfvars.example") unintentionally re-includes
"*.tfvars.json".
| "attribute.repository" = "assertion.repository" | ||
| } | ||
|
|
||
| attribute_condition = "assertion.repository == '${var.github_repository}'" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure and locate relevant files
git ls-files | head -20Repository: Templar-Protocol/contracts
Length of output: 589
🏁 Script executed:
# Examine the current state of the infrastructure/gcp/main.tf file around line 79
wc -l infrastructure/gcp/main.tfRepository: Templar-Protocol/contracts
Length of output: 99
🏁 Script executed:
# Read the context around line 79 in the main.tf file
sed -n '70,90p' infrastructure/gcp/main.tfRepository: Templar-Protocol/contracts
Length of output: 726
🏁 Script executed:
# Search for all GitHub workflows that use id-token:write or id-token permissions
find .github/workflows -type f \( -name "*.yml" -o -name "*.yaml" \) 2>/dev/null | head -20Repository: Templar-Protocol/contracts
Length of output: 399
🏁 Script executed:
# Search for id-token permissions in all workflows
rg -l 'id-token:\s*write' .github/workflows/Repository: Templar-Protocol/contracts
Length of output: 252
🏁 Script executed:
# Examine the workflows that use id-token:write to understand trigger conditions
for workflow in $(rg -l 'id-token:\s*write' .github/workflows/); do
echo "=== $workflow ==="
cat "$workflow" | grep -A5 -B5 'id-token:'
doneRepository: Templar-Protocol/contracts
Length of output: 1509
🏁 Script executed:
# Check the deploy-production.yml workflow for on: conditions
head -50 .github/workflows/deploy-production.ymlRepository: Templar-Protocol/contracts
Length of output: 933
🏁 Script executed:
# Check the deploy-staging.yml workflow for on: conditions
head -50 .github/workflows/deploy-staging.ymlRepository: Templar-Protocol/contracts
Length of output: 1391
🏁 Script executed:
# Check infrastructure-gcp.yml for id-token and trigger conditions
head -60 .github/workflows/infrastructure-gcp.ymlRepository: Templar-Protocol/contracts
Length of output: 1834
🏁 Script executed:
# Check if there are any other references to github_repository variable or WIF configuration
rg -n 'attribute_condition|github_repository|assertion\.ref' infrastructure/gcp/main.tfRepository: Templar-Protocol/contracts
Length of output: 357
🏁 Script executed:
# Verify the complete context of the WIF resource to understand the full scope
sed -n '60,92p' infrastructure/gcp/main.tfRepository: Templar-Protocol/contracts
Length of output: 1392
Narrow WIF trust beyond repository-level only.
The current attribute condition allows any OIDC token from the repository to mint credentials, regardless of branch. Since assertion.ref is already mapped but unused, add it to the condition to restrict credential access to specific branches (e.g., production deploys from main only, infrastructure changes from main and dev).
- attribute_condition = "assertion.repository == '${var.github_repository}'"
+ attribute_condition = "assertion.repository == '${var.github_repository}' && assertion.ref == 'refs/heads/main'"Also update the IAM member binding to include the ref constraint for consistency:
- member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository/${var.github_repository}"
+ member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository/${var.github_repository}/attribute.ref/refs/heads/main"📝 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.
Suggested change
| attribute_condition = "assertion.repository == '${var.github_repository}'" | |
| attribute_condition = "assertion.repository == '${var.github_repository}' && assertion.ref == 'refs/heads/main'" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/gcp/main.tf` at line 79, The attribute condition currently
allows any token from the repository by using attribute_condition =
"assertion.repository == '${var.github_repository}'"; update this to include a
ref check using assertion.ref (e.g. combine repository and branch checks like
assertion.repository == '${var.github_repository}' && (assertion.ref ==
'refs/heads/main' || assertion.ref == 'refs/heads/dev') or drive the allowed
refs from a variable) and mirror the same ref constraint in the IAM member
binding that references the OIDC token so both the attribute_condition and the
IAM binding include assertion.ref to restrict WIF to the desired branches.
Contributor
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!
Comment on lines
+123
to
+124
| "--near-treasury-account", "${NEAR_TREASURY_ACCOUNT}", | ||
| "--near-treasury-key", "${NEAR_TREASURY_KEY}" |
Contributor
There was a problem hiding this comment.
Do not pass private keys via process arguments.
Line 124 (--near-treasury-key) and Line 178 (--signer-key) expose secrets through process metadata (ps, container inspect, crash dumps). Move these to Docker secrets (or an equivalent secret file mechanism) instead of CLI flags.
Also applies to: 177-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/compose.yaml` around lines 123 - 124, The compose file currently
passes private keys as CLI args ("--near-treasury-key" and "--signer-key"),
which exposes secrets in process metadata; remove these flags from the service
command and instead wire the keys through Docker secrets (declare the secrets in
the compose file, add them under the service's "secrets:" block, and mount them
as files) and update the application start logic (the code that reads CLI args
in the service entrypoint/command) to read the private key from the secret file
path (or from an environment variable that points to that file) rather than from
process arguments; ensure you reference and change the exact flags
"--near-treasury-key" and "--signer-key" and the service command/entrypoint that
supplies them.
|
|
||
| # Prepare stage | ||
| FROM chef AS planner | ||
| COPY . . |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the Dockerfile
fd -HI '^Dockerfile$' service/Repository: Templar-Protocol/contracts
Length of output: 88
🏁 Script executed:
# Read the Dockerfile to understand the context around lines 18 and 30
cat -n service/DockerfileRepository: Templar-Protocol/contracts
Length of output: 1832
🏁 Script executed:
# Run the verification script provided in the comment
#!/bin/bash
set -euo pipefail
files="$(fd -HI '^\.dockerignore$' || true)"
if [ -z "${files}" ]; then
echo "No .dockerignore found. Add one next to the Docker build context."
exit 0
fi
echo "Found .dockerignore files:"
printf '%s\n' "${files}"
echo
while IFS= read -r f; do
echo "--- ${f} ---"
sed -n '1,200p' "${f}"
done <<< "${files}"
echo
echo "Checking for common required patterns:"
while IFS= read -r f; do
echo "File: ${f}"
rg -n '^(target/|\.git/|\.env|.*secret.*|.*pem|.*key)$' "${f}" || true
done <<< "${files}"Repository: Templar-Protocol/contracts
Length of output: 1495
Add missing exclusions to the root .dockerignore to protect cache efficiency and prevent file leakage.
The root .dockerignore exists but is missing critical exclusions. Add .git/, .env, .env.local, *.key, and *.pem to prevent:
- Repository metadata leakage (
.git/) - Secrets exposure (
.env*,*.key,*.pem) - Unnecessary cache invalidation
Currently, the root .dockerignore only partially protects the build context (excludes target/ and IDE files but not .git/ or secrets).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/Dockerfile` at line 18, The Dockerfile uses a broad COPY . . which
sends the entire repo into the build context; update the root .dockerignore to
exclude sensitive and cache-invalidating files by adding entries for .git/,
.env, .env.local, *.key, and *.pem (use trailing slash for .git/ and wildcard
patterns for keys/pems) so those files are not included in the context when the
Dockerfile's COPY . . runs; place the .dockerignore at the repository root and
commit the updated file to ensure builds and cache behave correctly.
peer2f00l
reviewed
Mar 9, 2026
peer2f00l
left a comment
peer2f00l
left a comment
Collaborator
There was a problem hiding this comment.
Please address the non-nitpick level comments from coderabbit or dismiss them if they are irrelevant.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (3)
infrastructure/gcp/terraform.tfvars.example (2)
32-34:⚠️ Potential issue | 🟡 MinorExample still uses world-open CIDR for relayer access.
This was flagged in a previous review. The example demonstrates a safer pattern with
admin_source_ranges(line 24) using203.0.113.10/32. Apply the same constrained approach here to prevent accidental copy-paste into production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/gcp/terraform.tfvars.example` around lines 32 - 34, The example terraform variable relayer_allowed_source_ranges currently uses an open CIDR "0.0.0.0/0"; change it to a constrained example CIDR (matching the safer pattern used by admin_source_ranges, e.g. "203.0.113.10/32") so that relayer_allowed_source_ranges demonstrates a restricted access pattern and avoids encouraging copy-pasting a world-open rule into production; update the example value for relayer_allowed_source_ranges accordingly and keep a brief comment indicating it should be tightened to specific IPs for production.
75-77:⚠️ Potential issue | 🟡 MinorSame world-open CIDR concern for funding bridge.
Apply a constrained example CIDR consistent with the
admin_source_rangespattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/gcp/terraform.tfvars.example` around lines 75 - 77, Replace the world-open CIDR in the funding_bridge_allowed_source_ranges example with a constrained example matching the admin_source_ranges pattern; update the funding_bridge_allowed_source_ranges array to use a non-global example CIDR (e.g. a /24 from a documentation/test range like 203.0.113.0/24 or a private range such as 10.0.0.0/24) so it demonstrates a safe, limited source range consistent with admin_source_ranges.service/compose.yaml (1)
12-13:⚠️ Potential issue | 🟡 MinorPort conflict risk with relayer and funding-bridge.
The
relayerservice binds to3000:3000(line 13), whilefunding-bridgedefaults to${PORT:-3001}:${PORT:-3001}(line 139). IfPORTis explicitly set to3000in.env, both services will attempt to bind to the same host port.Consider using distinct environment variables per service (e.g.,
RELAYER_PORT,FUNDING_BRIDGE_PORT) to avoid accidental collisions.Also applies to: 138-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/compose.yaml` around lines 12 - 13, The relayer service is hard-coded to bind host port 3000 (service name relayer) which can collide with funding-bridge when PORT is set to 3000; update both service port declarations to use distinct environment variables (e.g., RELAYER_PORT and FUNDING_BRIDGE_PORT) with sensible defaults (e.g., "${RELAYER_PORT:-3000}:${RELAYER_PORT:-3000}" and "${FUNDING_BRIDGE_PORT:-3001}:${FUNDING_BRIDGE_PORT:-3001}") and update the .env and any docs or startup scripts to define those new variables so each service (relayer and funding-bridge) cannot accidentally bind the same host port.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-plz.yml:
- Around line 16-24: The workflow uses YAML anchors (&checkout, &install-rust)
which are uncommon in GitHub Actions; replace the anchored/aliased steps with
explicit step definitions (expand the &checkout/*checkout and &install-rust
usages into full "Checkout repository" and "Install Rust toolchain" steps) or
extract the duplicated logic into a reusable workflow or composite action and
call that instead; update every occurrence (e.g., the "Checkout repository" and
"Install Rust toolchain" step blocks) so the workflow is clear and easy to read
for other contributors.
- Around line 10-31: Add concurrency control to the "release-plz-release" job to
prevent overlapping runs (use the workflow-level or job-level concurrency key
referencing the branch or ref) and decide whether to pin release-plz/action to a
commit SHA instead of the moving tag "release-plz/[email protected]"; update the job
that currently uses release-plz/[email protected] (the "Run release-plz" step) to use
a specific SHA if you choose to hard-pin, or add a comment explaining why the
tag is acceptable, and add a concurrency entry (e.g., a unique group based on
github.ref or workflow) to the same job to avoid race conditions during rapid
commits.
In `@infrastructure/gcp/README.md`:
- Around line 142-143: The README currently says relayer_allowed_source_ranges
and funding_bridge_allowed_source_ranges default to 0.0.0.0/0 but variables.tf
sets them to [""]; make these consistent by either (A) updating the README to
state the actual defaults are an empty list ([""]) and add the same tightening
warning, referencing the variable names relayer_allowed_source_ranges and
funding_bridge_allowed_source_ranges, or (B) change the defaults in variables.tf
to ["0.0.0.0/0"] if you intended open access by default; pick one solution and
ensure both variables.tf and README.md mention the same default and the
production tightening guidance.
In `@infrastructure/gcp/runtime.tf`:
- Around line 326-378: Add a shielded_instance_config block to the
google_compute_instance.accumulator resource to enable secure boot, vTPM and
integrity monitoring for defense-in-depth; update the resource (and the other
similar instances referenced in the comment) to include shielded_instance_config
with enable_secure_boot = true, enable_vtpm = true, and
enable_integrity_monitoring = true and ensure the runtime_source_image supports
Shielded VM features before enabling them.
In `@infrastructure/gcp/scripts/setup-secret-manager-from-tfvars.sh`:
- Around line 13-26: The argument parsing loop (while ... case ...) currently
assigns "${2:-}" to PROJECT_ID, TFVARS_PATH, and RUNTIME_SA_EMAIL which allows a
following flag to be captured as a value; update each case branch to validate
the candidate value before assigning (e.g., capture VAL="${2:-}" and if VAL is
empty or matches the prefix --* then print a clear usage/error and exit
non‑zero), otherwise assign the validated value to
PROJECT_ID/TFVARS_PATH/RUNTIME_SA_EMAIL and shift 2; ensure the validation logic
is applied in the --project, --tfvars and --runtime-sa branches so flags cannot
be accepted as values.
- Around line 56-75: The AWK code relies on GNU-specific match(..., m) array
captures and will fail under non-gawk awks; to fix, invoke gawk explicitly
instead of awk in the pipeline that populates SECRETS (the readarray -t SECRETS
< <( ... ) block) so the match($0, ..., m) form works as written and continues
to read from "${TFVARS_PATH}"; alternatively, replace the AWK-based extraction
with a POSIX-safe grep/sed pipeline to extract the quoted values if you prefer
not to require gawk.
In `@infrastructure/gcp/templates/daemon-startup.sh.tftpl`:
- Around line 41-54: The secret fetch using curl that sets secret_payload lacks
timeout and retry handling and can hang if Secret Manager is unavailable; modify
the curl call that builds secret_payload (the block reading
/opt/templar/${container_name}.secret-env and using $TOKEN, $project_id,
$secret_id) to include a bounded timeout and retries (for example --max-time and
--retry/--retry-delay/--retry-connrefused) or wrap the curl in a small retry
loop with exponential backoff and a limited number of attempts, fail gracefully
if all retries exhaust, and preserve existing checks that exit when
secret_payload is empty or null so startup doesn’t hang indefinitely.
- Around line 51-52: The secret decoding pipeline incorrectly applies a
base64url-to-base64 translation using tr '_-' '/+' which corrupts standard
RFC4648 base64 from GCP; update the secret decoding in the secret_value
assignment (the line that defines secret_value and currently pipes through tr
'_-' '/+') to remove the tr '_-' '/+' step so it only base64-decodes
secret_payload (keep tr -d '\r\n' if needed) before appending the env line for
container_name.env.
In `@infrastructure/gcp/terraform.tfvars.example`:
- Around line 37-47: The example terraform vars file places DB credentials
directly in relayer_env.DATABASE_URL; move any sensitive connection
strings/credentials into relayer_secret_env instead. Update the example to
remove the password from relayer_env.DATABASE_URL (leave a host-only placeholder
or omit it) and add a relayer_secret_env entry (e.g., key DATABASE_URL) that
describes storing the full secret via Secret Manager; reference relayer_env and
relayer_secret_env in the comment so users know credentials belong in
relayer_secret_env.
In `@infrastructure/gcp/variables.tf`:
- Around line 427-431: The variable funding_bridge_allowed_source_ranges has an
invalid default of [""], causing Terraform CIDR validation errors; update the
variable block for funding_bridge_allowed_source_ranges to use the same fix
applied to relayer_allowed_source_ranges (replace the default [""] with an empty
list default [] so no empty-string CIDR is provided) and keep the type as
list(string) and description unchanged.
- Around line 203-207: The variable relayer_allowed_source_ranges has an invalid
default of [""] which will break firewall creation; change its default to a
valid value—use an empty list [] if you want to require explicit configuration,
or ["0.0.0.0/0"] only if you intend to allow all traffic by default (discouraged
for production); update the variable "relayer_allowed_source_ranges" default
accordingly in variables.tf.
In `@service/compose.yaml`:
- Around line 41-46: Replace the fragile pgrep-based container healthchecks with
connection checks that verify the service is accepting connections: change the
templar-relayer healthcheck (currently using CMD-SHELL with "pgrep -f
'templar-relayer'") to a TCP or HTTP probe such as a shell command that uses
curl against an HTTP health endpoint (e.g. "curl -f http://localhost:3000/health
|| exit 1") or netcat for TCP (e.g. "sh -c 'nc -z localhost 3000' || exit 1");
do the same for the accumulator and liquidator healthcheck entries and ensure
the container images include curl or netcat so the chosen check will run.
- Around line 12-13: The relayer service declares replicas: 2 while binding a
static host port via ports: - 3000:3000 which will fail in Docker Swarm (only
one container can bind the host port); fix by either removing the host port
mapping so relayer uses the Swarm ingress/mesh (delete or change the ports
entry), change publishing to Swarm ingress mode (use Docker Compose v3 published
mode instead of host binding) so the routing mesh load-balances across replicas,
or constrain to a single instance (set replicas to 1 or add mode: host with
placement constraints) depending on desired topology; locate the relayer service
block and update the ports or replicas/settings accordingly.
In `@service/Dockerfile`:
- Around line 40-51: Add a Docker HEALTHCHECK to the runtime stage so
orchestrators can detect container health: in the Dockerfile runtime stage (the
FROM debian:${DEBIAN_VERSION}-slim AS runtime block that copies /app/bin/* and
sets USER 10001:10001) add a HEALTHCHECK instruction (e.g. HEALTHCHECK
--interval=30s --timeout=5s --start-period=5s CMD-SHELL curl -f
http://localhost:8080/health || exit 1) that targets your service's common
health endpoint; if no common endpoint exists, instead add a brief comment in
the runtime stage documenting that healthchecks are handled externally by
orchestration (Terraform/GCP) so reviewers know the omission is intentional.
In `@service/funding-bridge/src/config.rs`:
- Around line 177-223: The two functions resolve_secret_key and read_env_value
are duplicated across services; extract them into a shared util module (e.g.,
config::env or common::config) and update both service/funding-bridge and
service/liquidator to call the shared functions; to reconcile differing error
types (FundingResult vs Result<_, String>) make resolve_secret_key generic over
the error/result type or accept an error-mapping closure (or require E:
From<ConfigError>/Into<FundingError>) so callers can convert the shared
ConfigError into their local error type, and keep read_env_value returning
Option<String> in the shared module.
- Around line 376-390: The test helper functions unique_temp_file_path and
restore_env are duplicated across services; extract them into a shared test
utility crate (e.g., a new workspace crate like test-utils) and replace the
local copies by importing the shared functions; update config.rs to remove
unique_temp_file_path and restore_env and use the shared crate's functions in
tests, ensuring function signatures match (unique_temp_file_path(prefix: &str)
-> PathBuf and restore_env(name: &str, value: Option<String>)) and add the new
crate as a dev-dependency where needed.
---
Duplicate comments:
In `@infrastructure/gcp/terraform.tfvars.example`:
- Around line 32-34: The example terraform variable
relayer_allowed_source_ranges currently uses an open CIDR "0.0.0.0/0"; change it
to a constrained example CIDR (matching the safer pattern used by
admin_source_ranges, e.g. "203.0.113.10/32") so that
relayer_allowed_source_ranges demonstrates a restricted access pattern and
avoids encouraging copy-pasting a world-open rule into production; update the
example value for relayer_allowed_source_ranges accordingly and keep a brief
comment indicating it should be tightened to specific IPs for production.
- Around line 75-77: Replace the world-open CIDR in the
funding_bridge_allowed_source_ranges example with a constrained example matching
the admin_source_ranges pattern; update the funding_bridge_allowed_source_ranges
array to use a non-global example CIDR (e.g. a /24 from a documentation/test
range like 203.0.113.0/24 or a private range such as 10.0.0.0/24) so it
demonstrates a safe, limited source range consistent with admin_source_ranges.
In `@service/compose.yaml`:
- Around line 12-13: The relayer service is hard-coded to bind host port 3000
(service name relayer) which can collide with funding-bridge when PORT is set to
3000; update both service port declarations to use distinct environment
variables (e.g., RELAYER_PORT and FUNDING_BRIDGE_PORT) with sensible defaults
(e.g., "${RELAYER_PORT:-3000}:${RELAYER_PORT:-3000}" and
"${FUNDING_BRIDGE_PORT:-3001}:${FUNDING_BRIDGE_PORT:-3001}") and update the .env
and any docs or startup scripts to define those new variables so each service
(relayer and funding-bridge) cannot accidentally bind the same host port.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08ab6069-7751-4d40-afce-66eb18a151e4
📒 Files selected for processing (15)
.github/workflows/release-plz.ymlinfrastructure/gcp/README.mdinfrastructure/gcp/main.tfinfrastructure/gcp/runtime.tfinfrastructure/gcp/scripts/setup-secret-manager-from-tfvars.shinfrastructure/gcp/templates/daemon-startup.sh.tftplinfrastructure/gcp/terraform.tfvars.exampleinfrastructure/gcp/variables.tfrelease-plz.tomlservice/Dockerfileservice/compose.yamlservice/funding-bridge/src/config.rsservice/funding-bridge/src/main.rsservice/liquidator/src/config.rsservice/liquidator/src/main.rs
Comment on lines
+10
to
+31
| release-plz-release: | ||
| name: Release-plz release | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| steps: | ||
| - &checkout | ||
| name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| persist-credentials: false | ||
| - &install-rust | ||
| name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
| - name: Run release-plz | ||
| uses: release-plz/[email protected] | ||
| with: | ||
| command: release | ||
| config: .release-plz.toml | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What is the latest version of the release-plz GitHub Action and is v0.5 valid?
💡 Result:
- Latest release-plz GitHub Action version (tagged release):
v0.5.128(released Mar 7, 2026). [1] - Is
v0.5valid? Yes.v0.5is a moving minor-version tag that points to the latest0.5.xrelease (currentlyv0.5.128). [2][3]
So uses: release-plz/[email protected] is valid (it will track the latest 0.5.x), or you can pin to v0.5.128 for an immutable version. [1][3]
Sources
[1] https://github.com/release-plz/action/releases (shows latest release v0.5.128)
[2] https://github.com/release-plz/action/tags (shows the v0.5 tag pointing at v0.5.128)
[3] https://release-plz.dev/docs/github/security (explains v0.5 tracks latest 0.5.x)
Consider adding concurrency control and evaluate SHA pinning for security hardening.
The release-plz/[email protected] is a valid moving tag that tracks the latest 0.5.x release, so no action needed on the version itself.
Two recommendations remain:
-
Add concurrency control: Without concurrency protection, rapid commits to
maincould trigger simultaneous release jobs, leading to race conditions or conflicting releases. -
Consider SHA pinning: For additional security, pin the action to a specific commit SHA (e.g.,
release-plz/action@<sha>) rather than a tag to prevent potential tag manipulation attacks.
Suggested improvement
release-plz-release:
name: Release-plz release
runs-on: ubuntu-latest
+ concurrency:
+ group: release-plz-release-${{ github.ref }}
+ cancel-in-progress: false
permissions:
contents: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-plz.yml around lines 10 - 31, Add concurrency
control to the "release-plz-release" job to prevent overlapping runs (use the
workflow-level or job-level concurrency key referencing the branch or ref) and
decide whether to pin release-plz/action to a commit SHA instead of the moving
tag "release-plz/[email protected]"; update the job that currently uses
release-plz/[email protected] (the "Run release-plz" step) to use a specific SHA if
you choose to hard-pin, or add a comment explaining why the tag is acceptable,
and add a concurrency entry (e.g., a unique group based on github.ref or
workflow) to the same job to avoid race conditions during rapid commits.
Comment on lines
+16
to
+24
| - &checkout | ||
| name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| persist-credentials: false | ||
| - &install-rust | ||
| name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider readability: YAML anchors work but are uncommon in GitHub Actions.
The YAML anchor syntax (&checkout, *checkout) is valid and will work correctly, but it's rarely seen in GitHub Actions workflows. Most teams prefer explicit step definitions or reusable workflows for clarity.
If the duplication becomes significant, consider creating a reusable workflow or composite action instead.
Also applies to: 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-plz.yml around lines 16 - 24, The workflow uses
YAML anchors (&checkout, &install-rust) which are uncommon in GitHub Actions;
replace the anchored/aliased steps with explicit step definitions (expand the
&checkout/*checkout and &install-rust usages into full "Checkout repository" and
"Install Rust toolchain" steps) or extract the duplicated logic into a reusable
workflow or composite action and call that instead; update every occurrence
(e.g., the "Checkout repository" and "Install Rust toolchain" step blocks) so
the workflow is clear and easy to read for other contributors.
Comment on lines
+326
to
+378
| resource "google_compute_instance" "accumulator" { | ||
| count = var.enable_runtime && var.accumulator_enabled ? 1 : 0 | ||
|
|
||
| project = var.project_id | ||
| zone = local.accumulator_zone_effective | ||
| name = "templar-accumulator" | ||
| machine_type = var.accumulator_machine_type | ||
| tags = ["templar-runtime", "templar-accumulator"] | ||
|
|
||
| boot_disk { | ||
| auto_delete = true | ||
|
|
||
| initialize_params { | ||
| image = var.runtime_source_image | ||
| size = var.accumulator_disk_size_gb | ||
| type = "pd-balanced" | ||
| } | ||
| } | ||
|
|
||
| network_interface { | ||
| subnetwork = google_compute_subnetwork.runtime[0].id | ||
|
|
||
| access_config {} | ||
| } | ||
|
|
||
| metadata_startup_script = templatefile("${path.module}/templates/daemon-startup.sh.tftpl", { | ||
| registry_host = "${local.artifact_registry_location}-docker.pkg.dev" | ||
| image = local.accumulator_image_effective | ||
| container_name = var.accumulator_container_name | ||
| binary_path = "/app/bin/accumulator" | ||
| project_id = var.project_id | ||
| env_file = local.accumulator_env_file | ||
| secret_env_file = local.accumulator_secret_env_file | ||
| }) | ||
|
|
||
| service_account { | ||
| email = google_service_account.runtime[0].email | ||
| scopes = ["https://www.googleapis.com/auth/cloud-platform"] | ||
| } | ||
|
|
||
| scheduling { | ||
| automatic_restart = true | ||
| on_host_maintenance = "MIGRATE" | ||
| preemptible = false | ||
| } | ||
|
|
||
| allow_stopping_for_update = true | ||
|
|
||
| depends_on = [ | ||
| google_project_iam_member.runtime_sa_roles, | ||
| google_secret_manager_secret_iam_member.runtime_secret_accessor | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enabling Shielded VM features for defense-in-depth.
The standalone instances lack shielded_instance_config, which provides boot integrity verification. While not critical, it's a recommended hardening measure for production workloads.
🛡️ Example configuration
resource "google_compute_instance" "accumulator" {
...
shielded_instance_config {
enable_secure_boot = true
enable_vtpm = true
enable_integrity_monitoring = true
}
...
}Note: Ensure the runtime_source_image supports Shielded VM features.
Also applies to: 380-432, 434-486
🧰 Tools
🪛 Checkov (3.2.508)
[high] 326-378: Ensure 'Block Project-wide SSH keys' is enabled for VM instances
(CKV_GCP_32)
[high] 326-378: Ensure VM disks for critical VMs are encrypted with Customer Supplied Encryption Keys (CSEK)
(CKV_GCP_38)
[low] 326-378: Ensure that Compute instances do not have public IP addresses
(CKV_GCP_40)
🪛 Trivy (0.69.3)
[warning] 326-378: Disable project-wide SSH keys for all instances
Instance allows use of project-level SSH keys.
Rule: GCP-0030
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
[error] 348-348: Instances should not have public IP addresses
Instance has a public IP allocated.
Rule: GCP-0031
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
[info] 335-343: VM disks should be encrypted with Customer Supplied Encryption Keys
Instance disk encryption does not use a customer managed key.
Rule: GCP-0033
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
[warning] 326-378: Instances should have Shielded VM VTPM enabled
Instance does not have VTPM for shielded VMs enabled.
Rule: GCP-0041
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
[warning] 326-378: Instances should have Shielded VM integrity monitoring enabled
Instance does not have shielded VM integrity monitoring enabled.
Rule: GCP-0045
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
[warning] 326-378: Instances should have Shielded VM secure boot enabled
Instance does not have shielded VM secure boot enabled.
Rule: GCP-0067
Resource: google_compute_instance.accumulator[0]
(IaC/Google)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/gcp/runtime.tf` around lines 326 - 378, Add a
shielded_instance_config block to the google_compute_instance.accumulator
resource to enable secure boot, vTPM and integrity monitoring for
defense-in-depth; update the resource (and the other similar instances
referenced in the comment) to include shielded_instance_config with
enable_secure_boot = true, enable_vtpm = true, and enable_integrity_monitoring =
true and ensure the runtime_source_image supports Shielded VM features before
enabling them.
Comment on lines
+12
to
+13
| ports: | ||
| - 3000:3000 |
Contributor
There was a problem hiding this comment.
Replicas with static port binding will fail in Swarm mode.
The relayer service specifies replicas: 2 but also binds to a static host port 3000:3000. In Docker Swarm, only one replica can bind to port 3000 on a given host. Either remove the port binding (use ingress/mesh networking), use mode: host with placement constraints, or reduce replicas to 1 per host.
🔧 Option: Use Swarm ingress routing mesh
ports:
- - 3000:3000
+ - target: 3000
+ published: 3000
+ mode: ingressWith ingress mode, Swarm's routing mesh will load-balance across replicas.
Also applies to: 19-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/compose.yaml` around lines 12 - 13, The relayer service declares
replicas: 2 while binding a static host port via ports: - 3000:3000 which will
fail in Docker Swarm (only one container can bind the host port); fix by either
removing the host port mapping so relayer uses the Swarm ingress/mesh (delete or
change the ports entry), change publishing to Swarm ingress mode (use Docker
Compose v3 published mode instead of host binding) so the routing mesh
load-balances across replicas, or constrain to a single instance (set replicas
to 1 or add mode: host with placement constraints) depending on desired
topology; locate the relayer service block and update the ports or
replicas/settings accordingly.
Comment on lines
+41
to
+46
| healthcheck: | ||
| test: ["CMD-SHELL", "pgrep -f 'templar-relayer' >/dev/null || exit 1"] | ||
| interval: 30s | ||
| timeout: 10s | ||
| retries: 5 | ||
| start_period: 20s |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more robust healthchecks than pgrep.
The pgrep -f 'templar-relayer' healthcheck only verifies the process is running, not that it's healthy or accepting connections. Since the relayer exposes a port (3000), an HTTP or TCP healthcheck would be more reliable. The same applies to accumulator and liquidator.
💡 Example: TCP healthcheck for relayer
healthcheck:
- test: ["CMD-SHELL", "pgrep -f 'templar-relayer' >/dev/null || exit 1"]
+ test: ["CMD-SHELL", "nc -z localhost 3000 || exit 1"]
interval: 30s
timeout: 10s
retries: 5
start_period: 20sNote: This requires netcat in the container, or use curl if there's a /health endpoint.
📝 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.
Suggested change
| healthcheck: | |
| test: ["CMD-SHELL", "pgrep -f 'templar-relayer' >/dev/null || exit 1"] | |
| interval: 30s | |
| timeout: 10s | |
| retries: 5 | |
| start_period: 20s | |
| healthcheck: | |
| test: ["CMD-SHELL", "nc -z localhost 3000 || exit 1"] | |
| interval: 30s | |
| timeout: 10s | |
| retries: 5 | |
| start_period: 20s |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/compose.yaml` around lines 41 - 46, Replace the fragile pgrep-based
container healthchecks with connection checks that verify the service is
accepting connections: change the templar-relayer healthcheck (currently using
CMD-SHELL with "pgrep -f 'templar-relayer'") to a TCP or HTTP probe such as a
shell command that uses curl against an HTTP health endpoint (e.g. "curl -f
http://localhost:3000/health || exit 1") or netcat for TCP (e.g. "sh -c 'nc -z
localhost 3000' || exit 1"); do the same for the accumulator and liquidator
healthcheck entries and ensure the container images include curl or netcat so
the chosen check will run.
Comment on lines
+40
to
+51
| FROM debian:${DEBIAN_VERSION}-slim AS runtime | ||
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| curl \ | ||
| libssl3 \ | ||
| procps \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| WORKDIR /app | ||
| COPY --from=builder /app/bin/* /app/bin/ | ||
|
|
||
| USER 10001:10001 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding HEALTHCHECK for orchestrator visibility.
Static analysis flags the missing HEALTHCHECK instruction. Since this is a multi-service image where different binaries may expose different health endpoints (or none), you may need to handle health checks at the orchestration layer (e.g., GCP health checks configured in Terraform) rather than in the Dockerfile itself. If a common health endpoint exists across services, adding a HEALTHCHECK here would improve container runtime visibility.
🧰 Tools
🪛 Checkov (3.2.508)
[low] 1-51: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Hadolint (2.14.0)
[warning] 41-41: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/Dockerfile` around lines 40 - 51, Add a Docker HEALTHCHECK to the
runtime stage so orchestrators can detect container health: in the Dockerfile
runtime stage (the FROM debian:${DEBIAN_VERSION}-slim AS runtime block that
copies /app/bin/* and sets USER 10001:10001) add a HEALTHCHECK instruction (e.g.
HEALTHCHECK --interval=30s --timeout=5s --start-period=5s CMD-SHELL curl -f
http://localhost:8080/health || exit 1) that targets your service's common
health endpoint; if no common endpoint exists, instead add a brief comment in
the runtime stage documenting that healthchecks are handled externally by
orchestration (Terraform/GCP) so reviewers know the omission is intentional.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/funding-bridge/src/external/solana.rs (1)
403-426: 🧹 Nitpick | 🔵 TrivialConsider propagating error details for debugging.
The
SolanaSdkHandler::from_base58(...).ok()at line 413 discards the error message. When handler creation fails, callers only see "Failed to configure Solana chain handler" without knowing why (invalid base58, wrong key length, etc.).Consider returning
Resultinstead ofOptionto preserve error context:♻️ Optional refactor to preserve error context
pub fn solana_sdk_handler( private_key: &str, rpc_url: Option<&str>, -) -> Option<Box<dyn ExternalChainHandler>> { +) -> Result<Box<dyn ExternalChainHandler>, String> { let mut config = SolanaSdkConfig::mainnet(); if let Some(rpc_url) = rpc_url { config.rpc_url = rpc_url.to_string(); } - let handler = SolanaSdkHandler::from_base58(config.clone(), private_key).ok(); - - if let Some(h) = handler { + let h = SolanaSdkHandler::from_base58(config.clone(), private_key)?; + info!( chain_id = %h.config.chain_id, rpc_url = %h.config.rpc_url, public_key = %h.public_key(), "Configured Solana SDK handler" ); - Some(Box::new(h)) - } else { - None - } + Ok(Box::new(h)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/funding-bridge/src/external/solana.rs` around lines 403 - 426, The current solana_sdk_handler function swallows errors by calling SolanaSdkHandler::from_base58(...).ok(); change the function to return a Result (e.g., Result<Box<dyn ExternalChainHandler>, anyhow::Error> or a project error type) instead of Option so you can propagate the failure from SolanaSdkHandler::from_base58; call .map(|h| Box::new(h) as Box<dyn ExternalChainHandler>) on Ok and map_err to convert the error into the chosen error type, and update the caller sites to handle the Result; also include the error details in the info/error log (use the error from map_err) so failures like invalid base58 or wrong key length are visible.
♻️ Duplicate comments (1)
infrastructure/gcp/terraform.tfvars.example (1)
38-53:⚠️ Potential issue | 🟡 MinorRemove
DATABASE_URLfromrelayer_envto avoid duplication.
DATABASE_URLappears in bothrelayer_env(line 39) andrelayer_secret_env(line 50). At runtime, the startup script appends secret values to the env file, resulting in duplicate entries. Since the full connection string with credentials should come from Secret Manager, remove it fromrelayer_env.Proposed fix
relayer_env = { - DATABASE_URL = "postgres://db-host:5432/relayer" RPC_URL = "https://rpc.mainnet.near.org" RELAY_ACCOUNT_ID = "relayer.near" UA_ACCOUNT_ID = "ua-relayer.near" UA_REGISTRY_ID = "v1.tmplr.near" UA_VERSION_KEY = "market-current" MONITOR_REGISTRY_ID = "v1.tmplr.near" PYTH_ORACLE_ID = "pyth-oracle.near" INTENTS_ID = "intents.near" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/gcp/terraform.tfvars.example` around lines 38 - 53, Remove the duplicate DATABASE_URL from the relayer_env map so the full connection string only comes from relayer_secret_env; update the relayer_env block to omit the DATABASE_URL key (leave DATABASE_URL only in relayer_secret_env) and ensure any startup/env-file logic reads the secret value from relayer_secret_env rather than relayer_env; verify references to relayer_env, relayer_secret_env and DATABASE_URL in deployment/startup scripts remain correct after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/accumulator/scripts/run_service.sh`:
- Around line 162-175: The current check_running_instances function uses a broad
regex "(/|^)accumulator( |$)" which can match unrelated commands; update
check_running_instances to detect the exact templar binary or service instance
instead—either by checking a PID file created by your service, using pgrep -x
"accumulator" to match the exact process name, or by matching the full
installation path (e.g., /opt/templar/bin/accumulator) when calling pgrep/pkill;
ensure the same exact match method is used for both the detection and the pkill
call so only the intended accumulator process is targeted.
- Around line 125-140: The check_binary function currently uses a relative path
("../../target/release/accumulator") which breaks if the script is invoked from
a different cwd; change check_binary to construct the binary_path from a known
absolute root (use the existing PROJECT_DIR or resolve the script/repo root via
realpath) so binary_path becomes "$PROJECT_DIR/target/release/accumulator" (or
the resolved absolute path), keep the same checks (existence and executable) and
the cargo build invocation but run it from PROJECT_DIR or pass the correct
package path so the built binary lands at that absolute path, and finally assign
that absolute path to BINARY_PATH.
In `@service/liquidator/src/config.rs`:
- Around line 316-319: Add a short doc comment on the build_config() function
stating it requires validate() to have been called before invocation (so callers
won't inadvertently trigger the expect on signer_key), and while you're there
improve the expect message on the signer_key field to be explicit (e.g.,
"SIGNER_KEY or SIGNER_KEY_FILE must be set before build_config()") to make the
invariant clear; reference build_config(), validate(), and the signer_key expect
expression when making these changes.
---
Outside diff comments:
In `@service/funding-bridge/src/external/solana.rs`:
- Around line 403-426: The current solana_sdk_handler function swallows errors
by calling SolanaSdkHandler::from_base58(...).ok(); change the function to
return a Result (e.g., Result<Box<dyn ExternalChainHandler>, anyhow::Error> or a
project error type) instead of Option so you can propagate the failure from
SolanaSdkHandler::from_base58; call .map(|h| Box::new(h) as Box<dyn
ExternalChainHandler>) on Ok and map_err to convert the error into the chosen
error type, and update the caller sites to handle the Result; also include the
error details in the info/error log (use the error from map_err) so failures
like invalid base58 or wrong key length are visible.
---
Duplicate comments:
In `@infrastructure/gcp/terraform.tfvars.example`:
- Around line 38-53: Remove the duplicate DATABASE_URL from the relayer_env map
so the full connection string only comes from relayer_secret_env; update the
relayer_env block to omit the DATABASE_URL key (leave DATABASE_URL only in
relayer_secret_env) and ensure any startup/env-file logic reads the secret value
from relayer_secret_env rather than relayer_env; verify references to
relayer_env, relayer_secret_env and DATABASE_URL in deployment/startup scripts
remain correct after removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd274683-55cf-4bc8-b7b7-bcc52b6a9c71
📒 Files selected for processing (19)
common/src/config/env.rscommon/src/config/mod.rscommon/src/lib.rsinfrastructure/gcp/README.mdinfrastructure/gcp/scripts/setup-secret-manager-from-tfvars.shinfrastructure/gcp/templates/daemon-startup.sh.tftplinfrastructure/gcp/terraform.tfvars.exampleinfrastructure/gcp/variables.tfservice/accumulator/.env.exampleservice/accumulator/README.mdservice/accumulator/scripts/run_service.shservice/accumulator/src/lib.rsservice/accumulator/src/main.rsservice/funding-bridge/src/app.rsservice/funding-bridge/src/config.rsservice/funding-bridge/src/external/solana.rsservice/funding-bridge/src/external/stellar.rsservice/liquidator/src/config.rsservice/liquidator/src/main.rs
Comment on lines
+125
to
+140
| check_binary() { | ||
| local binary_path="../../target/release/accumulator" | ||
|
|
||
| if [ ! -f "$binary_path" ]; then | ||
| print_warn "Binary not found at $binary_path" | ||
| print_info "Building accumulator..." | ||
| cargo build --release -p templar-accumulator --bin accumulator | ||
| fi | ||
|
|
||
| if [ ! -x "$binary_path" ]; then | ||
| print_error "Binary at $binary_path is not executable" | ||
| exit 1 | ||
| fi | ||
|
|
||
| BINARY_PATH="$binary_path" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Relative binary path assumes specific working directory.
The path ../../target/release/accumulator assumes the script is run from service/accumulator/ directory. While line 17 does cd "$PROJECT_DIR", this makes the binary path ../../target/release/accumulator resolve relative to service/accumulator/.
Consider using an absolute path or resolving from the repository root for robustness:
♻️ Suggested improvement
check_binary() {
- local binary_path="../../target/release/accumulator"
+ # Resolve to repo root (service/accumulator -> service -> repo_root)
+ local repo_root
+ repo_root="$(cd "$PROJECT_DIR/../.." && pwd)"
+ local binary_path="$repo_root/target/release/accumulator"
if [ ! -f "$binary_path" ]; then
print_warn "Binary not found at $binary_path"
print_info "Building accumulator..."
- cargo build --release -p templar-accumulator --bin accumulator
+ (cd "$repo_root" && cargo build --release -p templar-accumulator --bin accumulator)
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.
Suggested change
| check_binary() { | |
| local binary_path="../../target/release/accumulator" | |
| if [ ! -f "$binary_path" ]; then | |
| print_warn "Binary not found at $binary_path" | |
| print_info "Building accumulator..." | |
| cargo build --release -p templar-accumulator --bin accumulator | |
| fi | |
| if [ ! -x "$binary_path" ]; then | |
| print_error "Binary at $binary_path is not executable" | |
| exit 1 | |
| fi | |
| BINARY_PATH="$binary_path" | |
| } | |
| check_binary() { | |
| # Resolve to repo root (service/accumulator -> service -> repo_root) | |
| local repo_root | |
| repo_root="$(cd "$PROJECT_DIR/../.." && pwd)" | |
| local binary_path="$repo_root/target/release/accumulator" | |
| if [ ! -f "$binary_path" ]; then | |
| print_warn "Binary not found at $binary_path" | |
| print_info "Building accumulator..." | |
| (cd "$repo_root" && cargo build --release -p templar-accumulator --bin accumulator) | |
| fi | |
| if [ ! -x "$binary_path" ]; then | |
| print_error "Binary at $binary_path is not executable" | |
| exit 1 | |
| fi | |
| BINARY_PATH="$binary_path" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/accumulator/scripts/run_service.sh` around lines 125 - 140, The
check_binary function currently uses a relative path
("../../target/release/accumulator") which breaks if the script is invoked from
a different cwd; change check_binary to construct the binary_path from a known
absolute root (use the existing PROJECT_DIR or resolve the script/repo root via
realpath) so binary_path becomes "$PROJECT_DIR/target/release/accumulator" (or
the resolved absolute path), keep the same checks (existence and executable) and
the cargo build invocation but run it from PROJECT_DIR or pass the correct
package path so the built binary lands at that absolute path, and finally assign
that absolute path to BINARY_PATH.
Comment on lines
+162
to
+175
| check_running_instances() { | ||
| if pgrep -f "(/|^)accumulator( |$)" > /dev/null; then | ||
| print_warn "Another accumulator instance appears to be running" | ||
| read -p "Kill existing instances? (y/N) " -n 1 -r | ||
| echo | ||
| if [[ $REPLY =~ ^[Yy]$ ]]; then | ||
| pkill -f "(/|^)accumulator( |$)" || true | ||
| sleep 2 | ||
| else | ||
| print_info "Aborting..." | ||
| exit 1 | ||
| fi | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
Process matching pattern may be too broad.
The regex "(/|^)accumulator( |$)" could match unrelated processes that happen to have "accumulator" in their command line (e.g., a different project's accumulator binary or a process with "accumulator" in its arguments).
Consider using a more specific pattern that includes the full path or the templar-specific binary name:
♻️ Suggested improvement
check_running_instances() {
- if pgrep -f "(/|^)accumulator( |$)" > /dev/null; then
+ if pgrep -f "target/release/accumulator" > /dev/null; then
print_warn "Another accumulator instance appears to be running"
read -p "Kill existing instances? (y/N) " -n 1 -r
echo
if [[ $REPLY =~ ^[Yy]$ ]]; then
- pkill -f "(/|^)accumulator( |$)" || true
+ pkill -f "target/release/accumulator" || true
sleep 2
else
print_info "Aborting..."
exit 1
fi
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.
Suggested change
| check_running_instances() { | |
| if pgrep -f "(/|^)accumulator( |$)" > /dev/null; then | |
| print_warn "Another accumulator instance appears to be running" | |
| read -p "Kill existing instances? (y/N) " -n 1 -r | |
| echo | |
| if [[ $REPLY =~ ^[Yy]$ ]]; then | |
| pkill -f "(/|^)accumulator( |$)" || true | |
| sleep 2 | |
| else | |
| print_info "Aborting..." | |
| exit 1 | |
| fi | |
| fi | |
| } | |
| check_running_instances() { | |
| if pgrep -f "target/release/accumulator" > /dev/null; then | |
| print_warn "Another accumulator instance appears to be running" | |
| read -p "Kill existing instances? (y/N) " -n 1 -r | |
| echo | |
| if [[ $REPLY =~ ^[Yy]$ ]]; then | |
| pkill -f "target/release/accumulator" || true | |
| sleep 2 | |
| else | |
| print_info "Aborting..." | |
| exit 1 | |
| fi | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/accumulator/scripts/run_service.sh` around lines 162 - 175, The
current check_running_instances function uses a broad regex "(/|^)accumulator(
|$)" which can match unrelated commands; update check_running_instances to
detect the exact templar binary or service instance instead—either by checking a
PID file created by your service, using pgrep -x "accumulator" to match the
exact process name, or by matching the full installation path (e.g.,
/opt/templar/bin/accumulator) when calling pgrep/pkill; ensure the same exact
match method is used for both the detection and the pkill call so only the
intended accumulator process is targeted.
Comment on lines
+316
to
+319
| signer_key: self | ||
| .signer_key | ||
| .clone() | ||
| .expect("SIGNER_KEY or SIGNER_KEY_FILE must be set before build_config"), |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The .expect() relies on validate() being called first—consider documenting this invariant.
While the current call flow in main.rs ensures validate() is called before build_config(), this implicit dependency could lead to a panic if a future caller skips validation. The expect message documents the requirement, which is acceptable, but consider adding a doc comment to build_config() noting that validate() must be called first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/liquidator/src/config.rs` around lines 316 - 319, Add a short doc
comment on the build_config() function stating it requires validate() to have
been called before invocation (so callers won't inadvertently trigger the expect
on signer_key), and while you're there improve the expect message on the
signer_key field to be explicit (e.g., "SIGNER_KEY or SIGNER_KEY_FILE must be
set before build_config()") to make the invariant clear; reference
build_config(), validate(), and the signer_key expect expression when making
these changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/accumulator/src/main.rs`:
- Around line 111-113: Replace the invalid "match ... else" construct when
extracting args.signer_key with a proper let-else or a match that returns an
anyhow error; specifically, in run_service use a let Some(signer_key) =
args.signer_key.clone() else { anyhow::bail!("SIGNER_KEY or SIGNER_KEY_FILE must
be set before run_service") }; (or equivalently match args.signer_key.clone() {
Some(v) => v, None => anyhow::bail!(...) }) so you stop panicking and return an
anyhow::Error instead of calling panic!; reference args.signer_key and
run_service when locating the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a22dafc-5174-449c-9af7-5eb19fc5299d
📒 Files selected for processing (1)
service/accumulator/src/main.rs
peer2f00l
reviewed
Mar 23, 2026
peer2f00l
left a comment
peer2f00l
left a comment
Collaborator
There was a problem hiding this comment.
After reading through, I'm not seeing anything too major. Admittedly, I'm lacking in experience when it comes to Terraform, so I'm tagging @vitaliybezz for a second look.
CodeRabbit mentioned writing secrets to artifacts, so please address that or resolve if it is incorrect/irrelevant.
Comment on lines
+9
to
+22
| relayer_zones_effective = length(var.relayer_zones) > 0 ? var.relayer_zones : ["${var.region}-b", "${var.region}-c"] | ||
| accumulator_zone_effective = coalesce( | ||
| var.accumulator_zone, | ||
| local.relayer_zones_effective[0] | ||
| ) | ||
| market_monitor_zone_effective = coalesce( | ||
| var.market_monitor_zone, | ||
| local.relayer_zones_effective[0] | ||
| ) | ||
| funding_bridge_zone_effective = coalesce( | ||
| var.funding_bridge_zone, | ||
| local.relayer_zones_effective[0] | ||
| ) | ||
|
|
Collaborator
There was a problem hiding this comment.
Definitely overkill for our current needs, but I suppose it doesn't hurt to future-proof.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/accumulator/src/main.rs (1)
329-363:⚠️ Potential issue | 🟡 MinorUse a helper for env var restoration to ensure panic-safety.
The manual restore block (lines 341–363) runs only after
parse_args_from().unwrap()andassert_eq!(). Any panic before that point leaves the mutated env vars unrestore. Extract arestore_envhelper like the one insrc/lib.rs(used byparse_args_from_supports_signer_key_file), or wrap mutations in a scoped guard, to ensure cleanup always runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/accumulator/src/main.rs` around lines 329 - 363, The test mutates environment variables directly and restores them only after calling Args::parse_args_from and asserting, which is not panic-safe; extract or reuse a helper (e.g., the restore_env helper used in src/lib.rs or create a scoped guard) to capture original values of REGISTRIES_ACCOUNT_IDS, SIGNER_ACCOUNT_ID, SIGNER_KEY, and SIGNER_KEY_FILE before mutating, register a cleanup closure/Drop implementation that restores or removes each var on Drop, then replace the current manual restore block with that helper so cleanup runs even if parse_args_from or the assertion panics; locate this logic around the test that calls Args::parse_args_from_from(["accumulator"]) and the manual restore block to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-service-image.yml:
- Around line 4-22: Add ".dockerignore" to the path filters used by the
pull_request and push triggers so changes to the root .dockerignore will run the
build job; update the "paths" arrays under the pull_request and push entries in
the workflow to include the string ".dockerignore" alongside the existing
entries so image rebuilds occur when .dockerignore changes.
- Around line 39-42: Replace all mutable action tags with immutable commit SHAs:
locate every uses: entry that currently references actions/checkout@v4,
docker/setup-buildx-action@v3 and the other uses lines using `@v6/`@v2, and swap
the tag (e.g., `@v4/`@v3/@v6/@v2) for the corresponding full commit SHA for that
action repository; update each uses: reference (actions/checkout,
docker/setup-buildx-action, docker/login-action, docker/build-push-action, etc.)
so the workflow pins to the exact commit SHA, then verify the SHAs point to the
intended release and run the workflow to confirm behavior.
In `@infrastructure/gcp/templates/daemon-startup.sh.tftpl`:
- Around line 64-65: The current decoding step uses tr -d '\r\n' which silently
strips newlines from decoded secrets (variable secret_value) and mangles
multiline secrets; change the logic in the secret payload handling around
secret_value/env_key/env_path to reject any decoded payloads that contain
newlines or carriage returns instead of removing them: decode payload into
secret_value (without tr), validate that secret_value contains no '\n' or '\r'
and, if it does, log a clear error (including env_key) and exit non‑zero so the
startup fails fast rather than writing a flattened value to env_path; remove the
tr -d '\r\n' usage and add the newline check + exit on failure.
In `@service/accumulator/src/main.rs`:
- Around line 17-27: The code leaves Args.signer_key as Option<SecretKey> and
performs validation in two places; instead encode the signer-key invariant in
the type by converting parsed Args into a validated config where signer_key is
SecretKey (non-Option) before passing to run_service. Add a constructor or
method (e.g. Args::into_validated or new ValidatedArgs) that performs the
existing checks from Args::validate and returns a ValidatedArgs struct with
signer_key: SecretKey, update main to call that and exit on error, change
run_service signature to accept ValidatedArgs (or the new config type) and
remove the redundant .context()/signer_key re-check there. Ensure all call sites
use the validated type so invalid states are unrepresentable.
---
Outside diff comments:
In `@service/accumulator/src/main.rs`:
- Around line 329-363: The test mutates environment variables directly and
restores them only after calling Args::parse_args_from and asserting, which is
not panic-safe; extract or reuse a helper (e.g., the restore_env helper used in
src/lib.rs or create a scoped guard) to capture original values of
REGISTRIES_ACCOUNT_IDS, SIGNER_ACCOUNT_ID, SIGNER_KEY, and SIGNER_KEY_FILE
before mutating, register a cleanup closure/Drop implementation that restores or
removes each var on Drop, then replace the current manual restore block with
that helper so cleanup runs even if parse_args_from or the assertion panics;
locate this logic around the test that calls
Args::parse_args_from_from(["accumulator"]) and the manual restore block to
update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 042360b0-de71-4b39-afa2-fb44ed89b23c
📒 Files selected for processing (4)
.dockerignore.github/workflows/build-service-image.ymlinfrastructure/gcp/templates/daemon-startup.sh.tftplservice/accumulator/src/main.rs
Comment on lines
+4
to
+22
| pull_request: | ||
| paths: | ||
| - ".github/workflows/build-service-image.yml" | ||
| - "Cargo.toml" | ||
| - "Cargo.lock" | ||
| - "common/**" | ||
| - "service/**" | ||
| - "test-utils/**" | ||
| - "universal-account/**" | ||
| push: | ||
| branches: [main, dev] | ||
| paths: | ||
| - ".github/workflows/build-service-image.yml" | ||
| - "Cargo.toml" | ||
| - "Cargo.lock" | ||
| - "common/**" | ||
| - "service/**" | ||
| - "test-utils/**" | ||
| - "universal-account/**" |
Contributor
There was a problem hiding this comment.
Add .dockerignore to both trigger path filters.
This job builds with context: ., but future commits that only change the root .dockerignore will skip the workflow entirely. That can change image contents or re-include sensitive files without rebuilding the image.
Suggested patch
pull_request:
paths:
+ - ".dockerignore"
- ".github/workflows/build-service-image.yml"
- "Cargo.toml"
- "Cargo.lock"
- "common/**"
- "service/**"
- "test-utils/**"
- "universal-account/**"
push:
branches: [main, dev]
paths:
+ - ".dockerignore"
- ".github/workflows/build-service-image.yml"
- "Cargo.toml"
- "Cargo.lock"
- "common/**"
- "service/**"
- "test-utils/**"
- "universal-account/**"📝 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.
Suggested change
| pull_request: | |
| paths: | |
| - ".github/workflows/build-service-image.yml" | |
| - "Cargo.toml" | |
| - "Cargo.lock" | |
| - "common/**" | |
| - "service/**" | |
| - "test-utils/**" | |
| - "universal-account/**" | |
| push: | |
| branches: [main, dev] | |
| paths: | |
| - ".github/workflows/build-service-image.yml" | |
| - "Cargo.toml" | |
| - "Cargo.lock" | |
| - "common/**" | |
| - "service/**" | |
| - "test-utils/**" | |
| - "universal-account/**" | |
| pull_request: | |
| paths: | |
| - ".dockerignore" | |
| - ".github/workflows/build-service-image.yml" | |
| - "Cargo.toml" | |
| - "Cargo.lock" | |
| - "common/**" | |
| - "service/**" | |
| - "test-utils/**" | |
| - "universal-account/**" | |
| push: | |
| branches: [main, dev] | |
| paths: | |
| - ".dockerignore" | |
| - ".github/workflows/build-service-image.yml" | |
| - "Cargo.toml" | |
| - "Cargo.lock" | |
| - "common/**" | |
| - "service/**" | |
| - "test-utils/**" | |
| - "universal-account/**" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-service-image.yml around lines 4 - 22, Add
".dockerignore" to the path filters used by the pull_request and push triggers
so changes to the root .dockerignore will run the build job; update the "paths"
arrays under the pull_request and push entries in the workflow to include the
string ".dockerignore" alongside the existing entries so image rebuilds occur
when .dockerignore changes.
Comment on lines
+39
to
+42
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/build-service-image.ymlRepository: Templar-Protocol/contracts
Length of output: 4422
Pin every action to an immutable commit SHA.
This workflow can publish deployable images, but each uses: entry currently trusts a mutable major tag (@v4, @v3, @v6, @v2). Pinning to full SHAs closes a supply-chain gap and makes reruns reproducible.
Lines 39, 42, 46, 68, 77, and 102 all require updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-service-image.yml around lines 39 - 42, Replace all
mutable action tags with immutable commit SHAs: locate every uses: entry that
currently references actions/checkout@v4, docker/setup-buildx-action@v3 and the
other uses lines using `@v6/`@v2, and swap the tag (e.g., `@v4/`@v3/@v6/@v2) for the
corresponding full commit SHA for that action repository; update each uses:
reference (actions/checkout, docker/setup-buildx-action, docker/login-action,
docker/build-push-action, etc.) so the workflow pins to the exact commit SHA,
then verify the SHAs point to the intended release and run the workflow to
confirm behavior.
Comment on lines
+64
to
+65
| secret_value="$(printf '%s' "$secret_payload" | base64 -d | tr -d '\r\n')" | ||
| printf '%s=%s\n' "$env_key" "$secret_value" >>"$env_path" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Docker --env-file support values containing literal newline characters, or are multiline secrets unsupported and expected to be delivered via files or another mechanism?
💡 Result:
No, Docker's --env-file does not support values containing literal newline characters. Multiline secrets are unsupported via --env-file and are expected to be delivered via files (e.g., bind mounts or volumes), Docker secrets (in Swarm mode or Compose), or other mechanisms like individual -e flags with shell-expanded values.
Citations:
- 1: docker cannot pass newlines from variables in
--env-filefiles moby/moby#12997 - 2: https://docs.docker.com/engine/reference/commandline/run/
- 3: Handle quotes in --env-file values consistently with Linux/WSL2 "source"ing docker/cli#3630
Fail fast on multiline secrets instead of silently stripping them.
Line 64 silently deletes every \r and \n from the decoded payload, which mangles PEM keys, pretty-printed JSON, and other multiline secrets. Docker's --env-file format does not support multiline values—they must be delivered via files or other mechanisms. Reject payloads containing newlines explicitly instead of flattening them.
Suggested fix
- secret_value="$(printf '%s' "$secret_payload" | base64 -d | tr -d '\r\n')"
- printf '%s=%s\n' "$env_key" "$secret_value" >>"$env_path"
+ secret_value="$(
+ printf '%s' "$secret_payload" | base64 -d
+ printf '_'
+ )"
+ secret_value="${secret_value%_}"
+ case "$secret_value" in
+ *$'\n'*|*$'\r'*)
+ echo "Secret '$secret_id' contains newlines and cannot be injected via --env-file"
+ exit 1
+ ;;
+ esac
+ printf '%s=%s\n' "$env_key" "$secret_value" >>"$env_path"📝 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.
Suggested change
| secret_value="$(printf '%s' "$secret_payload" | base64 -d | tr -d '\r\n')" | |
| printf '%s=%s\n' "$env_key" "$secret_value" >>"$env_path" | |
| secret_value="$( | |
| printf '%s' "$secret_payload" | base64 -d | |
| printf '_' | |
| )" | |
| secret_value="${secret_value%_}" | |
| case "$secret_value" in | |
| *$'\n'*|*$'\r'*) | |
| echo "Secret '$secret_id' contains newlines and cannot be injected via --env-file" | |
| exit 1 | |
| ;; | |
| esac | |
| printf '%s=%s\n' "$env_key" "$secret_value" >>"$env_path" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/gcp/templates/daemon-startup.sh.tftpl` around lines 64 - 65,
The current decoding step uses tr -d '\r\n' which silently strips newlines from
decoded secrets (variable secret_value) and mangles multiline secrets; change
the logic in the secret payload handling around secret_value/env_key/env_path to
reject any decoded payloads that contain newlines or carriage returns instead of
removing them: decode payload into secret_value (without tr), validate that
secret_value contains no '\n' or '\r' and, if it does, log a clear error
(including env_key) and exit non‑zero so the startup fails fast rather than
writing a flattened value to env_path; remove the tr -d '\r\n' usage and add the
newline check + exit on failure.
Comment on lines
+17
to
+27
| let args = match Args::parse_args() { | ||
| Ok(args) => args, | ||
| Err(err) => { | ||
| eprintln!("{err}"); | ||
| std::process::exit(2); | ||
| } | ||
| }; | ||
| if let Err(err) = args.validate() { | ||
| eprintln!("{err}"); | ||
| std::process::exit(2); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "main.rs" | grep accumulatorRepository: Templar-Protocol/contracts
Length of output: 103
🏁 Script executed:
head -150 service/accumulator/src/main.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 6123
🏁 Script executed:
grep -r "struct Args" service/accumulator/src/ --include="*.rs" -A 20Repository: Templar-Protocol/contracts
Length of output: 1622
🏁 Script executed:
grep -r "fn validate" service/accumulator/src/ --include="*.rs" -B 2 -A 15Repository: Templar-Protocol/contracts
Length of output: 2299
🏁 Script executed:
sed -n '1,60p' service/accumulator/src/lib.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 2562
🏁 Script executed:
sed -n '80,120p' service/accumulator/src/lib.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 1490
🏁 Script executed:
sed -n '70,95p' service/accumulator/src/lib.rs | cat -nRepository: Templar-Protocol/contracts
Length of output: 961
Encode the signer-key invariant in the type, not in two runtime checks.
main calls args.validate() after parsing (lines 24-27), then run_service re-checks args.signer_key again with .context() (lines 112-115). This leaves Args in an invalid state representable in the type system: signer_key remains Option<SecretKey> even after validation. Per the coding guideline "Prefer parsing over validation; express invariants in types and make invalid states unrepresentable in Rust," split raw CLI parsing from validated runtime config, or make signer_key non-optional before passing args downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/accumulator/src/main.rs` around lines 17 - 27, The code leaves
Args.signer_key as Option<SecretKey> and performs validation in two places;
instead encode the signer-key invariant in the type by converting parsed Args
into a validated config where signer_key is SecretKey (non-Option) before
passing to run_service. Add a constructor or method (e.g. Args::into_validated
or new ValidatedArgs) that performs the existing checks from Args::validate and
returns a ValidatedArgs struct with signer_key: SecretKey, update main to call
that and exit on error, change run_service signature to accept ValidatedArgs (or
the new config type) and remove the redundant .context()/signer_key re-check
there. Ensure all call sites use the validated type so invalid states are
unrepresentable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is