feat/proto generation #505
Conversation
WalkthroughThe changes introduce new scripts in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PackageManager
participant CoreGenScript
participant IndexerGenScript
User->>PackageManager: run generate:core-proto-ts
PackageManager->>CoreGenScript: Execute
CoreGenScript->>CoreGenScript: Generate TypeScript from proto
CoreGenScript-->>PackageManager: Return generated files
User->>PackageManager: run generate:indexer-proto-ts
PackageManager->>IndexerGenScript: Execute
IndexerGenScript->>IndexerGenScript: Generate TypeScript from proto
IndexerGenScript-->>PackageManager: Return generated files
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a critical CVE?Contains a Critical Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
This reverts commit 70931a9.
|
@coderabbitai summary |
Actions performedSummary regeneration triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (12)
proto/.gitignore (1)
6-6: Add a trailing slash for consistency.For consistency with the other
.gitignoreentries, add a trailing slash to thecore/proto-tsentry.Apply this diff:
-core/proto-ts +core/proto-ts/proto/core/publish.sh (1)
17-26: Remove the commented-out code.The commented-out code seems to be related to publishing the
@injectivelabs/chain-apipackage, which is no longer needed. Consider removing this code to improve the script's readability and maintainability.Apply this diff to remove the commented-out code:
-## Publish chain-api -# cd ./../chain-api || exit - -# v=$(npm view @injectivelabs/chain-api version) -# echo "current package version: $v" - -# v1="${v%.*}.$((${v##*.}+1))" -# echo "new package version: $v1" - -# npm version $v1 -# npm publishproto/core/stub/package.json.chain-api.template (1)
5-7: Update the placeholder test script.The
scriptssection includes a placeholder test script that should be replaced with actual tests to ensure the package's functionality. Consider adding meaningful tests to verify the expected behavior of the package.proto/core/stub/package.json.core-proto-ts.template (1)
5-7: Consider adding relevant scripts.The
testscript is just a placeholder that always fails. Consider adding scripts for common tasks such as:
- Building the package
- Running tests (once implemented)
- Linting the code
- Generating documentation
Having these scripts defined in the
package.jsonmakes it easier for developers to perform these tasks consistently.proto/README.md (6)
3-3: Add a comma to improve readability.Consider adding a comma after "Within this folder" to improve the readability of the sentence.
Apply this diff to add the comma:
-Within this folder we have the logic to generate proto definitions from the `injective-core` and `injective-indexer` repos. +Within this folder, we have the logic to generate proto definitions from the `injective-core` and `injective-indexer` repos.Tools
LanguageTool
[typographical] ~3-~3: It appears that a comma is missing.
Context: # 🌟 TS Proto Generation Within this folder we have the logic to generate proto def...(DURING_THAT_TIME_COMMA)
9-9: Use "ensure" instead of "make sure" to strengthen the wording.Consider replacing "make sure" with "ensure" to strengthen the wording of the sentence.
Apply this diff to make the change:
-please make sure you read them before you open a PR. +please ensure you read them before you open a PR.Tools
LanguageTool
[style] ~9-~9: Consider using a different verb to strengthen your wording.
Context: ...ge-specific contribution guides, please make sure you read them before you open a PR. --...(MAKE_SURE_ENSURE)
26-26: Use an en dash for the year range.Consider using an en dash (–) instead of a hyphen (-) for the year range to adhere to typographical conventions.
Apply this diff to make the change:
-Copyright © 2021 - 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021 – 2022 Injective Labs Inc. (https://injectivelabs.org/)Tools
LanguageTool
[typographical] ~26-~26: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
Markdownlint
26-26: null
Bare URL used(MD034, no-bare-urls)
26-26: Convert the bare URL to a link.The bare URL should be converted to a link to improve readability and make it clickable.
Apply this diff to make the change:
-Copyright © 2021 – 2022 Injective Labs Inc. (https://injectivelabs.org/) +Copyright © 2021 – 2022 Injective Labs Inc. ([https://injectivelabs.org/](https://injectivelabs.org/))Tools
LanguageTool
[typographical] ~26-~26: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
Markdownlint
26-26: null
Bare URL used(MD034, no-bare-urls)
28-28: Add alt text for the image.The image should have an alt text to improve accessibility for users with screen readers.
Apply this diff to add the alt text:
-<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" style="width: 300px; max-width: 100%; height: auto" /> +<a href="https://iili.io/mNneZN.md.png"><img src="https://iili.io/mNneZN.md.png" alt="Injective Labs Inc." style="width: 300px; max-width: 100%; height: auto" />Tools
Markdownlint
28-28: null
Images should have alternate text (alt text)(MD045, no-alt-text)
33-33: Convert the bare URL to a link.The bare URL should be converted to a link to improve readability and make it clickable.
Apply this diff to make the change:
-http://www.apache.org/licenses/ +[http://www.apache.org/licenses/](http://www.apache.org/licenses/)Tools
Markdownlint
33-33: null
Bare URL used(MD034, no-bare-urls)
proto/core/stub/index.ts.template (2)
24-25: Check if the commented-out exports are necessaryLines 24 and 25 contain commented-out exports for
CosmosCapabilityV1Beta1CapabilityandCosmosCapabilityV1Beta1Genesis. If these modules are required, consider uncommenting them to include them in the export list.
85-89: Review the need for commented-out IBC Transfer V2 exportsLines 85 to 89 have commented-out exports related to IBC Transfer V2 modules. If these exports are needed for your project, you might want to uncomment them.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
proto/core/package-lock.jsonis excluded by!**/package-lock.jsonproto/indexer/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (23)
- package.json (1 hunks)
- proto/.gitignore (1 hunks)
- proto/README.md (1 hunks)
- proto/core/gen.sh (1 hunks)
- proto/core/package.json (1 hunks)
- proto/core/publish.sh (1 hunks)
- proto/core/stub/index.ts.template (1 hunks)
- proto/core/stub/package.json.chain-api.template (1 hunks)
- proto/core/stub/package.json.cjs.template (1 hunks)
- proto/core/stub/package.json.core-proto-ts.template (1 hunks)
- proto/core/stub/package.json.esm.template (1 hunks)
- proto/core/stub/tsconfig.json.cjs.template (1 hunks)
- proto/core/stub/tsconfig.json.esm.template (1 hunks)
- proto/indexer/gen.sh (1 hunks)
- proto/indexer/package.json (1 hunks)
- proto/indexer/publish.sh (1 hunks)
- proto/indexer/stub/index.ts.template (1 hunks)
- proto/indexer/stub/package.json.cjs.template (1 hunks)
- proto/indexer/stub/package.json.esm.template (1 hunks)
- proto/indexer/stub/package.json.indexer-api.template (1 hunks)
- proto/indexer/stub/package.json.indexer-proto-ts.template (1 hunks)
- proto/indexer/stub/tsconfig.json.cjs.template (1 hunks)
- proto/indexer/stub/tsconfig.json.esm.template (1 hunks)
Additional context used
LanguageTool
proto/README.md
[typographical] ~3-~3: It appears that a comma is missing.
Context: # 🌟 TS Proto Generation Within this folder we have the logic to generate proto def...(DURING_THAT_TIME_COMMA)
[style] ~9-~9: Consider using a different verb to strengthen your wording.
Context: ...ge-specific contribution guides, please make sure you read them before you open a PR. --...(MAKE_SURE_ENSURE)
[typographical] ~26-~26: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...a> --- ## 🔓 License Copyright © 2021 - 2022 Injective Labs Inc. (https://inject...(DASH_RULE)
[typographical] ~30-~30: Do not use a colon (:) before a series that is introduced by a preposition (‘under’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ginally released by Injective Labs Inc. under:
Apache License
Version 2....(RP_COLON)
Markdownlint
proto/README.md
26-26: null
Bare URL used(MD034, no-bare-urls)
33-33: null
Bare URL used(MD034, no-bare-urls)
28-28: null
Images should have alternate text (alt text)(MD045, no-alt-text)
Shellcheck
proto/core/gen.sh
[warning] 9-9: TS_PROTO_TEMPLATE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 23-23: counter appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 75-75: Quote this to prevent word splitting.
(SC2046)
proto/core/publish.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
proto/indexer/gen.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
proto/indexer/publish.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
Additional comments not posted (36)
proto/.gitignore (1)
1-5: LGTM!The
.gitignoreentries are correctly excluding the generated files and directories related to the TypeScript proto generation process.proto/core/package.json (1)
1-8: LGTM!The
package.jsonfile correctly specifies the necessary development dependencies for generating TypeScript code from proto definitions. The versions specified are the latest stable versions at the time of writing.proto/indexer/package.json (1)
1-8: LGTM!The
package.jsonfile specifies the necessary development dependencies for generating TypeScript code from proto definitions. The versions specified are the latest stable versions at the time of writing.proto/indexer/stub/package.json.esm.template (1)
1-17: LGTM!The
package.json.esm.templatefile is well-structured and includes the necessary configuration for an ESM package. The dependencies and scripts are appropriate for a package that generates TypeScript code from protocol buffer definitions and facilitates gRPC communication.proto/indexer/stub/package.json.cjs.template (1)
1-17: LGTM!The
package.jsonfile is well-structured and includes the necessary dependencies and scripts for a TypeScript CommonJS module. The dependencies are standard libraries commonly used in TypeScript projects, and thegenscript is appropriately defined to run the TypeScript compiler.proto/core/stub/package.json.esm.template (1)
1-18: LGTM!The
package.jsonfile for theproto/core/stubdirectory is well-structured and includes the necessary configuration for a TypeScript project involving protocol buffers and gRPC.
- The "type" field is correctly set to "module" for using ECMAScript modules.
- The "scripts" section includes a "gen" script to run the TypeScript compiler.
- The "dependencies" section lists the required libraries for working with protobuf and gRPC, such as
@injectivelabs/grpc-web,google-protobuf,@bufbuild/protobuf,protobufjs, andrxjs.- The "devDependencies" section includes the necessary type definitions for "long" and "node", as well as the TypeScript compiler.
The package configuration looks good and is ready to be used for the TypeScript proto generation process.
proto/core/stub/package.json.cjs.template (4)
1-18: LGTM!The
package.jsonfile is well-structured and includes the necessary fields for a CommonJS module.
6-12: Review the dependencies.The dependencies section includes the required libraries for working with gRPC, protobuf, and RxJS. The versions seem to be properly specified.
13-17: Review the devDependencies.The devDependencies section includes the necessary type definitions and the TypeScript compiler. The versions seem to be properly specified.
3-5: Verify thegenscript.The
genscript runs the TypeScript compiler (tsc). Ensure that the TypeScript configuration file (tsconfig.json) is present in the same directory or a parent directory.Run the following script to verify the presence of the
tsconfig.jsonfile:Verification successful
To gather more information about the file generation process and verify the content of the
tsconfig.json.cjs.template, I'll run the following scripts:
The
genscript is correctly configured for TypeScript compilation.The
tsconfig.json.cjs.templatefile contains a valid TypeScript configuration for CommonJS modules, with appropriate settings for the project. When the actualtsconfig.jsonfile is generated from this template, it will work correctly with thetsccommand specified in thegenscript.While we couldn't find specific file generation scripts in the immediate directory, the presence of template files suggests that there's likely a higher-level build process or CI/CD pipeline that handles the generation of actual files from these templates.
To ensure everything works as expected:
- Verify that the higher-level build process correctly generates the
package.jsonandtsconfig.jsonfiles from their respective templates.- Confirm that the generated files are placed in the correct locations relative to each other.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of the `tsconfig.json` file. # Test: Search for the `tsconfig.json` file in the current and parent directories. # Expect: At least one occurrence of the file. fd tsconfig.json proto/core/stubLength of output: 120
Script:
#!/bin/bash # Search for scripts that might generate files from templates echo "Searching for file generation scripts:" rg -i "template|generate" proto/core/stub # Display the content of tsconfig.json.cjs.template echo -e "\nContent of tsconfig.json.cjs.template:" cat proto/core/stub/tsconfig.json.cjs.template # Check for build or setup scripts in parent directories echo -e "\nSearching for build or setup scripts in parent directories:" fd -e sh -e js -e ts -p "build|setup" proto/coreLength of output: 1101
proto/core/stub/tsconfig.json.esm.template (1)
1-16: LGTM!The TypeScript configuration for ESM modules looks good:
- It targets modern JavaScript features with "es6" and "esnext".
- Strict type checking is enabled to catch potential issues.
- The output directory is set correctly.
- Interoperability with CommonJS modules is enabled.
- Relevant libraries are included.
- The "node_modules" directory is properly excluded.
The configuration is well-suited for generating ESM modules with improved code quality and faster compilation.
proto/indexer/stub/tsconfig.json.esm.template (1)
1-16: LGTM! The TypeScript configuration looks good.This configuration is set up for generating modern, ES module-compatible JavaScript code along with TypeScript declaration files. Here are the key aspects:
- Targets ES6 and uses the ESNext module system.
- Generates declaration files and outputs to the
esmdirectory.- Enables strict type checking to catch potential issues.
- Allows JavaScript files to be included, which is useful for gradual migration.
- Includes specific
libfiles for compatibility with different environments and features.Overall, this configuration is well-structured and suitable for its intended purpose.
proto/core/stub/tsconfig.json.cjs.template (1)
1-16: LGTM!The TypeScript configuration for the CommonJS module looks good. The compiler options are appropriately set for a Node.js project, enabling strict type checking, generating declaration files, and supporting interoperability between CommonJS and ES modules. The inclusion of specific built-in type definitions allows the usage of necessary JavaScript features and APIs.
proto/indexer/stub/tsconfig.json.cjs.template (1)
1-16: LGTM!The TypeScript configuration file for the indexer stub is set up correctly with appropriate compiler options for generating CommonJS modules. The configuration follows best practices by enabling strict type checking, compatibility options, and excluding the "node_modules" directory.
proto/core/publish.sh (2)
1-2: LGTM!The shebang line correctly specifies bash as the interpreter for the script.
6-13: LGTM!The code segment correctly retrieves the current version of the package, increments the patch version, updates the package version, and publishes the new version.
proto/indexer/publish.sh (1)
1-26: LGTM!The script follows a clear and logical flow for publishing a new version of the
@injectivelabs/indexer-proto-tspackage. The package version is incremented using string manipulation and arithmetic operations, which is a common pattern in bash scripts.The deprecated section for publishing the
indexer-apipackage is properly commented out, providing clarity on its status.Tools
Shellcheck
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
proto/core/stub/package.json.chain-api.template (1)
23-26: Dependencies look good.The
dependenciessection correctly lists the required dependencies,@injectivelabs/grpc-webandgoogle-protobuf, with specific versions. These dependencies seem necessary for the package's functionality.proto/indexer/stub/package.json.indexer-api.template (2)
1-23: Package metadata looks good!The package name, version, description, repository, author, license, and issue tracking information are properly specified and follow the recommended conventions.
24-27: Dependencies look fine.The package dependencies are properly specified, and the versions are using the caret (
^) to allow for compatible updates. The@injectivelabs/grpc-webdependency seems to be a custom package for gRPC-Web support, and thegoogle-protobufdependency is a well-known library for protocol buffers.proto/indexer/stub/index.ts.template (1)
1-14: LGTM! The index file provides a clean and organized way to export sub-modules.This
index.ts.templatefile serves as a central entry point for exporting various sub-modules related to the Injective protocol. The approach of re-exporting the sub-modules using theexport * assyntax offers several benefits:
- It allows other parts of the codebase to import the required sub-modules from a single location, improving code organization and maintainability.
- The consistent naming convention (
Injectiveprefix + component name +Rpcsuffix) makes it easy to understand the purpose of each sub-module.- The relative import paths indicate that the sub-modules are located in the same directory, keeping the related files together.
Overall, this approach promotes a clean and modular structure for the Injective protocol's TypeScript code.
proto/core/stub/package.json.core-proto-ts.template (4)
1-25: LGTM!The package metadata is complete and follows the expected format. The name, version, description, and other fields provide clear and relevant information about the package.
8-10: LGTM!The package entry points are correctly defined for both CommonJS and ECMAScript module systems. This allows the package to be used in a variety of environments.
26-31: LGTM!The package dependencies are appropriate for a package that provides generated gRPC bindings. The inclusion of
google-protobuf,protobufjs, andrxjsaligns with the package's purpose.
32-36: LGTM!The package development dependencies are appropriate for a TypeScript package. The inclusion of
@types/long,@types/node, andtypescriptprovides the necessary type definitions and compiler for developing the package.proto/indexer/stub/package.json.indexer-proto-ts.template (4)
1-27: LGTM!The package metadata is well-defined and follows good practices:
- The package name follows the
@scope/package-nameconvention.- The version indicates that this is the initial version of the package.
- The description clearly explains the purpose of the package.
sideEffectsis set tofalsefor optimization.- Both CommonJS and ECMAScript module entry points are provided.
- Important metadata like repository, author, and license are included.
28-33: LGTM!The dependencies are appropriate for a package that provides generated gRPC bindings:
@injectivelabs/grpc-webis included for gRPC-Web support.google-protobufandprotobufjsare necessary for working with Protocol Buffers.rxjsis a common choice for reactive programming with gRPC-Web.
34-38: LGTM!The devDependencies are suitable for a TypeScript package:
@types/longprovides type definitions for thelonglibrary, which is often used with Protocol Buffers.@types/nodeprovides type definitions for Node.js, enabling the use of Node.js APIs in TypeScript.typescriptis necessary for compiling the TypeScript code.
1-39: LGTM!The
package.jsonfile is well-structured and includes all the essential sections:
- Metadata: name, version, description, entry points, repository, etc.
- Scripts: build and test (placeholder)
- Dependencies: required packages for the generated gRPC bindings
- DevDependencies: TypeScript-related dependencies for development
The file provides a solid foundation for the
@injectivelabs/indexer-proto-tspackage.proto/README.md (2)
13-20: LGTM!The support section looks good. It provides a clear list of support channels with appropriate links.
36-38: LGTM!The final section with the tagline looks good. It provides a clear and concise message that aligns with the project's vision.
package.json (4)
17-17: LGTM!The addition of the
generate:core-proto-tsscript for generating TypeScript code from core protocol buffers is a valid and common approach for code generation tasks. It aligns with the PR objective of introducing protocol generation functionality.
18-18: Verify the publishing process.The addition of the
publish:core-proto-tsscript for publishing the generated TypeScript code for core protocol buffers is a valid approach. It aligns with the PR objective of introducing protocol generation functionality.Please ensure that the
publish.shscript correctly handles the publishing process, such as authenticating with the package registry, versioning, and any necessary pre-publish steps.
19-19: LGTM!The addition of the
generate:indexer-proto-tsscript for generating TypeScript code from indexer protocol buffers is a valid and common approach for code generation tasks. It aligns with the PR objective of introducing protocol generation functionality.
20-20: Verify the publishing process.The addition of the
publish:indexer-proto-tsscript for publishing the generated TypeScript code for indexer protocol buffers is a valid approach. It aligns with the PR objective of introducing protocol generation functionality.Please ensure that the
publish.shscript correctly handles the publishing process, such as authenticating with the package registry, versioning, and any necessary pre-publish steps.proto/indexer/gen.sh (1)
115-117: Verify the file extensions in cleanup commandsThe
findcommands are deleting files with extensions*.jse,*.tse, and*.jsone. These seem uncommon and may indicate a typo or unintended deletions.Double-check these extensions to ensure that only unwanted files are deleted and that important files are not accidentally removed.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/sdk-ts/src/core/accounts/PrivateKey.ts (2 hunks)
- packages/sdk-ts/src/core/modules/tx/utils/tx.ts (3 hunks)
- proto/core/gen.sh (1 hunks)
- proto/core/publish.sh (1 hunks)
- proto/core/stub/index.ts.template (1 hunks)
- proto/indexer/gen.sh (1 hunks)
- proto/indexer/publish.sh (1 hunks)
Additional context used
Shellcheck
proto/core/gen.sh
[warning] 71-71: Quote this to prevent word splitting.
(SC2046)
proto/core/publish.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
proto/indexer/gen.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
proto/indexer/publish.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
Additional comments not posted (20)
proto/core/publish.sh (1)
3-4: ** Usecd ... || exitfor the firstcdcommand.**The past review comment is still valid and applicable to the current code segment. To handle the case where
cdfails, usecd ... || exitfor the firstcdcommand. This will ensure that the script exits if it cannot change to the./proto/coredirectory.Apply this diff to fix the issue:
-cd ./proto/core +cd ./proto/core || exit cd ./proto-ts || exitTools
Shellcheck
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
proto/indexer/gen.sh (6)
1-1: Add a shebang line to specify the shell interpreterAs mentioned in a previous review, the script lacks a shebang line at the beginning, which specifies the interpreter to execute the script. It's recommended to add
#!/bin/bashat the top to ensure it's interpreted correctly.Tools
Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
32-32: Correct thefindcommand inproto_dirsassignmentAs pointed out in a previous review, the
findcommand used to populateproto_dirshas an incorrect usage of the-pathand-pruneoptions, which might lead to unintended results or an empty variable.Modify the command to correctly find directories containing
.protofiles:-proto_dirs=$(find $PROTO_DIR -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq) +proto_dirs=$(find $PROTO_DIR -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
37-40: Fix the loop to correctly rename proto files in multiple directoriesAs mentioned in a previous review, the loop assumes
$proto_dirsis a single directory, which may not be the case if multiple directories contain.protofiles. This can cause themvcommand to fail or skip files.Adjust the loop to iterate over each directory and process the files within:
-for file in $proto_dirs/goadesign_goagen_*.proto; do +for dir in $proto_dirs; do + for file in "$dir"/goadesign_goagen_*.proto; do new_file=$(basename "$file" | sed 's/^goadesign_goagen_//') - mv "$file" "$proto_dirs/$new_file" + mv "$file" "$dir/$new_file" + done done
54-54: Handle filenames with spaces in theprotoccommandAs pointed out in a previous review and flagged by Shellcheck, the unquoted command substitution may cause issues if any
.protofilenames contain spaces. This could lead toprotocfailing to process all intended files.Modify the command to safely handle filenames with spaces:
- $(find "${dir}" -maxdepth 1 -name '*.proto') + $(find "${dir}" -maxdepth 1 -name '*.proto' -print0 | xargs -0)Tools
Shellcheck
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
57-58: Ensure.jsand.d.tsfiles are available before deletionAs mentioned in a previous review, the script deletes all
.jsand.d.tsfiles in$TS_OUTPUT_DIR/proto, but later attempts to modify.d.tsfiles in lines 81-87. Deleting these files before modification may result in errors.Reorder the script to delete these files after all necessary modifications are complete:
# gen using ts-proto npm --prefix $ROOT_DIR install for dir in $proto_dirs; do protoc \ --plugin="$ROOT_DIR/node_modules/.bin/protoc-gen-ts_proto" \ --ts_proto_opt="esModuleInterop=true" \ --ts_proto_opt="forceLong=string" \ --ts_proto_opt="env=both" \ --ts_proto_opt="useExactTypes=false" \ --ts_proto_opt="outputClientImpl=grpc-web" \ --ts_proto_out="$TS_OUTPUT_DIR/proto" \ -I "$PROTO_DIR" \ $(find "${dir}" -maxdepth 1 -name '*.proto' -print0 | xargs -0) done +# Modify .d.ts files here (lines 81-87) + +# Now safe to delete .js and .d.ts files find $TS_OUTPUT_DIR/proto -name "*.js" -type f -delete find $TS_OUTPUT_DIR/proto -name "*.d.ts" -type f -delete
81-87: Confirm the existence of.d.tsfiles before modificationAs pointed out in a previous review, after deleting
.d.tsfiles in line 58, the script attempts to modify them here. This will fail if the files no longer exist.Ensure that the modifications to
.d.tsfiles occur before they are deleted, or remove the deletion if it's unnecessary.proto/core/gen.sh (5)
17-19: ****The comment from the previous review still applies:
Quote variables in
rm -rfcommands to prevent globbing and word splitting.It's a good practice to quote variables in
rm -rfcommands to avoid unexpected behavior if variables contain spaces or are empty.Apply the following diff to fix the issue:
-rm -rf $BUILD_DIR -rm -rf $PROTO_DIR -rm -rf $TS_OUTPUT_DIR +rm -rf "$BUILD_DIR" +rm -rf "$PROTO_DIR" +rm -rf "$TS_OUTPUT_DIR"
40-41: ****The comment from the previous review still applies:
Quote variables in commands to prevent word splitting and globbing.
Variables like
$BUILD_DIR,$PROTO_DIR, and branch variables should be quoted when used in commands to prevent issues if they contain spaces.Apply the following diff to fix the issue:
-git clone https://github.com/InjectiveLabs/injective-core.git $BUILD_DIR/injective-core -b $injective_core_branch --depth 1 --single-branch > /dev/null -cp -r $BUILD_DIR/injective-core/proto/injective $PROTO_DIR +git clone https://github.com/InjectiveLabs/injective-core.git "$BUILD_DIR"/injective-core -b "$injective_core_branch" --depth 1 --single-branch > /dev/null +cp -r "$BUILD_DIR"/injective-core/proto/injective "$PROTO_DIR"
44-46: ****The comment from the previous review about quoting variables also applies to these lines:
Quote variables in commands to prevent word splitting and globbing.
Apply the following diff to fix the issue:
-git clone https://github.com/InjectiveLabs/cosmos-sdk.git $BUILD_DIR/cosmos-sdk -b $cosmos_sdk_branch --depth 1 --single-branch > /dev/null -git clone https://github.com/InjectiveLabs/wasmd $BUILD_DIR/wasmd -b $wasmd_branch --depth 1 --single-branch > /dev/null -git clone https://github.com/InjectiveLabs/ibc-go $BUILD_DIR/ibc-go -b $ibc_go_branch --depth 1 --single-branch > /dev/null +git clone https://github.com/InjectiveLabs/cosmos-sdk.git "$BUILD_DIR"/cosmos-sdk -b "$cosmos_sdk_branch" --depth 1 --single-branch > /dev/null +git clone https://github.com/InjectiveLabs/wasmd "$BUILD_DIR"/wasmd -b "$wasmd_branch" --depth 1 --single-branch > /dev/null +git clone https://github.com/InjectiveLabs/ibc-go "$BUILD_DIR"/ibc-go -b "$ibc_go_branch" --depth 1 --single-branch > /dev/null
71-71: ****The comment from the previous review still applies:
Quote the command substitution to prevent word splitting.
In the
protoccommand, thefindcommand substitution should be quoted to prevent word splitting, which can cause issues if filenames contain spaces.Apply the following diff to fix the issue:
- $(find "${dir}" -maxdepth 1 -name '*.proto') + "$(find "${dir}" -maxdepth 1 -name '*.proto')"This is also confirmed by the static analysis hint from Shellcheck.
Tools
Shellcheck
[warning] 71-71: Quote this to prevent word splitting.
(SC2046)
1-139: LGTM!The overall script looks good with no additional issues found. It follows good practices such as:
- Using
set -eo pipefailfor error handling- Using meaningful variable names and consistent naming convention
- Well-structured with clear comments and sections for each step
- Handling cleanup of temporary directories at the end
The script is complex but well-written and follows a logical flow for generating TypeScript code from proto definitions.
Tools
Shellcheck
[warning] 71-71: Quote this to prevent word splitting.
(SC2046)
packages/sdk-ts/src/core/modules/tx/utils/tx.ts (2)
Line range hint
203-216: LGTM!The changes to the
createTxRawEIP712function are consistent with the migration fromInjectiveTypesV1TxExttoInjectiveTypesV1Beta1TxExt. The logic remains the same, and the updated type is used correctly for theextensionparameter.
Line range hint
227-239: LGTM!The changes to the
createWeb3Extensionfunction are consistent with the migration fromInjectiveTypesV1TxExttoInjectiveTypesV1Beta1TxExt. The logic remains the same, and the updated type is used correctly for creating theweb3Extension.packages/sdk-ts/src/core/accounts/PrivateKey.ts (2)
12-12: LGTM!The import statement update from
InjectiveTypesV1TxExttoInjectiveTypesV1Beta1TxExtaligns with the semantic versioning convention and is unlikely to cause breaking changes.
363-363: Verify the consistency of the type update across the codebase.The usage of
InjectiveTypesV1Beta1TxExtfor decoding the extension value is consistent with the import statement update. However, please ensure that all other occurrences ofInjectiveTypesV1TxExthave been consistently updated toInjectiveTypesV1Beta1TxExtto maintain coherence and avoid potential issues.Run the following script to verify the consistency of the type update:
Verification successful
Type update consistently applied across the codebase
The verification process confirms that the update from
InjectiveTypesV1TxExttoInjectiveTypesV1Beta1TxExthas been consistently applied throughout the codebase. No occurrences of the old type were found, and the new type is correctly used in both relevant files:PrivateKey.tsandtx.ts. This consistency ensures that the codebase maintains coherence and avoids potential issues related to type mismatches.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all occurrences of `InjectiveTypesV1TxExt` have been updated to `InjectiveTypesV1Beta1TxExt`. # Test 1: Search for occurrences of `InjectiveTypesV1TxExt`. Expect: No results found. rg --type ts 'InjectiveTypesV1TxExt' # Test 2: Search for occurrences of `InjectiveTypesV1Beta1TxExt`. Expect: At least one result found. rg --type ts 'InjectiveTypesV1Beta1TxExt'Length of output: 737
proto/core/stub/index.ts.template (4)
107-108: Typo in 'IbcLightcients' remainsThe typo in the module names on lines 107 and 108 is still present. It should be
IbcLightclientsinstead ofIbcLightcients.
153-153: Ensure consistent versioning in export namesThe export on line 153 is missing the
V1Beta1version information, which is included in other exports for consistency.
24-25: Verify if the commented-out exports are intentionalLines 24 and 25 contain exports that are commented out. Please verify if these modules are intentionally excluded or if they should be included.
You can run the following script to check if any code in the codebase imports these modules:
#!/bin/bash # Description: Search for usages of the commented-out modules. # Expected result: No files should import these modules. rg 'from ".*\/capability\/v1\/' --type=typescript
85-89: Confirm the exclusion of IbcApplicationsTransferV2 modulesThe exports for
IbcApplicationsTransferV2modules are commented out in lines 85 to 89. Please confirm whether these modules are intentionally excluded or should be included in the exports.You can run the following script to check if these modules are used elsewhere in the codebase:
Verification successful
Exclusion of IbcApplicationsTransferV2 modules confirmed
After a thorough search of the codebase, it appears that the exclusion of IbcApplicationsTransferV2 modules is intentional. Our verification process included searching for:
- Imports of these modules
- Any mentions of the path 'ibc/applications/transfer/v2'
- Case-insensitive references to 'IbcApplicationsTransferV2'
None of these searches returned any results, indicating that these modules are not used anywhere in the codebase. Therefore, commenting out the exports for these modules (lines 85-89) is appropriate and doesn't seem to impact the rest of the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for imports of the IbcApplicationsTransferV2 modules. # Expected result: No files should import these modules. rg 'from ".*\/ibc\/applications\/transfer\/v2\/' --type=typescriptLength of output: 246
Script:
#!/bin/bash # Description: Search for imports and references of the IbcApplicationsTransferV2 modules. # Search for imports echo "Searching for imports of IbcApplicationsTransferV2 modules:" rg 'from ".*\/ibc\/applications\/transfer\/v2\/' --type=typescript # Search for any references to IbcApplicationsTransferV2 echo "Searching for references to IbcApplicationsTransferV2 modules:" rg 'IbcApplicationsTransferV2' --type=typescriptLength of output: 534
Script:
#!/bin/bash # Description: Search for imports and references of the IbcApplicationsTransferV2 modules. # Search for imports echo "Searching for imports of IbcApplicationsTransferV2 modules:" rg 'from ".*\/ibc\/applications\/transfer\/v2\/' --type-add 'ts:*.ts,*.tsx' -t ts # Search for any references to IbcApplicationsTransferV2 echo "Searching for references to IbcApplicationsTransferV2 modules:" rg 'IbcApplicationsTransferV2' --type-add 'ts:*.ts,*.tsx' -t ts # If the above searches don't yield results, search for any mentions of ibc/applications/transfer/v2 echo "Searching for any mentions of ibc/applications/transfer/v2:" rg 'ibc\/applications\/transfer\/v2' --type-add 'ts:*.ts,*.tsx' -t tsLength of output: 831
Script:
#!/bin/bash # Description: Final search for imports and references of the IbcApplicationsTransferV2 modules. # Search for imports echo "Searching for imports of IbcApplicationsTransferV2 modules:" rg 'from ".*/ibc/applications/transfer/v2/' --type-add 'ts:*.ts,*.tsx' -t ts # Search for any mentions of ibc/applications/transfer/v2 echo "Searching for any mentions of ibc/applications/transfer/v2:" rg 'ibc/applications/transfer/v2' --type-add 'ts:*.ts,*.tsx' -t ts # Search for any mentions of IbcApplicationsTransferV2 (case-insensitive) echo "Searching for any mentions of IbcApplicationsTransferV2 (case-insensitive):" rg -i 'ibcapplicationstransferv2' --type-add 'ts:*.ts,*.tsx' -t tsLength of output: 631
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
README.mdfile detailing the proto generation process and contribution guidelines.Chores
.gitignoreto exclude specific directories from version control.