Conversation
…lash - Update writeJson to use stringifyObject instead of JSON.stringify to properly handle ethers BigNumber serialization - Rename removeEndingSlash to removeTrailingSlash for consistency with JSDoc and common terminology Addresses review feedback from #7558 and #7559 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Move sort-yaml-arrays rule to infra/eslint-rules (only consumer) - Remove unused no-restricted-imports-from-exports rule - Remove eslint-plugin-yml and yaml-eslint-parser deps from utils - Export sortNestedArrays, transformYaml, ArraySortConfig from utils 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ions Remove generic type parameters from readJson, tryReadJson, readJsonFromDir, readYamlOrJson, yamlParse, readYaml, tryReadYaml, and readYamlFromDir. These functions now return the implicit `any` from JSON.parse()/yaml parse(), avoiding false impression of type validation. Callers are responsible for ensuring type safety. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 7b6b16e The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Update writeYaml to use stringifyObject (with 'yaml' format) for consistency with writeJson. This ensures proper ethers BigNumber serialization when writing YAML files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update all call sites in cli and infra packages to remove generic type arguments after the read functions were changed to return implicit any. This fixes the build errors caused by "Expected 0 type arguments, but got 1". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
once #7610 is merged and pulled into this branch, the infra-test action will pass again |
paulbalaji
requested review from
Mo-Hussain,
Xaroz,
antigremlin,
ltyu,
xeno097 and
yorhodes
as code owners
📝 WalkthroughWalkthroughThis change renames the utility function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes The changes follow consistent patterns—mostly straightforward renaming across related files and two similar serialization replacements. Reviewers should verify that:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🐳 Monorepo Docker Image Built SuccessfullyImage Tags: |
antigremlin
left a comment
antigremlin
left a comment
There was a problem hiding this comment.
That made me think more...
Revert the removal of generic type parameters from JSON/YAML read functions. Type-casts provide compile-time safety even without runtime validation, which is better than implicit any. Also revert moving eslint-rules from utils to infra, as hyperlane-registry depends on @hyperlane-xyz/utils and uses these rules. Keep the writeJson/writeYaml stringifyObject changes for BigNumber serialization and the removeEndingSlash -> removeTrailingSlash rename. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
paulbalaji
changed the title
paulbalaji
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
typescript/utils/src/fs/json.ts (1)
10-23: PR objective mismatch: JSON read helpers still expose generics (<T>).
The PR summary says the JSON/YAML read fns dropped generics to avoid implying runtime validation, butreadJson<T>,tryReadJson<T>, andreadJsonFromDir<T>still have them here. Either update these signatures (and callers) or tweak the PR description/objectives so they match reality.Also applies to: 52-54
typescript/utils/src/fs/yaml.ts (1)
23-43: PR objective mismatch: YAML read helpers still expose generics (<T>).
Same story as JSON:yamlParse<T>,readYaml<T>,tryReadYaml<T>,readYamlFromDir<T>still carry generics, which conflicts with the stated “remove generics to avoid implying validation.”Also applies to: 72-74
🧹 Nitpick comments (4)
typescript/utils/src/fs/utils.ts (1)
5-13:removeTrailingSlashhas a couple sharp edges ('/'->'', only trims one slash, ignores\).
If this is meant to be generally safe, consider preserving root, trimming repeated slashes, and handling Windows separators. Also, if you want a non-breaking rename, keepremoveEndingSlashas a deprecated alias.export function removeTrailingSlash(dirPath: string): string { - if (dirPath.endsWith('/')) { - return dirPath.slice(0, -1); - } - return dirPath; + if (!dirPath) return dirPath; + // Preserve POSIX root + if (dirPath === '/') return dirPath; + // Trim one-or-more trailing separators (POSIX + Windows) + return dirPath.replace(/[\\/]+$/, ''); } + +/** @deprecated Use removeTrailingSlash */ +export function removeEndingSlash(dirPath: string): string { + return removeTrailingSlash(dirPath); +}typescript/utils/src/fs/index.ts (1)
6-15: Public export updated—consider exporting a deprecatedremoveEndingSlashalias too (if you want non-breaking).
If you take the alias approach intypescript/utils/src/fs/utils.ts, re-export it here so downstreams don’t get smacked.typescript/utils/src/fs/utils.test.ts (1)
6-47: Tests updated fine—maybe add edge cases if you harden the impl.
If you accept the suggestedreplace(/[\\/]+$/, '')style trim, add cases for'/','////', and'C:\\path\\'(if Windows support matters).typescript/cli/src/utils/files.ts (1)
7-28: Rename propagation through CLI re-exports looks clean.
If you decide to keep a deprecatedremoveEndingSlashfor compatibility, you may also want to re-export it from here (temporarily) to keep CLI-side imports steady.Also applies to: 34-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/beige-foxes-compare.md(1 hunks)typescript/cli/src/commands/warp.ts(2 hunks)typescript/cli/src/utils/files.ts(2 hunks)typescript/utils/src/fs/index.ts(1 hunks)typescript/utils/src/fs/json.ts(2 hunks)typescript/utils/src/fs/utils.test.ts(2 hunks)typescript/utils/src/fs/utils.ts(1 hunks)typescript/utils/src/fs/yaml.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: paulbalaji
Repo: hyperlane-xyz/hyperlane-monorepo PR: 6943
File: rust/main/config/mainnet_config.json:965-965
Timestamp: 2025-08-26T13:46:37.695Z
Learning: In the repository hyperlane-xyz/hyperlane-monorepo, skip reviewing the file rust/main/config/testnet_config.json in future code reviews as requested by paulbalaji.
Learnt from: paulbalaji
Repo: hyperlane-xyz/hyperlane-monorepo PR: 6943
File: rust/main/config/mainnet_config.json:965-965
Timestamp: 2025-08-26T13:46:37.695Z
Learning: In the repository hyperlane-xyz/hyperlane-monorepo, skip reviewing the file rust/main/config/mainnet_config.json in future code reviews as requested by paulbalaji.
Learnt from: paulbalaji
Repo: hyperlane-xyz/hyperlane-monorepo PR: 6943
File: rust/main/config/testnet_config.json:34-35
Timestamp: 2025-08-26T13:45:52.227Z
Learning: Skip reviewing mainnet_config.json and testnet_config.json configuration files in typescript/infra/config/ and rust/main/config/ directories as requested by paulbalaji to reduce review noise.
Learnt from: ltyu
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7577
File: typescript/cli/src/commands/submit.ts:60-71
Timestamp: 2025-12-11T16:29:47.997Z
Learning: In the hyperlane-xyz/hyperlane-monorepo repository, the `writeYamlOrJson` utility function (from typescript/cli/src/utils/files.ts) automatically handles directory creation when writing files, so there's no need to explicitly check or create directories before calling it.
Learnt from: paulbalaji
Repo: hyperlane-xyz/hyperlane-monorepo PR: 6943
File: typescript/infra/config/environments/testnet4/ism/verification.json:3835-3939
Timestamp: 2025-08-26T13:45:29.848Z
Learning: Skip reviewing verification.json files in typescript/infra/config/environments/testnet4/ism/verification.json and similar verification.json files as requested by paulbalaji.
📚 Learning: 2025-12-11T16:29:47.997Z
Learnt from: ltyu
Repo: hyperlane-xyz/hyperlane-monorepo PR: 7577
File: typescript/cli/src/commands/submit.ts:60-71
Timestamp: 2025-12-11T16:29:47.997Z
Learning: In the hyperlane-xyz/hyperlane-monorepo repository, the `writeYamlOrJson` utility function (from typescript/cli/src/utils/files.ts) automatically handles directory creation when writing files, so there's no need to explicitly check or create directories before calling it.
Applied to files:
typescript/cli/src/commands/warp.tstypescript/utils/src/fs/yaml.ts
🧬 Code graph analysis (4)
typescript/utils/src/fs/utils.ts (2)
typescript/cli/src/utils/files.ts (1)
removeTrailingSlash(39-39)typescript/utils/src/fs/index.ts (1)
removeTrailingSlash(11-11)
typescript/utils/src/fs/utils.test.ts (3)
typescript/cli/src/utils/files.ts (1)
removeTrailingSlash(39-39)typescript/utils/src/fs/index.ts (1)
removeTrailingSlash(11-11)typescript/utils/src/fs/utils.ts (1)
removeTrailingSlash(8-13)
typescript/utils/src/fs/json.ts (2)
typescript/utils/src/fs/utils.ts (1)
writeToFile(77-80)typescript/utils/src/index.ts (1)
stringifyObject(157-157)
typescript/utils/src/fs/yaml.ts (3)
typescript/utils/src/fs/index.ts (2)
writeYaml(31-31)writeToFile(14-14)typescript/utils/src/fs/utils.ts (1)
writeToFile(77-80)typescript/utils/src/index.ts (1)
stringifyObject(157-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (60)
- GitHub Check: cli-evm-e2e-matrix (core-read)
- GitHub Check: cli-evm-e2e-matrix (warp-check-4)
- GitHub Check: cli-evm-e2e-matrix (warp-send)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-basic)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-config)
- GitHub Check: cli-evm-e2e-matrix (warp-check-1)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-simple-updates)
- GitHub Check: cli-evm-e2e-matrix (warp-read)
- GitHub Check: cli-evm-e2e-matrix (warp-deploy-1)
- GitHub Check: cli-evm-e2e-matrix (warp-check-5)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-rebalancing-config)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-hook-updates)
- GitHub Check: cli-evm-e2e-matrix (relay)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-submitters)
- GitHub Check: cli-evm-e2e-matrix (core-apply)
- GitHub Check: cli-evm-e2e-matrix (warp-extend-recovery)
- GitHub Check: cli-evm-e2e-matrix (warp-check-2)
- GitHub Check: cli-evm-e2e-matrix (core-deploy)
- GitHub Check: cli-evm-e2e-matrix (warp-check-3)
- GitHub Check: cli-evm-e2e-matrix (core-check)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-ism-updates)
- GitHub Check: cli-evm-e2e-matrix (warp-init)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-2)
- GitHub Check: cli-evm-e2e-matrix (warp-apply-ownership-updates)
- GitHub Check: cli-evm-e2e-matrix (warp-deploy-2)
- GitHub Check: cli-evm-e2e-matrix (warp-bridge-1)
- GitHub Check: cli-evm-e2e-matrix (warp-rebalancer)
- GitHub Check: cli-evm-e2e-matrix (core-init)
- GitHub Check: cli-cross-chain-e2e-matrix (warp-apply)
- GitHub Check: env-test-matrix (mainnet3, ethereum, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, igp)
- GitHub Check: env-test-matrix (testnet4, sepolia, core)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, igp)
- GitHub Check: env-test-matrix (mainnet3, arbitrum, core)
- GitHub Check: env-test-matrix (mainnet3, optimism, core)
- GitHub Check: cli-cosmos-e2e-matrix (warp-read)
- GitHub Check: env-test-matrix (mainnet3, ethereum, igp)
- GitHub Check: cli-radix-e2e-matrix (warp-apply-ownership-updates)
- GitHub Check: cli-radix-e2e-matrix (warp-deploy)
- GitHub Check: yarn-test-run
- GitHub Check: cli-radix-e2e-matrix (core-deploy)
- GitHub Check: cli-radix-e2e-matrix (core-apply)
- GitHub Check: cli-cosmos-e2e-matrix (warp-deploy)
- GitHub Check: cli-cross-chain-e2e-matrix (warp-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (core-read)
- GitHub Check: cli-cosmos-e2e-matrix (core-check)
- GitHub Check: cli-cosmos-e2e-matrix (core-deploy)
- GitHub Check: cli-cosmos-e2e-matrix (core-apply)
- GitHub Check: cosmos-sdk-e2e-run
- GitHub Check: cli-radix-e2e-matrix (warp-apply-route-extension)
- GitHub Check: cli-install-test-run
- GitHub Check: aleo-sdk-e2e-run
- GitHub Check: infra-test
- GitHub Check: e2e-matrix (evm)
- GitHub Check: agent-configs (testnet4)
- GitHub Check: lint-prettier
- GitHub Check: agent-configs (mainnet3)
- GitHub Check: lander-coverage
- GitHub Check: lint-rs
- GitHub Check: test-rs
🔇 Additional comments (3)
typescript/utils/src/fs/json.ts (1)
3-3: Good move switchingwriteJsontostringifyObject, but verify determinism + BigNumber coverage.
This should fix ethers BigNumber serialization, but I’d double-checkstringifyObject(..., 'json', 2)is stable (key ordering if you rely on clean diffs) and handles the BigNumber variants you care about (ethers v5BigNumber, ethers v6 types, bigint).Also applies to: 25-31
typescript/cli/src/commands/warp.ts (1)
41-47: Rename usage looks wired up right (coercenow callsremoveTrailingSlash).
Nothing spooky here—just make sure there aren’t any lingeringremoveEndingSlashimports in other CLI commands.Also applies to: 122-127
typescript/utils/src/fs/yaml.ts (1)
10-12: The implementation already preserves output stability and handles BigNumber correctly. Line 232 instringifyObjectincludessortMapEntries: truewhen callingyamlStringify, so deterministic ordering is maintained. BigNumber serialization is handled viaethersBigNumberSerializer(passed toJSON.stringifyfirst), which converts ethers BigNumber objects and nativebiginttypes to strings before the YAML conversion step. The two-step serialization approach is solid—no action needed here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7609 +/- ##
============================
============================
🚀 New features to boost your workflow:
|
3 participants
Summary
stringifyObjectinwriteJsonandwriteYamlfor proper ethers BigNumber serializationremoveEndingSlashtoremoveTrailingSlashfor consistencyChanges
writeJsonandwriteYamlnow usestringifyObjectto handle ethers BigNumber serialization (converts to hex strings instead of{ _hex: "...", _isBigNumber: true })removeEndingSlashrenamed toremoveTrailingSlashWhat's NOT changing
<T>) on read functions are kept - they provide compile-time type safety@hyperlane-xyz/utils- needed for hyperlane-registry compatibilityTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.