Fix: Resolve protobuf generation bloat with sparse mode filtering #1525
Conversation
- Add comprehensive error handling with set -e and proper exit codes - Implement automatic cleanup with trap to prevent temp directory leaks - Add validation for proto file existence before processing - Enhance cosmos dependency handling with conditional download - Validate generated output files exist and are non-empty - Improve user feedback with success indicators and detailed error messages - Clean up empty error files automatically - Add proper error checking for git clone, curl download, and protobuf generation
📝 WalkthroughWalkthroughUpdates protobuf generation scripts for MAYAChain and THORChain with strict error handling, tooling checks, temp dir management, sparse pbjs/pbts generation, and validations. Adds a minimal common proto for THORChain. Restructures MAYAChain TypeScript declarations and removes several cosmos types from runtime JS. Trims multiple THORChain TS declarations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xchain-thorchain/src/types/proto/MsgCompiled.d.ts (1)
550-560: Ensure “long” is installed and protobufjs is configured for 64-bit integersThe generated
MsgCompiled.d.tsdefines several fields as(number|Long), but nolongdependency is declared in any package—includingpackages/xchain-thorchain. Without it, protobufjs will fall back to plain numbers, risking precision loss for int64 values.• In
packages/xchain-thorchain/package.json, add:"dependencies": { "long": "^5.2.0" }• After installing, in your runtime initialization (or before any decode/encode calls), wire up protobufjs:
import { util } from "protobufjs"; import Long from "long"; util.Long = Long; util.configure();• If you’re using
pbjs/pbtsto regenerate the code, include the--longflag so the generated code expects aLongtype.
🧹 Nitpick comments (9)
packages/xchain-mayachain/genMsgs.sh (2)
55-55: Prefer trap-based cleanup so TMP_DIR always gets removedIf the script fails before this line, the temp directory is leaked.
Add near the top (after TMP_DIR is set):
# robust cleanup cleanup() { [ -d "$TMP_DIR" ] && rm -rf "$TMP_DIR" } trap cleanup EXITThen this explicit rm can be removed.
10-12: Optional: mktemp failure guardOn some systems mktemp can fail; with current script it will proceed with an empty TMP_DIR. Consider:
Add:
TMP_DIR=$(mktemp -d) || { echo "Error: mktemp failed"; exit 1; }packages/xchain-thorchain/genMsgs.sh (7)
25-28: Shallow clone and make the reference configurable for reproducibilityUsing a moving branch (“develop”) makes outputs non-reproducible. Allow pinning via env var and shallow clone for speed.
Apply this diff:
-if ! (cd "$TMP_DIR" && git clone --branch develop https://gitlab.com/thorchain/thornode); then +if ! (cd "$TMP_DIR" && git clone --branch "${THORNODE_REF:-develop}" --depth 1 https://gitlab.com/thorchain/thornode); then echo "Error: Failed to clone thornode repository" exit 1 fiYou can pin a commit/tag in CI with: THORNODE_REF= ./genMsgs.sh
52-67: Pin coin.proto and add curl resiliency flagsFetching from “main” is non-deterministic. Add retries and consider pinning to a specific cosmos-sdk commit to make generation reproducible.
Apply this diff:
- if ! curl -f -o "$COSMOS_COIN_PROTO" \ - "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ + "https://raw.githubusercontent.com/cosmos/cosmos-sdk/${COSMOS_REF:-main}/proto/cosmos/base/v1beta1/coin.proto"; then echo "Error: Failed to download cosmos coin.proto" exit 1 fiOptionally set COSMOS_REF to a tag/commit in CI for reproducible builds.
73-85: Ensure output directory exists before writing JS bindingsAvoid failures when the target directory is missing.
Apply this diff:
-if ! yarn pbjs -w commonjs -t static-module \ +mkdir -p "$(dirname "$MSG_COMPILED_OUTPUTFILE")" +if ! yarn pbjs -w commonjs -t static-module \
90-95: Ensure typings output directory existsMirror the pbjs change for typings.
Apply this diff:
-if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then
9-9: Optional: mktemp failure guardEnsure TMP_DIR was created successfully.
Add:
TMP_DIR=$(mktemp -d) || { echo "Error: mktemp failed"; exit 1; }
4-4: Optional: stricter shell flags-e is great. Consider -u and -o pipefail for tighter safety.
Example:
set -euo pipefailBe mindful of unbound variables when enabling -u.
73-73: Add preflight checks for required CLI toolsTo improve the UX when
yarn pbjsoryarn pbtsaren’t installed (as evidenced by empty or confusing error output), add an early “preflight” block at the top ofpackages/xchain-thorchain/genMsgs.shthat:
- Verifies
yarnis on$PATH- Confirms the ProtobufJS CLI (
pbjs) can be invoked- Confirms the TypeScript definitions CLI (
pbts) can be invokedSuggested insertion (before the first
yarn pbjs …):# Preflight: ensure toolchain is installed for meaningful error messages if ! command -v yarn >/dev/null 2>&1; then echo "Error: ‘yarn’ not found; please install Yarn and try again." exit 1 fi if ! yarn pbjs --version >/dev/null 2>&1; then echo "Error: ProtobufJS CLI (‘pbjs’) not available. Install with:" echo " yarn add -D protobufjs" exit 1 fi if ! yarn pbts --help >/dev/null 2>&1; then echo "Error: TypeScript definitions CLI (‘pbts’) not available. Install with:" echo " yarn add -D protobufjs" exit 1 fiThis ensures missing dependencies fail fast with clear instructions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/xchain-mayachain/genMsgs.sh(1 hunks)packages/xchain-thorchain/genMsgs.sh(1 hunks)packages/xchain-thorchain/src/types/proto/MsgCompiled.d.ts(1 hunks)
⏰ 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: build
🔇 Additional comments (7)
packages/xchain-thorchain/src/types/proto/MsgCompiled.d.ts (7)
640-646: New Status enum looks consistent with common namespace usageEnum definition and placement under common match downstream references.
647-795: ObservedTx schema surface LGTM; confirm int64 handling strategy (number vs Long)Fields like blockHeight/finaliseHeight are (number|Long). Ensure consuming code consistently normalizes these (e.g., to string or Long) to avoid precision loss in JS.
Would you like me to add a small helper in this package to coerce (number|Long) to a safe bigint/string for callers?
797-891: Attestation uses PascalCase field names; verify this matches the protoGenerated field casing (PubKey, Signature) is unusual relative to other camelCase fields. If this mirrors the proto, fine; just calling it out for downstream expectations.
If the proto uses snake_case, consider regenerating with the appropriate options to enforce consistent casing. Otherwise, we can leave as-is.
1331-1521: NetworkFee and QuorumNetworkFee additions: surface is fineEncoding/decoding/static methods match protobufjs generated patterns.
2117-2313: PriceFeed and AttestPriceFeed additions look goodInterfaces/classes include the expected encode/decode helpers. No issues spotted.
1-4: Double-check “export = types;” integration across consumersThis CommonJS-style export is correct for pbts --name types. Ensure imports in xchain-thorchain code use
import types = require('./MsgCompiled')or compatible TS config.If consumers use ESM default/named imports, confirm there are no breaking changes. I can scan call sites on request.
2411-2499: No duplicate declarations in MsgCompiled.d.tsAll of the newly added symbols (
Status,ObservedTx,Attestation,AttestTx,QuorumTx,QuorumState,NetworkFee,AttestNetworkFee,QuorumNetworkFee,Solvency,AttestSolvency,QuorumSolvency,ErrataTx,AttestErrataTx,QuorumErrataTx,PriceFeed,AttestPriceFeed,QuorumPriceFeed,QuorumPriceFeedBatch,OraclePrice,AttestationBatch) were each found exactly once in the file. No duplicateinterface,class, orenumdeclarations exist.
packages/xchain-mayachain/genMsgs.sh
Outdated
| # Download cosmos/base/v1beta1/coin.proto from cosmossdk if needed | ||
| if [ ! -f "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" ]; then | ||
| tput setaf 2 | ||
| echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" | ||
| tput sgr0 | ||
| mkdir -p "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1" | ||
| curl -o "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" \ | ||
| "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden coin.proto download (fail on HTTP errors, retries, clear messaging)
Current curl lacks -f/-L and doesn’t abort on failures; a partial/empty file will break generation.
Apply this diff:
-# Download cosmos/base/v1beta1/coin.proto from cosmossdk if needed
-if [ ! -f "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" ]; then
- tput setaf 2
- echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk"
- tput sgr0
- mkdir -p "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1"
- curl -o "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" \
- "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"
-fi
+# Download cosmos/base/v1beta1/coin.proto from cosmossdk if needed
+COSMOS_COIN_PROTO="$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto"
+if [ ! -f "$COSMOS_COIN_PROTO" ]; then
+ tput setaf 2
+ echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk"
+ tput sgr0
+ mkdir -p "$(dirname "$COSMOS_COIN_PROTO")"
+ if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \
+ "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then
+ echo "Error: Failed to download cosmos coin.proto"
+ exit 1
+ fi
+ echo "✓ Downloaded cosmos coin.proto"
+else
+ echo "✓ cosmos coin.proto already exists"
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Download cosmos/base/v1beta1/coin.proto from cosmossdk if needed | |
| if [ ! -f "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" ]; then | |
| tput setaf 2 | |
| echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" | |
| tput sgr0 | |
| mkdir -p "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1" | |
| curl -o "$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" \ | |
| "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto" | |
| fi | |
| # Download cosmos/base/v1beta1/coin.proto from cosmossdk if needed | |
| COSMOS_COIN_PROTO="$TMP_DIR/mayanode/third_party/proto/cosmos/base/v1beta1/coin.proto" | |
| if [ ! -f "$COSMOS_COIN_PROTO" ]; then | |
| tput setaf 2 | |
| echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" | |
| tput sgr0 | |
| mkdir -p "$(dirname "$COSMOS_COIN_PROTO")" | |
| if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ | |
| "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then | |
| echo "Error: Failed to download cosmos coin.proto" | |
| exit 1 | |
| fi | |
| echo "✓ Downloaded cosmos coin.proto" | |
| else | |
| echo "✓ cosmos coin.proto already exists" | |
| fi |
🤖 Prompt for AI Agents
In packages/xchain-mayachain/genMsgs.sh around lines 23 to 31, harden the
coin.proto download: use curl with -f -L -S and retry flags (e.g., --retry and
--retry-delay) so HTTP errors and redirects fail fast and retry, write to a
temporary file and only move it into place on successful curl to avoid partial
files, remove any temp file on failure, check curl's exit code and exit the
script non-zero on failure, and update the echo/tput messages to clearly report
success or the HTTP/error returned.
| tput setaf 2 | ||
| echo "Cleaning up $TMP_DIR" | ||
| tput sgr0 | ||
| rm -rf "$TMP_DIR" |
There was a problem hiding this comment.
tput can break the script under set -e in headless/CI; make tput non-fatal
tput exits non-zero when TERM is unset or capabilities are missing; with set -e that aborts the script prematurely.
Apply this diff to make all tput invocations non-fatal:
- tput setaf 2
+ tput setaf 2 || true
- tput sgr0
+ tput sgr0 || trueRepeat the same change for the other occurrences within the file (status banners near checkout, proto checks, download, pbjs/pbts headings, and final success echo).
Also applies to: 22-24, 31-33, 55-57, 71-72, 87-89, 111-113
🤖 Prompt for AI Agents
In packages/xchain-thorchain/genMsgs.sh around lines 14-17 (and also update
occurrences at 22-24, 31-33, 55-57, 71-72, 87-89, 111-113), make all tput
invocations non-fatal by appending a safe fallback so they don't break the
script under set -e; change each tput call to use a no-op on failure (e.g., tput
setaf 2 || true and tput sgr0 || true) so the color/status banners still attempt
to print but won't abort the script if TERM/capabilities are missing.
|
Will need to be reviewed post #1480 being merged. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/xchain-thorchain/genMsgs.sh (1)
14-16: Make tput non-fatal under set -etput exits non-zero when TERM or capabilities are missing; with set -e this will abort CI/headless runs.
- tput setaf 2 + tput setaf 2 || true - tput sgr0 + tput sgr0 || trueRepeat the same change for the other occurrences in the selected ranges.
Also applies to: 29-31, 52-54, 71-73, 95-97
packages/xchain-mayachain/genMsgs.sh (1)
14-16: Make tput non-fatal under set -eSame concern as in the THORChain script; prevents CI/headless aborts.
- tput setaf 2 + tput setaf 2 || true - tput sgr0 + tput sgr0 || trueApply to all selected ranges.
Also applies to: 29-31, 52-54, 71-73, 95-97
🧹 Nitpick comments (9)
packages/xchain-thorchain/genMsgs.sh (4)
18-23: Harden curl: follow redirects, retry, and write atomicallyPrevents partial files and transient failures.
- if ! curl -f -o "$COSMOS_COIN_PROTO" \ - "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + tmp="$(mktemp "${COSMOS_COIN_PROTO}.XXXX")" + if ! curl -fSL --retry 3 --retry-delay 2 -o "$tmp" \ + "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + rm -f "$tmp" echo "Error: Failed to download cosmos coin.proto" exit 1 fi - echo "✓ Downloaded cosmos coin.proto" + mv "$tmp" "$COSMOS_COIN_PROTO" + echo "✓ Downloaded cosmos coin.proto"
68-68: Make sed in-place portable across GNU/BSDsed -i differs on macOS. Using a backup suffix works everywhere.
-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "${MSG_COMPILED_OUTPUTFILE}.bak"
46-49: Quote numeric test for safetyMinor robustness when nounset is enabled.
-if [ $MISSING_FILES -eq 1 ]; then +if [ "$MISSING_FILES" -eq 1 ]; then
4-4: Tighten shell safety flagsAdd nounset and pipefail for stricter failure semantics.
-set -e # Exit on any error +set -euo pipefail # Exit on error, undefined var, and fail pipelinespackages/xchain-mayachain/genMsgs.sh (5)
18-23: Harden curl: follow redirects, retry, and write atomicallyReduces flakiness and avoids partial writes.
- if ! curl -f -o "$COSMOS_COIN_PROTO" \ - "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + tmp="$(mktemp "${COSMOS_COIN_PROTO}.XXXX")" + if ! curl -fSL --retry 3 --retry-delay 2 -o "$tmp" \ + "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + rm -f "$tmp" echo "Error: Failed to download cosmos coin.proto" exit 1 fi - echo "✓ Downloaded cosmos coin.proto" + mv "$tmp" "$COSMOS_COIN_PROTO" + echo "✓ Downloaded cosmos coin.proto"
74-78: Align pbts with Thorchain (name) and ensure output dir existsKeeps namespaces consistent and avoids missing dir errors.
+mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" -if ! yarn pbts "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then
68-68: Use portable sed -iBSD vs GNU sed compatibility.
-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "${MSG_COMPILED_OUTPUTFILE}.bak"
46-49: Quote numeric testMinor safety improvement.
-if [ $MISSING_FILES -eq 1 ]; then +if [ "$MISSING_FILES" -eq 1 ]; then
4-4: Enable nounset and pipefailImproves failure semantics across the script.
-set -e # Exit on any error +set -euo pipefail # Exit on error, undefined var, and fail pipelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/xchain-mayachain/genMsgs.sh(1 hunks)packages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts(2 hunks)packages/xchain-mayachain/src/types/proto/MsgCompiled.js(8 hunks)packages/xchain-thorchain/genMsgs.sh(2 hunks)
⏰ 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: build
🔇 Additional comments (2)
packages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts (1)
23-24: LGTM: new Asset.secured boolean is consistent and backwards-compatibleInterface + class field added with optional typing and defaulting; no breaking changes expected.
If upstream proto is the source of truth, please confirm field number 6 is unused/reserved there to avoid wire conflicts. I can script-check the local protos if needed.
Also applies to: 51-53
packages/xchain-mayachain/src/types/proto/MsgCompiled.js (1)
32-33: Verify upstream/common proto definitions don’t use field number 6 for Asset.secured
Ensure no upstream/common .proto assigns tag 6 to any Asset field to avoid decoding ambiguity.
packages/xchain-thorchain/genMsgs.sh
Outdated
| if ! yarn pbjs -w commonjs -t static-module \ | ||
| -p proto \ | ||
| "proto/common/minimal_common.proto" \ | ||
| "proto/types/minimal_msg_deposit.proto" \ | ||
| "proto/types/minimal_msg_send.proto" \ | ||
| "$COSMOS_COIN_PROTO" \ | ||
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then | ||
| echo "Error: Failed to generate JavaScript bindings" | ||
| cat pbjs_errors.txt | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure output directory exists before pbjs; fail clearly if tools are missing
Avoids failures on clean checkouts; surfaces missing binaries early.
+mkdir -p "$(dirname "$MSG_COMPILED_OUTPUTFILE")"
+command -v yarn >/dev/null || { echo "Error: yarn not found"; exit 1; }
if ! yarn pbjs -w commonjs -t static-module \
-p proto \
"proto/common/minimal_common.proto" \
"proto/types/minimal_msg_deposit.proto" \
"proto/types/minimal_msg_send.proto" \
"$COSMOS_COIN_PROTO" \
-o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! yarn pbjs -w commonjs -t static-module \ | |
| -p proto \ | |
| "proto/common/minimal_common.proto" \ | |
| "proto/types/minimal_msg_deposit.proto" \ | |
| "proto/types/minimal_msg_send.proto" \ | |
| "$COSMOS_COIN_PROTO" \ | |
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then | |
| echo "Error: Failed to generate JavaScript bindings" | |
| cat pbjs_errors.txt | |
| exit 1 | |
| fi | |
| # Ensure the output directory exists | |
| mkdir -p "$(dirname "$MSG_COMPILED_OUTPUTFILE")" | |
| # Fail early if yarn isn't installed | |
| command -v yarn >/dev/null || { echo "Error: yarn not found"; exit 1; } | |
| if ! yarn pbjs -w commonjs -t static-module \ | |
| -p proto \ | |
| "proto/common/minimal_common.proto" \ | |
| "proto/types/minimal_msg_deposit.proto" \ | |
| "proto/types/minimal_msg_send.proto" \ | |
| "$COSMOS_COIN_PROTO" \ | |
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then | |
| echo "Error: Failed to generate JavaScript bindings" | |
| cat pbjs_errors.txt | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In packages/xchain-thorchain/genMsgs.sh around lines 55 to 65, the script runs
pbjs without ensuring the output directory exists or that required tools are
installed; update the script to first check for required binaries (yarn and
pbjs) and exit with a clear error if missing, then create (mkdir -p) the parent
directory of $MSG_COMPILED_OUTPUTFILE before invoking pbjs so the command won't
fail on clean checkouts; ensure errors from missing tools or directory creation
are printed and cause an exit with non-zero status.
Naq302
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/xchain-thorchain/genMsgs.sh (3)
14-17: Make tput non-fatal under set -e (CI/headless safe).tput can exit non-zero when TERM/capabilities are missing; with set -e this aborts the script. Guard all tput calls.
- tput setaf 2 + tput setaf 2 || true - tput sgr0 + tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true @@ -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true @@ -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true @@ -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true @@ -tput sgr0 +tput sgr0 || trueAlso applies to: 22-24, 31-33, 55-57, 71-72, 88-90, 112-114
73-83: Ensure output dir exists and fail fast if yarn/pbjs missing.-tput sgr0 -if ! yarn pbjs -w commonjs -t static-module --sparse \ +tput sgr0 +mkdir -p "$(dirname "$MSG_COMPILED_OUTPUTFILE")" +command -v yarn >/dev/null || { echo "Error: yarn not found"; exit 1; } +command -v pbjs >/dev/null || { echo "Error: pbjs not found (install protobufjs-cli)"; exit 1; } +if ! yarn pbjs -w commonjs -t static-module --sparse \
91-95: Create typings output dir and guard pbts availability.-if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" +command -v pbts >/dev/null || { echo "Error: pbts not found (install protobufjs-cli)"; exit 1; } +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; thenpackages/xchain-mayachain/genMsgs.sh (3)
25-28: Guarded shallow clone for speed/reliability.-if ! (cd "$TMP_DIR" && git clone https://gitlab.com/mayachain/mayanode); then +if ! (cd "$TMP_DIR" && git clone --depth 1 --single-branch https://gitlab.com/mayachain/mayanode); then
73-79: Ensure output dir exists and fail fast if yarn/pbjs missing.-tput sgr0 -if ! yarn pbjs -w commonjs -t static-module --sparse \ +tput sgr0 +mkdir -p "$(dirname "$MSG_COMPILED_OUTPUTFILE")" +command -v yarn >/dev/null || { echo "Error: yarn not found"; exit 1; } +command -v pbjs >/dev/null || { echo "Error: pbjs not found (install protobufjs-cli)"; exit 1; } +if ! yarn pbjs -w commonjs -t static-module --sparse \
92-96: Create typings output dir and guard pbts availability.-if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" +command -v pbts >/dev/null || { echo "Error: pbts not found (install protobufjs-cli)"; exit 1; } +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then
🧹 Nitpick comments (9)
packages/xchain-thorchain/genMsgs.sh (4)
25-28: Shallow, single-branch clone for speed/reliability.-if ! (cd "$TMP_DIR" && git clone --branch develop https://gitlab.com/thorchain/thornode); then +if ! (cd "$TMP_DIR" && git clone --depth 1 --single-branch --branch develop https://gitlab.com/thorchain/thornode); then
59-63: Harden coin.proto download (follow redirects, retry, fail clearly).- if ! curl -f -o "$COSMOS_COIN_PROTO" \ - "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ + "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then echo "Error: Failed to download cosmos coin.proto" exit 1 fi
85-85: Make sed -i portable across GNU/BSD.-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "$MSG_COMPILED_OUTPUTFILE".bak
4-4: Consider stricter shell flags for robustness.-set -e # Exit on any error +set -Eeuo pipefail # Stricter: exit on error, trap ERR, unset vars error, pipefailpackages/xchain-mayachain/genMsgs.sh (3)
59-63: Harden coin.proto download (follow redirects, retry, fail clearly).- if ! curl -f -o "$COSMOS_COIN_PROTO" \ + if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then
86-86: Make sed -i portable across GNU/BSD.-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "$MSG_COMPILED_OUTPUTFILE".bak
4-4: Consider stricter shell flags for robustness.-set -e # Exit on any error +set -Eeuo pipefail # Stricter: exit on error, trap ERR, unset vars error, pipefailpackages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts (2)
124-154: Long type exposure can trip strict TS configs.Properties like
decimals: (number|Long)assume ambient Long types. If consumers use strict ESM/isolatedModules, consider re-exportingimport Longtype or documenting the requirement to depend onlong.
2-4: Flatten or rename to avoid double-nestedtypes.types.*
Regenerate your protobuf TS definitions with--name proto(e.g.,export = proto; namespace proto { … }) or moveMsgDeposit/MsgSendunder the root namespace inpackages/xchain-mayachain/src/types/proto/MsgCompiled.d.tsso consumers can usetypes.MsgSend/types.MsgDepositinstead oftypes.types.MsgSend/types.types.MsgDeposit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/xchain-mayachain/genMsgs.sh(1 hunks)packages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts(1 hunks)packages/xchain-mayachain/src/types/proto/MsgCompiled.js(0 hunks)packages/xchain-thorchain/genMsgs.sh(2 hunks)
💤 Files with no reviewable changes (1)
- packages/xchain-mayachain/src/types/proto/MsgCompiled.js
🧰 Additional context used
🧬 Code graph analysis (3)
packages/xchain-thorchain/genMsgs.sh (1)
packages/xchain-mayachain/genMsgs.sh (1)
cleanup(12-19)
packages/xchain-mayachain/genMsgs.sh (1)
packages/xchain-thorchain/genMsgs.sh (1)
cleanup(12-19)
packages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts (2)
packages/xchain-mayanode/src/generated/mayanodeApi/api.ts (2)
Coin(590-609)Tx(3047-3090)packages/xchain-client/src/types.ts (1)
Fee(163-163)
⏰ 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: build
🔇 Additional comments (5)
packages/xchain-thorchain/genMsgs.sh (2)
34-50: Proto existence checks LGTM.
97-106: Post-generation validations LGTM.packages/xchain-mayachain/genMsgs.sh (2)
34-50: Proto existence checks LGTM.
98-107: Post-generation validations LGTM.packages/xchain-mayachain/src/types/proto/MsgCompiled.d.ts (1)
740-771: MsgSend address types switched toUint8Array. Verify downstream impact.If previous surface used strings/bech32, this is a breaking change for callers. Confirm downstream encode/decode helpers handle byte arrays.
Would you like a quick adapter utility to convert bech32 <-> Uint8Array for consumers?
| tput setaf 2 | ||
| echo "Cleaning up $TMP_DIR" | ||
| tput sgr0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make tput non-fatal under set -e (mirrors Thorchain fix).
- tput setaf 2
+ tput setaf 2 || true
- tput sgr0
+ tput sgr0 || true
@@
-tput setaf 2
+tput setaf 2 || true
@@
-tput sgr0
+tput sgr0 || true
@@
-tput setaf 2
+tput setaf 2 || true
@@
-tput sgr0
+tput sgr0 || true
@@
-tput setaf 2
+tput setaf 2 || true
@@
-tput sgr0
+tput sgr0 || trueAlso applies to: 31-33, 55-57, 71-72, 89-91, 113-115
🤖 Prompt for AI Agents
In packages/xchain-mayachain/genMsgs.sh around lines 14-16 (and also update
occurrences at 31-33, 55-57, 71-72, 89-91, 113-115), tput calls are currently
fatal when running under set -e; make each tput invocation non-fatal by ensuring
it cannot cause the script to exit (for example append a noop fallback like "||
true" or guard with a command check), and update every listed tput call
consistently so color output attempts fail silently when tput is unavailable or
errors.
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)
packages/xchain-thorchain/genMsgs.sh (1)
95-97: BSD/GNU sed portability: use -i.bak to avoid macOS failures.sed -i without suffix fails on macOS; use a portable in-place edit and remove the backup.
-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "$MSG_COMPILED_OUTPUTFILE.bak"
♻️ Duplicate comments (4)
packages/xchain-thorchain/genMsgs.sh (2)
22-30: Make tput non-fatal under set -e (especially in EXIT trap).tput can exit non-zero in CI/headless (unset TERM), which will abort cleanup and potentially leak TMP_DIR.
cleanup() { if [ -d "$TMP_DIR" ]; then - tput setaf 2 + tput setaf 2 || true echo "Cleaning up $TMP_DIR" - tput sgr0 + tput sgr0 || true rm -rf "$TMP_DIR" fi }
33-35: Make all banner tput calls non-fatal.Prevents premature exits in CI with minimal/no color support.
-tput setaf 2 +tput setaf 2 || true echo "Checking out https://gitlab.com/thorchain/thornode to $TMP_DIR" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Checking proto files" -tput sgr0 +tput sgr0 || true @@ - tput setaf 2 + tput setaf 2 || true echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" - tput sgr0 + tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_TYPES_OUTPUTFILE" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "✓ Successfully generated protobuf bindings" -tput sgr0 +tput sgr0 || trueAlso applies to: 42-44, 66-68, 81-83, 99-101, 123-125
packages/xchain-mayachain/genMsgs.sh (2)
22-30: Make tput non-fatal under set -e (cleanup trap).Avoid cleanup aborts/leaked TMP_DIR when TERM is unset.
cleanup() { if [ -d "$TMP_DIR" ]; then - tput setaf 2 + tput setaf 2 || true echo "Cleaning up $TMP_DIR" - tput sgr0 + tput sgr0 || true rm -rf "$TMP_DIR" fi }
33-35: Make all tput status calls non-fatal.Prevents failures in headless CI.
-tput setaf 2 +tput setaf 2 || true echo "Checking out https://gitlab.com/mayachain/mayanode to $TMP_DIR" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Checking proto files" -tput sgr0 +tput sgr0 || true @@ - tput setaf 2 + tput setaf 2 || true echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" - tput sgr0 + tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_TYPES_OUTPUTFILE" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "✓ Successfully generated protobuf bindings" -tput sgr0 +tput sgr0 || trueAlso applies to: 42-44, 66-68, 82-83, 100-102, 125-126
🧹 Nitpick comments (4)
packages/xchain-thorchain/genMsgs.sh (2)
36-39: Shallow, single-branch clone for speed and reliability.Reduces network/IO and avoids unexpected history rewrites.
-if ! (cd "$TMP_DIR" && git clone --branch develop https://gitlab.com/thorchain/thornode); then +if ! (cd "$TMP_DIR" && git clone --depth 1 --single-branch --branch develop https://gitlab.com/thorchain/thornode); then
102-106: Create typings output directory before pbts (symmetry, flake-proof).Rare, but keeps behavior robust if outputs diverge later.
-if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; thenpackages/xchain-mayachain/genMsgs.sh (2)
36-39: Use a shallow clone (and consider single-branch) for faster, more reliable CI.-if ! (cd "$TMP_DIR" && git clone https://gitlab.com/mayachain/mayanode); then +if ! (cd "$TMP_DIR" && git clone --depth 1 --single-branch https://gitlab.com/mayachain/mayanode); then
103-107: Create typings output directory before pbts.-if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then +mkdir -p "$(dirname "$MSG_COMPILED_TYPES_OUTPUTFILE")" +if ! yarn pbts --name types "$MSG_COMPILED_OUTPUTFILE" -o "$MSG_COMPILED_TYPES_OUTPUTFILE" 2>pbts_errors.txt; then
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xchain-mayachain/genMsgs.sh(1 hunks)packages/xchain-thorchain/genMsgs.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/xchain-thorchain/genMsgs.sh (1)
packages/xchain-mayachain/genMsgs.sh (1)
cleanup(23-30)
packages/xchain-mayachain/genMsgs.sh (1)
packages/xchain-thorchain/genMsgs.sh (1)
cleanup(23-30)
⏰ 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: build
| # Fix import to be ESM-compatible (no omitted file extension) | ||
| sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
BSD/GNU sed portability: use -i.bak to support macOS.
-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE"
+sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "$MSG_COMPILED_OUTPUTFILE.bak"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Fix import to be ESM-compatible (no omitted file extension) | |
| sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" | |
| # Fix import to be ESM-compatible (no omitted file extension) | |
| sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && \ | |
| rm -f "$MSG_COMPILED_OUTPUTFILE.bak" |
🤖 Prompt for AI Agents
In packages/xchain-mayachain/genMsgs.sh around lines 96 to 98, the sed -i usage
is not portable to macOS; update the sed invocation to use the backup-style flag
(e.g., replace -i with -i.bak) so it works with BSD sed, and after running sed
remove the generated .bak backup file (remove the specific backup for
MSG_COMPILED_OUTPUTFILE) to avoid leaving stray files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/xchain-mayachain/genMsgs.sh (2)
22-31: tput can exit non‑zero under set -e; guard all calls.
Unavailability (e.g., non-TTY/CI) will abort the script.cleanup() { if [ -d "$TMP_DIR" ]; then - tput setaf 2 + tput setaf 2 || true echo "Cleaning up $TMP_DIR" - tput sgr0 + tput sgr0 || true rm -rf "$TMP_DIR" fi } @@ -tput setaf 2 +tput setaf 2 || true echo "Checking out https://gitlab.com/mayachain/mayanode to $TMP_DIR" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Checking proto files" -tput sgr0 +tput sgr0 || true @@ - tput setaf 2 + tput setaf 2 || true echo "Downloading cosmos/base/v1beta1/coin.proto from cosmossdk" - tput sgr0 + tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "Generating $MSG_COMPILED_TYPES_OUTPUTFILE" -tput sgr0 +tput sgr0 || true @@ -tput setaf 2 +tput setaf 2 || true echo "✓ Successfully generated protobuf bindings" -tput sgr0 +tput sgr0 || trueAlso applies to: 33-35, 41-45, 66-68, 81-83, 100-102, 124-126
96-98: Make sed -i portable (BSD/macOS).
Current -i form breaks on macOS.-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +sed -i.bak -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" && rm -f "$MSG_COMPILED_OUTPUTFILE.bak"
🧹 Nitpick comments (2)
packages/xchain-mayachain/genMsgs.sh (2)
7-12: Preflight should also verify pbjs/pbts availability (via yarn).
Catches missing protobufjs-cli earlier with a clearer error.done + +# Ensure protobufjs CLI tools are available via yarn +for cmd in pbjs pbts; do + if ! yarn -s "$cmd" --version >/dev/null 2>&1; then + echo "Error: 'yarn $cmd' is not available (install protobufjs-cli)." + exit 1 + fi +done
120-122: Nit: keep workspace clean when logs are empty.
This is fine; consider writing logs to TMP_DIR to avoid repo pollution.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xchain-mayachain/genMsgs.sh(1 hunks)packages/xchain-thorchain/genMsgs.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xchain-thorchain/genMsgs.sh
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-mayachain/genMsgs.sh (1)
packages/xchain-thorchain/genMsgs.sh (1)
cleanup(23-30)
⏰ 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: build
🔇 Additional comments (5)
packages/xchain-mayachain/genMsgs.sh (5)
4-4: Good: strict/defensive shell settings are enabled.
set -euo pipefail is appropriate here.
63-78: Good: hardened download with -fSL and retries; clear errors.
Also nice use of mkdir -p and explicit path var.
109-118: Good: post-generation integrity checks.
Existence and non-empty validations are on point.
58-61: Bug: exit check only triggers when exactly one file is missing.
Use “non-zero” instead of “== 1”.-if [ $MISSING_FILES -eq 1 ]; then +if [ "$MISSING_FILES" -ne 0 ]; then echo "Error: Required proto files are missing" exit 1 fiLikely an incorrect or invalid review comment.
80-95: Verify that your pbjs installation supports the--sparseflag. Runnpx pbjs --help(oryarn pbjs --help) to confirm--sparseis listed; if it’s missing, upgrade yourprotobufjsCLI to a version that includes--sparse(e.g. ≥7.2.0).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xchain-thorchain/genMsgs.sh (1)
106-108: Make sed in-place portable (GNU/BSD).
sed -iwithout backup fails on macOS/BSD. Add a cross-platform guard.Apply this diff:
-# Fix import to be ESM-compatible (no omitted file extension) -sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +# Fix import to be ESM-compatible (no omitted file extension), portable across GNU/BSD sed +if sed --version >/dev/null 2>&1; then + sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +else + sed -i '' -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +fi
♻️ Duplicate comments (2)
packages/xchain-thorchain/genMsgs.sh (2)
134-136: Also make final tput non-fatal.Mirror the earlier guard to keep CI happy.
-tput setaf 2 + tput setaf 2 || true echo "✓ Successfully generated protobuf bindings" -tput sgr0 +tput sgr0 || true
23-31: Make tput non-fatal under set -e (CI-safe).
tputexits non-zero when TERM/capabilities are missing, which will abort the script. Apply non-fatal guards here and at othertputsites. (Matches prior feedback.)Apply this diff:
cleanup() { if [ -d "$TMP_DIR" ]; then - tput setaf 2 + tput setaf 2 || true echo "Cleaning up $TMP_DIR" - tput sgr0 + tput sgr0 || true rm -rf "$TMP_DIR" fi }Repeat
|| truefor all othertputcalls in this script.
🧹 Nitpick comments (3)
packages/xchain-thorchain/common_minimal.proto (1)
16-20: Minor: sanity-check decimals type.If
decimalsis bounded (0–18/0–30 typical),int64is fine but larger-than-needed. If you want stricter typing, consideruint32.packages/xchain-thorchain/genMsgs.sh (2)
90-104: Guard pbjs availability and sparse-flag support.Before invoking, verify that
yarn pbjsexists and supports--sparse; fail early with a clear message if not.Apply this diff:
+tput setaf 2 || true +echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" +tput sgr0 || true +# Ensure pbjs is available and supports --sparse +if ! yarn pbjs -h >/dev/null 2>&1; then + echo "Error: pbjs CLI not available via yarn"; exit 1 +fi +if ! yarn pbjs -h 2>/dev/null | grep -q -- '--sparse'; then + echo "Error: pbjs does not support --sparse. Please upgrade protobufjs CLI."; exit 1 +fi if ! yarn pbjs -w commonjs -t static-module --sparse \ -p "$TMP_DIR/thornode/proto" \ -p "$TMP_DIR/thornode/third_party/proto" \ "$TMP_DIR/thornode/proto/thorchain/v1/common/common.proto" \ "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_deposit.proto" \ "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_send.proto" \ -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then
73-88: Nice: resilient cosmos proto fetch.Use of
-fSLwith retries and directory creation is solid. Consider adding a short timeout to avoid long stalls in CI.- if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ + if ! curl -fSL --connect-timeout 10 --max-time 60 --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/xchain-thorchain/common_minimal.proto(1 hunks)packages/xchain-thorchain/genMsgs.sh(2 hunks)packages/xchain-thorchain/src/types/proto/MsgCompiled.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/xchain-thorchain/src/types/proto/MsgCompiled.d.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-thorchain/genMsgs.sh (1)
packages/xchain-mayachain/genMsgs.sh (1)
cleanup(23-30)
🪛 Buf (1.55.1)
packages/xchain-thorchain/common_minimal.proto
2-2: Files with package "common" must be within a directory "common" relative to root but were in directory "packages/xchain-thorchain".
(PACKAGE_DIRECTORY_MATCH)
⏰ 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: build
🔇 Additional comments (2)
packages/xchain-thorchain/common_minimal.proto (1)
6-14: Confirm THORChain compatibility for Asset.secured.
securedwas added for MAYAChain per PR notes. Verify THORChain doesn’t rely on a different Asset schema that would make this confusing to downstreams; if it’s MAYA-only, consider separate minimal protos per chain to avoid accidental cross-chain drift.packages/xchain-thorchain/genMsgs.sh (1)
119-132: Good defensive checks on generated artifacts.Existence and non-empty validations plus error log cleanup look correct.
| syntax = "proto3"; | ||
| package common; | ||
|
|
||
| option go_package = "gitlab.com/thorchain/thornode/v3/common"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve Buf package-directory mismatch (will fail linting).
Buf expects files with package common; to live under a common/ directory. This file sits in packages/xchain-thorchain/, triggering PACKAGE_DIRECTORY_MATCH. Move it under a matching path (e.g., packages/xchain-thorchain/proto/common/common_minimal.proto) and update the script copy path accordingly.
🧰 Tools
🪛 Buf (1.55.1)
2-2: Files with package "common" must be within a directory "common" relative to root but were in directory "packages/xchain-thorchain".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In packages/xchain-thorchain/common_minimal.proto lines 1-5, Buf's
PACKAGE_DIRECTORY_MATCH fails because the file declares "package common;" but is
located at packages/xchain-thorchain/ instead of a common/ subdirectory. Move
the file to a matching path such as
packages/xchain-thorchain/proto/common/common_minimal.proto (or
packages/xchain-thorchain/common/common_minimal.proto) so the on-disk path
matches the proto package, and update any build/copy scripts, CI configs, and
import paths that reference the old location to point to the new path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/xchain-thorchain/genMsgs.sh (2)
33-39: Guard these tput callsSame tput non-fatal concern here.
-tput setaf 2 +tput setaf 2 || true echo "Checking out https://gitlab.com/thorchain/thornode to $TMP_DIR" -tput sgr0 +tput sgr0 || true
22-30: Make tput non-fatal under set -e (also applies to all other tput calls)tput exits non-zero in headless CI (TERM unset), which aborts the script. Add a no-op fallback.
cleanup() { if [ -d "$TMP_DIR" ]; then - tput setaf 2 + tput setaf 2 || true echo "Cleaning up $TMP_DIR" - tput sgr0 + tput sgr0 || true rm -rf "$TMP_DIR" fi }Repeat for lines printing status banners (Lines 33-35, 42-44, 77-79, 92-94, 111-113, 135-137).
🧹 Nitpick comments (6)
packages/xchain-thorchain/proto/common/common_minimal.proto (2)
1-4: Confirm Buf module root or adjust package path to satisfy PACKAGE_DIRECTORY_MATCHBuf reports a package-directory mismatch for package "common". Either:
- set the Buf module root to packages/xchain-thorchain/proto in buf.yaml/buf.work.yaml, or
- move this file under a top-level common/ per Buf’s expectation, or
- rename the package to match its directory path policy.
Would you like a buf.yaml patch to set the module root to packages/xchain-thorchain/proto?
4-4: Verify go_package matches current thornodeEnsure the go_package path (…/v3/common) matches the current thornode branch you clone (develop). Misalignment won’t affect JS generation but may trip linters/buf.
packages/xchain-thorchain/genMsgs.sh (4)
62-73: Pathing fix via SCRIPT_DIR looks good; also guard tputCopy now works regardless of CWD. Add non-fatal tput.
-tput setaf 2 +tput setaf 2 || true echo "Using minimal common.proto to avoid bloat" -tput sgr0 +tput sgr0 || true
74-89: Pin cosmos coin.proto to a commit for reproducibilityFetching from main can drift and break sparse gen. Prefer a pinned commit/tag.
- if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ - "https://raw.githubusercontent.com/cosmos/cosmos-sdk/main/proto/cosmos/base/v1beta1/coin.proto"; then + COSMOS_SDK_REF="v0.50.10" # or specific commit SHA + if ! curl -fSL --retry 3 --retry-delay 2 -o "$COSMOS_COIN_PROTO" \ + "https://raw.githubusercontent.com/cosmos/cosmos-sdk/${COSMOS_SDK_REF}/proto/cosmos/base/v1beta1/coin.proto"; then
107-109: Make sed portable across macOS and LinuxBSD sed needs a backup suffix; this keeps CI/dev parity.
-sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +if sed --version >/dev/null 2>&1; then + sed -i -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +else + sed -i '' -E 's|"(protobufjs/minimal)"|"\1.js"|' "$MSG_COMPILED_OUTPUTFILE" +fi
131-137: Final tput guards and broader trapGuard tput, and consider trapping INT/TERM for cleanup.
-[ ! -s pbjs_errors.txt ] && rm -f pbjs_errors.txt -[ ! -s pbts_errors.txt ] && rm -f pbts_errors.txt - -tput setaf 2 +[ ! -s pbjs_errors.txt ] && rm -f pbjs_errors.txt +[ ! -s pbts_errors.txt ] && rm -f pbts_errors.txt + +tput setaf 2 || true echo "✓ Successfully generated protobuf bindings" -tput sgr0 +tput sgr0 || trueAnd near the trap:
-trap cleanup EXIT +trap cleanup EXIT INT TERM
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xchain-thorchain/genMsgs.sh(2 hunks)packages/xchain-thorchain/proto/common/common_minimal.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-thorchain/genMsgs.sh (1)
packages/xchain-mayachain/genMsgs.sh (1)
cleanup(23-30)
🪛 Buf (1.55.1)
packages/xchain-thorchain/proto/common/common_minimal.proto
2-2: Files with package "common" must be within a directory "common" relative to root but were in directory "packages/xchain-thorchain/proto/common".
(PACKAGE_DIRECTORY_MATCH)
⏰ 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: build
🔇 Additional comments (7)
packages/xchain-thorchain/proto/common/common_minimal.proto (1)
6-14: Verify presence of ‘secured’ in upstream Asset. Could not locate thornode’s common.proto locally—please confirm that field 6 (secured) exists in THORChain’sAssetmessage to maintain wire/field compatibility.packages/xchain-thorchain/genMsgs.sh (6)
4-4: Strict mode is goodset -euo pipefail is appropriate here.
7-12: Preflight tool checks: good coverageCovers yarn, git, curl, sed upfront.
17-19: Output dir guard: goodPrevents pbjs from failing on clean checkouts.
45-60: Proto existence loop: solidClear messages and proper failure if any are missing.
111-118: pbts step and errors: goodClear failure messaging and logs.
120-129: Post-gen validation: goodDefensive checks for non-empty outputs.
| # Generate Protobuf JS bindings using sparse mode to only include referenced types | ||
| tput setaf 2 | ||
| echo "Generating $MSG_COMPILED_OUTPUTFILE" | ||
| echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" | ||
| tput sgr0 | ||
| yarn pbjs -w commonjs -t static-module \ | ||
| if ! yarn pbjs -w commonjs -t static-module --sparse \ | ||
| -p "$TMP_DIR/thornode/proto" \ | ||
| -p "$TMP_DIR/thornode/third_party/proto" \ | ||
| "$TMP_DIR/thornode/proto/thorchain/v1/common/common.proto" \ | ||
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_deposit.proto" \ | ||
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_send.proto" \ | ||
| "$TMP_DIR/thornode/third_party/proto/cosmos/base/v1beta1/coin.proto" \ | ||
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt | ||
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify pbjs supports --sparse before use
Older pbjs lacks --sparse; fail fast with a clear message.
+tput setaf 2 || true
+echo "Checking pbjs supports --sparse"
+tput sgr0 || true
+if ! yarn pbjs --help | grep -q -- '--sparse'; then
+ echo "Error: pbjs does not support --sparse. Update protobufjs-cli."
+ exit 1
+fi
if ! yarn pbjs -w commonjs -t static-module --sparse \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Generate Protobuf JS bindings using sparse mode to only include referenced types | |
| tput setaf 2 | |
| echo "Generating $MSG_COMPILED_OUTPUTFILE" | |
| echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" | |
| tput sgr0 | |
| yarn pbjs -w commonjs -t static-module \ | |
| if ! yarn pbjs -w commonjs -t static-module --sparse \ | |
| -p "$TMP_DIR/thornode/proto" \ | |
| -p "$TMP_DIR/thornode/third_party/proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/common/common.proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_deposit.proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_send.proto" \ | |
| "$TMP_DIR/thornode/third_party/proto/cosmos/base/v1beta1/coin.proto" \ | |
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt | |
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then | |
| # Generate Protobuf JS bindings using sparse mode to only include referenced types | |
| tput setaf 2 | |
| echo "Generating $MSG_COMPILED_OUTPUTFILE (using sparse mode to avoid bloat)" | |
| tput sgr0 | |
| tput setaf 2 || true | |
| echo "Checking pbjs supports --sparse" | |
| tput sgr0 || true | |
| if ! yarn pbjs --help | grep -q -- '--sparse'; then | |
| echo "Error: pbjs does not support --sparse. Update protobufjs-cli." | |
| exit 1 | |
| fi | |
| if ! yarn pbjs -w commonjs -t static-module --sparse \ | |
| -p "$TMP_DIR/thornode/proto" \ | |
| -p "$TMP_DIR/thornode/third_party/proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/common/common.proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_deposit.proto" \ | |
| "$TMP_DIR/thornode/proto/thorchain/v1/types/msg_send.proto" \ | |
| -o "$MSG_COMPILED_OUTPUTFILE" 2>pbjs_errors.txt; then |
1 participant
Summary
Fixes protobuf generation scripts to eliminate 20K+ line bloat while maintaining functionality and ESM compatibility. Builds on existing
error handling improvements with architectural fixes.
Key Changes
Bloat Elimination (New)
common.protoinclusion, fixing output from 1,068 to 2,336 lines--sparsemode filtering to include only referenced typesEnhanced Error Handling (Existing)
set -eand proper exit codesNew Asset Field
Problem Solved
Test Results
require("protobufjs/minimal.js")Resolves protobuf bloat issue while maintaining robust error handling and clean output.
Summary by CodeRabbit