Add production field to data messages #135
Conversation
|
Warning Rate limit exceeded@fracek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
packages/protocol/src/client.ts (2)
26-26: Consider making the default timeout configurable.The default timeout is hardcoded. Consider making it configurable through environment variables or configuration options.
-const DEFAULT_TIMEOUT_MS = 45_000; +const DEFAULT_TIMEOUT_MS = Number(process.env.DNA_DEFAULT_TIMEOUT_MS ?? "45000");
150-163: Simplify cursor handling logic and improve error messages.The cursor handling logic is complex and uses multiple assertions without clear error messages. Consider:
- Adding descriptive error messages to assertions
- Extracting the cursor comparison logic to a separate method
- Simplifying the nested conditions
+ function isCursorMatched(endCursor: Cursor, target: Cursor): boolean { + if (endCursor.orderKey !== target.orderKey) return false; + return !target.uniqueKey || target.uniqueKey === endCursor.uniqueKey; + } - assert(value.message.$case === "data"); - assert(decodedMessage._tag === "data"); + assert(value.message.$case === "data", "Expected data message"); + assert(decodedMessage._tag === "data", "Expected data message tag"); - const { orderKey, uniqueKey } = endingCursor; const endCursor = decodedMessage.data.endCursor; + assert(endCursor, "Expected end cursor in data message"); - if (orderKey === endCursor?.orderKey) { - if (!uniqueKey || uniqueKey === endCursor.uniqueKey) { - shouldStop = true; - return { done: false, value: decodedMessage }; - } + if (isCursorMatched(endCursor, endingCursor)) { + shouldStop = true; + return { done: false, value: decodedMessage }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
change/@apibara-protocol-70715784-8c5b-4a67-8852-e732f96e76d6.json(1 hunks)examples/starknet-client/src/main.ts(1 hunks)packages/protocol/proto/stream.proto(2 hunks)packages/protocol/src/client.ts(2 hunks)packages/protocol/src/proto/stream.ts(8 hunks)packages/protocol/src/stream.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- change/@apibara-protocol-70715784-8c5b-4a67-8852-e732f96e76d6.json
🧰 Additional context used
🪛 GitHub Actions: Build
examples/starknet-client/src/main.ts
[error] 83-85: Code formatting issue: Long line needs to be split into multiple lines for better readability
🔇 Additional comments (11)
packages/protocol/src/stream.ts (3)
37-62: LGTM! Well-structured schema definition.The implementation follows the established pattern, properly handles all enum values, and includes appropriate fallbacks.
64-64: LGTM! Consistent type export.The type export correctly follows the established pattern.
144-144: LGTM! Consistent data structure modification.The
productionfield is correctly added to both the schema struct and type definition.Also applies to: 169-169
packages/protocol/src/proto/stream.ts (4)
66-74: LGTM! Well-documented enum definition.The enum values and documentation are clear and consistent with the protobuf definition.
76-92: LGTM! Robust JSON to enum conversion.The function properly handles all input values with appropriate fallback.
94-106: LGTM! Complete enum to JSON conversion.The function handles all enum values with appropriate fallback.
885-885: LGTM! Comprehensive Data interface modifications.The production field is consistently implemented across all related functions with proper defaults and serialization logic.
Also applies to: 902-904, 943-949, 965-965, 983-985, 1002-1002
packages/protocol/proto/stream.proto (2)
109-110: LGTM! Well-defined message field.The production field is properly documented and correctly defined with appropriate field number.
137-144: LGTM! Clear and well-documented enum.The enum values are properly defined and documented with clear explanations.
examples/starknet-client/src/main.ts (1)
78-79: Verify the hardcoded orderKey value.The orderKey has been updated from 800_000n to 1_078_335n. Consider making this value configurable through command-line arguments to improve flexibility.
- orderKey: 1_078_335n, + orderKey: BigInt(args.startingBlock ?? "1078335"),packages/protocol/src/client.ts (1)
171-173: LGTM! Good practice using finally block.The timeout is properly cleared in the finally block, preventing potential memory leaks.
examples/starknet-client/src/main.ts
Outdated
| }, | ||
| }); | ||
|
|
||
| for await (const message of client.streamData(request)) { | ||
| for await (const message of client.streamData(request, { timeout: 40_000 })) { |
There was a problem hiding this comment.
Address the formatting issue and consider extracting the timeout value.
- Split the long line into multiple lines for better readability.
- Consider making the timeout configurable through command-line arguments.
- for await (const message of client.streamData(request, { timeout: 40_000 })) {
+ const streamOptions = {
+ timeout: args.timeout ?? 40_000,
+ };
+ for await (const message of client.streamData(request, streamOptions)) {Add this to the command args:
timeout: {
type: "number",
default: 40_000,
description: "Timeout in milliseconds for data streaming",
},🧰 Tools
🪛 GitHub Actions: Build
[error] 83-85: Code formatting issue: Long line needs to be split into multiple lines for better readability
packages/protocol/src/client.ts
Outdated
| // biome-ignore lint/suspicious/noExplicitAny: any is ok | ||
| const t: Promise<{ done: boolean; value: any }> = new Promise( | ||
| (_, reject) => { | ||
| clock = setTimeout(() => { | ||
| reject(new Error(`No message received in ${timeout}ms`)); | ||
| }, timeout); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for timeout.
- Create a specific error type for timeout errors.
- Include more context in the error message.
+class TimeoutError extends Error {
+ constructor(timeout: number) {
+ super(`No message received in ${timeout}ms`);
+ this.name = 'TimeoutError';
+ }
+}
- reject(new Error(`No message received in ${timeout}ms`));
+ reject(new TimeoutError(timeout));Committable suggestion skipped: line range outside the PR's diff.
fracek
force-pushed
the
update-to-protocol
branch
3 times, most recently
from
8245237 to
1d0247c
Compare
fracek
force-pushed
the
update-to-protocol
branch
from
1d0247c to
5797581
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/protocol/src/stream.test.ts (1)
98-98: Consider adding test cases for other production values.The test correctly verifies encoding/decoding of the "backfill" production value (encoded as 1). Consider adding test cases for other production values to ensure complete coverage of the enum.
Also applies to: 124-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
change/@apibara-protocol-70715784-8c5b-4a67-8852-e732f96e76d6.json(1 hunks)examples/starknet-client/src/main.ts(1 hunks)packages/indexer/src/indexer.test.ts(16 hunks)packages/indexer/src/internal/testing.ts(1 hunks)packages/plugin-drizzle/tests/storage.test.ts(28 hunks)packages/plugin-mongo/tests/storage.test.ts(28 hunks)packages/plugin-sqlite/tests/kv.test.ts(18 hunks)packages/plugin-sqlite/tests/persistence.test.ts(28 hunks)packages/protocol/proto/stream.proto(2 hunks)packages/protocol/src/client.ts(2 hunks)packages/protocol/src/proto/stream.ts(8 hunks)packages/protocol/src/stream.test.ts(2 hunks)packages/protocol/src/stream.ts(3 hunks)packages/protocol/src/testing/client.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- change/@apibara-protocol-70715784-8c5b-4a67-8852-e732f96e76d6.json
- examples/starknet-client/src/main.ts
- packages/protocol/proto/stream.proto
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (19)
packages/protocol/src/testing/client.test.ts (1)
12-16: LGTM! Consistent implementation of the production field.The production field is correctly added to the mock data responses with a consistent value of "backfill".
Also applies to: 49-53
packages/indexer/src/internal/testing.ts (1)
59-59: LGTM! Production field added to mock message generator.The production field is correctly added to the generateMockMessages function, maintaining consistency with other test files.
packages/indexer/src/indexer.test.ts (1)
134-134: LGTM! Comprehensive coverage of production field in test scenarios.The production field is consistently added across all test scenarios, including error cases, maintaining the integrity of the tests while accommodating the new field.
Also applies to: 144-144, 154-154, 171-171, 181-181, 191-191, 201-201, 218-218, 228-228, 238-238, 331-331, 341-341, 351-351, 370-370, 380-380, 390-390
packages/protocol/src/stream.ts (3)
37-64: LGTM! Well-structured enum implementation.The
DataProductionimplementation follows the established pattern, with proper handling of unknown/unrecognized values and clear documentation.
144-144: LGTM! Proper integration of the new field.The
productionfield is correctly added to theDatastructure with the appropriate type.
169-169: LGTM! Type definition properly updated.The
StreamDataResponsetype is correctly updated to include the newproductionfield.packages/protocol/src/client.ts (4)
26-33: LGTM! Well-structured error handling.The timeout implementation follows best practices with:
- A reasonable default timeout value
- A dedicated error class with descriptive messages
41-44: LGTM! Clear interface documentation.The
timeoutoption is properly documented with its unit of measurement.
126-130: LGTM! Robust timeout handling.The timeout implementation is well-structured with:
- Proper default value handling
- Clear error handling using the custom error class
Also applies to: 137-144
146-180: LGTM! Proper resource cleanup.The implementation ensures proper cleanup of timeouts in both success and error cases, while maintaining existing functionality.
packages/protocol/src/proto/stream.ts (5)
66-106: LGTM! Well-documented enum definition.The
DataProductionenum is properly defined with:
- Clear documentation for each value
- Comprehensive JSON conversion functions
885-885: LGTM! Proper default value.The
productionfield is initialized to 0 (UNKNOWN), which is the appropriate default.
902-904: LGTM! Efficient encoding.The
productionfield is encoded efficiently, only when it differs from the default value.
943-949: LGTM! Proper decoding implementation.The
productionfield is correctly decoded with proper type casting.
965-965: LGTM! Complete JSON support.The JSON conversion is properly implemented with:
- Correct use of helper functions
- Proper handling of undefined values
Also applies to: 983-985, 1002-1002
packages/plugin-drizzle/tests/storage.test.ts (1)
Line range hint
702-1162: LGTM! Consistent implementation of the production field.The changes correctly add the
productionfield with "backfill" value to all mock data responses, aligning with the PR objective. The test logic and assertions remain unchanged.packages/plugin-sqlite/tests/kv.test.ts (1)
127-127: LGTM! The changes align with the PR objectives.The addition of the
production: "backfill"field to mock data responses is consistent across all test cases and doesn't affect the existing test logic or assertions.Also applies to: 137-137, 147-147, 164-164, 174-174, 184-184, 194-194, 211-211, 221-221, 305-305, 315-315, 325-325, 342-342, 352-352, 362-362, 372-372, 389-389, 399-399, 479-479, 489-489, 499-499, 516-516, 526-526, 536-536, 546-546, 563-563, 573-573
packages/plugin-sqlite/tests/persistence.test.ts (1)
64-64: LGTM! The changes align with the PR objectives.The addition of the
production: "backfill"field to mock data responses is consistent across all test cases and doesn't affect the existing test logic or assertions.Also applies to: 74-74, 84-84, 101-101, 111-111, 121-121, 131-131, 148-148, 158-158, 168-168, 303-303, 313-313, 323-323, 340-340, 350-350, 360-360, 370-370, 387-387, 397-397, 479-479, 489-489, 499-499, 516-516, 526-526, 536-536, 546-546, 563-563, 573-573
packages/plugin-mongo/tests/storage.test.ts (1)
456-456: LGTM! The changes align with the PR objectives.The addition of the
production: "backfill"field to mock data responses is consistent across all test cases and doesn't affect the existing test logic or assertions.Also applies to: 466-466, 476-476, 493-493, 503-503, 513-513, 523-523, 540-540, 550-550, 560-560, 659-659, 669-669, 679-679, 696-696, 706-706, 716-716, 726-726, 743-743, 753-753, 852-852, 862-862, 872-872, 889-889, 899-899, 909-909, 919-919, 936-936, 946-946
fracek
force-pushed
the
update-to-protocol
branch
from
5797581 to
4f827ba
Compare
No description provided.