feat(ensnode-sdk): define a generic Result type
#1539
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
@greptile review |
Greptile SummaryIntroduced a standardized
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as API Consumer
participant API as ENSNode API
participant Result as Result Module
Note over Client,Result: Success Path
Client->>API: Call operation(params)
API->>Result: resultOk({ valueCode, data })
Result-->>API: ResultOk<ValueType>
API-->>Client: { resultCode: "ok", value: { valueCode, ...data } }
Client->>Client: isResultOk() → true
Client->>Client: Switch on value.valueCode
Note over Client,Result: Error Path (Non-Transient)
Client->>API: Call operation(invalid_params)
API->>Result: resultError({ errorCode, message })
Result-->>API: ResultError<ErrorType>
API-->>Client: { resultCode: "error", value: { errorCode, message } }
Client->>Client: isResultError() → true
Client->>Client: Handle error based on errorCode
Note over Client,Result: Error Path (Transient)
Client->>API: Call operation(params)
API->>Result: errorTransient({ errorCode })
Result->>Result: Add transient: true flag
API->>Result: resultError(transientError)
Result-->>API: ResultError<ErrorTransient<ErrorType>>
API-->>Client: { resultCode: "error", value: { errorCode, transient: true } }
Client->>Client: isResultError() → true
Client->>Client: isErrorTransient() → true
Client->>Client: Retry operation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tk-o
force-pushed
the
feat/ensnode-result-type
branch
from
de55bce to
8969812
Compare
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new generic Result type to the ENSNode SDK, providing a standardized way to represent operation outcomes with both successful and error states. The Result type follows functional programming patterns and includes utilities for type checking and error handling.
Changes:
- Added a new
resultmodule with generic types for representing operation outcomes - Implemented builder functions and type guards for working with Result types
- Added support for marking errors as transient for retry logic
- Included comprehensive tests demonstrating usage patterns
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/result/types.ts | Defines the core Result type system including ResultOk, ResultError, and ErrorTransient types |
| packages/ensnode-sdk/src/shared/result/utils.ts | Provides utility functions for building and type-checking Result instances |
| packages/ensnode-sdk/src/shared/result/utils.test.ts | Comprehensive test suite demonstrating Result type usage patterns |
| packages/ensnode-sdk/src/shared/result/index.ts | Exports all public types and utilities from the result module |
| packages/ensnode-sdk/src/shared/index.ts | Re-exports the result module for SDK consumers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks. Reviewed and shared feedback
Co-authored-by: Copilot <[email protected]> Co-authored-by: Tomasz Kopacki <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return buildResultExampleOpOk("example.eth"); | ||
| } else { | ||
| return buildResultInternalServerError( | ||
| "Invariant violation: random number is not less than 0.5", |
There was a problem hiding this comment.
The error message is confusing. When Math.random() < 0.5 is false, it means the random number is greater than or equal to 0.5. The message "random number is not less than 0.5" is technically correct but awkward. Consider using a clearer message such as "random number is greater than or equal to 0.5" or "random number exceeded threshold".
| "Invariant violation: random number is not less than 0.5", | |
| "Invariant violation: random number is greater than or equal to 0.5", |
| /** | ||
| * Classifies a result returned by an operation. | ||
| */ | ||
| export const ResultCodes = { | ||
| /** | ||
| * Indefinite result. | ||
| */ | ||
| Loading: "loading", | ||
|
|
||
| /** | ||
| * Successful result. | ||
| */ | ||
| Ok: "ok", | ||
|
|
||
| /** | ||
| * Server error: the operation failed due to an unexpected error internally within the server. | ||
| */ | ||
| InternalServerError: "internal-server-error", | ||
|
|
||
| /** | ||
| * Server error: the requested resource was not found. | ||
| */ | ||
| NotFound: "not-found", | ||
|
|
||
| /** | ||
| * Server error: the request was invalid. | ||
| */ | ||
| InvalidRequest: "invalid-request", | ||
|
|
||
| /** | ||
| * Client error: the connection to the server failed. | ||
| */ | ||
| ConnectionError: "connection-error", | ||
|
|
||
| /** | ||
| * Client error: the request timed out. | ||
| */ | ||
| RequestTimeout: "request-timeout", | ||
|
|
||
| /** | ||
| * Client error: received an unrecognized result from the server for an operation. | ||
| */ | ||
| ClientUnrecognizedOperationResult: "client-unrecognized-operation-result", | ||
| } as const; | ||
|
|
||
| /** | ||
| * List of ResultCodes that represent server error results. | ||
| */ | ||
| export const RESULT_CODE_SERVER_ERROR_CODES = [ | ||
| ResultCodes.InternalServerError, | ||
| ResultCodes.NotFound, | ||
| ResultCodes.InvalidRequest, | ||
| ] as const; | ||
|
|
||
| /** | ||
| * List of ResultCodes that represent client error results. | ||
| */ | ||
| export const RESULT_CODE_CLIENT_ERROR_CODES = [ | ||
| ResultCodes.ConnectionError, | ||
| ResultCodes.RequestTimeout, | ||
| ResultCodes.ClientUnrecognizedOperationResult, | ||
| ] as const; | ||
|
|
||
| /** | ||
| * List of all error codes the client can return (client-originated + relayed from server). | ||
| */ | ||
| export const RESULT_CODE_ALL_ERROR_CODES = [ | ||
| ...RESULT_CODE_CLIENT_ERROR_CODES, | ||
| ...RESULT_CODE_SERVER_ERROR_CODES, | ||
| ] as const; | ||
|
|
||
| /** | ||
| * List of all ResultCodes. | ||
| */ | ||
| export const RESULT_CODE_ALL_CODES = [ | ||
| ResultCodes.Loading, | ||
| ResultCodes.Ok, | ||
| ...RESULT_CODE_ALL_ERROR_CODES, | ||
| ] as const; | ||
|
|
||
| /** | ||
| * Classifies a result returned by an operation. | ||
| */ | ||
| export type ResultCode = (typeof ResultCodes)[keyof typeof ResultCodes]; | ||
|
|
||
| /** | ||
| * ResultCode for a result that is not yet determined. | ||
| */ | ||
| export type ResultCodeIndefinite = typeof ResultCodes.Loading; | ||
|
|
||
| /** | ||
| * ResultCode for a result that has been determined. | ||
| */ | ||
| export type ResultCodeDefinite = Exclude<ResultCode, ResultCodeIndefinite>; | ||
|
|
||
| /** | ||
| * ResultCode for an error result that may be determined by the server. | ||
| */ | ||
| export type ResultCodeServerError = (typeof RESULT_CODE_SERVER_ERROR_CODES)[number]; | ||
|
|
||
| /** | ||
| * ResultCode for an error result that may be determined by the client. | ||
| */ | ||
| export type ResultCodeClientError = (typeof RESULT_CODE_CLIENT_ERROR_CODES)[number]; | ||
|
|
||
| /************************************************************ | ||
| * Compile-time helpers to ensure invariants expected of | ||
| * definitions above are maintained and don't become | ||
| * out of sync. | ||
| ************************************************************/ | ||
|
|
||
| export type ExpectTrue<T extends true> = T; | ||
|
|
||
| export type ResultCodesFromList<List extends readonly ResultCode[]> = List[number]; | ||
|
|
||
| export type AssertResultCodeSuperset< | ||
| Union extends ResultCode, | ||
| List extends readonly ResultCode[], | ||
| > = Union extends ResultCodesFromList<List> ? true : false; | ||
|
|
||
| export type AssertResultCodeSubset< | ||
| Union extends ResultCode, | ||
| List extends readonly ResultCode[], | ||
| > = ResultCodesFromList<List> extends Union ? true : false; | ||
|
|
||
| export type AssertResultCodeExact< | ||
| Union extends ResultCode, | ||
| List extends readonly ResultCode[], | ||
| > = AssertResultCodeSuperset<Union, List> extends true | ||
| ? AssertResultCodeSubset<Union, List> extends true | ||
| ? true | ||
| : false | ||
| : false; | ||
|
|
||
| /** | ||
| * Intentionally unused type alias used only for compile-time verification that | ||
| * the `ResultCode` union exactly matches the entries in `RESULT_CODE_ALL_CODES`. | ||
| * If this type ever fails to compile, it indicates that one of the above | ||
| * invariants has been broken and the result code definitions are out of sync. | ||
| */ | ||
| type _CompileTimeCheck_ResultCodeMatchesUnion = ExpectTrue< | ||
| AssertResultCodeExact<ResultCode, typeof RESULT_CODE_ALL_CODES> | ||
| >; |
There was a problem hiding this comment.
The result-code.ts file lacks dedicated test coverage. While the compile-time type assertions provide some validation, runtime tests would be valuable for verifying the exported constants and type guards. Consider adding a result-code.test.ts file to test the structure of ResultCodes, the correctness of the categorized lists (RESULT_CODE_SERVER_ERROR_CODES, RESULT_CODE_CLIENT_ERROR_CODES, RESULT_CODE_ALL_CODES), and any runtime behavior.
| ): ResultInternalServerError => { | ||
| return { | ||
| resultCode: ResultCodes.InternalServerError, | ||
| errorMessage: errorMessage ?? "An unknown internal server error occurred.", |
There was a problem hiding this comment.
The error message parameter uses the nullish coalescing operator which only checks for null or undefined. If an empty string is passed, it will be used instead of the default message, potentially resulting in error results with empty error messages. Consider using a truthy check instead, such as errorMessage || "An unknown internal server error occurred." to fall back to the default message when an empty string is provided.
| ): ResultInvalidRequest => { | ||
| return { | ||
| resultCode: ResultCodes.InvalidRequest, | ||
| errorMessage: errorMessage ?? "Invalid request.", |
There was a problem hiding this comment.
The error message parameter uses the nullish coalescing operator which only checks for null or undefined. If an empty string is passed, it will be used instead of the default message, potentially resulting in error results with empty error messages. Consider using a truthy check instead, such as errorMessage || "Invalid request." to fall back to the default message when an empty string is provided.
| ): ResultConnectionError => { | ||
| return { | ||
| resultCode: ResultCodes.ConnectionError, | ||
| errorMessage: errorMessage ?? "Connection error.", |
There was a problem hiding this comment.
The error message parameter uses the nullish coalescing operator which only checks for null or undefined. If an empty string is passed, it will be used instead of the default message, potentially resulting in error results with empty error messages. Consider using a truthy check instead, such as errorMessage || "Connection error." to fall back to the default message when an empty string is provided.
| "errorMessage" in unrecognizedResult && | ||
| typeof unrecognizedResult.errorMessage === "string" | ||
| ) { | ||
| errorMessage = unrecognizedResult.errorMessage; |
There was a problem hiding this comment.
The extracted errorMessage is not validated for empty strings. If the unrecognizedResult object contains an empty string for errorMessage, it will be used instead of the default message, potentially resulting in an error result with an empty error message. Consider adding a truthy check, such as checking if the errorMessage is not empty before using it.
| errorMessage = unrecognizedResult.errorMessage; | |
| const candidateErrorMessage = unrecognizedResult.errorMessage; | |
| if (candidateErrorMessage.trim().length > 0) { | |
| errorMessage = candidateErrorMessage; | |
| } |
| } catch (error) { | ||
| // handle client-side errors | ||
| if (error === "connection-error") { | ||
| return buildResultConnectionError(); | ||
| } else { | ||
| return buildResultRequestTimeout(); | ||
| } |
There was a problem hiding this comment.
The error handling pattern here is unrealistic. The condition error === "connection-error" suggests errors are thrown as string literals, which is not typical JavaScript/TypeScript practice. While this is example code meant for demonstration, it could mislead developers. Consider using a more realistic pattern such as checking error types or error codes from actual Error objects, or add a comment explaining that this is simplified for demonstration purposes.
| ): ResultNotFound => { | ||
| return { | ||
| resultCode: ResultCodes.NotFound, | ||
| errorMessage: errorMessage ?? "Requested resource not found.", |
There was a problem hiding this comment.
The error message parameter uses the nullish coalescing operator which only checks for null or undefined. If an empty string is passed, it will be used instead of the default message, potentially resulting in error results with empty error messages. Consider using a truthy check instead, such as errorMessage || "Requested resource not found." to fall back to the default message when an empty string is provided.
| ): ResultRequestTimeout => { | ||
| return { | ||
| resultCode: ResultCodes.RequestTimeout, | ||
| errorMessage: errorMessage ?? "Request timed out.", |
There was a problem hiding this comment.
The error message parameter uses the nullish coalescing operator which only checks for null or undefined. If an empty string is passed, it will be used instead of the default message, potentially resulting in error results with empty error messages. Consider using a truthy check instead, such as errorMessage || "Request timed out." to fall back to the default message when an empty string is provided.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive result handling system for the ENSNode SDK. It establishes abstract base interfaces for different result states (Ok, Error, Loading), defines a centralized result code classification system with server and client error variants, provides concrete error result types with builder functions, and includes practical examples demonstrating how to structure server operations, client wrappers, React hooks, and routing patterns using the Result data model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Wrapper as op-client<br/>(callExampleOp)
participant Server as op-server<br/>(exampleOp)
participant ErrorHandler as Error Handler
Client->>Wrapper: callExampleOp(address)
Wrapper->>Server: exampleOp(address)
alt Valid Address
Server-->>Wrapper: Ok or InternalServerError
Wrapper->>Wrapper: isRecognized?
alt Recognized ResultCode
Wrapper-->>Client: ExampleOpServerResult
else Unrecognized Code
Wrapper-->>Client: ClientUnrecognizedOperationResult
end
else Invalid Address (zero)
Server-->>Wrapper: InvalidRequest
Wrapper-->>Client: InvalidRequest
else Exception Thrown
ErrorHandler->>ErrorHandler: Catch error
alt Connection Error
ErrorHandler-->>Client: ResultConnectionError
else Request Timeout
ErrorHandler-->>Client: ResultRequestTimeout
end
end
sequenceDiagram
participant DXHook as dx-hook<br/>(myExampleDXHook)
participant Hook as op-hook<br/>(useExampleOp)
participant Wrapper as op-client<br/>(callExampleOp)
participant Server as op-server<br/>(exampleOp)
DXHook->>Hook: useExampleOp(address)
alt Random: Return Loading
Hook->>Hook: buildResultExampleOpLoading()
Hook-->>DXHook: ResultExampleOpLoading
DXHook->>DXHook: Log loading state
else Random: Return Result
Hook->>Wrapper: callExampleOp(address)
Wrapper->>Server: exampleOp(address)
Server-->>Wrapper: ExampleOpServerResult
Wrapper-->>Hook: ExampleOpClientResult
Hook-->>DXHook: ExampleOpClientResult
alt Success (Ok)
DXHook->>DXHook: Log data.name
else Error
DXHook->>DXHook: Log resultCode & errorMessage
alt suggestRetry = true
DXHook->>DXHook: Log retry hint
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ensnode-sdk/src/shared/result/examples/op-hook.ts`:
- Around line 28-34: The function useExampleOp misleadingly uses the React "use"
prefix but does not call any hooks; rename the exported symbol useExampleOp to
getExampleOpResult (and update all internal references/imports) to avoid
violating the Hooks naming convention, or alternatively add a top-of-file
comment above the function clarifying that it is a synchronous example and not a
React hook; update any tests or imports that reference useExampleOp and keep the
existing logic in buildResultExampleOpLoading and callExampleOp unchanged.
♻️ Duplicate comments (3)
packages/ensnode-sdk/src/shared/result/result-common.ts (1)
8-23: Standard builder pattern for internal server errors.The interface and builder correctly implement the error result pattern with sensible defaults (
suggestRetry: truefor transient server errors).The use of
??forerrorMessagewas previously discussed. Empty strings would be accepted rather than falling back to the default.packages/ensnode-sdk/src/shared/result/examples/op-client.ts (1)
48-54: Error handling uses unrealistic pattern for example code.The condition
error === "connection-error"suggests errors are thrown as string literals, which is not typical JavaScript/TypeScript practice. Consider adding a clarifying comment or using a more realistic pattern like checkingerror instanceof Errorand inspecting error properties.packages/ensnode-sdk/src/shared/result/examples/op-server.ts (1)
66-68: Consider a clearer error message wording.The message "random number is not less than 0.5" is technically accurate but reads awkwardly. A clearer phrasing would be "random number is greater than or equal to 0.5" or "random number exceeded threshold".
| export const useExampleOp = (address: Address): ExampleOpHookResult => { | ||
| if (Math.random() < 0.5) { | ||
| return buildResultExampleOpLoading(address); | ||
| } else { | ||
| return callExampleOp(address); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hook naming may mislead developers.
The function useExampleOp uses the use prefix convention for React hooks, but it doesn't use any React hooks internally (no useState, useEffect, etc.). In React, the use prefix signals that a function follows the Rules of Hooks. Consider either:
- Adding a comment clarifying this is a simplified example
- Renaming to something like
getExampleOpResultto avoid confusion
🤖 Prompt for AI Agents
In `@packages/ensnode-sdk/src/shared/result/examples/op-hook.ts` around lines 28 -
34, The function useExampleOp misleadingly uses the React "use" prefix but does
not call any hooks; rename the exported symbol useExampleOp to
getExampleOpResult (and update all internal references/imports) to avoid
violating the Hooks naming convention, or alternatively add a top-of-file
comment above the function clarifying that it is a synchronous example and not a
React hook; update any tests or imports that reference useExampleOp and keep the
existing logic in buildResultExampleOpLoading and callExampleOp unchanged.
Substantial PR
Reviewer Focus (Read This First)
What reviewers should focus on
Where should reviewers spend most of their time?
Call out:
If your answer is "everything," explain why targeted review isn't possible.
Limit this to 1-3 areas.
ENSNode SDK has a new
resultmodule to introduce standardized response data model for ENSNode APIs.Problem & Motivation
Why this exists
Explain:
Keep this tight. Bullets are fine.
What Changed (Concrete)
What actually changed
List the concrete behavioral or structural changes in this PR.
This should be a factual inventory, not a narrative.
Prefer numbered bullets.
If this list starts getting long, that's a signal the PR may need splitting.
Design & Planning
How this approach was chosen
Self-Review
What you caught yourself
Describe what changed after you reviewed your own diff end-to-end.
This is not optional.
Cross-Codebase Alignment
Related code you checked
Show that you looked beyond just the files in this PR.
Focus on directly related domains. Don't boil the ocean.
Downstream & Consumer Impact
Who this affects and how
Explain how this change impacts:
Call out:
Point to actual diffs where possible.
Testing Evidence
How this was validated
Explain:
Scope Reductions
What you intentionally didn't do
List follow-ups you identified but explicitly deferred to keep this PR reviewable.
Link issues where applicable and explain the tradeoffs.
Risk Analysis
How this could go wrong
Call out:
Pre-Review Checklist (Blocking)
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.