fix: allow passing grpc options into client classes #612
Conversation
📝 WalkthroughWalkthroughAdds typed GrpcWebTransport options and centralized lazy gRPC client initialization; transports are recreated when metadata changes (invalidating cached clients); consolidates proto build scripts into shared _scripts and updates proto output paths; bumps multiple package versions. Changes
Sequence Diagram(s)mermaid Note over Consumer,Transport: Construction / options flow Note over Consumer,ClientClass: Lazy initialization Note over Consumer: Metadata update path Consumer->>ClientClass: client.request(...) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @packages/sdk-ts/src/client/chain/grpc/ChainGrpcTxFeesApi.ts:
- Line 6: AbacusGrpcApi currently extends BaseGrpcConsumer but lacks the new
transport options pattern; import GrpcWebTransportAdditionalOptions, change the
class constructor to accept constructor(endpoint: string, options?:
GrpcWebTransportAdditionalOptions), and pass options into the superclass by
calling super(endpoint, options) so AbacusGrpcApi matches the other gRPC API
classes' refactor.
In @packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.ts:
- Line 6: Update the AbacusGrpcApi class constructor to match the other gRPC API
classes by adding the optional GrpcWebTransportAdditionalOptions parameter:
change the constructor signature from constructor(endpoint: string) to include
(endpoint: string, options?: GrpcWebTransportAdditionalOptions), import the
GrpcWebTransportAdditionalOptions type if missing, and pass the options through
to any transport creation or initialization code inside AbacusGrpcApi so it
behaves consistently with the other Chain/Indexer gRPC API implementations.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-ts/src/client/wasm/neptune/service.ts (1)
29-29: Fix typo in comment.There's an extra space in "instan ce" that should be "instance".
🔎 Proposed fix
- * Constructs a new NeptuneService instan ce. + * Constructs a new NeptuneService instance.
🧹 Nitpick comments (2)
packages/sdk-ts/src/client/wasm/neptune/service.ts (1)
28-37: Update JSDoc to document the new options parameter.The JSDoc comment should include documentation for the new
optionsparameter to help users understand how to customize gRPC transport behavior.🔎 Proposed documentation update
/** * Constructs a new NeptuneService instance. * @param network The network to use (default: Mainnet). * @param endpoints Optional custom network endpoints. + * @param options Optional gRPC web transport configuration options. */packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.ts (1)
15-16: LGTM! Consistent with the PR-wide pattern.The constructor correctly accepts and forwards the optional gRPC transport options. The implementation is backwards-compatible and follows the same clean pattern applied across all API clients in this PR.
Optional: Consider adding JSDoc for the new parameter
While not required, documenting the
optionsparameter could help future developers understand its purpose:+ /** + * @param endpoint - The gRPC endpoint URL + * @param options - Optional gRPC-Web transport configuration + */ constructor(endpoint: string, options?: GrpcWebTransportAdditionalOptions) {This suggestion applies to all API classes modified in this PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.tspackages/sdk-ts/src/client/base/GrpcWebRpcTransport.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcDistributionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcIbcApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcStakingApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTxFeesApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcArchiverApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcPortfolioApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcSpotApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcWeb3GwApi.tspackages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.tspackages/sdk-ts/src/client/wasm/neptune/service.tspackages/sdk-ts/src/utils/grpc.ts
🧰 Additional context used
🧬 Code graph analysis (32)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.ts (3)
packages/sdk-ts/src/client/base/BaseIndexerGrpcConsumer.ts (1)
BaseIndexerGrpcConsumer(8-11)packages/sdk-ts/src/client/indexer/types/index.ts (1)
IndexerModule(32-32)packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (1)
GrpcWebRpcTransport(21-61)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.ts (3)
packages/sdk-ts/src/client/olp/grpc/index.ts (2)
OLPGrpcApi(3-3)OLPGrpcApi(3-3)packages/sdk-ts/src/client/base/BaseIndexerGrpcConsumer.ts (1)
BaseIndexerGrpcConsumer(8-11)packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.ts (2)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (1)
BaseGrpcConsumer(16-203)packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (1)
packages/sdk-ts/src/utils/grpc.ts (1)
GrpcWebTransportAdditionalOptions(30-30)
🔇 Additional comments (44)
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.ts (1)
8-8: LGTM! Constructor correctly extended to support transport options.The import and constructor signature changes properly thread the optional
GrpcWebTransportAdditionalOptionsthrough to the base class, enabling customization of gRPC transport behavior without breaking existing code.Also applies to: 17-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.ts (1)
6-6: LGTM! Consistent implementation of transport options support.The changes follow the same correct pattern as other API classes, properly forwarding transport options to the base consumer.
Also applies to: 15-16
packages/sdk-ts/src/client/chain/grpc/ChainGrpcIbcApi.ts (1)
7-7: LGTM! Transport options correctly propagated.The implementation maintains consistency with the pattern established across other Chain gRPC API classes.
Also applies to: 16-17
packages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.ts (1)
6-6: LGTM! Consistent pattern applied.The changes correctly enable transport options support following the established pattern.
Also applies to: 15-16
packages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.ts (1)
9-9: LGTM! Clean implementation of optional transport options.The addition of the
GrpcWebTransportAdditionalOptionsparameter is well-implemented:
- Type-only import keeps the bundle size optimized
- Optional parameter maintains backward compatibility
- Proper forwarding to the base class
Also applies to: 18-19
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.ts (1)
5-6: LGTM! Consistent with the pattern across other API clients.The implementation correctly adds transport options support following the same pattern as other gRPC API classes.
Also applies to: 14-15
packages/sdk-ts/src/utils/grpc.ts (3)
1-8: LGTM! Well-designed type definition.The
GrpcWebTransportAdditionalOptionstype correctly omitsbaseUrlsince it's provided separately as the first parameter togetGrpcWebTransport. The use of type-only import forGrpcWebOptionsis also appropriate for TypeScript optimization.
17-25: LGTM! Clean implementation with proper option spreading.The spread operator usage (
...options) safely handles both defined and undefined cases, making the options parameter truly optional without additional null checks.
30-30: LGTM! Appropriate re-exports for public API.Re-exporting both the type and the transport class provides a clean API surface for consumers of this module.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.ts (1)
8-8: LGTM! Consistent implementation across indexer APIs.The changes follow the same pattern as the chain gRPC APIs, maintaining consistency across the SDK. The optional parameter ensures backward compatibility with existing code.
Also applies to: 17-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.ts (1)
10-10: LGTM! Completes the consistent pattern across all gRPC clients.The implementation aligns perfectly with the other API classes in this PR, providing a uniform interface for transport customization throughout the SDK.
Also applies to: 19-20
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.ts (1)
7-7: LGTM! Clean implementation of transport options support.The type-only import and optional parameter correctly enable gRPC transport customization while maintaining backward compatibility.
Also applies to: 25-26
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.ts (1)
6-6: LGTM! Consistent with the transport options pattern.The implementation correctly follows the same pattern established across the SDK.
Also applies to: 15-16
packages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.ts (1)
8-8: LGTM! Proper extension of Chain gRPC API with transport options.The implementation correctly integrates with
BaseGrpcConsumerusing the same pattern as the Indexer APIs.Also applies to: 17-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.ts (1)
6-6: LGTM! Clean implementation.The changes properly enable transport options for the Auction API.
Also applies to: 16-17
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.ts (1)
26-26: LGTM! Constructor correctly extended to support optional gRPC transport options.The import and constructor changes properly enable custom transport configuration while maintaining backward compatibility.
Also applies to: 49-50
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.ts (1)
6-6: LGTM! Constructor changes align with the PR objective.The implementation correctly threads optional gRPC transport options through to the base class.
Also applies to: 16-17
packages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.ts (1)
6-6: LGTM! Consistent implementation of optional transport options.The constructor extension follows the same pattern as other gRPC API clients in this PR.
Also applies to: 15-16
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.ts (1)
6-7: LGTM! Transport options support properly added.The constructor changes maintain consistency with the broader pattern across the SDK's gRPC clients.
Also applies to: 15-16
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.ts (1)
6-6: LGTM! Constructor properly extended for transport customization.The implementation is consistent with the pattern established across all gRPC API clients in this PR.
Also applies to: 15-16
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcWeb3GwApi.ts (1)
13-13: LGTM! Clean implementation of optional transport options.The import and constructor changes correctly enable optional gRPC transport customization while maintaining backward compatibility.
Also applies to: 21-22
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcArchiverApi.ts (1)
6-6: LGTM! Consistent with the pattern.The changes properly extend the Archiver API to accept optional transport options and forward them to the base consumer.
Also applies to: 15-16
packages/sdk-ts/src/client/chain/grpc/ChainGrpcStakingApi.ts (1)
8-8: LGTM! Follows the established pattern.The staking API correctly implements optional transport options support.
Also applies to: 17-18
packages/sdk-ts/src/client/wasm/neptune/service.ts (1)
43-43: LGTM! Options properly forwarded.The ChainGrpcWasmApi correctly receives the transport options.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.ts (1)
6-6: LGTM! Consistent implementation.The EVM API correctly implements the optional transport options pattern, maintaining consistency across all Chain gRPC APIs.
Also applies to: 16-17
packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.ts (1)
14-15: LGTM! Clean implementation of optional gRPC options.The constructor correctly accepts and forwards the optional
GrpcWebTransportAdditionalOptionsparameter to the base class, maintaining backwards compatibility while enabling transport customization.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.ts (1)
14-15: LGTM! Consistent implementation.The constructor properly threads the optional gRPC transport options through to the base class, enabling callers to customize transport behavior while maintaining backwards compatibility.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.ts (1)
16-17: LGTM! Well-executed pattern.The optional
optionsparameter is correctly forwarded to the base class constructor, maintaining the API's backwards compatibility while enabling advanced transport configuration.packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts (1)
17-18: LGTM! Proper implementation.The constructor change correctly accepts and propagates the optional
GrpcWebTransportAdditionalOptions, allowing transport customization without breaking existing code.packages/sdk-ts/src/client/chain/grpc/ChainGrpcDistributionApi.ts (1)
8-8: LGTM! Clean implementation of transport options support.The addition of the optional
GrpcWebTransportAdditionalOptionsparameter follows a clean pattern: type-only import and proper forwarding to the base class. This maintains backward compatibility while enabling transport customization.Also applies to: 17-18
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.ts (1)
6-6: LGTM! Consistent implementation across indexer APIs.The changes mirror the pattern used in chain gRPC APIs, maintaining consistency across the SDK. The type-only import and optional parameter ensure backward compatibility.
Also applies to: 15-16
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcSpotApi.ts (1)
7-7: LGTM! Pattern applied consistently.The implementation follows the established pattern for adding transport options support. Clean and straightforward.
Also applies to: 25-26
packages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.ts (1)
9-9: LGTM! Uniform implementation.The changes are consistent with other chain gRPC API classes, maintaining a uniform API surface across the SDK.
Also applies to: 20-21
packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (2)
1-4: LGTM! Improved type consistency.Replacing the inline type definition with the imported
GrpcWebTransportAdditionalOptionstype centralizes the type definition and ensures consistency across the SDK. This is a good refactoring practice.Also applies to: 24-25
24-25: Base class integration verified—no issues found.BaseGrpcConsumer properly accepts and handles the
optionsparameter: it's stored as a protected property (line 21), passed to GrpcWebRpcTransport in the constructor (line 26), and preserved when recreating transport instances in setMetadata() via spread operator (line 33). BaseIndexerGrpcConsumer inherits this correct behavior.packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
10-10: LGTM! Clean options threading.The addition of
GrpcWebTransportAdditionalOptionssupport is well implemented: the type import is appropriately marked as type-only, the options are stored as a protected property, and correctly passed through to theGrpcWebRpcTransportconstructor.Also applies to: 21-21, 23-27
31-36: Options preservation logic is correct.The spreading pattern
{ ...this.options, meta: this.metadata }correctly preserves non-metadata options (like timeout, baseUrl, etc.) from the initial construction while replacing any existing metadata with the new value. This is the expected behavior for asetMetadataoperation.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcPortfolioApi.ts (1)
6-6: LGTM! Consistent pattern applied.The constructor correctly accepts optional
GrpcWebTransportAdditionalOptionsand forwards them to the base class. The type-only import and backward-compatible signature are appropriate.Also applies to: 15-18
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.ts (1)
6-6: LGTM! Options support added consistently.The implementation correctly follows the pattern established in the base class: type-only import, optional parameter in constructor, and proper forwarding to
super().Also applies to: 16-20
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.ts (1)
6-6: LGTM! Pattern correctly applied.Options support added following the same pattern as other API classes. The implementation is correct and backward compatible.
Also applies to: 15-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.ts (1)
6-6: LGTM! Consistent cross-module implementation.The Chain API classes follow the same options pattern as the Indexer API classes, demonstrating good consistency across the SDK. Implementation is correct.
Also applies to: 15-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.ts (1)
8-8: LGTM! Clean addition of transport options support.The type import and constructor changes correctly enable custom gRPC transport configuration while maintaining backward compatibility through the optional parameter.
Also applies to: 17-18
packages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.ts (1)
11-11: LGTM! Consistent implementation of transport options.The changes match the pattern used across other gRPC API classes and correctly propagate the optional transport options to the base consumer.
Also applies to: 23-24
packages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.ts (1)
6-6: LGTM! Transport options properly extended to OLP gRPC API.The implementation correctly adds transport customization capability to the OLP client while maintaining backward compatibility.
Also applies to: 13-14
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.ts
Outdated
Show resolved
Hide resolved
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
- @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected] - @injectivelabs/[email protected]
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts:
- Around line 30-38: The cached _client in BaseGrpcConsumer isn't invalidated
when setMetadata recreates the transport, so future requests still use the old
transport; update setMetadata to clear/invalidate the cached _client (e.g., set
this._client = undefined/null) after replacing this.transport so the lazy getter
will rebuild the client with the new GrpcWebRpcTransport (constructed with
this.endpoint, this.options, meta: this.metadata).
🧹 Nitpick comments (2)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
48-64: Consider documenting the single-client constraint.The
initClientimplementation assumes each subclass uses exactly one client type (via the getter pattern). While this is safe with the current usage, consider adding a comment to make this constraint explicit and prevent future misuse.🔎 Suggested documentation enhancement
/** * Lazily initializes and returns the gRPC client. * Call this from a getter in subclasses to avoid constructor boilerplate. + * + * Note: This method caches a single client instance. Each subclass should + * only use one client type. The recommended pattern is a single private getter: * * @example * private get client() { * return this.initClient(MyGrpcClient) * } */ protected initClient<TClient>( ClientClass: new (transport: GrpcWebRpcTransport) => TClient, ): TClient { if (!this._client) { this._client = new ClientClass(this.transport) } return this._client as TClient }
24-27: Consider the alternative API design suggested in past review.A previous review suggested combining options and metadata into a single
setMetadatamethod with a unifiedGrpcMetadataOptionstype, rather than passing options through the constructor. While the current constructor-based approach is valid, the setter-based pattern might offer a more fluent API if options are typically set alongside metadata.Based on past review comments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
packages/sdk-ts/src/client/abacus/grpc/AbacusGrpcApi.tspackages/sdk-ts/src/client/base/BaseGrpcConsumer.tspackages/sdk-ts/src/client/base/GrpcWebRpcTransport.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcDistributionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcIbcApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcStakingApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTxFeesApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcArchiverApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcPortfolioApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcSpotApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcWeb3GwApi.tspackages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.tspackages/sdk-ts/src/client/wasm/neptune/service.tspackages/sdk-ts/src/core/tx/api/TxGrpcApi.tspackages/sdk-ts/src/types/grpc.tspackages/sdk-ts/src/types/index.tspackages/sdk-ts/src/utils/grpc.ts
💤 Files with no reviewable changes (1)
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcWeb3GwApi.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.ts
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.ts
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.ts
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.ts
- packages/sdk-ts/src/client/abacus/grpc/AbacusGrpcApi.ts
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcSpotApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcDistributionApi.ts
- packages/sdk-ts/src/client/wasm/neptune/service.ts
- packages/sdk-ts/src/utils/grpc.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.ts
- packages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.ts
- packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
packages/sdk-ts/src/types/grpc.ts (1)
GrpcWebTransportAdditionalOptions(3-3)packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (1)
GrpcWebRpcTransport(19-59)
⏰ 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: Publish
🔇 Additional comments (30)
packages/sdk-ts/src/types/index.ts (1)
4-4: LGTM!The type-only export is correctly used to expose the types from
grpc.js. The placement is consistent with other type exports in the file.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.ts (1)
45-47: LGTM! Same caching verification applies.The lazy initialization pattern is consistent with the broader PR changes. As noted in the review of
IndexerGrpcMegaVaultApi.ts, please ensure thatinitClientcaches client instances to avoid creating a new client on each access.packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (1)
11-11: LGTM! Type refactoring is compatible withgetGrpcWebTransport.The refactoring to use the named
GrpcWebTransportAdditionalOptionstype improves consistency and reusability. ThegetGrpcWebTransportfunction signature inpackages/sdk-ts/src/utils/grpc.tscorrectly accepts this type for its options parameter, ensuring full type compatibility.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.ts (1)
12-14: The lazy initialization pattern withthis.initClient(InjectiveMegavaultRPCClient)is correctly implemented. The base classBaseGrpcConsumerproperly caches the client instance in a private_clientfield, returning the same instance on subsequent calls rather than creating a new client on each property access.packages/sdk-ts/src/types/grpc.ts (1)
1-3: The type definition is correct.The
@protobuf-ts/grpcweb-transportlibrary does exportGrpcWebOptionswith abaseUrlproperty, confirming that theOmitapproach is appropriate. The implementation properly excludesbaseUrl, suggesting it's handled separately by the transport layer.packages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.ts (1)
12-14: Lazy initialization pattern looks good.The lazy getter pattern is consistent with the broader refactor across gRPC API classes. Assuming
initClientproperly memoizes (flagged for verification in ChainGrpcTokenFactoryApi), this change correctly defers client creation until first use.packages/sdk-ts/src/client/chain/grpc/ChainGrpcIbcApi.ts (1)
13-15: Lazy initialization pattern is consistent.The implementation correctly follows the lazy initialization pattern used across other ChainGrpc API classes in this PR.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.ts (1)
11-13: Lazy initialization pattern is consistent.The implementation correctly follows the same lazy initialization pattern as other ChainGrpc API classes, deferring client creation until first access.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.ts (1)
12-14: No action needed —BaseGrpcConsumer.initClientalready implements proper memoization.
BaseIndexerGrpcConsumerextendsBaseGrpcConsumer, which implementsinitClientwith client caching via the private_clientproperty (BaseGrpcConsumer.ts, lines 60-63). The client is instantiated once and reused on subsequent getter calls.Likely an incorrect or invalid review comment.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.ts (1)
12-14: No action required—initClientproperly memoizes client instances.The
BaseGrpcConsumer.initClientmethod implements caching via a private_clientfield (line 22) that stores the singleton client instance. The method checksif (!this._client)before creating a new instance (lines 60–62), ensuring subsequent calls return the cached client. Multiple accesses to the lazy getter (this.client) within a single method will safely reuse the same memoized instance without performance degradation.packages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.ts (1)
14-16: LGTM! Lazy initialization pattern correctly applied.The lazy getter pattern defers client instantiation until first use, improving performance and memory efficiency. The implementation is clean and consistent with the broader refactoring across gRPC API classes.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcPortfolioApi.ts (1)
12-14: LGTM! Consistent lazy initialization.The lazy getter pattern is correctly applied and aligns with the refactoring across both chain and indexer gRPC APIs.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts (1)
14-16: LGTM! Lazy initialization applied correctly.The getter-based lazy initialization is well-implemented and maintains compatibility with all existing method calls throughout the class.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcTxFeesApi.ts (1)
12-14: LGTM! Lazy initialization correctly implemented.The lazy getter pattern is properly applied and consistent with the broader refactoring effort across all gRPC API classes.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcStakingApi.ts (1)
14-16: LGTM! Lazy initialization pattern correctly applied.The lazy getter approach is well-implemented and maintains compatibility with all method calls throughout this class. The pattern is consistent with the broader refactoring across all gRPC API classes.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.ts (1)
12-14: LGTM!The lazy client initialization is correctly implemented and consistent with the pattern applied across all gRPC API classes in this PR.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.ts (1)
12-14: LGTM!The lazy client initialization is correctly implemented and follows the same pattern applied consistently across all gRPC API classes in this PR.
packages/sdk-ts/src/core/tx/api/TxGrpcApi.ts (1)
26-28: LGTM!The lazy client initialization is correctly implemented and aligns with the pattern consistently applied across all gRPC API classes throughout this PR.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.ts (1)
12-14: LGTM!The lazy client initialization is correctly implemented and maintains consistency with the pattern applied across all gRPC API classes in this PR.
packages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.ts (1)
12-14: Caching behavior is correctly implemented.The
initClientmethod inBaseGrpcConsumerproperly caches the client instance using a private_clientproperty with anif (!this._client)guard, ensuring the client is created only once and reused on subsequent calls. The lazy getter pattern inChainGrpcEvmApiis safe and efficient.packages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.ts (1)
19-21: LGTM - Lazy initialization pattern.The lazy getter correctly delegates to
initClient. AssuminginitClientcaches the client instance (as verified in BaseGrpcConsumer), this approach defers client creation until first access while avoiding repeated instantiation.packages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.ts (1)
10-12: LGTM - Consistent with lazy initialization pattern.The getter properly initializes
InjectiveDmmV2RPCClientviainitClient, deferring instantiation until first access.packages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.ts (1)
14-16: LGTM - Lazy initialization applied correctly.The private getter defers
CosmosMintV1Beta1QueryClientcreation until first access, consistent with the repository-wide refactor.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.ts (1)
12-14: LGTM - Lazy initialization completed.The getter correctly implements the lazy initialization pattern for
InjectiveTradingRPCClient, completing the repository-wide refactor.packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcArchiverApi.ts (1)
12-14: TheinitClientmethod properly caches the client instance, so no performance concerns.The
initClientmethod inBaseGrpcConsumerchecksif (!this._client)before creating a new instance, ensuring the client is instantiated only once and reused on subsequent getter accesses.packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
24-27: LGTM!The constructor correctly accepts optional
GrpcWebTransportAdditionalOptionsand forwards them to the transport layer, enabling configuration of transport-level concerns like timeouts and interceptors while maintaining backward compatibility.
10-10: LGTM!The
GrpcWebTransportAdditionalOptionstype import and protectedoptionsfield are correctly declared and enable proper type-safe forwarding of transport configuration through the class hierarchy.Also applies to: 21-21
packages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.ts (1)
12-14: LGTM!The conversion to lazy client initialization via the getter pattern is clean and consistent with the broader refactoring. The public API remains unchanged, and all RPC method bindings are preserved correctly.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.ts (1)
12-14: LGTM!The lazy client initialization pattern is correctly applied. All auction-related RPC methods maintain their existing bindings and behavior through the getter.
packages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.ts (1)
12-14: LGTM!The lazy initialization pattern is consistently applied across all Mito API methods. The extensive API surface maintains correct bindings and behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts:
- Around line 33-41: setMetadata recreates this.transport but doesn't clear the
cached _client, so initClient keeps returning the old client bound to the stale
transport; fix by invalidating the cached client when recreating the transport
(e.g., set this._client = undefined/null) inside setMetadata after updating
this.transport and before returning, ensuring subsequent calls to initClient
create a new client using the updated metadata/options; update references in
setMetadata, _client, initClient, transport, metadata, and GrpcWebRpcTransport
accordingly.
- Around line 46-48: clearMetadata currently only clears this.metadata but
leaves this.transport and the cached this._client using stale metadata; update
clearMetadata to set this.metadata = undefined, invalidate the cached client by
setting this._client = undefined, and recreate or replace the transport so it no
longer carries old metadata (e.g., call the existing transport
factory/initializer — look for a createTransport/buildTransport/initTransport
method or the transport construction logic used in the constructor — and assign
the new transport to this.transport). Ensure any error handling around transport
recreation matches existing patterns.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts
🔇 Additional comments (2)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
24-28: LGTM!The constructor correctly accepts and stores the gRPC transport options, then passes them to the transport initialization.
54-70: LGTM!The lazy initialization pattern is well-implemented. The method correctly caches the client instance and provides clear usage guidance in the JSDoc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (1)
71-79: Consider type safety implications of shared client cache.The single
_clientcache (typed asunknown) is shared across all potential client types. If a subclass callsinitClient(ClientA)followed byinitClient(ClientB), the second call would incorrectly return the cachedClientAinstance cast toClientB.In practice, the intended usage pattern (each subclass defines a single
clientgetter callinginitClientwith oneClientClass) prevents this issue. However, nothing enforces this at compile time.For stricter type safety, consider alternatives like:
- Subclasses declaring their own typed
_clientfield- Using a
WeakMap<ClientClass, instance>for multi-client scenariosGiven the current usage patterns shown in the PR, the practical risk is low.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.tspackages/sdk-ts/src/types/grpc.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk-ts/src/types/grpc.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (2)
packages/sdk-ts/src/client/base/GrpcWebRpcTransport.ts (1)
GrpcWebRpcTransport(19-59)packages/sdk-ts/src/types/grpc.ts (1)
GrpcWebTransportAdditionalOptions(3-8)
🔇 Additional comments (4)
packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts (4)
10-10: LGTM! Structural changes support lazy initialization pattern.The new import, fields, and access modifiers appropriately support the refactoring to enable passing gRPC options through the constructor and enabling lazy client initialization in subclasses.
Also applies to: 17-18, 20-20, 22-22
24-28: LGTM! Constructor properly threads options to transport.The constructor correctly stores the options and passes them to the
GrpcWebRpcTransport, enabling configuration of gRPC behavior (including metadata) at initialization time while maintaining backward compatibility with the optional parameter.
33-45: LGTM! Past critical issue resolved.The method now correctly invalidates the cached client (line 42) when recreating the transport with new metadata. This ensures subsequent calls to
initClientwill create a fresh client instance using the updated transport, addressing the critical cache staleness issue identified in previous reviews.The implementation properly preserves existing options while applying new metadata.
50-56: LGTM! Past major issue resolved.The method now correctly recreates the transport (line 53) and invalidates the cached client (line 55), ensuring metadata changes take effect properly. This addresses the major issue identified in previous reviews.
The implementation resets to the constructor-provided configuration, which is appropriate given the method's deprecated status and the guidance to manage options via the constructor.
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 (2)
protoV2/test-all-gen.sh (1)
20-20: Critical: Replace hardcoded absolute path with relative path.The hardcoded absolute path
/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkgwill fail on other developer machines and CI environments.🔎 Proposed fix
- cd "/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkg" + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + cd "$SCRIPT_DIR/$pkg"Apply this at the top of the loop to make the path relative to the script location.
protoV2/run-all-gen.sh (1)
9-9: Critical: Replace hardcoded absolute BASE_DIR with relative path.The hardcoded absolute path
/Users/leeruianthomas/Public/injective/injective-ts/protoV2will fail on other developer machines and CI environments.🔎 Proposed fix
-BASE_DIR="/Users/leeruianthomas/Public/injective/injective-ts/protoV2" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +BASE_DIR="$SCRIPT_DIR"
🤖 Fix all issues with AI agents
In @protoV2/core/generate-dts.mjs:
- Around line 1-40: The generate-dts.mjs script duplicates logic present in
protoV2/indexer/generate-dts.mjs and protoV2/mito/generate-dts.mjs; refactor by
extracting the shared logic (traversal and copy behavior implemented in
copyAsDts and its inner traverse, plus the top-level copyFileSync for
index.d.ts) into a single reusable script (e.g.,
protoV2/scripts/generate-dts.mjs) that accepts src/out args, then update each
package to call that script with the appropriate parameters instead of
duplicating the function.
In @protoV2/core/package.json.template:
- Around line 5-23: The package.json template points main/types/files at root
(fields "main", "types", and "files") but the tsup build uses outDir:
'proto-ts', so compiled artifacts live under proto-ts/; fix by aligning outputs:
either change the tsup configuration symbol outDir in tsup.config.ts to '.' so
build emits index.js and index.d.ts to project root, or update the
package.json.template entries ("main", "types", "exports" and "files") to
reference the proto-ts/ subdirectory (e.g., "proto-ts/index.js",
"proto-ts/index.d.ts", "proto-ts/generated") so imports and published files
match the actual build artifacts.
In @protoV2/indexer/tsup.config.ts:
- Line 48: vitest.config.ts still points to the old `/esm` output paths after
tsup changed outDir to `proto-ts`; update the path aliases in vitest.config.ts
so they remove `/esm`: change the alias for
'protoV2/core/proto-ts/esm/generated' to 'protoV2/core/proto-ts/generated',
change 'protoV2/indexer/proto-ts/esm/generated' to
'protoV2/indexer/proto-ts/generated', and change
'protoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs' to
'protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs' so vitest
resolves the generated proto modules correctly.
🧹 Nitpick comments (3)
protoV2/core/fix-imports.mjs (1)
27-29: LGTM! Directory path update aligns with the migration.The change from
proto-ts/esmtoproto-tscorrectly reflects the new output structure described in the broader repository migration.Consider consolidating duplicate fix-imports.mjs scripts.
This script is duplicated across protoV2/core, protoV2/abacus, protoV2/mito, and protoV2/olp with identical logic. Consider moving it to a shared location (e.g.,
protoV2/_scripts/fix-imports.mjs) and invoking it from each package's build process, passing the target directory as an argument.🔎 Proposed consolidation approach
Create
protoV2/_scripts/fix-imports.mjs:+#!/usr/bin/env node +import { readFileSync, writeFileSync, readdirSync, statSync } from 'fs' +import { join, dirname } from 'path' +import { fileURLToPath } from 'url' + +const targetDir = process.argv[2] +if (!targetDir) { + console.error('Usage: fix-imports.mjs <target-directory>') + process.exit(1) +} + +// Recursively find all .js files +function findJsFiles(dir) { + const results = [] + const items = readdirSync(dir) + + for (const item of items) { + const fullPath = join(dir, item) + const stat = statSync(fullPath) + + if (stat.isDirectory()) { + results.push(...findJsFiles(fullPath)) + } else if (item.endsWith('.js')) { + results.push(fullPath) + } + } + + return results +} + +const files = findJsFiles(targetDir) +// ... rest of the logicThen invoke from each package:
node ../../../_scripts/fix-imports.mjs proto-tsprotoV2/olp/tsup.config.ts (1)
1-65: Consider extracting shared tsup configuration.The tsup configuration logic (including the
findFileshelper and entry-building code) is duplicated across multiple protoV2 modules (abacus, core, indexer, mito, olp). Consider extracting this into a shared configuration file to reduce duplication and improve maintainability.For example, create
protoV2/shared/tsup-base.config.ts:export function createProtoConfig(externals: string[]) { // shared findFiles and config generation logic }Then each module's tsup.config.ts could simply:
import { createProtoConfig } from '../shared/tsup-base.config' export default createProtoConfig([ '@protobuf-ts/runtime', '@protobuf-ts/runtime-rpc', '@protobuf-ts/grpcweb-transport', ])protoV2/test-all-gen.sh (1)
56-56: Consider tightening the pattern to match only exact 'proto-ts' output.The current pattern would incorrectly pass for
outDir: 'proto-ts/other'(any non-esm subdirectory). While this works for the current proto-ts vs proto-ts/esm validation, a more precise pattern would prevent false positives.🔎 More precise pattern
- if grep -q "outDir.*'proto-ts'" tsup.config.ts && ! grep -q "outDir.*'proto-ts/esm'" tsup.config.ts; then + if grep -q "outDir.*['\"]proto-ts['\"]" tsup.config.ts; thenThis matches
proto-tsas a complete string (not as a prefix) using word boundaries implicit in the quote characters.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
protoV2/README.mdprotoV2/abacus/fix-imports.mjsprotoV2/abacus/gen.shprotoV2/abacus/generate-dts.mjsprotoV2/abacus/package.json.templateprotoV2/abacus/tsup.config.jsprotoV2/abacus/tsup.config.tsprotoV2/core/fix-imports.mjsprotoV2/core/gen.shprotoV2/core/generate-dts.mjsprotoV2/core/package.json.templateprotoV2/core/tsup.config.jsprotoV2/core/tsup.config.tsprotoV2/indexer/fix-imports.mjsprotoV2/indexer/gen.shprotoV2/indexer/generate-dts.mjsprotoV2/indexer/package.json.templateprotoV2/indexer/tsup.config.jsprotoV2/indexer/tsup.config.tsprotoV2/mito/fix-imports.mjsprotoV2/mito/gen.shprotoV2/mito/generate-dts.mjsprotoV2/mito/package.json.templateprotoV2/mito/tsup.config.jsprotoV2/mito/tsup.config.tsprotoV2/olp/fix-imports.mjsprotoV2/olp/gen.shprotoV2/olp/generate-dts.mjsprotoV2/olp/package.json.templateprotoV2/olp/tsup.config.jsprotoV2/olp/tsup.config.tsprotoV2/run-all-gen.shprotoV2/test-all-gen.sh
💤 Files with no reviewable changes (5)
- protoV2/olp/tsup.config.js
- protoV2/mito/tsup.config.js
- protoV2/core/tsup.config.js
- protoV2/abacus/tsup.config.js
- protoV2/indexer/tsup.config.js
✅ Files skipped from review due to trivial changes (4)
- protoV2/mito/gen.sh
- protoV2/olp/gen.sh
- protoV2/core/gen.sh
- protoV2/abacus/generate-dts.mjs
🧰 Additional context used
🧬 Code graph analysis (7)
protoV2/indexer/generate-dts.mjs (2)
protoV2/abacus/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/core/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)
protoV2/olp/generate-dts.mjs (3)
protoV2/abacus/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/core/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/indexer/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)
protoV2/core/fix-imports.mjs (4)
protoV2/abacus/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/indexer/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/mito/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/olp/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)
protoV2/core/generate-dts.mjs (4)
protoV2/abacus/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/indexer/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/mito/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/olp/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)
protoV2/mito/generate-dts.mjs (2)
protoV2/abacus/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)protoV2/core/generate-dts.mjs (6)
files(11-11)srcPath(14-14)stat(15-15)newOutDir(18-18)dtsFile(22-22)outPath(23-23)
protoV2/indexer/fix-imports.mjs (4)
protoV2/abacus/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/core/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/mito/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)protoV2/olp/fix-imports.mjs (3)
outDir(28-28)__dirname(6-6)files(29-29)
protoV2/abacus/fix-imports.mjs (1)
protoV2/core/fix-imports.mjs (1)
__dirname(6-6)
🔇 Additional comments (16)
protoV2/README.md (2)
78-78: Documentation accurately reflects the new output structure.The build output path update from
proto-ts/esm/toproto-ts/is consistent with the directory structure changes throughout the protoV2 migration.
92-92: Directory structure documentation correctly updated.The output path change from
proto-ts/esm/toproto-ts/accurately documents the new build output location.protoV2/olp/generate-dts.mjs (2)
2-34: LGTM: Core copy logic is correct.The recursive directory traversal and file copying logic is implemented correctly. The path updates from
proto-ts/esmtoproto-tsare consistent throughout.
37-38: package.json.template is correctly updated.The export paths in protoV2/olp/package.json.template correctly align with the changes in generate-dts.mjs. The
mainandtypesfields point to root-level files (./index.jsand./index.d.ts), and the exports configuration properly references./generated/*for the generated files, matching the new output structure that removes theesmsubdirectory level.protoV2/abacus/tsup.config.ts (1)
48-48: LGTM: Output directory updated correctly.The tsup output directory has been changed from
proto-ts/esmtoproto-ts, aligning with the overall restructuring. This change is consistent with the updates to generate-dts.mjs and package.json.template.protoV2/abacus/gen.sh (1)
90-90: LGTM: Console output message updated.The success message correctly reflects the new output directory structure (
proto-ts/instead ofproto-ts/esm/).protoV2/olp/tsup.config.ts (1)
48-48: LGTM: Output directory updated correctly.The tsup output directory has been changed from
proto-ts/esmtoproto-ts, consistent with the restructuring applied across all protoV2 modules.protoV2/abacus/package.json.template (1)
5-24: LGTM: Package structure updated consistently across all modules.All five protoV2 modules (abacus, core, indexer, mito, olp) have received identical, matching updates to their package.json.template files. The export mappings and entry points correctly reflect the new root-level output structure, with consistent configuration of:
- Root-level main/types pointing to
./index.jsand./index.d.ts- Wildcard exports for
./generated/*to allow direct proto imports- Explicit
.jsexport mappingsThe changes are internally consistent and properly structured for ESM.
protoV2/core/tsup.config.ts (1)
48-48: LGTM!The output directory update is consistent with the broader refactoring across protoV2 modules.
protoV2/indexer/gen.sh (1)
87-87: LGTM!The message correctly reflects the updated output directory path.
protoV2/indexer/fix-imports.mjs (1)
27-29: LGTM!The updated scan path is consistent with the tsup config changes and matches the pattern used in other protoV2 modules (abacus, core, mito, olp).
protoV2/mito/tsup.config.ts (1)
48-48: LGTM!The output directory change is consistent with the refactoring across all protoV2 modules.
protoV2/run-all-gen.sh (1)
41-47: Directory structure updates look correct.The changes to check
proto-ts/generatedandproto-ts/*.jsinstead ofproto-ts/esmalign with the broader output directory restructuring. The file counting logic correctly aggregates JavaScript files from both the proto-ts root and the generated subdirectory.Also applies to: 66-74
protoV2/olp/package.json.template (1)
5-23: LGTM! Consistent with the repository-wide packaging refactor.The entry points and exports configuration correctly mirrors the changes in the core package, moving from
esm/subdirectory to root-level paths.protoV2/indexer/package.json.template (1)
5-23: LGTM! Packaging configuration aligns with the root-level output structure.The updates are consistent with the broader protoV2 migration, correctly configuring entry points and exports to reference root index files.
protoV2/mito/package.json.template (1)
5-23: LGTM! Entry points correctly updated to root-level structure.The package configuration properly aligns with the new output directory structure. Note that if external consumers were explicitly importing from
esm/paths (e.g.,@injectivelabs/mito-proto-ts-v2/esm/...), those imports would break, though the primary entry point imports should continue to work.
| #!/usr/bin/env node | ||
| import { readdirSync, statSync, copyFileSync, mkdirSync } from 'fs'; | ||
| import { join, dirname, relative } from 'path'; | ||
| import { readdirSync, statSync, copyFileSync, mkdirSync } from 'fs' | ||
| import { join, relative } from 'path' | ||
|
|
||
| console.log('Generating .d.ts files...'); | ||
| console.log('Generating .d.ts files...') | ||
|
|
||
| // Simply copy the .ts files as .d.ts files | ||
| // Since protobuf-ts generates type-safe code, the .ts files ARE the type definitions | ||
| function copyAsDts(srcDir, outDir) { | ||
| function traverse(currentSrcDir, currentOutDir) { | ||
| const files = readdirSync(currentSrcDir); | ||
| const files = readdirSync(currentSrcDir) | ||
|
|
||
| for (const file of files) { | ||
| const srcPath = join(currentSrcDir, file); | ||
| const stat = statSync(srcPath); | ||
| const srcPath = join(currentSrcDir, file) | ||
| const stat = statSync(srcPath) | ||
|
|
||
| if (stat.isDirectory()) { | ||
| const newOutDir = join(currentOutDir, file); | ||
| mkdirSync(newOutDir, { recursive: true }); | ||
| traverse(srcPath, newOutDir); | ||
| const newOutDir = join(currentOutDir, file) | ||
| mkdirSync(newOutDir, { recursive: true }) | ||
| traverse(srcPath, newOutDir) | ||
| } else if (file.endsWith('.ts') && !file.endsWith('.d.ts')) { | ||
| const dtsFile = file.replace(/\.ts$/, '.d.ts'); | ||
| const outPath = join(currentOutDir, dtsFile); | ||
|
|
||
| console.log(`Copying ${relative('src', srcPath)} -> ${relative('proto-ts/esm', outPath)}`); | ||
| copyFileSync(srcPath, outPath); | ||
| const dtsFile = file.replace(/\.ts$/, '.d.ts') | ||
| const outPath = join(currentOutDir, dtsFile) | ||
|
|
||
| console.log( | ||
| `Copying ${relative('src', srcPath)} -> ${relative('proto-ts', outPath)}`, | ||
| ) | ||
| copyFileSync(srcPath, outPath) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| traverse(srcDir, outDir); | ||
| traverse(srcDir, outDir) | ||
| } | ||
|
|
||
| // Copy src files as .d.ts | ||
| copyAsDts('src/generated', 'proto-ts/esm/generated'); | ||
| copyFileSync('src/index.ts', 'proto-ts/esm/index.d.ts'); | ||
| copyAsDts('src/generated', 'proto-ts/generated') | ||
| copyFileSync('src/index.ts', 'proto-ts/index.d.ts') | ||
|
|
||
| console.log('✅ .d.ts files generated successfully!'); | ||
| console.log('✅ .d.ts files generated successfully!') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Approve path changes; consider consolidating duplicate generate-dts.mjs files.
The output path updates from proto-ts/esm to proto-ts are correct and align with the broader directory restructuring in this PR.
However, this file is nearly identical to protoV2/indexer/generate-dts.mjs and protoV2/mito/generate-dts.mjs. Consider extracting the common logic into a shared utility script that accepts the package paths as parameters.
💡 Suggested consolidation approach
Create a shared protoV2/scripts/generate-dts.mjs that accepts source/destination arguments:
#!/usr/bin/env node
// Usage: node generate-dts.mjs <srcDir> <outDir>
const srcDir = process.argv[2] || 'src/generated'
const outDir = process.argv[3] || 'proto-ts/generated'
// ... rest of the logicThen call it from each package's script:
node ../scripts/generate-dts.mjs src/generated proto-ts/generated🤖 Prompt for AI Agents
In @protoV2/core/generate-dts.mjs around lines 1 - 40, The generate-dts.mjs
script duplicates logic present in protoV2/indexer/generate-dts.mjs and
protoV2/mito/generate-dts.mjs; refactor by extracting the shared logic
(traversal and copy behavior implemented in copyAsDts and its inner traverse,
plus the top-level copyFileSync for index.d.ts) into a single reusable script
(e.g., protoV2/scripts/generate-dts.mjs) that accepts src/out args, then update
each package to call that script with the appropriate parameters instead of
duplicating the function.
protoV2/indexer/tsup.config.ts
Outdated
| { | ||
| entry: entries, | ||
| outDir: 'proto-ts/esm', | ||
| outDir: 'proto-ts', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for any remaining references to the old proto-ts/esm path
rg -n 'proto-ts/esm' --type=ts --type=json --type=sh -g '!*.lock' -g '!node_modules/**' 2>/dev/null || echo "No matches found"Repository: InjectiveLabs/injective-ts
Length of output: 448
🏁 Script executed:
# Also search more broadly in case extensions vary
rg -n 'proto-ts/esm' -g '!*.lock' -g '!node_modules/**' -g '!.git/**' 2>/dev/null | head -50 || echo "No matches found"Repository: InjectiveLabs/injective-ts
Length of output: 448
🏁 Script executed:
# Check the tsup.config.ts file for context
cat -n protoV2/indexer/tsup.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 2074
🏁 Script executed:
# Find and examine package.json files related to proto
find . -name 'package.json' -type f ! -path '*/node_modules/**' ! -path '*/.git/**' | head -20Repository: InjectiveLabs/injective-ts
Length of output: 891
🏁 Script executed:
# Check for references to proto-ts path (without /esm)
rg -n 'proto-ts' --type=json -g '!node_modules/**' -g '!*.lock' 2>/dev/null | head -30Repository: InjectiveLabs/injective-ts
Length of output: 1224
🏁 Script executed:
# Check for core proto tsup.config.ts
find . -path '*/protoV2/core/tsup.config.ts' -type fRepository: InjectiveLabs/injective-ts
Length of output: 99
🏁 Script executed:
# Examine vitest.config.ts alias/path references
sed -n '170,195p' vitest.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 745
🏁 Script executed:
# Check if vitest.config.ts has been updated
rg -n "proto-ts/generated" vitest.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 52
🏁 Script executed:
# Check core proto tsup.config.ts for outDir
grep -n "outDir" protoV2/core/tsup.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 98
🏁 Script executed:
# Get full context of vitest.config.ts around the problematic paths
sed -n '173,195p' vitest.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 680
Update vitest.config.ts path aliases to match new output structure.
The tsup.config.ts change to outDir: 'proto-ts' is correct, but vitest.config.ts still references the old paths. The following aliases must be updated to remove /esm:
- Line 175:
'protoV2/core/proto-ts/esm/generated'→'protoV2/core/proto-ts/generated' - Line 183:
'protoV2/indexer/proto-ts/esm/generated'→'protoV2/indexer/proto-ts/generated' - Line 188:
'protoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs'→'protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs'
Without these updates, vitest will fail to resolve the proto modules since tsup now outputs to proto-ts/generated not proto-ts/esm/generated.
🤖 Prompt for AI Agents
In @protoV2/indexer/tsup.config.ts at line 48, vitest.config.ts still points to
the old `/esm` output paths after tsup changed outDir to `proto-ts`; update the
path aliases in vitest.config.ts so they remove `/esm`: change the alias for
'protoV2/core/proto-ts/esm/generated' to 'protoV2/core/proto-ts/generated',
change 'protoV2/indexer/proto-ts/esm/generated' to
'protoV2/indexer/proto-ts/generated', and change
'protoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs' to
'protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs' so vitest
resolves the generated proto modules correctly.
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)
protoV2/test-all-gen.sh (1)
20-20: Remove hardcoded absolute path.The script contains a hardcoded absolute path
/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkgwhich will break for other developers and CI environments.🔎 Proposed fix
- cd "/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkg" + cd "$(dirname "$0")/$pkg"This makes the path relative to the script's location, ensuring portability across different environments.
🤖 Fix all issues with AI agents
In @protoV2/indexer/tsup.config.ts:
- Around line 1-4: Update vitest.config.ts path aliases to remove the obsolete
"/esm" segment so imports resolve: replace occurrences of
"protoV2/core/proto-ts/esm/generated" with "protoV2/core/proto-ts/generated",
"protoV2/indexer/proto-ts/esm/generated" with
"protoV2/indexer/proto-ts/generated", and
"protoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs" with
"protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs" (these are
the alias entries currently causing vitest resolution failures).
🧹 Nitpick comments (5)
protoV2/_scripts/fix-imports.mjs (1)
66-66: Use path-agnostic string manipulation for cross-platform compatibility.The hardcoded
/separator may not work correctly on Windows systems.🔎 Proposed fix
- const relativePath = fullPath.replace(process.cwd() + '/', '') + const relativePath = fullPath.replace(process.cwd() + path.sep, '')Alternatively, use
path.relative():- const relativePath = fullPath.replace(process.cwd() + '/', '') + const relativePath = path.relative(process.cwd(), fullPath)protoV2/test-all-gen.sh (1)
56-60: Strengthen regex patterns for tsup.config.ts validation.The current grep patterns may produce false positives or negatives depending on formatting (e.g., extra whitespace, different quote styles, multiline configs).
🔎 Alternative approach using more flexible pattern matching
Consider using a more lenient pattern or parsing the TypeScript config:
- if grep -q "createProtoTsupConfig" tsup.config.ts; then + if grep -qE "createProtoTsupConfig|from.*tsup-base.config" tsup.config.ts; then echo "✅ tsup.config.ts uses shared configuration" - elif grep -q "outDir.*['\"]proto-ts['\"]" tsup.config.ts && ! grep -q "outDir.*['\"]proto-ts/esm['\"]" tsup.config.ts; then + elif grep -qE "outDir[[:space:]]*:[[:space:]]*['\"]proto-ts['\"]" tsup.config.ts && ! grep -qE "proto-ts/esm" tsup.config.ts; then echo "✅ tsup.config.ts configured correctly (local config)" else echo "⚠️ Warning: tsup.config.ts might not have correct outDir"This handles varying whitespace more robustly.
protoV2/README.md (1)
83-83: Consider adding language identifier to fenced code block.The fenced code block at line 83 is missing a language identifier. Consider adding
bashor leave it empty with triple backticks for consistency with the rest of the document.🔎 Proposed fix
-``` +```bash protoV2/protoV2/_scripts/generate-dts.mjs (1)
1-46: Approve consolidation; add error handling for robustness.The consolidation of duplicate generate-dts.mjs files into a shared script successfully addresses the past review comment. Multiple packages now reference this shared implementation.
However, the script lacks error handling for file system operations. If source directories don't exist or file operations fail, the build will crash without a clear error message.
🔎 Suggested error handling improvements
+import { existsSync } from 'fs' + console.log('Generating .d.ts files...') +// Validate source directory exists +if (!existsSync('src/generated')) { + console.error('❌ Error: src/generated directory not found. Run from a package directory.') + process.exit(1) +} + // Simply copy the .ts files as .d.ts files // Since protobuf-ts generates type-safe code, the .ts files ARE the type definitions function copyAsDts(srcDir, outDir) { function traverse(currentSrcDir, currentOutDir) { - const files = readdirSync(currentSrcDir) + let files + try { + files = readdirSync(currentSrcDir) + } catch (error) { + console.error(`❌ Error reading directory ${currentSrcDir}:`, error.message) + process.exit(1) + } for (const file of files) { const srcPath = join(currentSrcDir, file) - const stat = statSync(srcPath) + let stat + try { + stat = statSync(srcPath) + } catch (error) { + console.error(`❌ Error accessing ${srcPath}:`, error.message) + process.exit(1) + } if (stat.isDirectory()) { const newOutDir = join(currentOutDir, file) - mkdirSync(newOutDir, { recursive: true }) + try { + mkdirSync(newOutDir, { recursive: true }) + } catch (error) { + console.error(`❌ Error creating directory ${newOutDir}:`, error.message) + process.exit(1) + } traverse(srcPath, newOutDir) } else if (file.endsWith('.ts') && !file.endsWith('.d.ts')) { const dtsFile = file.replace(/\.ts$/, '.d.ts') const outPath = join(currentOutDir, dtsFile) console.log( `Copying ${relative('src', srcPath)} -> ${relative('proto-ts', outPath)}`, ) - copyFileSync(srcPath, outPath) + try { + copyFileSync(srcPath, outPath) + } catch (error) { + console.error(`❌ Error copying ${srcPath} to ${outPath}:`, error.message) + process.exit(1) + } } } }protoV2/_scripts/tsup-base.config.mjs (1)
9-33: Add error handling for file system operations.The
findFiles()function lacks error handling. If the directory doesn't exist, permissions are insufficient, or file operations fail, the build will crash without a clear error message.🔎 Suggested error handling
+import { existsSync } from 'fs' + export function findFiles(dir) { + if (!existsSync(dir)) { + console.error(`❌ Error: Directory ${dir} not found`) + process.exit(1) + } + const result = [] function traverse(currentDir) { - const files = readdirSync(currentDir) + let files + try { + files = readdirSync(currentDir) + } catch (error) { + console.error(`❌ Error reading directory ${currentDir}:`, error.message) + process.exit(1) + } for (const file of files) { const fullPath = path.join(currentDir, file) - const stat = statSync(fullPath) + let stat + try { + stat = statSync(fullPath) + } catch (error) { + console.error(`❌ Error accessing ${fullPath}:`, error.message) + process.exit(1) + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
protoV2/README.mdprotoV2/_scripts/fix-imports.mjsprotoV2/_scripts/generate-dts.mjsprotoV2/_scripts/tsup-base.config.mjsprotoV2/abacus/generate-dts.mjsprotoV2/abacus/package.jsonprotoV2/abacus/tsup.config.tsprotoV2/core/fix-imports.mjsprotoV2/core/generate-dts.mjsprotoV2/core/package.jsonprotoV2/core/tsup.config.tsprotoV2/indexer/fix-imports.mjsprotoV2/indexer/package.jsonprotoV2/indexer/tsup.config.tsprotoV2/mito/fix-imports.mjsprotoV2/mito/generate-dts.mjsprotoV2/mito/package.jsonprotoV2/mito/tsup.config.tsprotoV2/olp/fix-imports.mjsprotoV2/olp/generate-dts.mjsprotoV2/olp/package.jsonprotoV2/olp/tsup.config.tsprotoV2/test-all-gen.sh
💤 Files with no reviewable changes (8)
- protoV2/core/generate-dts.mjs
- protoV2/olp/generate-dts.mjs
- protoV2/core/fix-imports.mjs
- protoV2/indexer/fix-imports.mjs
- protoV2/mito/fix-imports.mjs
- protoV2/olp/fix-imports.mjs
- protoV2/abacus/generate-dts.mjs
- protoV2/mito/generate-dts.mjs
🧰 Additional context used
🧬 Code graph analysis (6)
protoV2/mito/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
protoV2/_scripts/generate-dts.mjs (2)
protoV2/_scripts/fix-imports.mjs (3)
outDir(31-31)files(32-32)stat(18-18)protoV2/_scripts/tsup-base.config.mjs (2)
files(13-13)stat(17-17)
protoV2/core/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
protoV2/_scripts/fix-imports.mjs (2)
protoV2/_scripts/generate-dts.mjs (1)
files(17-17)protoV2/_scripts/tsup-base.config.mjs (2)
files(13-13)fullPath(16-16)
protoV2/olp/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
protoV2/indexer/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
🪛 markdownlint-cli2 (0.18.1)
protoV2/README.md
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
protoV2/mito/package.json (1)
6-7: LGTM! Consolidation to shared build scripts.The migration to centralized scripts in
../_scripts/improves maintainability and reduces duplication across packages.protoV2/olp/package.json (1)
6-7: LGTM! Consistent migration to shared scripts.Build scripts now reference the centralized utilities, maintaining consistency with other protoV2 packages.
protoV2/_scripts/fix-imports.mjs (1)
30-32: The working directory assumption is already correctly implemented and documented. The script's header comment explicitly states "Must be run from a package directory (e.g., protoV2/core/)", and all five package.json scripts (abacus, core, indexer, mito, olp) invokefix-imports.mjsusingnode ../_scripts/fix-imports.mjsfrom their respective package directories. Since npm/pnpm scripts automatically use the package directory as the working directory, theprocess.cwd()call correctly resolves toproto-tswithin each package. No changes required.protoV2/olp/tsup.config.ts (1)
1-4: LGTM! Clean migration to shared configuration.The consolidation to
createProtoTsupConfigeliminates duplication and centralizes build logic. The shared base config properly provides all necessary options for proto packages: it auto-discovers entry points fromsrc/generated, specifies correct external dependencies (@protobuf-ts modules), outputs toproto-tsin ESM format, and intentionally disables DTS generation for performance. The OLP package structure aligns with the expected pattern, wheresrc/generatedis created and populated during the build process viagen.sh.protoV2/README.md (1)
78-78: LGTM! Documentation accurately reflects the build consolidation.The updates correctly document the shift from
npmtopnpm, the new shared scripts location (_scripts/), and the simplified output directory structure (proto-ts/instead ofproto-ts/esm/). These changes align with the code refactoring across the protoV2 packages.Also applies to: 86-88, 95-97
protoV2/core/package.json (1)
6-7: LGTM! Build scripts correctly reference shared utilities.The migration from local scripts to shared
_scripts/generate-dts.mjsand_scripts/fix-imports.mjseliminates duplication and centralizes build tooling across protoV2 packages.protoV2/abacus/tsup.config.ts (1)
1-4: LGTM! Consolidation to shared tsup config reduces duplication.The migration to
createProtoTsupConfigfrom the shared base config eliminates redundant file discovery and entry construction logic. This follows the DRY principle and ensures consistent build configuration across all protoV2 packages.protoV2/core/tsup.config.ts (1)
1-4: LGTM! Consistent with the shared config migration.This change follows the same consolidation pattern as the other protoV2 packages, delegating configuration to the shared
createProtoTsupConfigfunction.protoV2/abacus/package.json (1)
7-8: LGTM! Script consolidation aligns with project-wide refactoring.The updated build scripts correctly reference the shared generate-dts.mjs and fix-imports.mjs from ../_scripts, consistent with the consolidation approach applied across other protoV2 packages.
protoV2/indexer/package.json (1)
6-7: LGTM! Script consolidation aligns with project-wide refactoring.The updated build scripts correctly reference the shared scripts from ../_scripts, consistent with the consolidation approach applied across other protoV2 packages.
protoV2/mito/tsup.config.ts (1)
1-4: LGTM! Tsup configuration successfully consolidated.The migration from inline configuration to the shared base config (
createProtoTsupConfig) eliminates duplication and aligns with the pattern applied across other protoV2 packages. The simplified config correctly delegates entry discovery and build options to the centralized implementation.protoV2/_scripts/tsup-base.config.mjs (1)
40-74: LGTM! Shared tsup configuration is well-structured.The
createProtoTsupConfig()function provides a solid foundation for standardizing tsup configuration across proto packages. The configuration choices are appropriate:
- Disabling DTS generation in tsup (with clear rationale) and handling it separately
- Correct external dependencies for protobuf-ts
- Appropriate bundling settings for proto packages
The robustness depends on adding error handling to
findFiles()as suggested in the previous comment.
| import { defineConfig } from 'tsup' | ||
| import { readdirSync, statSync } from 'fs' | ||
| import { createProtoTsupConfig } from '../_scripts/tsup-base.config.mjs' | ||
|
|
||
| function findFiles(dir: string): string[] { | ||
| const result: string[] = [] | ||
|
|
||
| function traverse(currentDir: string) { | ||
| const files = readdirSync(currentDir) | ||
|
|
||
| for (const file of files) { | ||
| const fullPath = path.join(currentDir, file) | ||
| const stat = statSync(fullPath) | ||
|
|
||
| if (stat.isDirectory()) { | ||
| traverse(fullPath) | ||
| } else if ( | ||
| file.endsWith('.ts') && | ||
| !file.endsWith('.d.ts') && | ||
| !file.endsWith('.spec.ts') | ||
| ) { | ||
| result.push(fullPath) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| traverse(dir) | ||
| return result | ||
| } | ||
|
|
||
| export default defineConfig(() => { | ||
| // Automatically find all generated files | ||
| const generatedFiles = findFiles('src/generated') | ||
|
|
||
| const entries: Record<string, string> = { | ||
| index: 'src/index.ts', | ||
| } | ||
|
|
||
| // Add all generated files as entry points | ||
| generatedFiles.forEach((file: string) => { | ||
| const key = path.relative('src', file).replace(/\.ts$/, '') | ||
| entries[key] = file | ||
| }) | ||
|
|
||
| return [ | ||
| { | ||
| entry: entries, | ||
| outDir: 'proto-ts/esm', | ||
| format: ['esm'], | ||
| dts: false, // Disable DTS generation in tsup (use generate-dts.mjs instead) | ||
| sourcemap: false, | ||
| clean: true, | ||
| splitting: false, | ||
| treeshake: false, | ||
| minify: false, | ||
| bundle: false, | ||
| outExtension: () => ({ js: '.js' }), | ||
| external: [ | ||
| '@protobuf-ts/runtime', | ||
| '@protobuf-ts/runtime-rpc', | ||
| '@protobuf-ts/grpcweb-transport', | ||
| ], | ||
| }, | ||
| ] | ||
| }) | ||
| export default defineConfig(createProtoTsupConfig) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if vitest.config.ts still contains outdated proto-ts/esm paths
# Search for old path patterns in vitest.config.ts
echo "=== Checking for outdated proto-ts/esm paths in vitest.config.ts ==="
rg -n "proto-ts/esm" vitest.config.ts || echo "✓ No proto-ts/esm references found"
echo ""
echo "=== Current proto-ts path references in vitest.config.ts ==="
rg -n "proto-ts" vitest.config.ts || echo "No proto-ts references found"
echo ""
echo "=== Full vitest.config.ts content ==="
cat vitest.config.ts 2>/dev/null || echo "vitest.config.ts not found in current directory"
echo ""
echo "=== Searching for vitest.config.ts in repository ==="
fd vitest.config.tsRepository: InjectiveLabs/injective-ts
Length of output: 6885
Update vitest.config.ts path aliases to remove outdated /esm references.
The tsup.config.ts consolidation is correct. However, vitest.config.ts still contains outdated path aliases referencing proto-ts/esm/generated that must be updated to match the new output structure.
Update lines 175, 183, and 188 in vitest.config.ts to remove /esm from these paths:
protoV2/core/proto-ts/esm/generated→protoV2/core/proto-ts/generatedprotoV2/indexer/proto-ts/esm/generated→protoV2/indexer/proto-ts/generatedprotoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs→protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs
Without this fix, vitest will fail to resolve proto module imports.
🤖 Prompt for AI Agents
In @protoV2/indexer/tsup.config.ts around lines 1 - 4, Update vitest.config.ts
path aliases to remove the obsolete "/esm" segment so imports resolve: replace
occurrences of "protoV2/core/proto-ts/esm/generated" with
"protoV2/core/proto-ts/generated", "protoV2/indexer/proto-ts/esm/generated" with
"protoV2/indexer/proto-ts/generated", and
"protoV2/indexer/proto-ts/esm/generated/injective_exchange_rpc_pb.mjs" with
"protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.mjs" (these are
the alias entries currently causing vitest resolution failures).
ThomasRalee
force-pushed
the
fix/grpc-options-parameter
branch
from
537e225 to
6464da0
Compare
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)
protoV2/test-all-gen.sh (1)
20-20: Remove hardcoded absolute path.The script contains a hardcoded absolute path specific to a single user's machine, making it non-portable and causing failures on other systems.
🔎 Proposed fix
- cd "/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkg" + cd "$(dirname "$0")/$pkg"Or use a relative path from the repository root:
- cd "/Users/leeruianthomas/Public/injective/injective-ts/protoV2/$pkg" + SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" + cd "$SCRIPT_DIR/$pkg"
🤖 Fix all issues with AI agents
In @protoV2/README.md:
- Line 83: The fenced code block in README.md is missing a language specifier;
update the opening backticks for the snippet that shows "protoV2/" (the triple
backtick before the directory listing) to include a language tag such as text or
bash (e.g., change ``` to ```text) so syntax highlighters render it correctly.
In @vitest.config.ts:
- Around line 175-189: Aliases in vitest.config.ts point to missing generated
proto outputs (e.g., '@injectivelabs/indexer-proto-ts-v2/generated',
'protoV2/indexer/proto-ts/generated/injective_exchange_rpc_pb.js') causing
module resolution failures; run the proto generation step (npm run generate) for
each proto package to produce the src/generated outputs (and run the build
scripts like generate-dts.mjs/tsup where required), or temporarily change the
aliases to point to the existing entry files (e.g.,
'protoV2/indexer/src/index.ts') that export from ./generated only after ensuring
./generated exists; verify the presence of the generated directories before
keeping the current aliases or remove/update the alias keys in vitest.config.ts
to match actual files.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
protoV2/README.mdprotoV2/_scripts/fix-imports.mjsprotoV2/_scripts/generate-dts.mjsprotoV2/_scripts/tsup-base.config.mjsprotoV2/abacus/generate-dts.mjsprotoV2/abacus/package.jsonprotoV2/abacus/tsup.config.tsprotoV2/core/fix-imports.mjsprotoV2/core/generate-dts.mjsprotoV2/core/package.jsonprotoV2/core/tsup.config.tsprotoV2/indexer/fix-imports.mjsprotoV2/indexer/package.jsonprotoV2/indexer/tsup.config.tsprotoV2/mito/fix-imports.mjsprotoV2/mito/generate-dts.mjsprotoV2/mito/package.jsonprotoV2/mito/tsup.config.tsprotoV2/olp/fix-imports.mjsprotoV2/olp/generate-dts.mjsprotoV2/olp/package.jsonprotoV2/olp/tsup.config.tsprotoV2/test-all-gen.shvitest.config.ts
💤 Files with no reviewable changes (8)
- protoV2/mito/fix-imports.mjs
- protoV2/olp/fix-imports.mjs
- protoV2/mito/generate-dts.mjs
- protoV2/core/fix-imports.mjs
- protoV2/indexer/fix-imports.mjs
- protoV2/core/generate-dts.mjs
- protoV2/olp/generate-dts.mjs
- protoV2/abacus/generate-dts.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- protoV2/indexer/package.json
- protoV2/_scripts/fix-imports.mjs
- protoV2/core/tsup.config.ts
- protoV2/_scripts/tsup-base.config.mjs
- protoV2/mito/tsup.config.ts
- protoV2/_scripts/generate-dts.mjs
- protoV2/core/package.json
- protoV2/olp/tsup.config.ts
🧰 Additional context used
🧬 Code graph analysis (2)
protoV2/abacus/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
protoV2/indexer/tsup.config.ts (1)
protoV2/_scripts/tsup-base.config.mjs (1)
createProtoTsupConfig(40-74)
🪛 markdownlint-cli2 (0.18.1)
protoV2/README.md
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
protoV2/README.md (1)
14-18: Clarify package manager usage in Quick Start.The Quick Start section uses
npm run generate, but line 78 indicates the build usespnpm run build. Clarify whether the project uses npm or pnpm (or both) for consistency.protoV2/abacus/tsup.config.ts (1)
1-4: LGTM! Clean delegation to shared config.The refactor correctly delegates tsup configuration to the centralized
createProtoTsupConfig, which maintains consistency across proto packages and reduces duplication.protoV2/mito/package.json (1)
6-7: LGTM! Build scripts correctly reference shared utilities.The updated paths to
../_scripts/generate-dts.mjsand../_scripts/fix-imports.mjsare correct for theprotoV2/mito/directory structure and align with the centralization pattern used across other proto packages.protoV2/olp/package.json (1)
6-7: LGTM! Consistent with other proto packages.The script paths correctly reference the shared
_scriptsdirectory, maintaining uniformity across the proto build configurations.protoV2/abacus/package.json (1)
7-8: LGTM! Completes the consistent pattern across proto packages.Build scripts correctly reference the centralized utilities in
../_scripts/, aligning with the changes inmito,olp, and other proto packages for unified build behavior.protoV2/test-all-gen.sh (1)
54-62: Validation logic correctly supports both shared and local configs.The updated conditional logic properly accommodates packages using either the centralized
createProtoTsupConfigor a local configuration with the correctoutDirsetting, aligning with the migration to root-based proto-ts output.
|
|
||
| ## Directory Structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block is missing a language identifier. Add text or bash after the opening backticks.
-```
+```text
protoV2/🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @protoV2/README.md at line 83, The fenced code block in README.md is missing
a language specifier; update the opening backticks for the snippet that shows
"protoV2/" (the triple backtick before the directory listing) to include a
language tag such as text or bash (e.g., change ``` to ```text) so syntax
highlighters render it correctly.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.