Fix swagger generation#233
Conversation
WalkthroughThe diff updates the Swagger generation pipeline and related metadata: adds a changelog line noting updated swagger files; Makefile now runs Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Shellcheck (0.11.0)proto/scripts/rename-conflicting-defs.sh[warning] 57-57: modified appears unused. Verify use (or export if used externally). (SC2034) ⏰ 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). (5)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 (1)
client/docs/config.json (1)
213-214: Possible typo:Proposalsmaps to singularGovV1Proposal.Line 214 maps the plural
Proposalsto singularGovV1Proposal, which appears inconsistent. Should this beGovV1Proposalsfor consistency?🔧 Suggested fix
"Proposal": "GovV1Proposal", - "Proposals": "GovV1Proposal", + "Proposals": "GovV1Proposals",
🤖 Fix all issues with AI agents
In `@client/docs/config.json`:
- Around line 217-218: The "Deposits" key in client/docs/config.json incorrectly
maps to the singular type "GovV1Deposit"; update the mapping for "Deposits" to
the plural/array type consistent with how "Proposals" was fixed (e.g.,
"GovV1Deposit[]" or the project's canonical plural deposit type) so that
"Deposit" -> "GovV1Deposit" remains singular and "Deposits" -> the plural/array
type points to the correct collection type.
- Around line 334-357: config.json references swagger files that the
swagger-protoc.sh script deletes (specifically
"./tmp-swagger-gen/cosmos/mint/v1beta1/query.swagger.json",
"./tmp-swagger-gen/cosmos/nft/v1beta1/query.swagger.json", and
"./tmp-swagger-gen/cosmos/params/v1beta1/query.swagger.json"), causing
swagger-combine to fail; fix by either removing these three entries (and their
associated operationIds rename blocks like
"Query_Params"->"MintV1beta1Query_Params",
"Query_Balance"->"NftV1beta1Query_Balance",
"Query_Params"->"ParamsV1beta1Query_Params") from client/docs/config.json or
stop deleting the corresponding directories in swagger-protoc.sh so the
referenced files remain available to swagger-combine, and ensure the chosen
change is applied consistently (update config.json if you intend to prune those
modules, or preserve generation in swagger-protoc.sh if you intend to keep
them).
In `@Makefile`:
- Around line 390-396: The Makefile references a missing script
proto/scripts/remove_conflicting_defs.sh and calls
proto/scripts/fix_conflicting_ids.sh which depends on jq but jq isn’t
documented; either add the missing remove_conflicting_defs.sh under
proto/scripts (implementing the intended conflict-removal logic) or remove the
Makefile invocation to proto/scripts/remove_conflicting_defs.sh and replace it
with a no-op comment, and update the proto/scripts/fix_conflicting_ids.sh
invocation to be robust (check for jq and fail with a clear message or run it
via the protoImage container like the swagger step); finally document jq as a
prerequisite in README.md and/or CONTRIBUTING.md and/or add an automated check
in the Makefile that validates jq is available before calling
fix_conflicting_ids.sh.
🧹 Nitpick comments (3)
proto/scripts/rename_conflicting_defs.sh (2)
54-56: Consider adding cleanup trap for robustness.If
jqormvfails mid-iteration,.tmpfiles may be left behind. Consider adding a trap at the script start to clean up on exit.♻️ Suggested improvement
#!/bin/sh set -e + +# Cleanup any leftover temp files on exit +cleanup() { + find "$SWAGGER_DIR" -name "*.swagger.json.tmp" -delete 2>/dev/null || true +} +trap cleanup EXIT SWAGGER_DIR="./tmp-swagger-gen"
61-63: Remove unnecessary no-op block.This conditional block does nothing and can be safely removed.
♻️ Suggested fix
- if [ "$modified" = "0" ]; then - : # No-op command (do nothing) - fiproto/scripts/fix_conflicting_ids.sh (1)
17-29: Consider usingread -rto prevent backslash interpretation.While swagger file paths are unlikely to contain backslashes, using
read -ris a shell scripting best practice to prevent unintended escape sequence interpretation.Suggested improvement
-find "$SWAGGER_DIR" -name "*.swagger.json" | sort | while read file; do +find "$SWAGGER_DIR" -name "*.swagger.json" | sort | while read -r file; do
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
proto/buf.lockis excluded by!**/*.lockx/feeabstraction/types/query.pb.gois excluded by!**/*.pb.gox/feeabstraction/types/tx.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/tx.pb.gois excluded by!**/*.pb.gox/rewards/types/query.pb.gois excluded by!**/*.pb.gox/rewards/types/tx.pb.gois excluded by!**/*.pb.gox/tokenfactory/types/query.pb.gois excluded by!**/*.pb.gox/tokenfactory/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
CHANGELOG.mdMakefileclient/docs/config.jsonclient/docs/swagger-ui/swagger.jsonclient/docs/swagger-ui/swagger.yamlproto/buf.yamlproto/scripts/fix_conflicting_ids.shproto/scripts/rename_conflicting_defs.shproto/scripts/swagger-combine.shproto/scripts/swagger-protoc.sh
🧰 Additional context used
🪛 Checkov (3.2.334)
client/docs/config.json
[high] 1-359: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-359: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-359: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_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). (5)
- GitHub Check: tests
- GitHub Check: golangci-lint
- GitHub Check: Analyze
- GitHub Check: test-e2e
- GitHub Check: liveness-test
🔇 Additional comments (10)
CHANGELOG.md (1)
26-26: LGTM!The changelog entry appropriately captures the swagger fix in the Fixed section. Consider being slightly more specific (e.g., "Fixed swagger generation by resolving conflicting operationIds and definitions") for better traceability to the actual changes.
proto/scripts/rename_conflicting_defs.sh (1)
1-7: LGTM - Well-structured script for definition conflict resolution.The approach of prefixing conflicting definitions with module-derived names is sound. The hardcoded
CONFLICTING_DEFSlist is maintainable for the known conflicts.proto/scripts/swagger-protoc.sh (2)
1-2: Good POSIX compliance improvements.Using
/bin/shwithset -eimproves portability. The script now correctly delegates post-processing to the other pipeline scripts.
11-11: LGTM!
[ -n "$query_file" ]is the correct POSIX-compliant replacement for the bash-specific[[ ! -z "$query_file" ]].client/docs/config.json (2)
13-16: LGTM - Query_ rename mappings align with cosmos-sdk v0.53.x.*The addition of
Query_Params,Query_Code, and similar mappings addresses operationId conflicts introduced by newer cosmos-sdk protobuf generation patterns.
1-8: Static analysis hints are false positives for this configuration file.Checkov flags about missing
securityDefinitions(CKV_OPENAPI_1, CKV_OPENAPI_4, CKV_OPENAPI_6) apply to OpenAPI spec files, not swagger-combine configuration files. This file defines merge rules, not the final API spec.However, ensure the generated
swagger.jsonoutput has appropriate security definitions if the API requires authentication.proto/buf.yaml (1)
11-11: Verify cosmos-sdk buf dependency version — v0.53.5 does not appear to exist.The web search confirms that the latest stable Cosmos SDK release is v0.53.4 (released July 25, 2025), not v0.53.5. Verify that this file should reference
buf.build/cosmos/cosmos-sdk:v0.53.4instead, or confirm if v0.53.5 is available in your buf registry.proto/scripts/swagger-combine.sh (1)
1-12: LGTM! Script structure is correct.The script properly enables strict error handling with
set -e, generates both YAML and JSON swagger outputs, and cleans up temporary directories. One minor note: the comment on line 4 says "Makes a swagger temp file" but this actually generates the final output files.proto/scripts/fix_conflicting_ids.sh (2)
58-77: Manual JSON construction is acceptable for alphanumeric IDs.The manual string concatenation approach for building
rename_jsonworks correctly for the expected alphanumeric operation IDs. If operation IDs could ever contain special characters (quotes, backslashes), this would need escaping, but that's not expected here.
79-108: LGTM! Config update logic is correct.The conditional logic properly handles both updating existing API entries and creating new ones. The merge operation
(.operationIds.rename // {}) + $renamescorrectly preserves existing rename mappings while adding new ones.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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)
client/docs/config.json (1)
218-218: Plural operationId maps to singular target.Similar to the
Deposits→GovV1Depositissue,Proposalsmaps to singularGovV1Proposalinstead ofGovV1Proposals. This inconsistency may cause confusion in the generated API documentation.🔧 Suggested fix
"Proposal": "GovV1Proposal", - "Proposals": "GovV1Proposal", + "Proposals": "GovV1Proposals",
🤖 Fix all issues with AI agents
In `@proto/scripts/swagger-protoc.sh`:
- Line 6: The find invocation that sets proto_dirs is using the malformed
fragment `-path -prune` which is dead/wrong; update the command in the
swagger-protoc.sh script (variable proto_dirs) to remove the invalid `-path
-prune -o` fragment and just search the target dirs for '*.proto' (e.g., use
find ./proto ./tmp_deps -name '*.proto' -print0 ... then dirname | sort | uniq),
and make the identical change in proto/scripts/protocgen.sh so both scripts use
a valid find syntax without the erroneous -path -prune token.
♻️ Duplicate comments (1)
client/docs/config.json (1)
221-222: Same issue:Depositsmaps to singularGovV1Deposit.This issue was already flagged in a previous review. The plural
Depositsshould map toGovV1Depositsfor consistency.
🧹 Nitpick comments (2)
client/docs/config.json (1)
89-90: Minor naming inconsistency.
Paramsis renamed toUpgradeParamswhileQuery_UpgradedConsensusStateuses the full versioned prefixUpgradeV1beta1Query_UpgradedConsensusState. Consider usingUpgradeV1beta1Paramsfor consistency with other modules (e.g.,StakingV1beta1Params,BankV1beta1Params).🔧 Suggested fix
"rename": { - "Params": "UpgradeParams", + "Params": "UpgradeV1beta1Params", "Query_UpgradedConsensusState": "UpgradeV1beta1Query_UpgradedConsensusState"proto/scripts/swagger-protoc.sh (1)
2-2: Consider enablingset -efor fail-fast behavior.With
set -ecommented out, the script will continue executing even if commands likebuf generateorfindfail, potentially producing incomplete swagger output without any error indication. If there's a specific reason to disable it (e.g., handling expected failures), consider using explicit error handling for those cases instead.-# set -e +set -e
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefileclient/docs/config.jsonclient/docs/swagger-ui/swagger.jsonclient/docs/swagger-ui/swagger.yamlproto/scripts/fix-conflicting-ids.shproto/scripts/swagger-protoc.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- proto/scripts/fix-conflicting-ids.sh
- Makefile
🧰 Additional context used
🪛 Checkov (3.2.334)
client/docs/config.json
[high] 1-323: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[high] 1-323: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-323: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_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). (5)
- GitHub Check: tests
- GitHub Check: liveness-test
- GitHub Check: test-e2e
- GitHub Check: golangci-lint
- GitHub Check: Analyze
🔇 Additional comments (12)
client/docs/config.json (10)
13-16: LGTM!The wasm/v1 operationId renames follow a consistent versioned naming pattern.
24-25: LGTM!The kiichain module operationId renames (tokenfactory, oracle, rewards, feeabstraction) follow consistent versioned naming patterns.
Also applies to: 33-34, 42-43, 51-52
60-63: LGTM!The bank module operationId renames follow the consistent versioned naming pattern.
140-150: LGTM!The evm/vm module comprehensively renames all conflicting operationIds with consistent versioned prefixes.
168-184: LGTM!The auth and distribution module operationId renames follow consistent versioned naming patterns.
200-208: LGTM!The gov/v1beta1 module operationId renames follow consistent versioned naming patterns.
235-243: LGTM!The new protocolpool/v1 module entry follows the consistent versioned naming pattern established in other modules.
256-257: LGTM!The slashing and IBC module operationId renames follow consistent versioned naming patterns.
Also applies to: 273-274, 282-283, 291-292
317-318: LGTM!The ibc/core/client/v1 module operationId renames appropriately disambiguate conflicting IDs.
1-7: Static analysis hints about security definitions are false positives for this file.The Checkov warnings (CKV_OPENAPI_1, CKV_OPENAPI_4, CKV_OPENAPI_6) about missing security definitions don't apply here. This is a
swagger-combineconfiguration file that merges multiple swagger specs, not the final OpenAPI specification. Security definitions should be handled either in the source swagger files or as a post-processing step after combination.proto/scripts/swagger-protoc.sh (2)
11-11: POSIX compatibility change looks correct.The change from bash-specific
[[ ! -z "$query_file" ]]to POSIX[ -n "$query_file" ]is appropriate given the/bin/shshebang.
16-27: Directory cleanup is clear and explicit.The removal of unused swagger generation directories is straightforward. The explicit listing makes it easy to see what's being excluded.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
Fixes faulty swagger generation by:
make proto-swagger-gen(might require sudo depending on docker permissioning)Type of change
How Has This Been Tested?
Manually, locally.
PR Checklist:
Make sure each step was done:
make lint-fix