fix(tron): route deployment MongoDB logging through the working update-deployment-logs.ts path#2018
fix(tron): route deployment MongoDB logging through the working update-deployment-logs.ts path#20180xDEnYO wants to merge 3 commits into
Conversation
…e-deployment-logs.ts path logDeployment() in script/utils/utils.ts shelled out to a `logDeployment` function in helperFunctions.sh that never existed, so every Tron deploy has silently no-op'd its MongoDB deployment-log write since the function was introduced. deployments/tron.json and tron.diamond.json were unaffected (written separately by saveContractAddress/updateDiamondJsonBatch); only the MongoDB record used by query-deployment-logs.ts was missing. Now calls the same `bunx tsx script/deploy/update-deployment-logs.ts add` command that EVM's logContractDeploymentInfo already uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalkthroughAdds an optional ChangesOptimizer runs config and deployment logging
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…g 200 logDeployment() passed a hardcoded '200' as --optimizer-runs to update-deployment-logs.ts, but foundry.toml's [profile.default] is 1000000 — every Tron deployment's MongoDB log record stored an inaccurate value. Adds getFoundryDefaultOptimizerRuns(), mirroring the existing solc/evm-version helpers, with the same TOML-parse-then-regex fallback (extended to match unquoted numeric values). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ipt/utils/utils.ts:102) CR says: the regex fallback captured only leading digits (`1_000` -> `1`) and the parsed/TOML value was never validated as a real integer. Extends the fallback regex to accept underscore-separated digits and normalizes them, then validates the final value with Number.isSafeInteger in both the fallback and the TOML happy-path getter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
script/utils/utils.test.ts (1)
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest reads the real
foundry.tomlinstead of mocking it, and doesn't cover validation branches.This test calls
getFoundryDefaultOptimizerRuns()directly against the actual repofoundry.toml, asserting a hardcoded value (1000000). It will break wheneveroptimizer_runslegitimately changes and doesn't test the function's own validation logic (missing/non-integer/negativeoptimizer_runs), which is the core new logic added in this PR.As per coding guidelines,
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,go,rb}should "Use mocking and stubbing libraries appropriate to your language/framework to isolate units under test," and**/*.{test,spec}.{ts,tsx}tests should "Ensure tests are deterministic and do not depend on execution order or external state."♻️ Example using bun:test's mock.module to isolate and cover error paths
import { describe, expect, it, + mock, // eslint-disable-next-line import/no-unresolved } from 'bun:test' -import { getFoundryDefaultOptimizerRuns } from './utils' +describe('getFoundryDefaultOptimizerRuns', () => { + it('returns optimizer_runs when present and valid', () => { + mock.module('node:fs', () => ({ + readFileSync: () => '[profile.default]\noptimizer_runs = 1_000_000\n', + })) + const { getFoundryDefaultOptimizerRuns } = require('./utils') + expect(getFoundryDefaultOptimizerRuns()).toBe(1000000) + }) -describe('getFoundryDefaultOptimizerRuns', () => { - it('returns the optimizer_runs value from [profile.default] in foundry.toml', () => { - expect(getFoundryDefaultOptimizerRuns()).toBe(1000000) - }) + it('throws when optimizer_runs is missing', () => { + mock.module('node:fs', () => ({ + readFileSync: () => '[profile.default]\nsolc_version = "0.8.20"\n', + })) + const { getFoundryDefaultOptimizerRuns } = require('./utils') + expect(() => getFoundryDefaultOptimizerRuns()).toThrow() + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/utils/utils.test.ts` around lines 1 - 17, The `getFoundryDefaultOptimizerRuns` test is coupled to the real `foundry.toml` and only checks one happy path, so it should be made deterministic by mocking the file-reading/parsing dependency used inside `script/utils/utils.ts` and validating the new branches too. Update the `getFoundryDefaultOptimizerRuns` spec to stub the TOML input for `foundry.toml`, then add separate cases for a valid `profile.default.optimizer_runs`, missing `optimizer_runs`, non-integer values, and negative values so the function’s validation logic is exercised without relying on repo state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@script/utils/utils.test.ts`:
- Around line 1-17: The `getFoundryDefaultOptimizerRuns` test is coupled to the
real `foundry.toml` and only checks one happy path, so it should be made
deterministic by mocking the file-reading/parsing dependency used inside
`script/utils/utils.ts` and validating the new branches too. Update the
`getFoundryDefaultOptimizerRuns` spec to stub the TOML input for `foundry.toml`,
then add separate cases for a valid `profile.default.optimizer_runs`, missing
`optimizer_runs`, non-integer values, and negative values so the function’s
validation logic is exercised without relying on repo state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dac73b29-48a1-46f4-8a56-226cbb7f81aa
📒 Files selected for processing (3)
script/common/types.tsscript/utils/utils.test.tsscript/utils/utils.ts
🔍 QA Review — EXSC-589🧠 What this ticket doesEvery Tron deployment since the function was introduced has been recording an inaccurate
📋 Ticket SummaryFix hardcoded optimizer-runs=200 in Tron MongoDB deployment log The ticket identifies the root cause (hardcoded Acceptance Criteria (inferred from ticket description — no explicit checklist):
🏷️ PR Naming — ✅ Pass
Follows conventional-commit style ( 🔎 Ticket Discoverability — ✅ Pass
Note: EXSC-589 does not appear in the PR title or branch name ( 🛡️ Version & Audit State
No Solidity contracts under ✅ Ticket Coverage — HighThe EXSC-589 fix is fully implemented. The helper follows the established pattern exactly, the type extension is minimal and correct, the regex correctly handles the unquoted integer format, and the shell command argument is now dynamic. The only gap is test completeness (see below).
|
| # | Severity | Type | Issue |
|---|---|---|---|
| 1 | 🟠 Medium | Test gap | getFoundryDefaultOptimizerRuns error branches have no test coverage |
| 2 | 🟡 Low | Test quality | Single test is coupled to live foundry.toml — brittle and non-isolated |
🟠 [Medium] getFoundryDefaultOptimizerRuns error branches have no test coverage
getFoundryDefaultOptimizerRuns() in script/utils/utils.ts contains explicit validation logic with three error-path branches:
typeof optimizerRuns !== 'number'— fires whenoptimizer_runsis absent from[profile.default]!Number.isSafeInteger(optimizerRuns)— fires when the parsed value is a float or overflows safe integer rangeoptimizerRuns < 0— fires when the value is negative
None of these branches are exercised by script/utils/utils.test.ts. The ticket description explicitly says "Cover with a unit test" and the PR author checked "I have added tests that cover the functionality / test the bug" — that coverage is incomplete. Bugs in the validation logic (e.g. a future refactor that accidentally removes the < 0 check) would go undetected.
Also uncovered in extractNumeric() (the regex fallback path): the case where the key is present but the parsed value fails Number.isSafeInteger validation (e.g. an astronomically large integer that overflows) — extractNumeric returns undefined in this case, triggering the outer throw, but this path has no test.
Suggested fix: add at minimum three further test cases using mock.module('node:fs', ...) to control the TOML content:
optimizer_runsabsent from[profile.default]→ expectsthrowoptimizer_runs = -1→ expectsthrowoptimizer_runs = 1_000_000(underscore-separated) → expectstoBe(1000000)(also validates the regex underscore-stripping path)
CodeRabbit flagged the same isolation issue as a nitpick with a concrete mock.module example.
🟢 [Low] Single test is coupled to live foundry.toml — brittle and non-isolated
script/utils/utils.test.ts line 14: expect(getFoundryDefaultOptimizerRuns()).toBe(1000000) calls the function directly against the real foundry.toml. This creates two problems:
- Fragility: if
optimizer_runsis legitimately changed infoundry.toml(e.g. tuned for a new compiler version), this test will fail on an unrelated PR with a confusing error message. - Non-isolation: the test is an integration test disguised as a unit test. It depends on file system state and the current directory when the test runner executes — making it environment-sensitive.
The ticket description says "Cover with a unit test" — a proper unit test should mock the file-reading dependency so it exercises only the helper's own logic, not the current value in foundry.toml.
Suggested fix: use mock.module('node:fs', () => ({ readFileSync: () => '[profile.default]\noptimizer_runs = 1000000\n' })) (or the Bun equivalent) to decouple from the real file. This is the same approach CodeRabbit recommended.
This is Low rather than Medium because the happy-path value of 1000000 is unlikely to change in the near term and the test still provides some regression protection, but it should be fixed before it causes false-positive CI failures on an unrelated change.
🧪 Test Coverage
| Layer | Score | Files reviewed |
|---|---|---|
| TypeScript unit tests | Partial | script/utils/utils.test.ts (new) |
| Deploy scripts | N/A | No deploy script changes in this PR |
Gaps — all are requested changes, every item must be addressed or explicitly accepted:
TypeScript unit test gaps:
[Medium] getFoundryDefaultOptimizerRuns— missing:throwbranch whenoptimizer_runsis absent from[profile.default][Medium] getFoundryDefaultOptimizerRuns— missing:throwbranch whenoptimizer_runs < 0[Medium] getFoundryDefaultOptimizerRuns— missing: validation of underscore-separated integer input (e.g.1_000_000) via the regex fallback path[Low] utils.test.ts— test reads realfoundry.tomlinstead of mocking the file dependency; makes the test brittle and non-deterministic across environments
🔗 Downstream Impact
Blocks: None listed in ticket relations.
Operational: TypeScript-only changes — no redeploy, no diamond cut, no EVM network impact. The fix corrects MongoDB deployment log data for future Tron deployments only. Historical records written with 200 are not back-filled; if historical accuracy matters, a one-time migration script would be needed (out of scope for this PR — not flagged as a required change, noting for completeness). No indexer or backend coordination required since deployment-log records are internal tooling data only.
QA Agent — 2026-07-03
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on 2 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟠 Medium | Test gap | script/utils/utils.test.ts — getFoundryDefaultOptimizerRuns error branches untested |
| 2 | 🟢 Low | Test quality | script/utils/utils.test.ts — test coupled to live foundry.toml (brittle, non-isolated) |
1. [Medium] Test gap — script/utils/utils.test.ts
- Missing:
throwcase whenoptimizer_runsis absent from[profile.default] - Missing:
throwcase whenoptimizer_runsis negative (e.g.-1) - Missing: valid underscore-separated integer (e.g.
1_000_000) via the regex fallback path — validates thereplace(/_/g, '')normalisation - The three error branches in
getFoundryDefaultOptimizerRuns()are the core new logic added by this PR; none are exercised
2. [Low] Test quality — script/utils/utils.test.ts (also flagged by CodeRabbit)
The single test (expect(getFoundryDefaultOptimizerRuns()).toBe(1000000)) reads the real foundry.toml directly. If optimizer_runs is legitimately changed in foundry.toml on a future PR, this test will fail with a confusing, misleading failure. Decouple the test from the repo file system by mocking node:fs readFileSync (e.g. via mock.module from bun:test) to supply controlled TOML content.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
3 participants
Which Linear task belongs to this PR?
Fixes EXSC-589 — EXSC-589 was originally filed as a follow-up for the hardcoded
optimizer-runs=200value (see below), now also fixed in this same PR.Why did I implement it this way?
While investigating why the Tron deploy runbook has a manual "update
deployments/tron.json/tron.diamond.json" step, I found that step is actually stale — those files are already written automatically bysaveContractAddress()/updateDiamondJsonBatch()during every Tron deploy.But that investigation surfaced a real, separate bug:
logDeployment()inscript/utils/utils.ts(used by every Tron deploy script) has been shelling out toscript/helperFunctions.sh logDeployment ...to write the MongoDB deployment-log record — excepthelperFunctions.shhas never had alogDeploymentfunction (onlylogContractDeploymentInfo) and has no argument dispatcher at the bottom of the file. So the shell-out has silently exited 0 and done nothing since it was introduced (c93177d10). Every Tron deploy since then has been missing from the MongoDB deployment log thatquery-deployment-logs.tsreads.This is Tron-specific — EVM deploys never go through this function;
deploySingleContract.shcalls the workinglogContractDeploymentInfobash function directly, which in turn invokesbunx tsx script/deploy/update-deployment-logs.ts add ....The fix: point
logDeployment()at that same, already-workingupdate-deployment-logs.ts addCLI instead of the nonexistent bash function, with the same field mappinglogContractDeploymentInfouses (contract/network/version/address/optimizer-runs/timestamp/constructor-args/verified/solc-version/evm-version). No behavior change for EVM; Tron deploys now actually land in MongoDB.I also left a comment on the Notion runbook explaining both findings and suggesting step 6 be simplified to point at the "end-to-end deploy flow" section instead of restating it.
Update: the hardcoded
--optimizer-runs 200finding below was originally deferred to EXSC-589 as a separate follow-up, but ended up landing on this same branch instead of a new PR — see the second/pr-readyentry./pr-ready(local CodeRabbit) — 2 findings, both resolvedscript/utils/utils.ts:304—--optimizer-runswas hardcoded to'200', butfoundry.toml's[profile.default].optimizer_runsis actually1000000, so the MongoDB deployment-log record stored an inaccurate value. Pre-existing (not introduced by this PR — the old broken command hardcoded the same"200"). Fixed by addinggetFoundryDefaultOptimizerRuns()(mirroringgetFoundryDefaultSolcVersion()/getFoundryDefaultEvmVersion()), tracked as EXSC-589.script/utils/utils.ts:102— the follow-up commit's regex fallback foroptimizer_runsonly captured leading digits (1_000→1) and didn't validate the result as a real integer. Fixed: regex now accepts underscore-separated digits, and the final value is validated withNumber.isSafeInteger.Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.mdChecklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)