-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Update protoc to v4.23.4, remove namespaces from generated prot… #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…o files, and add a new HTTP request configuration file.
WalkthroughAdds an automated post-processing step to strip exported TypeScript namespace wrappers from generated protobuf files. Introduces a Node.js script to remove namespace blocks, invokes it from the proto generation shell script, and updates generated proto files and tsconfig formatting. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/scripts/generate-proto.sh (1)
1-7: Consider hardening the generation script withset -euo pipefailThe new
node src/scripts/remove-namespaces.jsstep is wired correctly afterprotoc. To make failures (missingnode, script errors, etc.) more obvious and avoid partial generations, consider adding strict shell options at the top:set -euo pipefailThis is optional but improves robustness of the proto generation pipeline.
src/scripts/remove-namespaces.js (1)
17-35: Namespace‑stripping regex is tightly coupled to current ts-proto outputThe regex
/export namespace \w+ \{[\s\S]*?\n\}/gworks today because the generatedexport namespace <Enum>blocks only contain simple type aliases (no nested{}), but it will misbehave if future ts-proto versions ever emit functions or other nested blocks inside the namespace.Not urgent, but for extra safety you could:
- Restrict matching further (e.g., only namespaces immediately following a
const Enum = { ... }pattern), or- Replace the regex with a small brace‑counting scanner that finds the correct closing
}for eachexport namespaceblock.Also, if you want to avoid any potential interaction with the regex
lastIndex, you can replace thetest(...)with a non‑stateful check such asconst hasNamespace = content.match(namespaceRegex); if (hasNamespace) { ... }.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/proto/api.ts(6 hunks)src/proto/tee-bundle.ts(2 hunks)src/scripts/generate-proto.sh(1 hunks)src/scripts/remove-namespaces.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
src/scripts/remove-namespaces.js
[error] ESLint parsing error: TSConfig does not include this file. Include the file in tsconfig.json or adjust ESLint parserOptions.project to cover this path. See: https://typescript-eslint.io/troubleshooting/typed-linting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file
[error] Process completed with exit code 1.
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
src/proto/tee-bundle.ts (1)
4-4: BodyType namespace removal is compatible with existing usageAll references to
BodyType.BODY_TYPE_*in the codebase are runtime value accesses (e.g.,bundle.bodyType !== BodyType.BODY_TYPE_K_OUTPUT), not type-position references. The new const object with union type pattern is fully compatible with these uses. No type-level breaks detected.src/proto/api.ts (1)
4-4: Enum namespace removals are safe—no breaking type-level references detectedThe generated pattern of
export const Enum = { ... } as const; export type Enum = typeof Enum[keyof typeof Enum];correctly replaces the removed namespace blocks. All 75 enum member references across the codebase use runtime value access (e.g.,ErrorCode.ERROR_INTERNAL,ZKProofEngine.ZK_ENGINE_GNARK) in conditional checks and assignments, not in type positions. The union type alias fully covers all type-level use cases. The PR introduces no breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tsconfig.json (1)
31-31: Reconsider including build scripts in the main TypeScript configuration.Adding
"src/**/*.js"to the include list means all JavaScript files undersrc/are now subject to the project's type checking. Build/utility scripts (like the newsrc/scripts/remove-namespaces.js) typically should not be part of the main TypeScript project's include configuration, as they are executed separately and may not conform to the same strict settings.Consider either:
- Excluding the scripts directory explicitly:
"exclude": ["node_modules", "src/scripts"]- Moving build scripts outside
src/to a dedicatedscripts/orbuild/directory at the project root- Using a separate
tsconfigfor build tooling if broader.jsinclusion is neededAre there legitimate application
.jsfiles insrc/that require inclusion in the TypeScript project, or is this addition primarily to include the namespace-removal build script?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scripts/remove-namespaces.js(1 hunks)tsconfig.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/remove-namespaces.js
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (1)
tsconfig.json (1)
3-8: Formatting changes are fine.The reformatting of the "lib" array and "#src/*" path mapping from single-line to multi-line is purely stylistic with no functional impact.
Also applies to: 23-25
…o files, and add a new HTTP request configuration file.
Description
Testing (ignore for documentation update)
Type of change
Checklist:
Additional Notes:
Note
Updates generated proto TS to protoc v4.23.4 and removes enum namespace wrappers via a new post-generation script, plus tsconfig includes JS files.
v4.23.4insrc/proto/api.tsandsrc/proto/tee-bundle.ts.src/proto/api.ts,src/proto/tee-bundle.ts).src/scripts/remove-namespaces.jsto strip namespaces from generated.tsfiles.src/scripts/generate-proto.shafterprotocruns.tsconfig.jsonto includesrc/**/*.js.Written by Cursor Bugbot for commit bed4f98. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.