API 2.2.0 #143
API 2.2.0 #143
Conversation
|
Warning Rate limit exceeded@mjadach-iv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 40 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. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes several changes across multiple files primarily focused on updating the Changes
Possibly related PRs
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: 29
🧹 Outside diff range and nitpick comments (30)
src/api/network/getMinimumTicketProbability.ts (1)
48-48: Avoid Throwing ZodError Directly to Prevent Exposing Internal DetailsThrowing a
ZodErrormay expose internal validation details to the end-user, which is not ideal for security and user experience. Instead, throw a generic error with a user-friendly message.Apply this diff to throw a more appropriate error:
- throw new ZodError(parsedRes.error.issues); + throw new sdkApiError({ + status: rawResponse.status, + statusText: 'Response validation failed.', + hoprdErrorPayload: parsedRes.error.issues + });This provides a consistent error structure while avoiding leaking internal validation details.
src/types/network.ts (1)
24-26: Consider Using BasePayload Directly if No Additional Fields Are AddedSince
GetMinimumNetworkProbabilityPayloadis identical toBasePayload, consider usingBasePayloaddirectly unless you plan to add more fields in the future.If a distinct type is unnecessary, you can simplify the code:
- export const GetMinimumNetworkProbabilityPayload = BasePayload; - export type GetMinimumNetworkProbabilityPayloadType = z.infer<typeof GetMinimumNetworkProbabilityPayload>; + export type GetMinimumNetworkProbabilityPayloadType = z.infer<typeof BasePayload>;This reduces redundancy and keeps your types concise.
src/api/network/getMinimumTicketProbability.spec.ts (2)
27-39: Enhance Test Assertions for 401 Error HandlingTo ensure that the correct error information is propagated, consider adding assertions that check the contents of the thrown error.
Update the test to verify the error details:
await expect( getMinimumTicketProbability({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT }) - ).rejects.toThrow(sdkApiError); + ).rejects.toThrowError(sdkApiError); + await expect( + getMinimumTicketProbability({ + apiToken: API_TOKEN, + apiEndpoint: API_ENDPOINT + }) + ).rejects.toMatchObject({ + status: 401, + statusText: 'string', + hoprdErrorPayload: { status: 'string', error: 'string' } + });This provides stronger guarantees that the function behaves as expected when encountering a 401 error.
40-52: Add Test Case for Unexpected Server ErrorsCurrently, there is no test covering unexpected server errors (e.g., 500 Internal Server Error). Adding such a test ensures that the function correctly handles server-side issues.
Would you like me to help write a test case for handling a 500 error response?
src/api/checks/isNodeEligible.ts (2)
11-13: Update JSDoc to Accurately Reflect Function PurposeThe JSDoc comment mentions that the function checks whether the node is healthy, but this function actually checks if the node is eligible. Please update the description for clarity.
52-52: Simplify Throwing ZodErrorInstead of creating a new
ZodError, you can throw the existing one fromisApiErrorResponse.error. This retains all error details and stack trace.Apply this diff to simplify error throwing:
- throw new ZodError(isApiErrorResponse.error.issues); + throw isApiErrorResponse.error;src/api/session/closeSession.ts (1)
23-63: Align Function Name with File Name for ConsistencyThe function
removeAliasis defined in a file namedcloseSession.ts. For clarity and consistency, consider renaming the function tocloseSessionor renaming the file toremoveAlias.tsto ensure the function name matches the file name.src/types/tickets.ts (1)
1-1: Simplify zod importThe explicit import of
booleanfrom zod is unnecessary since it's already available through theznamespace.-import { boolean, z } from 'zod'; +import { z } from 'zod';src/api/network/getTicketPrice.spec.ts (1)
Line range hint
13-24: Consider adding edge cases to the success testWhile the basic success case is covered, consider adding tests for:
- Response with zero price
- Response with very large price values
- Response with different number formats
it('handles edge cases', async function () { const cases = [ { price: '0' }, { price: '999999999999999999' }, { price: '1e18' } ]; for (const testCase of cases) { nock(API_ENDPOINT) .get(`/api/v3/network/price`) .reply(200, testCase); const response = await getTicketPrice({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT }); expect(response.price).toEqual(testCase.price); } });src/api/session/getSessions.spec.ts (2)
14-28: Enhance success test coverageWhile the basic success case is covered, consider adding tests for:
- Empty alias list
- Special characters in alias names
- Case sensitivity handling
it('should handle various alias scenarios', async function () { const testCases = [ {}, // empty list { 'user.name': '0x1234...', '@special': '0x5678...' }, // special chars { 'ALICE': '0x1234...', 'alice': '0x5678...' } // case sensitivity ]; for (const expectedResponse of testCases) { nock(API_ENDPOINT) .get('/api/v3/aliases') .reply(200, expectedResponse); const result = await getAliases({ apiEndpoint: API_ENDPOINT, apiToken: API_TOKEN }); expect(result).toEqual(expectedResponse); } });
15-18: Use realistic Ethereum addresses in test dataThe current test uses simplified Ethereum addresses. Consider using checksummed addresses for more realistic testing.
- alice: '0x1234567890123456789012345678901234567890', - bob: '0x0987654321098765432109876543210987654321' + alice: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + bob: '0x123438f44e742d35Cc6634C0532925a3b844Bc45'src/api/peers/getPeer.ts (1)
13-26: Consider extracting the transition logic to a shared utility.This transition logic appears in multiple files (getPeer.ts, pingPeer.ts). Consider extracting it to a shared utility function to maintain consistency and reduce duplication.
// src/utils/transitionHelpers.ts export const getDestinationParam = ( payload: { destination?: string; peerId?: string }, functionName: string ): string => { let destination = ''; if (payload.peerId) { console.warn(`[HOPR SDK: ${functionName}] peerId key is deprecated. Please use destination key`); destination = payload.peerId; } if (payload.destination) { destination = payload.destination; } if (!destination) { throw new Error('Missing required parameter: destination'); } return destination; };src/types/node.ts (1)
61-61: LGTM! Consider enhancing the documentation.The addition of the optional
providerfield is well-structured and maintains backward compatibility. However, the comment could be more descriptive about the field's purpose.Consider expanding the inline comment to describe what the
providerfield represents:- provider: z.string().optional(), //from HORPd 2.2.0 + provider: z.string().optional(), // Provider information added in HORPd 2.2.0 to identify the node's infrastructure providersrc/api/messages/sendMessage.ts (1)
16-34: Consider improving type safety during transition periodThe current implementation relies on runtime checks for required parameters. Consider enhancing type safety while maintaining backward compatibility.
type LegacyPayload = Omit<SendMessagePayloadType, 'destination'> & { peerId: string; }; type ModernPayload = Omit<SendMessagePayloadType, 'peerId'> & { destination: string; }; // Update the function signature export const sendMessage = async ( payload: ModernPayload | LegacyPayload ): Promise<string> => { // ... rest of the implementation };src/types/session.ts (4)
61-62: Remove commented-out codeRemove unused commented-out code to maintain cleaner codebase. If these types are needed in the future, they can be added back through version control.
- // export const GetAliasResponse = z.object({ peerId: z.string() }); - // export type GetAliasResponseType = z.infer<typeof GetAliasResponse>; - // export type SetSessionPayloadType = z.infer<typeof SetSessionPayload>; - // export const CloseSessionResponse = z.object({ peerId: z.string() }); - // export type CloseSessionResponseType = z.infer<typeof GetAliasResponse>;Also applies to: 63-64, 65-66, 83-85
8-9: Standardize enum casingThe
SessionProtocolsenum uses lowercase whileSessionCapabilitiesuses PascalCase. Consider standardizing the casing across enums.- const SessionProtocols = z.enum([ "udp", "tcp"]); + const SessionProtocols = z.enum([ "UDP", "TCP"]);
11-16: Add JSDoc comments for exported typesAdd descriptive JSDoc comments for exported types to improve code documentation and IDE support.
Example:
/** * Represents the base session payload structure. * @property {string} ip - The IP address for the session * @property {number} port - The port number for the session * @property {SessionProtocols} protocol - The protocol used for the session * @property {string} target - The target identifier for the session */ export const SessionPayload = BasePayload.extend({ // ... existing implementation });Also applies to: 38-48, 71-74
42-47: Consider flattening nested objects in payload structureThe nested structure in
SetSessionPayloadCallforpathandtargetseems unnecessary and could be simplified.export const SetSessionPayloadCall = BasePayload.extend({ capabilities: SessionCapabilities, destination: z.string(), listenHost: z.string(), - path: z.object({ - Hops: z.number(), - }), - target: z.object({ - Plain: z.string(), - }) + pathHops: z.number(), + targetPlain: z.string(), });src/api/session/closeSession.spec.ts (1)
27-45: Enhance test descriptions and add timeout testThe test descriptions could be more consistent and descriptive. Also, consider adding a test for network timeout scenarios.
Suggested improvements:
- Use consistent test descriptions:
- test('should return 401 if authentication failed' + test('should throw sdkApiError when authentication fails with 401'
- Add timeout test:
test('should throw error when request times out', async function () { nock(API_ENDPOINT) .delete(`/api/v3/aliases/${ALIAS}`) .delay(2000) // Delay the response .reply(204); await expect( removeAlias({ apiEndpoint: API_ENDPOINT, apiToken: API_TOKEN, alias: ALIAS, timeout: 1000 // Set timeout to 1 second }) ).rejects.toThrow(); });Also applies to: 47-63, 65-82
src/api/tickets/resetTicketStatistics.spec.ts (1)
13-27: Enhance response validationThe test only validates
winningCountwhile ignoring other fields in the response. Consider validating all fields for completeness.const response = await getTicketStatistics({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT }); - expect(response.winningCount).toEqual(0); + expect(response).toEqual({ + neglectedValue: 'string', + redeemedValue: 'string', + rejectedValue: 'string', + unredeemedValue: 'string', + winningCount: 0 + });src/api/aliases/setAlias.ts (1)
23-38: Consider using TypeScript discriminated unionsThe transition between
peerIdanddestinationcould be better handled using TypeScript's discriminated unions.type SetAliasPayload = { apiEndpoint: string; apiToken: string; alias: string; } & ( | { peerId: string; destination?: never } | { destination: string; peerId?: never } );This would provide better type safety and clearer API contracts during the transition period.
src/api/node/getInfo.spec.ts (1)
31-31: Enhance test coverage for new provider fieldThe test only verifies
hoprTokenbut doesn't assert the newly addedproviderfield. Consider adding assertions for all response fields to ensure complete coverage.expect(response.hoprToken).toEqual( '0xD4fdec44DB9D44B8f2b6d529620f9C0C7066A2c1' ); + expect(response.provider).toEqual('http://localhost:8545/');src/api/peers/pingPeer.spec.ts (2)
Line range hint
14-47: Well-structured transition period handlingGood approach to maintain backward compatibility by testing both
peerIdanddestinationparameters. However, consider consolidating duplicate assertions into a shared test helper to reduce code duplication.const testPingPeerSuccess = async (param: { peerId?: string, destination?: string }) => { nock(API_ENDPOINT) .post(`/api/v3/peers/${BUDDY_PEER_ID}/ping`) .reply(200, { latency: 10, reportedVersion: '2.2.0' } as PingPeerResponseType); const response = await pingPeer({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT, ...param }); expect(response.latency).toEqual(10); }; it('handles successful response using peerId', () => testPingPeerSuccess({ peerId: BUDDY_PEER_ID })); it('handles successful response using destination', () => testPingPeerSuccess({ destination: BUDDY_PEER_ID }));
Line range hint
57-99: Consider consolidating error test casesThe error test cases are identical except for the error code and response. Consider using a test helper function to reduce duplication.
const errorCases = [ { code: 400, response: { status: 'INVALID_PEERID' } }, { code: 401, response: { status: 'string', error: 'string' } }, // ... other cases ]; errorCases.forEach(({ code, response }) => { it(`throws a custom error when hoprd api response is an ${code} error`, async () => { nock(API_ENDPOINT) .post(`/api/v3/peers/${BUDDY_PEER_ID}/ping`) .reply(code, response); await expect( pingPeer({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT, destination: BUDDY_PEER_ID }) ).rejects.toThrow(sdkApiError); }); });src/api/messages/sendMessage.spec.ts (1)
7-7: Document the significance of TAG valueThe TAG constant has been updated from 8 to 4677. Consider adding a comment explaining the significance of this specific value for better maintainability.
src/api/aliases/setAlias.spec.ts (1)
Line range hint
50-120: Update error test cases to use new parameterThe error handling test cases still use the old
peerIdparameter. Consider updating these to use the newdestinationparameter for consistency, or add parallel test cases to cover both parameters during the transition period.test('should return 400 if invalid peerId was provided', async function () { nock(API_ENDPOINT) - .post('/api/v3/aliases', { peerId: PEER_ID, alias: ALIAS }) + .post('/api/v3/aliases', { destination: PEER_ID, alias: ALIAS }) .reply(400, { status: 'INVALID_PEERID' }); await expect( setAlias({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT, - peerId: PEER_ID, + destination: PEER_ID, alias: ALIAS }) ).rejects.toThrow(sdkApiError); });src/api/peers/getPeer.spec.ts (1)
40-64: Consider clarifying test descriptionWhile the test implementation is correct, consider making the description more specific about what constitutes a "successful response" (e.g., "handles successful peer lookup using destination parameter").
src/api/session/setSession.spec.ts (1)
11-31: Improve test descriptions for clarityThe test descriptions could be more specific about what they're testing.
Consider updating the test descriptions:
- describe('setAlias function', () => { + describe('setAlias', () => { /* Transition period between 2.1 and 2.2 */ - test('should return 201 and undefined if successful using peerId', async function () { + test('should successfully create alias using legacy peerId parameter', async function () {src/api/channels/openChannel.spec.ts (2)
14-37: Extract test data to constantsThe test uses hardcoded values that could be reused across multiple test cases.
Consider extracting common test data:
+ const TEST_CHANNEL_ID = '0x04e50b7ddce9770f58cebe51f33b472c92d1c40384759f5a0b1025220bf15ec5'; + const TEST_TRANSACTION_RECEIPT = '0x37954ca4a630aa28f045df2e8e604cae22071046042e557355acf00f4ef20d2e'; + const TEST_PEER_ADDRESS = '16Uiu2HAmUsJwbECMroQUC29LQZZWsYpYZx1oaM1H9DBoZHLkYn12';
Line range hint
67-144: Update error handling tests to use new parameterThe error handling tests still use the old
peerAddressparameter while the success case usesdestination.Consider updating all error handling tests to use the new parameter for consistency:
openChannel({ apiToken: API_TOKEN, apiEndpoint: API_ENDPOINT, - peerAddress: '16Uiu2HAmUsJwbECMroQUC29LQZZWsYpYZx1oaM1H9DBoZHLkYn12', + destination: '16Uiu2HAmUsJwbECMroQUC29LQZZWsYpYZx1oaM1H9DBoZHLkYn12', amount: '1000000' })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (38)
package.json(1 hunks)src/api/aliases/setAlias.spec.ts(2 hunks)src/api/aliases/setAlias.ts(1 hunks)src/api/channels/openChannel.spec.ts(2 hunks)src/api/checks/adapter.ts(2 hunks)src/api/checks/index.ts(1 hunks)src/api/checks/isNodeEligible.spec.ts(1 hunks)src/api/checks/isNodeEligible.ts(1 hunks)src/api/messages/sendMessage.spec.ts(2 hunks)src/api/messages/sendMessage.ts(1 hunks)src/api/network/getMinimumTicketProbability.spec.ts(1 hunks)src/api/network/getMinimumTicketProbability.ts(1 hunks)src/api/network/getTicketPrice.spec.ts(1 hunks)src/api/network/index.ts(1 hunks)src/api/node/getInfo.spec.ts(1 hunks)src/api/peers/getPeer.spec.ts(6 hunks)src/api/peers/getPeer.ts(1 hunks)src/api/peers/pingPeer.spec.ts(6 hunks)src/api/peers/pingPeer.ts(1 hunks)src/api/session/adapter.ts(1 hunks)src/api/session/closeSession.spec.ts(1 hunks)src/api/session/closeSession.ts(1 hunks)src/api/session/getSessions.spec.ts(1 hunks)src/api/session/getSessions.ts(1 hunks)src/api/session/index.ts(1 hunks)src/api/session/setSession.spec.ts(1 hunks)src/api/session/setSession.ts(1 hunks)src/api/tickets/resetTicketStatistics.spec.ts(1 hunks)src/api/tickets/resetTicketStatistics.ts(1 hunks)src/types/aliases.ts(1 hunks)src/types/channels.ts(1 hunks)src/types/index.ts(1 hunks)src/types/messages.ts(1 hunks)src/types/network.ts(1 hunks)src/types/node.ts(1 hunks)src/types/peers.ts(2 hunks)src/types/session.ts(1 hunks)src/types/tickets.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/api/checks/index.ts
- src/api/network/index.ts
- src/api/session/index.ts
🔇 Additional comments (15)
src/api/session/getSessions.ts (1)
1-68: LGTM
The implementation is clean and follows best practices, including consistent error handling and type validation.
src/types/index.ts (1)
14-14: Verify the session module exists
The addition of the session module export looks good, following the established pattern.
✅ Verification successful
Session module exists and is correctly referenced
The session.ts module is present at src/types/session.ts, confirming that the export statement correctly references an existing module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the session types module exists
fd --type f "session.ts" src/types/
Length of output: 56
src/types/tickets.ts (1)
19-27: LGTM: Reset statistics type definitions
The new type definitions for reset statistics are well-structured and properly documented. The use of z.boolean() for a simple success response is appropriate.
src/types/aliases.ts (1)
Line range hint 36-36: Consider updating GetAliasResponse for API consistency
Since the API is transitioning from peerId to destination, consider updating the GetAliasResponse type to maintain consistency across the API.
src/api/network/getTicketPrice.spec.ts (1)
9-9: LGTM! Fixed incorrect test suite description
The test suite description has been correctly updated from "pingPeer" to "getTicketPrice".
package.json (1)
3-3: Version bump looks appropriate.
The version change from 2.1.10 to 2.2.0-beta.0 follows semantic versioning and correctly indicates new features with the minor version bump. The beta tag suggests this is a pre-release version.
Let's verify this is consistent with the repository's versioning strategy:
src/api/peers/pingPeer.ts (2)
13-26: Consider using shared transition logic.
This transition logic is duplicated from getPeer.ts. Consider using the shared utility function suggested earlier to maintain consistency across the codebase.
13-28: Verify impact of parameter transition.
The transition from peerId to destination represents a breaking change. Let's verify all consumers of these APIs are updated.
✅ Verification successful
Let me analyze the search results to verify the impact of the parameter transition from peerId to destination.
From the search results, I can see:
- The transition code is present in multiple files:
- src/api/peers/pingPeer.ts
- src/api/peers/getPeer.ts
- src/api/messages/sendMessage.ts
- Current usage in tests and e2e:
- e2e/peerInfo.spec.ts still uses
peerId - The types in src/types/peers.ts show both
peerIdanddestinationare optional
Let me verify if there are any type definitions that need to be checked:
Let me try a different approach to check the type definitions:
Based on the comprehensive analysis, I can now provide the final response:
Breaking change transition is properly handled
The codebase has implemented a proper transition strategy for the parameter change from peerId to destination:
- Both fields are properly typed as optional in the payload types (
PingPeerPayloadandGetPeerPayload) - Appropriate warning messages are in place for deprecated usage
- The code handles both parameters during the transition period
- Type definitions support both old and new parameter names
The implementation follows best practices for managing breaking changes by maintaining backward compatibility while encouraging migration to the new parameter name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of getPeer and pingPeer to ensure they're updated
echo "Searching for API usage patterns..."
rg -A 2 "getPeer|pingPeer" --type ts
# Look for any remaining uses of peerId in the codebase
echo "Checking for remaining peerId usage..."
rg "peerId.*:" --type ts
Length of output: 13090
Script:
#!/bin/bash
# Check the type definitions for pingPeer and getPeer
ast-grep --pattern 'export type $name = z.infer<typeof $_>' src/types/peers.ts
Length of output: 80
Script:
#!/bin/bash
# Check the content of the peers types file
cat src/types/peers.ts
# Also check for any usage of these types
rg "PingPeerPayloadType|GetPeerPayloadType" --type ts
Length of output: 1594
src/types/messages.ts (1)
19-19: Verify the rationale for the minimum tag value
The minimum tag value has been set to 2^10 (1024). Please document the reasoning behind this specific threshold to help future maintainers understand the constraint.
src/api/messages/sendMessage.spec.ts (2)
Line range hint 14-19: LGTM: Proper parameter transition in PAYLOAD
The PAYLOAD object correctly uses the new destination parameter while maintaining all other required fields.
26-47: Well-structured transition test
Good practice: Adding a dedicated test case for backward compatibility during the transition period. This ensures the API continues to support both peerId and destination parameters.
src/api/aliases/setAlias.spec.ts (2)
Line range hint 15-31: LGTM: Clear transition period test case
Good practice: The test clearly indicates it's for the transition period and properly tests the legacy peerId parameter.
33-48: LGTM: New parameter test implementation
The new test case properly validates the updated destination parameter functionality.
src/api/peers/getPeer.spec.ts (2)
Line range hint 13-38: LGTM: Clear transition period test
Good practice: The test case is well-documented as part of the transition period and properly validates the legacy peerId parameter.
Line range hint 65-119: LGTM: Consistent error handling tests
All error handling test cases have been properly updated to use the new destination parameter, maintaining consistency throughout the test suite.
| const apiEndpointParsed = new URL(payload.apiEndpoint).href; | ||
| const rawResponse = await fetchWithTimeout( | ||
| `${apiEndpointParsed}api/v3/network/probability`, |
There was a problem hiding this comment.
Ensure Proper URL Construction to Prevent Malformed URLs
The current URL construction using string concatenation may result in malformed URLs if payload.apiEndpoint does not end with a slash. This can lead to requests being sent to incorrect endpoints.
Apply this diff to correctly construct the URL:
- const apiEndpointParsed = new URL(payload.apiEndpoint).href;
- const rawResponse = await fetchWithTimeout(
- `${apiEndpointParsed}api/v3/network/probability`,
+ const apiUrl = new URL('/api/v3/network/probability', payload.apiEndpoint).href;
+ const rawResponse = await fetchWithTimeout(
+ apiUrl,This change uses the URL constructor to properly resolve the endpoint, ensuring that the path is correctly appended regardless of whether apiEndpoint ends with a slash.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const apiEndpointParsed = new URL(payload.apiEndpoint).href; | |
| const rawResponse = await fetchWithTimeout( | |
| `${apiEndpointParsed}api/v3/network/probability`, | |
| const apiUrl = new URL('/api/v3/network/probability', payload.apiEndpoint).href; | |
| const rawResponse = await fetchWithTimeout( | |
| apiUrl, |
| const jsonResponse = await rawResponse.json(); | ||
| const parsedRes = GetMinimumNetworkProbabilityResponse.safeParse(jsonResponse); |
There was a problem hiding this comment.
Handle JSON Parsing Errors Gracefully
If the response is not valid JSON, calling rawResponse.json() will throw an unhandled exception. It's important to catch this error to provide a meaningful message to the user.
Apply this diff to handle JSON parsing errors:
- const jsonResponse = await rawResponse.json();
- const parsedRes = GetMinimumNetworkProbabilityResponse.safeParse(jsonResponse);
+ let jsonResponse;
+ try {
+ jsonResponse = await rawResponse.json();
+ } catch (e) {
+ throw new sdkApiError({
+ status: rawResponse.status,
+ statusText: 'Invalid JSON response received from server.',
+ hoprdErrorPayload: null
+ });
+ }
+ const parsedRes = GetMinimumNetworkProbabilityResponse.safeParse(jsonResponse);This ensures that if the response body is not valid JSON, the error is caught and a consistent sdkApiError is thrown.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const jsonResponse = await rawResponse.json(); | |
| const parsedRes = GetMinimumNetworkProbabilityResponse.safeParse(jsonResponse); | |
| let jsonResponse; | |
| try { | |
| jsonResponse = await rawResponse.json(); | |
| } catch (e) { | |
| throw new sdkApiError({ | |
| status: rawResponse.status, | |
| statusText: 'Invalid JSON response received from server.', | |
| hoprdErrorPayload: null | |
| }); | |
| } | |
| const parsedRes = GetMinimumNetworkProbabilityResponse.safeParse(jsonResponse); |
| if (rawResponse.status > 499) { | ||
| throw new Error(rawResponse.statusText); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Error Handling for Server Errors
Throwing a generic Error with rawResponse.statusText may not provide sufficient context to the caller. Additionally, status codes starting from 500 are server errors, so the condition should use >= 500.
Apply this diff to improve error feedback:
- if (rawResponse.status > 499) {
- throw new Error(rawResponse.statusText);
+ if (rawResponse.status >= 500) {
+ throw new sdkApiError({
+ status: rawResponse.status,
+ statusText: rawResponse.statusText || 'Server Error',
+ hoprdErrorPayload: null
+ });This change provides more detailed error information and uses the custom sdkApiError for consistency with other error handling in the SDK.
Committable suggestion skipped: line range outside the PR's diff.
| export const GetMinimumNetworkProbabilityResponse = z.object({ | ||
| probability: z.number() | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Validation Constraints to Probability Field
The probability field represents a probability and should be constrained between 0 and 1 to reflect valid probability values.
Apply this diff to enforce value constraints:
export const GetMinimumNetworkProbabilityResponse = z.object({
- probability: z.number()
+ probability: z.number().min(0).max(1)
});This change ensures that only valid probability values are accepted, enhancing data integrity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const GetMinimumNetworkProbabilityResponse = z.object({ | |
| probability: z.number() | |
| }); | |
| export const GetMinimumNetworkProbabilityResponse = z.object({ | |
| probability: z.number().min(0).max(1) | |
| }); |
| const apiEndpointParsed = new URL(payload.apiEndpoint).href; | ||
| const rawResponse = await fetchWithTimeout( | ||
| `${apiEndpointParsed}eligiblez`, |
There was a problem hiding this comment.
Ensure Proper URL Construction to Avoid Errors
Concatenating URLs can lead to missing or duplicate slashes, causing incorrect API endpoint formation. Use the URL constructor to properly handle URL paths.
Apply this diff to fix the URL construction:
- `${apiEndpointParsed}eligiblez`,
+ new URL('eligiblez', apiEndpointParsed).href,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const apiEndpointParsed = new URL(payload.apiEndpoint).href; | |
| const rawResponse = await fetchWithTimeout( | |
| `${apiEndpointParsed}eligiblez`, | |
| const apiEndpointParsed = new URL(payload.apiEndpoint).href; | |
| const rawResponse = await fetchWithTimeout( | |
| new URL('eligiblez', apiEndpointParsed).href, |
src/api/aliases/setAlias.ts
Outdated
| if(!payload.destination && !payload.peerId) { | ||
| console.error('[HOPR SDK: setAlias] Please provide destination'); | ||
| } |
There was a problem hiding this comment.
Enhance error handling for missing destination
The function logs an error but continues execution when both destination and peerId are missing. Consider throwing an error instead.
if(!payload.destination && !payload.peerId) {
- console.error('[HOPR SDK: setAlias] Please provide destination');
+ throw new Error('[HOPR SDK: setAlias] Either destination or peerId must be provided');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(!payload.destination && !payload.peerId) { | |
| console.error('[HOPR SDK: setAlias] Please provide destination'); | |
| } | |
| if(!payload.destination && !payload.peerId) { | |
| throw new Error('[HOPR SDK: setAlias] Either destination or peerId must be provided'); | |
| } |
src/api/aliases/setAlias.ts
Outdated
| if(payload.peerId) { | ||
| console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key'); | ||
| body.peerId = payload.peerId; | ||
| } | ||
| if(payload.destination) { | ||
| console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key'); | ||
| body.destination = payload.destination; | ||
| } |
There was a problem hiding this comment.
Fix inconsistent deprecation warnings
The warning message for destination incorrectly refers to peerId being deprecated.
if(payload.peerId) {
console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key');
body.peerId = payload.peerId;
}
if(payload.destination) {
- console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key');
body.destination = payload.destination;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(payload.peerId) { | |
| console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key'); | |
| body.peerId = payload.peerId; | |
| } | |
| if(payload.destination) { | |
| console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key'); | |
| body.destination = payload.destination; | |
| } | |
| if(payload.peerId) { | |
| console.warn('[HOPR SDK: setAlias] peerId key is deprecated. Please use destination key'); | |
| body.peerId = payload.peerId; | |
| } | |
| if(payload.destination) { | |
| body.destination = payload.destination; | |
| } |
| destination: z.string().optional(), | ||
| peerId: z.string().optional(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider deprecation strategy for peerId parameter
The code shows a transition from peerId to destination. Consider:
- Adding JSDoc deprecation notice for
peerId - Documenting the migration timeline
tag: z.number().min(Math.pow(2, 10)).max(Math.pow(2, 16)),
body: z.string(),
destination: z.string().optional(),
+ /** @deprecated Use destination instead. Will be removed in version 3.0.0 */
peerId: z.string().optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| destination: z.string().optional(), | |
| peerId: z.string().optional(), | |
| destination: z.string().optional(), | |
| /** @deprecated Use destination instead. Will be removed in version 3.0.0 */ | |
| peerId: z.string().optional(), |
| peerAddress: z.string().optional(), | ||
| destination: z.string().optional(), |
There was a problem hiding this comment.
Consider adding validation to ensure at least one identifier is provided
Making both peerAddress and destination optional could lead to invalid states where neither field is provided. Consider adding a validation rule to ensure at least one of these fields is present.
Example implementation using Zod's refinement:
export const OpenChannelPayload = BasePayload.extend({
peerAddress: z.string().optional(),
destination: z.string().optional(),
amount: z.string()
}).refine(
+ data => data.peerAddress != null || data.destination != null,
+ { message: "Either peerAddress or destination must be provided" }
);Committable suggestion skipped: line range outside the PR's diff.
src/api/session/setSession.spec.ts
Outdated
| import nock from 'nock'; | ||
| import { setSession } from './setSession'; | ||
| import { sdkApiError } from '../../utils'; | ||
| import { SetAliasPayloadType } from '../../types'; |
There was a problem hiding this comment.
File name doesn't match the tested functionality
The file is named setSession.spec.ts but tests the setAlias function. This mismatch could lead to confusion.
Consider renaming the file to setAlias.spec.ts to match the functionality being tested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/types/session.ts (3)
8-9: Consider using uppercase for enum valuesThe enum values are currently in lowercase. For better consistency with TypeScript conventions, consider using uppercase for enum values.
-const SessionProtocols = z.enum([ "udp", "tcp"]); -const SessionCapabilities = z.enum([ "Retransmission", "Segmentation"]); +const SessionProtocols = z.enum([ "UDP", "TCP"]); +const SessionCapabilities = z.enum([ "RETRANSMISSION", "SEGMENTATION"]);
38-48: Consider adding validation for network-related fieldsThe
SetSessionPayloadCallstructure contains network-related fields that could benefit from additional validation:
listenHostshould validate IP address formatpath.Hopsshould have a minimum value checkexport const SetSessionPayloadCall = BasePayload.extend({ capabilities: SessionCapabilities.array(), destination: z.string(), - listenHost: z.string(), + listenHost: z.string().regex(/^(\d{1,3}\.){3}\d{1,3}:\d{1,5}$/), path: z.object({ - Hops: z.number(), + Hops: z.number().min(1), }), target: z.object({ Plain: z.string(), }) });
63-68: Remove commented-out codeThere are several blocks of commented-out type definitions. These should be either implemented or removed to maintain code cleanliness.
Also applies to: 85-87
src/api/session/setSession.ts (2)
12-22: Update JSDoc to match current implementationThe JSDoc comments reference outdated concepts:
- Mentions "alias" and "HOPR address" which aren't relevant to this function
- Parameter descriptions don't match the actual parameters
Update the documentation to accurately reflect the current session management functionality:
/** - * Instead of using HOPR address, we can assign HOPR address to a specific name called alias. - * Give an address a more memorable alias and use it instead of Hopr address. - * Aliases are kept locally and are not saved or shared on the network. + * Creates a new session with specified protocol and capabilities. * * @param apiEndpoint - The API endpoint * @param apiToken - The API token to be used for authentication. - * @param body - A object containing the peer ID and alias to link. + * @param protocol - The session protocol (UDP/TCP) + * @param destination - The destination peer ID + * @param capabilities - Array of session capabilities + * @param listenHost - The local host to listen on + * @param path - The path configuration + * @param target - The target configuration * @returns A Promise that resolves to true if alias successfully linked to peerId. * @throws An error that occurred while processing the request. */
31-40: Add timeout validationThe
timeoutparameter is used infetchWithTimeoutbut there's no validation to ensure it's a positive number.Consider adding validation in the
SetSessionPayloadtype:// In src/types/session.ts export const SetSessionPayload = SetSessionPayloadCall.extend({ protocol: SessionProtocols, + timeout: z.number().positive().optional() });src/api/session/setSession.spec.ts (3)
6-10: Use constants for test configurationConsider moving test configuration to a separate file to promote reusability across test files.
Create a new file
src/api/session/__tests__/constants.ts:export const TEST_CONFIG = { API_ENDPOINT: 'http://localhost:3001', API_TOKEN: 'S3CR3T-T0K3N', API_TOKEN_INVALID: 'my-invalid-api-token', PEER_ID: 'peer123', PROTOCOL: 'udp' } as const;
32-50: Add type assertion for response objectThe test response object should be typed to ensure it matches
SetSessionResponseType.- const resp = { + const resp: SetSessionResponseType = { ip: "127.0.0.1", port: 5542, protocol: "tcp", target: "example.com:80" }
28-123: Add missing test casesThe test suite could benefit from additional test cases:
- Network timeout scenario
- Invalid response format
- Empty response body
Would you like me to provide example implementations for these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/api/session/setSession.spec.ts(1 hunks)src/api/session/setSession.ts(1 hunks)src/types/session.ts(1 hunks)
🔇 Additional comments (1)
src/api/session/setSession.ts (1)
42-45: Ensure consistent error handling for server errors
When the response status is greater than 499, the code throws a generic Error with rawResponse.statusText. To maintain consistency in error handling and provide more detailed error information, consider throwing an sdkApiError in this case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/api/session/closeSession.ts (1)
17-22: Update Function Documentation to Reflect ParametersThe JSDoc comments list parameters
apiEndpoint,apiToken, andbody, but the function actually accepts a singlepayloadparameter. Please update the documentation to reflect the actual function signature for clarity and maintainability.Apply this diff:
/** * Unassign an alias from a PeerId. * - * @param apiEndpoint - The API endpoint - * @param apiToken - The API token used to authenticate the request. - * @param body - The payload containing the details of the alias to remove. + * @param payload - The payload containing the API endpoint, token, and session details. * @returns A Promise that resolves to true if the alias was successfully removed. * @throws An error that occurred while processing the request. */src/api/session/closeSession.spec.ts (1)
49-57: Enhance Error Handling Assertions in TestsIn the tests for error responses (401, 403, 422), consider adding assertions to verify that the thrown
sdkApiErrorcontains the expected status and error messages. This would improve test robustness by ensuring that not only an error is thrown but that it contains the correct information.Example diff for the 401 test:
await expect( closeSession({ apiEndpoint: API_ENDPOINT, apiToken: API_TOKEN_INVALID, protocol: PROTOCOL, ...body }) -).rejects.toThrow(sdkApiError); +).rejects.toThrowError( + new sdkApiError({ + status: 401, + statusText: expectedResponse.status, + hoprdErrorPayload: expectedResponse + }) +);Apply similar assertions to the other error tests.
Also applies to: 68-76, 88-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/api/session/closeSession.spec.ts(1 hunks)src/api/session/closeSession.ts(1 hunks)src/api/session/setSession.spec.ts(1 hunks)src/api/session/setSession.ts(1 hunks)src/types/session.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/session/setSession.spec.ts
- src/api/session/setSession.ts
🔇 Additional comments (2)
src/api/session/closeSession.ts (1)
45-47: Ensure Consistent Error Handling for Server Errors
When the response status is greater than 499, the code throws a generic Error with rawResponse.statusText. To maintain consistency in error handling and provide more detailed error information, consider throwing an sdkApiError in this case.
Apply this diff:
if (rawResponse.status > 499) {
- throw new Error(rawResponse.statusText);
+ throw new sdkApiError({
+ status: rawResponse.status,
+ statusText: rawResponse.statusText
+ });
}src/types/session.ts (1)
43-44: Verify the Casing of Object Keys in path and target
The keys Hops and Plain are capitalized in the definitions of path and target. Typically, object keys use camelCase (hops, plain). Please verify if the API requires these keys to be capitalized. If not, consider updating them to camelCase for consistency.
Run the following script to check for occurrences of Hops and Plain in the codebase:
Also applies to: 46-47
✅ Verification successful
Let me check the API schema to understand if this capitalization is required.
Let me check the content of these README files and any related code to understand the API requirements.
Let me check the actual API documentation mentioned in the README to verify the expected format.
Based on the code inspection, I can now provide the final response:
The capitalized keys Hops and Plain are part of the API contract
The capitalization of Hops and Plain in the session type definitions is intentional as it matches the API contract. This is evidenced by:
- The type definitions in
src/types/session.tswhich define the API schema using Zod - The test file
src/api/session/setSession.spec.tswhich shows the expected payload format - The SDK's purpose to interact with HOPRd's REST API as stated in the README
Keep the capitalization as is since it needs to match the external API contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the usage of 'Hops' and 'Plain' to verify if capitalization is required.
# Search for 'Hops' in code
rg 'Hops'
# Search for 'Plain' in code
rg 'Plain'
Length of output: 219
Script:
#!/bin/bash
# Check for any API schema, configuration, or documentation files
fd -e json -e yaml -e yml | grep -i 'schema\|api\|swagger\|openapi'
# Also check README files for API documentation
fd README
Length of output: 143
Script:
#!/bin/bash
# Check README contents for API documentation
cat README.md
cat src/types/README.md
# Look for any session-related API implementations or tests
rg -A 5 -B 5 'session' --type ts
Length of output: 16800
Script:
# Check for any references to the HOPR REST API documentation
curl -s https://docs.hoprnet.org/developers/rest-api || echo "Unable to fetch external documentation"
# Look for the actual session type definition
cat src/types/session.ts
Length of output: 15988
| export const SetSessionPayloadCall = BasePayload.extend({ | ||
| capabilities: SessionCapabilities.array(), | ||
| destination: z.string(), | ||
| listenHost: z.string(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Consistent Naming for Listener Host and IP
The parameter names listenHost (line 41) and listeningIp (line 74) may refer to the same concept. For consistency and clarity, consider using the same naming convention for these parameters.
Also applies to: 74-74
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/types/session.ts (5)
11-16: Strengthen IP and port validationConsider adding more specific validation for IP addresses and port numbers to prevent invalid values.
export const SessionPayload = z.object({ - ip: z.string(), - port: z.number(), + ip: z.string().ip(), + port: z.number().min(1).max(65535), protocol: SessionProtocols, target: z.string() });
38-48: Extract nested object types for better reusabilityConsider extracting the nested object types into separate definitions for better reusability and maintainability.
+export const PathConfig = z.object({ + Hops: z.number() +}); + +export const TargetConfig = z.object({ + Plain: z.string() +}); + export const SetSessionPayloadCall = BasePayload.extend({ capabilities: SessionCapabilities.array(), destination: z.string(), listenHost: z.string(), - path: z.object({ - Hops: z.number() - }), - target: z.object({ - Plain: z.string() - }) + path: PathConfig, + target: TargetConfig });
63-67: Remove commented out and duplicate codeThis section contains commented out code and a duplicate type definition. If these types are not needed, they should be removed to maintain code cleanliness.
82-84: Simplify type inference formattingThe type inference can be written more concisely on a single line.
-export type CloseSessionPayloadCallType = z.infer< - typeof CloseSessionPayloadCall ->; +export type CloseSessionPayloadCallType = z.infer<typeof CloseSessionPayloadCall>;
87-89: Remove unused commented out codeThese commented out types should be removed if they are no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
src/api/aliases/setAlias.ts(1 hunks)src/api/channels/openChannel.spec.ts(2 hunks)src/api/messages/sendMessage.ts(1 hunks)src/api/network/getMinimumTicketProbability.ts(1 hunks)src/api/network/index.ts(1 hunks)src/api/peers/getPeer.ts(1 hunks)src/api/peers/pingPeer.ts(1 hunks)src/api/session/closeSession.spec.ts(1 hunks)src/api/session/closeSession.ts(1 hunks)src/api/session/getSessions.spec.ts(1 hunks)src/api/session/getSessions.ts(1 hunks)src/api/session/setSession.spec.ts(1 hunks)src/api/session/setSession.ts(1 hunks)src/api/tickets/resetTicketStatistics.ts(1 hunks)src/types/messages.ts(1 hunks)src/types/network.ts(1 hunks)src/types/peers.ts(2 hunks)src/types/session.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- src/api/network/index.ts
- src/types/network.ts
- src/api/peers/pingPeer.ts
- src/api/channels/openChannel.spec.ts
- src/types/messages.ts
- src/api/peers/getPeer.ts
- src/api/messages/sendMessage.ts
- src/types/peers.ts
- src/api/aliases/setAlias.ts
- src/api/tickets/resetTicketStatistics.ts
- src/api/session/closeSession.spec.ts
- src/api/session/setSession.spec.ts
- src/api/session/getSessions.spec.ts
- src/api/session/getSessions.ts
- src/api/session/closeSession.ts
- src/api/network/getMinimumTicketProbability.ts
- src/api/session/setSession.ts
🔇 Additional comments (3)
src/types/session.ts (3)
1-10: LGTM! Well-structured type definitions
The imports and enum definitions are clear, properly typed, and follow TypeScript best practices.
24-32: LGTM! Clean type definitions for getSessions
The payload and response types are well-structured and properly extend BasePayload.
41-41: Ensure consistent naming between listenHost and listeningIp
The parameter names listenHost and listeningIp appear to represent the same concept but use different naming conventions.
Also applies to: 74-74
Update of the SDK to support REST API provided by the 2.2.0-rc1
Summary by CodeRabbit
Release Notes
New Features
@hoprnet/hopr-sdkpackage.getAliases,setAlias,getAlias, andremoveAlias.getMinimumTicketProbability.isNodeEligiblemethod.Improvements
peerIdtodestinationin several APIs for better clarity and flexibility.providerfield in the node info response.Tests
Documentation