Conversation
📝 WalkthroughWalkthroughA patch changes AltVMFileSubmitter's error logging when reading existing transaction files: invalid or empty initial files no longer log an error; the write path and control flow remain unchanged. A changeset declaring a patch release for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript/deploy-sdk/src/AltVMFileSubmitter.ts (1)
31-41:⚠️ Potential issue | 🟡 MinorDon’t silently swallow non-empty read errors.
Right now the bare
catchhides parse/permission problems and then overwrites the file with no signal. That’s risky if the file isn’t empty but corrupted. A simple guard keeps the empty-file behavior while still warning on real read failures.🧰 Suggested adjustment
+import { existsSync, statSync } from 'fs'; import { readYamlOrJson, writeYamlOrJson } from '@hyperlane-xyz/utils/fs'; @@ - try { - const maybeExistingTxs = readYamlOrJson(filepath); // Can throw if file is empty - assert( - Array.isArray(maybeExistingTxs), - `Target filepath ${filepath} has existing data, but is not an array. Overwriting.`, - ); - allTxs.unshift(...maybeExistingTxs); - } catch { - // if file is empty we simply continue and write the first contents of the file below - } + try { + if (existsSync(filepath) && statSync(filepath).size > 0) { + const maybeExistingTxs = readYamlOrJson(filepath); + assert( + Array.isArray(maybeExistingTxs), + `Target filepath ${filepath} has existing data, but is not an array. Overwriting.`, + ); + allTxs.unshift(...maybeExistingTxs); + } + } catch (err) { + this.logger.warn({ err }, `Failed to read existing txs at ${filepath}. Overwriting.`); + }
antigremlin
left a comment
There was a problem hiding this comment.
Please explain what the bug was.
Added more context in the pr description |
antigremlin
left a comment
There was a problem hiding this comment.
Please update the changeset
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/big-yaks-protect.md:
- Line 5: Update the changeset description to past tense by changing the
summary/title line "fix: replace error with debug log in altvm file submitter"
to use past tense (e.g., "fix: replaced error with debug log in altvm file
submitter") so the changeset describes what changed; edit the
.changeset/big-yaks-protect.md file and update that heading string accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7990 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
🚀 New features to boost your workflow:
|
Description
This PR fixes a bug where the cli throws if the altvm file submitter opens an empty file due to expecting valid json/yaml content.
Drive-by changes
Related issues
Backward compatibility
Testing
Summary by CodeRabbit
Bug Fixes
Chores