Conversation
|
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:
WalkthroughPopulates the Tron production deployment state and adds rate-limit retry utilities plus Tron-aware health-check logic (RPC wrappers, ownership and whitelist checks, and command retry helpers). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@script/deploy/healthCheck.ts`:
- Around line 36-84: Add a colocated Bun test file for the new helper
execWithRateLimitRetry that provides 100% coverage: create e.g.
healthCheck.test.ts using Bun's describe/it/expect and stub/mock
child_process.execSync (the symbol used inside execWithRateLimitRetry) to
simulate (1) immediate success returning a string, (2) initial 429/rate-limit
errors then success to verify retry delay/attempts, and (3) persistent
429/errors to hit max-retries and throw; assert returned output or thrown error
and verify retry count/ordering where applicable. Ensure tests import
execWithRateLimitRetry from healthCheck.ts and clean up/restore the execSync
mock between tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@script/deploy/healthCheck.ts`:
- Around line 81-84: The throw new Error('Max retries reached') statement is
dead/unreachable because the retry loop always either returns result or
re-throws the last error when retries reach maxRetries; remove the unreachable
throw to clean up the code. Locate the retry loop that uses retries and
maxRetries (the block that returns result on success and re-throws error when
retries >= maxRetries) and delete the final throw new Error('Max retries
reached') line so control flow is clear and no dead code remains.
🧹 Nitpick comments (3)
script/deploy/healthCheck.test.ts (2)
12-19: Fix import order violations flagged by ESLint.The imports should be reordered: external libs first (sorted alphabetically), then local imports. The
process.env.BUN_TEST = 'true'assignment before theexecWithRateLimitRetryimport is intentional to set the test environment flag, but the other imports should be reorganized.♻️ Proposed fix for import order
-import { describe, it, expect, beforeEach, afterEach, spyOn } from 'bun:test' -import { consola } from 'consola' import * as childProcess from 'child_process' +import { describe, it, expect, beforeEach, afterEach, spyOn } from 'bun:test' +import { consola } from 'consola' + // Set test environment before importing process.env.BUN_TEST = 'true' import { execWithRateLimitRetry } from './healthCheck'
247-258: Consider wrapping the string in an Error to satisfy ESLint.The test correctly verifies behavior when non-Error objects are thrown, which is a valid edge case. However, ESLint flags
throw 'string error'as a violation ofno-throw-literal. You could test the same scenario by catching and re-throwing or using a custom approach.🔧 Optional fix to satisfy ESLint
it('should throw immediately on non-Error object thrown', async () => { + // eslint-disable-next-line no-throw-literal execSyncSpy.mockImplementationOnce(() => { throw 'string error' })script/deploy/healthCheck.ts (1)
85-90: Move import to the top of the file with other local imports.The import statement for Tron utilities is placed after the
execWithRateLimitRetryfunction definition. While ES modules hoist imports, this unconventional placement can confuse maintainers. Per coding guidelines, imports should be grouped at the top: external libs → config files → internal utils/helpers.♻️ Suggested organization
Move the import block at lines 85-90 to after line 34 (before the new function), keeping it with other local imports:
import targetState from './_targetState.json' import { checkIsDeployedTron, getCoreFacets as getTronCoreFacets, getTronCorePeriphery, parseTroncastFacetsOutput, } from './tron/utils' /** * Execute a command with retry logic for rate limit errors (429) * ... */ export async function execWithRateLimitRetry(
…e, added checks for whitelist integrity
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 (2)
script/deploy/healthCheck.ts (2)
2-37:⚠️ Potential issue | 🟡 MinorFix duplicate/late imports to satisfy ESLint import order.
Line 35 and Line 108 import
retryWithRateLimittwice, and there’s an import block after executable code. ESLint also flags empty lines within the import group.Suggested cleanup
-import { hexToTronAddress, retryWithRateLimit } from './tron/utils' +import { + checkIsDeployedTron, + getCoreFacets as getTronCoreFacets, + getTronCorePeriphery, + hexToTronAddress, + parseTroncastFacetsOutput, + retryWithRateLimit, +} from './tron/utils' import targetState from './_targetState.json' ... -import { - checkIsDeployedTron, - getCoreFacets as getTronCoreFacets, - getTronCorePeriphery, - parseTroncastFacetsOutput, - retryWithRateLimit, -} from './tron/utils'Also applies to: 108-114
1136-1286:⚠️ Potential issue | 🟠 MajorAdd Bun tests for the newly introduced Tron helper functions.
Lines 1139–1285 introduce multiple new helpers (wallet selection, selector normalization, Tron ownership checks, ABI decoding). Please extend the colocated Bun tests to cover success and error paths.
As per coding guidelines, “New TypeScript helpers must be covered by a colocated
*.test.tsfile using Bun (describe/it/expect) with 100% coverage.”
🤖 Fix all issues with AI agents
In `@script/deploy/healthCheck.ts`:
- Around line 997-1003: The variable rpcUrl assigned in the isTron block is
unused and causing a lint error; remove the assignment "const rpcUrl =
networksConfig[networkLower].rpcUrl" from the block (keep the surrounding logic
that reads deployedContracts['LiFiDiamond'], calls
ensureTronAddress(refundWallet, tronWeb), and uses isTron/tronWeb) so no other
behavior changes occur.
- Around line 1375-1612: The inner function parseArray declared inside
checkWhitelistIntegrityTron triggers ESLint no-inner-declarations; move
parseArray to module scope (e.g., alongside other helpers) and update
checkWhitelistIntegrityTron to call the top-level parseArray; ensure the
signature stays parseArray(str: string, start: number): [unknown[], number] and
any references to it (in the JSON.parse fallback within
checkWhitelistIntegrityTron) are adjusted to use the extracted helper.
In `@script/deploy/tron/utils.ts`:
- Around line 33-106: Replace the loose any types and add Bun tests: change the
error parameter types in isRateLimitError and retryWithRateLimit from any to
unknown and add proper narrowing (e.g., derive errorMessage via typeof checks
and optional chaining when accessing message/property) so both functions safely
handle non-Error inputs; then add a colocated Bun test file
script/deploy/tron/utils.test.ts that covers: successful operation (no retries),
rate-limit retries (simulate errors matching '429'/'rate limit' and verify retry
delays/onRetry calls and eventual success), exhausted retries throwing the last
error, non-rate-limit errors propagating immediately, connection error detection
(ECONNREFUSED/ETIMEDOUT when includeConnectionErrors true/false), and edge cases
(error as string/null/unknown), ensuring 100% coverage for both isRateLimitError
and retryWithRateLimit.
🧹 Nitpick comments (1)
script/deploy/healthCheck.ts (1)
116-197: Consider reusingexecWithRateLimitRetryto avoid duplicate spawn/retry logic.
callTronContractreimplements the same spawn+retry flow. Reusing the helper keeps the retry policy consistent and reduces duplication.Refactor sketch
- const result = await retryWithRateLimit( - () => { /* spawn logic */ }, - 3, - [1000, 2000, 3000], - (attempt, delay) => { ... }, - false - ) + const result = await execWithRateLimitRetry( + ['bun', ...args], + 2000, + 3, + [1000, 2000, 3000] + )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@script/deploy/healthCheck.ts`:
- Around line 35-37: The file currently imports retryWithRateLimit twice (once
at the top with hexToTronAddress and again later inside the module), causing a
redeclaration and import-in-body ESLint error; fix by consolidating all imports
from './tron/utils' into the existing top-level import (include
retryWithRateLimit, hexToTronAddress and any other symbols used) and remove the
later duplicate import statement so no imports appear inside the module body and
the top import is the sole source of those symbols.
🧹 Nitpick comments (2)
script/deploy/healthCheck.ts (2)
81-85: Redundant error message assignment.Line 84 creates an
Errorwith a message, then immediately overwrites themessageproperty. This is redundant and uses a type assertion toany.Suggested fix
child.on('close', (code) => { if (code !== 0) { - const error = new Error( - `Command failed with exit code ${code}: ${stderr || stdout}` - ) - ;(error as any).message = stderr || stdout || `Exit code ${code}` - reject(error) + reject(new Error(stderr || stdout || `Exit code ${code}`)) } else { resolve(stdout) } })
125-197: Code duplication withexecWithRateLimitRetry.The spawn execution logic inside
callTronContract(lines 150-182) duplicates the pattern fromexecWithRateLimitRetry(lines 61-95). Consider reusingexecWithRateLimitRetryinstead:Suggested refactor
async function callTronContract( contractAddress: string, functionSignature: string, params: string[], returnType: string, rpcUrl: string ): Promise<string> { const args = [ 'bun', 'run', 'script/troncast/index.ts', 'call', contractAddress, `${functionSignature} returns (${returnType})`, ...(params.length > 0 ? [params.join(',')] : []), '--rpc-url', rpcUrl, ] - // Add initial delay for Tron to avoid rate limits - await new Promise((resolve) => setTimeout(resolve, 2000)) - - // Execute with retry logic for rate limits using spawn to avoid shell issues - const result = await retryWithRateLimit( - () => { - return new Promise<string>((resolve, reject) => { - const child = spawn('bun', args.slice(1), { - stdio: ['ignore', 'pipe', 'pipe'], - }) - // ... duplicate spawn logic - }) - }, - // ... - ) + const result = await execWithRateLimitRetry(args, 2000, 3, [1000, 2000, 3000]) return result.trim() }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/healthCheck.ts (1)
1063-1116:⚠️ Potential issue | 🟡 MinorSAFE configuration check silently fails for Tron.
When
isTronis true,publicClientisundefined. The SAFE check block (lines 1063-1116) gates onenvironment === 'production'but does not gate onisTron. If Tron has asafeAddressin its network config,getSafeInfoFromContract(publicClient, safeAddress)will throw becausepublicClientisundefined, producing a misleading error:"Failed to get SAFE information: ...". Consider adding an explicit Tron skip message:if (environment === 'production') { + if (isTron) { + consola.info('Skipping SAFE checks for Tron network (not supported yet)') + } else { consola.box('Checking SAFE configuration...') // ... existing EVM logic + } }
🤖 Fix all issues with AI agents
In `@script/deploy/healthCheck.ts`:
- Line 44: Swap the import order so the JSON import is before the utils import:
move the "import targetState from './_targetState.json'" statement above the
"import ... from './tron/utils'" import to satisfy the ESLint import/order rule
(ensure the import of targetState appears in the earlier group than imports from
'./tron/utils').
🧹 Nitpick comments (3)
script/deploy/healthCheck.ts (3)
125-197: Duplicated spawn-and-retry logic — extract to reuseexecWithRateLimitRetry.
callTronContract(lines 125-197) manually spawnsbunwith retry logic nearly identical toexecWithRateLimitRetry(lines 56-114). The only differences are the hardcoded 2s delay and the command construction. SinceexecWithRateLimitRetryalready acceptsinitialDelay,callTronContractcan delegate to it:Refactor sketch
async function callTronContract( contractAddress: string, functionSignature: string, params: string[], returnType: string, rpcUrl: string ): Promise<string> { const args = [ 'bun', 'run', 'script/troncast/index.ts', 'call', contractAddress, `${functionSignature} returns (${returnType})`, ...(params.length > 0 ? [params.join(',')] : []), '--rpc-url', rpcUrl, ] - await new Promise((resolve) => setTimeout(resolve, 2000)) - - const result = await retryWithRateLimit( - () => { - return new Promise<string>((resolve, reject) => { - // ... 30+ lines duplicated from execWithRateLimitRetry - }) - }, - 3, [1000, 2000, 3000], - ... - ) + const result = await execWithRateLimitRetry( + args, + 2000, // initial delay for Tron + 3, + [1000, 2000, 3000] + ) return result.trim() }This removes ~50 lines of duplication.
86-92: Error.messageis overwritten immediately after construction.On lines 88-91 (and identically 169-172), an
Erroris constructed with a descriptive message, then.messageis immediately overwritten to just the raw stderr/stdout. This discards the"Command failed with exit code …"prefix. If this is intentional (for rate-limit string matching inretryWithRateLimit), a comment would clarify intent. If not, the overwrite is a bug.const error = new Error( `Command failed with exit code ${code}: ${stderr || stdout}` ) - ;(error as any).message = stderr || stdout || `Exit code ${code}` + // Raw message preserved for rate-limit detection in retryWithRateLimit reject(error)If both the prefix and the raw message are needed, consider a custom property (e.g.,
(error as any).rawOutput = stderr || stdout) rather than silently mutating.message.Also applies to: 167-173
1508-1539: Whitelist granular check: each pair incurs a 500ms delay + RPC round-trip — can be very slow.For large whitelists,
checkWhitelistIntegrityTronissues onecallTronContractBooleancall per expected pair (line 1515), each preceded by a 500msdelayTron(). With, say, 200 pairs this loop alone takes >100 seconds of sleeping plus RPC time.The getter-array check (Step 2/2, lines 1547-1610) already validates the same data offline via set comparison. Consider making the per-pair granular check optional (or batching with a single
getAllContractSelectorPairscomparison), and only falling back to per-pair calls if discrepancies are found.
…dularity - Consolidate delay constants in script/deploy/tron/constants.ts - Extract Tron health check functions to healthCheckTronUtils.ts - Replace setTimeout with centralized sleep() utility across 9 files - Improve RPC URL handling to prioritize environment variables
…tor sleep management on tron contract calls
…Tron/EVM from the provided context
4 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/SMAR-62
Why did I implement it this way?
Because Tron needed a delay in order to correctly check the deployed contracts
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)