chore/indexer-olp-schema-update #613
Conversation
📝 WalkthroughWalkthroughConsolidates generated OLP proto outputs into a unified goagen_olp_injective_dmm_v2 module, updates client imports/exports, adjusts transformer mappings to use Date objects, and makes several proto-derived fields optional; also broadens proto generation source path. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.ts (1)
259-262: Guard the optionalmarketIdparameter before assignment.The
marketIdparameter is optional but assigned directly torequest.marketIdwithout checking if it exists. The established pattern in this file guards optional parameters before assignment (seefetchEpochs,fetchRewardsEligibility), and the test confirms this method is called withoutmarketId. Add the guard to maintain consistency:Suggested fix
request.epochId = epochId - request.marketId = marketId + if (marketId) { + request.marketId = marketId + }
🤖 Fix all issues with AI agents
In `@protoV2/olp/src/index.js`:
- Around line 60-63: The export chain in index.js lists
InjectiveVolatilityRPCClient but that symbol is not assigned anywhere; remove
InjectiveVolatilityRPCClient from the export declaration so the chain only
declares actual exported identifiers (e.g., change the chain to export DmmPb and
InjectiveDmmV2Client only) and search for any leftover references to
InjectiveVolatilityRPCClient in this module to delete or replace them; update
the exports line where exports.DmmPb = exports.InjectiveVolatilityRPCClient =
exports.InjectiveDmmV2Client = void 0 to exclude InjectiveVolatilityRPCClient
(use exports.DmmPb = exports.InjectiveDmmV2Client = void 0) so consumers no
longer receive an undefined export.
- Around line 65-76: Remove the orphaned export for
InjectiveVolatilityRPCClient: delete the export assignment that leaves
InjectiveVolatilityRPCClient undefined (the pattern like exports.DmmPb =
exports.InjectiveDmmV2Client = void 0) and ensure only InjectiveDmmV2Client and
DmmPb remain exported; keep the existing require and Object.defineProperty for
InjectiveDmmV2Client and the __importStar export of DmmPb, but do not declare or
export InjectiveVolatilityRPCClient anywhere in this file.
In `@protoV2/olp/src/index.template.js`:
- Around line 60-63: Remove the leftover undefined export by deleting the
`exports.InjectiveVolatilityRPCClient` entry from the export initialization;
update the export assignment so only `exports.DmmPb` and
`exports.InjectiveDmmV2Client` are initialized (e.g., `exports.DmmPb =
exports.InjectiveDmmV2Client = void 0`) and ensure no other code references
`InjectiveVolatilityRPCClient`.
🧹 Nitpick comments (1)
packages/sdk-ts/src/client/olp/grpc/transformers/index.ts (1)
282-310: Parameter naming inconsistency.The parameter is named
rewardbut it represents a volume object of typeGrpcAccountVolume. Consider renaming tovolumefor clarity and consistency with the method namegrpcAccountVolumesToAccountVolumes.Suggested fix
static grpcAccountVolumesToAccountVolumes( - reward: GrpcAccountVolume, + volume: GrpcAccountVolume, ): AccountVolume { return { - date: reward.date, - volume: reward.volume, - epochId: reward.epochId, + date: volume.date, + volume: volume.volume, + epochId: volume.epochId, // ... rest of fields updated similarly } }
📜 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 (11)
packages/sdk-ts/package.jsonpackages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.tspackages/sdk-ts/src/client/olp/grpc/transformers/index.tspackages/sdk-ts/src/client/olp/grpc/types/index.tsprotoV2/olp/gen.shprotoV2/olp/src/index.d.tsprotoV2/olp/src/index.jsprotoV2/olp/src/index.template.d.tsprotoV2/olp/src/index.template.jsprotoV2/olp/src/index.template.tsprotoV2/olp/src/index.ts
🔇 Additional comments (17)
protoV2/olp/gen.sh (1)
41-41: Verify the broader proto collection scope is intentional.The path changed from
$ROOT_DIR/gen/api/prototo$ROOT_DIR/gen, which now collects all*.protofiles from the entiregendirectory tree instead of just theapi/protosubdirectory. This may include additional proto files that weren't previously part of the generation.Confirm this broader scope is intentional and doesn't inadvertently include unwanted proto files (e.g., test protos, internal protos, deprecated definitions).
packages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.ts (3)
2-3: LGTM - imports updated to new proto module.The import paths correctly reference the new consolidated
goagen_olp_injective_dmm_v2_pbmodule andInjectiveDmmV2Client, aligning with the proto schema migration.
10-12: LGTM - client initialization updated.The client accessor correctly uses the new
InjectiveDmmV2Clienttype.
267-267: LGTM - method binding updated correctly.The method binding to
getMarketRewardsRangeis consistent with the new client interface.packages/sdk-ts/src/client/olp/grpc/types/index.ts (3)
1-1: LGTM - import updated to new proto module.The import path correctly references the new consolidated
goagen_olp_injective_dmm_v2_pbmodule.
52-60: Field is properly declared as optional - no consumer code currently accesses it.The
averageDailyMakerVolumePercentagefield is correctly marked as optional (?: string) in theRewardsEligibilityinterface. A comprehensive search found no consumer code accessing this field, so there are no existing violations to address. TypeScript's type system will enforce proper null-safety for any future code that accesses this field.
81-93: Optional fields inEpochConfigV2are handled correctly — transformer preserves the optional nature through direct assignment and type definitions ensure type safety.The transformer code (
grpcEpochToEpoch) correctly passes throughpermanenceVolumeThresholdandqualifyingVolumeThresholdas optional values without requiring explicit undefined checks. The return typeEpochV2properly referencesEpochConfigV2with these fields marked as optional, maintaining type safety throughout the transform pipeline. No consumer code in the repository directly accesses these fields in a way that would cause unsafe undefined dereferences.packages/sdk-ts/package.json (1)
334-334: LGTM - dependency bump aligns with schema migration.The version bump from 1.17.3 to 1.17.5 aligns with the related changes migrating to the new
goagen_olp_injective_dmm_v2_pbmodule andInjectiveDmmV2Client, which are actively used in the OLP gRPC API implementation.protoV2/olp/src/index.template.d.ts (1)
1-2: No breaking changes detected.The current exports (
InjectiveDmmV2ClientandDmmPb) are correct and already in use throughout the codebase inpackages/sdk-ts/src/client/olp/. Claims of removed exports (InjectiveDmmV2RPCClient,InjectiveVolatilityRPCClient,TimestampPb,VolatilityPb) are not supported—these were either never properly exported or have no internal usage. The consolidated single-client API is functioning as intended.Likely an incorrect or invalid review comment.
protoV2/olp/src/index.template.ts (1)
1-7: LGTM!Template file is consistent with the generated
index.tsoutput.packages/sdk-ts/src/client/olp/grpc/transformers/index.ts (3)
1-1: LGTM!Import updated to use the new consolidated protobuf module.
312-329: LGTM!The
rewardsEligibilityResponseToRewardsEligibilitytransformer correctly adds new fields and maintains consistent Date handling patterns.
34-71: Confirm the date field format from the protobuf schema.The transformer correctly assumes
startDate,endDate,createdAt, andupdatedAtare strings and usesnew Date()directly. However, this cannot be definitively confirmed from the repository alone since the OLP proto definitions are in the external dependency@injectivelabs/olp-proto-ts-v2v1.17.5. Verify that the generatedDmmPb.EpochV2type defines these fields as strings (not protobufTimestampobjects with seconds/nanos fields), as the current pattern would fail silently if those fields were Timestamp message types.protoV2/olp/src/index.d.ts (1)
1-2: LGTM!Declaration file is consistent with the TypeScript implementation.
protoV2/olp/src/index.template.js (1)
65-76: LGTM!The client and namespace exports are correctly wired to the new unified protobuf module.
protoV2/olp/src/index.ts (1)
1-7: Clean consolidation of exports to the new unified protobuf module.The index file now properly re-exports from the consolidated
goagen_olp_injective_dmm_v2_pbmodule. Note that downstream consumers in the codebase (OLPGrpcApi.ts, transformers, types) are already importing directly from the generated modules rather than through these re-exports, so there is no breaking change impact. However, the template files (index.template.jsandindex.template.ts) should be updated to match this consolidated structure if they're used to regenerate the index file.protoV2/olp/src/index.js (1)
3-58: Auto-generated module helpers are standard.These TypeScript-compiled helper functions follow the expected pattern and require no changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
note - additions:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.