HTTP and Network Error structure refactor #2463
Conversation
…messages - 2329 porting
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds HttpException and HttpNetworkException and updates FetchHttpClient to uniformly throw them for non-OK and network errors. Introduces handleHttpError to map HTTP errors to ThorError and refactors many thorest methods/tests to use centralized try/catch + handleHttpError. Adjusts exports and test HTTP mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Thorest as Thorest method
participant Client as FetchHttpClient
participant Fetch as fetch()
participant ErrMap as handleHttpError
participant Caller as Caller (test/app)
Thorest->>Client: get/post(url, opts)
Client->>Fetch: fetch(url, init, AbortSignal)
alt Network / timeout / Abort
Fetch--x Client: throws (AbortError/Network)
Client-->>Thorest: throws HttpNetworkException
Thorest->>ErrMap: handleHttpError(fqp, HttpNetworkException)
ErrMap-->>Thorest: ThorError (status:0, networkErrorType, url)
Thorest-->>Caller: throws ThorError
else HTTP non-OK
Fetch-->>Client: Response(ok=false)
Client->>Client: read body/text
Client-->>Thorest: throws HttpException(status, statusText, body, url)
Thorest->>ErrMap: handleHttpError(fqp, HttpException)
ErrMap-->>Thorest: ThorError (status, statusText, url, cause)
Thorest-->>Caller: throws ThorError
else HTTP OK
Fetch-->>Client: Response(ok=true)
Client-->>Thorest: Response
Thorest->>Thorest: parse JSON / build ThorResponse
Thorest-->>Caller: ThorResponse | null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Giulia Argiolas <155436416+giuliamorg92@users.noreply.github.com>
|
@coderabbitai 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/sdk/src/thor/thorest/node/methods/RetrieveTransactionsFromTransactionPool.ts (1)
59-77: Avoid double-reading the Response body in the error path (json() + text()).If response.json() throws, the body is already consumed; await response.text() will likely throw (Body has already been used), masking the original parse error.
Proposed fix: read once, parse, and reuse the same text in the error.
- if (response.ok) { - try { - const json = (await response.json()) as TransactionsIDsJSON; - return { - request: this, - response: new TransactionsIDs(json) - }; - } catch (error) { - throw new ThorError( - fqp, - error instanceof Error ? error.message : 'Bad response.', - { - url: response.url, - body: await response.text() - }, - error instanceof Error ? error : undefined, - response.status - ); - } - } else { + if (response.ok) { + const text = await response.text(); + try { + const json = JSON.parse(text) as TransactionsIDsJSON; + return { + request: this, + response: new TransactionsIDs(json) + }; + } catch (error) { + throw new ThorError( + fqp, + error instanceof Error ? error.message : 'Bad response.', + { + url: response.url, + body: text + }, + error instanceof Error ? error : undefined, + response.status + ); + } + } else {Note: Keeping the improved message selection (error.message) is good.
packages/sdk/src/thor/thorest/fees/methods/SuggestPriorityFee.ts (2)
47-65: Include JSON parsing in the try/catch to standardize error mapping.If
response.json()throws (malformed JSON), the error currently bypasses theThorErrorwrapper. Pull the parse into the try/catch and keep the parsed body (if any) in the error context.- if (response.ok) { - const json = (await response.json()) as GetFeesPriorityResponseJSON; - try { - return { - request: this, - response: new GetFeesPriorityResponse(json) - }; - } catch (error) { - throw new ThorError( - fqp, - error instanceof Error ? error.message : 'Bad response.', - { - url: response.url, - body: json - }, - error instanceof Error ? error : undefined, - response.status - ); - } + if (response.ok) { + let json: GetFeesPriorityResponseJSON | undefined; + try { + json = (await response.json()) as GetFeesPriorityResponseJSON; + return { + request: this, + response: new GetFeesPriorityResponse(json) + }; + } catch (error) { + throw new ThorError( + fqp, + error instanceof Error ? error.message : 'Bad response.', + { + url: response.url, + body: json + }, + error instanceof Error ? error : undefined, + response.status + ); + }
43-46: Wrap the GET call to convert network errors into ThorError consistently.If
httpClient.getthrows (network/DNS/timeout), it escapes unwrapped. Catch and convert toThorError(status 0).- const response = await httpClient.get( - SuggestPriorityFee.PATH, - SuggestPriorityFee.QUERY - ); + let response: Awaited<ReturnType<HttpClient['get']>>; + try { + response = await httpClient.get( + SuggestPriorityFee.PATH, + SuggestPriorityFee.QUERY + ); + } catch (error) { + throw new ThorError( + fqp, + error instanceof Error ? error.message : 'Network error.', + { url: SuggestPriorityFee.PATH.path }, + error instanceof Error ? error : undefined, + 0 + ); + }packages/sdk/src/thor/thorest/fees/methods/RetrieveHistoricalFeeData.ts (1)
68-79: Replace ThorError with centralized handler (handleHttpError).
Update packages/sdk/src/thor/thorest/fees/methods/RetrieveHistoricalFeeData.ts (catch at ~lines 68–79) to:throw handleHttpError(fqp, error);— addimport { handleHttpError } from '@thor/thorest/utils';and remove theThorErrorimport if it becomes unused.packages/sdk/src/thor/thorest/node/methods/RetrieveExpandedTransactionsFromTransactionPool.ts (1)
70-88: Clone the response before parsing to avoid double-readCalling response.text() after response.json() can throw because the body is already consumed — clone the response and use the clone for text() in the catch.
Apply this minimal diff:
- if (response.ok) { - try { - const json = (await response.json()) as TransactionsJSON; + if (response.ok) { + const resClone = response.clone(); + try { + const json = (await response.json()) as TransactionsJSON; return { request: this, response: new Transactions(json) }; } catch (error) { throw new ThorError( fqp, - error instanceof Error ? error.message : 'Bad response.', + error instanceof Error ? error.message : 'Bad response.', { url: response.url, - body: await response.text() + body: await resClone.text() }, error instanceof Error ? error : undefined, response.status ); } } else {packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts (1)
197-199: Query string construction is incorrectReturning
${this.head}&misses the parameter name and leading “?”. It should encode head as a query param.- get query(): string { - return this.head === undefined ? '' : `${this.head}&`; - } + get query(): string { + return this.head === undefined ? '' : `?head=${this.head}`; + }
🧹 Nitpick comments (68)
packages/sdk/tests/common/logging/prettylogger.unit.test.ts (3)
6-7: Remove unused type import 'LogItem'.Lint flags it as unused; safe to drop.
- type LogItem,
14-18: Restore spies after each test to avoid leakage.Adds resilience if future tests rely on real console.
beforeEach(() => { jest.clearAllMocks(); LoggerRegistry.getInstance().clearRegisteredLogger(); delete process.env.SDK_LOG_VERBOSITY; }); + afterEach(() => { + jest.restoreAllMocks(); + });
74-74: Redundant env override after setConfig.You already set verbosity via setConfig; setting SDK_LOG_VERBOSITY again is unnecessary here.
- process.env.SDK_LOG_VERBOSITY = 'debug';packages/sdk/tests/thor/thorest/blocks/RetrieveExpandedBlock.solo.test.ts (3)
25-25: Fix Prettier indentation (lint blocker).Prettier flags extra spaces before
try {}. Align with the surrounding block.- try { + try {
21-21: Rename suite for accuracy.The suite name says “RetrieveRegularBlock” but the file/tests target “RetrieveExpandedBlock”.
-describe('RetrieveRegularBlock SOLO tests', () => { +describe('RetrieveExpandedBlock SOLO tests', () => {
33-36: Assert error shape and prefer semantic cause checks.File: packages/sdk/tests/thor/thorest/blocks/RetrieveExpandedBlock.solo.test.ts (lines 33–36) — HttpNetworkException and HttpException are exported from packages/sdk/src/common/http, so cause checks are available.
Keep the [0, 400] check but also assert status is numeric and, if present, verify the cause's type:
- const thorError = error as ThorError; - expect([0, 400]).toContain(thorError.status); + const thorError = error as ThorError; + expect(thorError).toMatchObject({ status: expect.any(Number) }); + expect([0, 400]).toContain(thorError.status); + // If Http* exceptions are wired as causes, prefer semantic verification: + if ('cause' in thorError && thorError.cause) { + expect(['HttpNetworkException', 'HttpException']) + .toContain((thorError as any).cause?.name); + }packages/sdk/src/thor/thorest/node/methods/RetrieveTransactionsFromTransactionPool.ts (4)
14-16: Fix FQP path to this file.The path includes GetTxPoolStatus.ts, which is misleading for diagnostics.
-const FQP = - 'packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts/RetrieveTransactionsFromTransactionPool.ts!'; +const FQP = + 'packages/sdk/src/thor/thorest/node/methods/RetrieveTransactionsFromTransactionPool.ts!';
54-54: Correct method name in the FQP context.The method is askTo, not ask. Helps stack traces/searchability.
- const fqp = `${FQP}ask(httpClient: HttpClient): ThorResponse<RetrieveTransactionsFromTransactionPool, TransactionsIDs>`; + const fqp = `${FQP}askTo(httpClient: HttpClient): ThorResponse<RetrieveTransactionsFromTransactionPool, TransactionsIDs>`;
78-88: Consider centralizing non-OK error handling with the new handleHttpError utility.For consistency with the PR’s new HTTP error handling flow, delegate to the shared mapper (status, statusText, cause, body extraction, network errors).
Would you like me to draft the import and replacement here once we confirm the canonical handleHttpError entry point?
129-133: URL‑encode origin in query string.Guard against any non-alphanumeric characters or future Address representations.
- const origin = - this.origin !== undefined ? `&origin=${this.origin}` : ''; + const origin = + this.origin !== undefined + ? `&origin=${encodeURIComponent(String(this.origin))}` + : '';packages/sdk/src/thor/thorest/fees/methods/SuggestPriorityFee.ts (4)
42-42: Fix the FQP signature string (typo/closing).Tighten the signature text to avoid confusing traces.
- const fqp = `${FQP}askTo(httpClient: HttpClient: Promise<ThorResponse<SuggestPriorityFee, GetFeesPriorityResponse>>`; + const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<SuggestPriorityFee, GetFeesPriorityResponse>>`;
16-16: Avoid hardcoding localhost docs in public JSDoc.Replace the local URL with a neutral description referencing the endpoint.
- * [Suggest a priority fee for a transaction to be included in a block](http://localhost:8669/doc/stoplight-ui/#/paths/fees-priority/get) + * Suggests a priority fee for a transaction to be included in a block. See API docs for GET /fees/priority.
29-29: Empty query string may produce a trailing “?”.If your
HttpClientappends?${query}, an empty string can yield/fees/priority?. Consider omitting the query or using an empty params object if supported.If you want, I can scan usages of
HttpQueryandHttpClient.getto confirm expected behavior across the repo.
13-13: Optional: derive FQP from the module URL to avoid drift.Hardcoding the file path can get out of sync after moves/renames. Consider using
import.meta.urlif your build targets ESM.I can check your tsconfig/module settings to ensure
import.meta.urlis safe to use.packages/sdk/tests/viem/clients/publicClient/events.solo.test.ts (3)
52-56: Assert nested shape explicitly (keep flat assertions if backward-compat is required).Given the move toward
eventLog, add assertions for the nested fields to avoid silent drift.Apply this diff:
const firstLog = logs[0]; expect(firstLog).toHaveProperty('address'); expect(firstLog).toHaveProperty('topics'); expect(firstLog).toHaveProperty('data'); expect(firstLog).toHaveProperty('meta'); + expect(firstLog).toHaveProperty('eventLog'); + expect(firstLog.eventLog).toHaveProperty('address'); + expect(firstLog.eventLog).toHaveProperty('topics'); + expect(firstLog.eventLog).toHaveProperty('data');
242-247: Mirror the nested assertions for getFilterLogs for consistency.
getFilterLogstests still only assert the flat shape; addeventLogchecks.Apply this diff:
if (logs.length > 0) { const firstLog = logs[0]; expect(firstLog).toHaveProperty('address'); expect(firstLog).toHaveProperty('topics'); expect(firstLog).toHaveProperty('data'); + expect(firstLog).toHaveProperty('eventLog'); + expect(firstLog.eventLog).toHaveProperty('address'); + expect(firstLog.eventLog).toHaveProperty('topics'); + expect(firstLog.eventLog).toHaveProperty('data'); }
61-63: Make debug logging resilient to both shapes (nested and flat).
In packages/sdk/tests/viem/clients/publicClient/events.solo.test.ts (lines 61–63) prefer a backward‑compatible fallback:- address: firstLog.eventLog.address, - topics: firstLog.eventLog.topics, - data: firstLog.eventLog.data + address: firstLog.eventLog?.address ?? (firstLog as any).address, + topics: firstLog.eventLog?.topics ?? (firstLog as any).topics, + data: firstLog.eventLog?.data ?? (firstLog as any).datapackages/sdk/package.json (2)
77-85: Align dependency versioning strategy.You’re mixing exact (ethers) and caret (others). Pick a policy (e.g., caret with lockfile or all pinned) to reduce drift and flaky CI.
77-85: If relying on native fetch (Node ≥18), declare engines.This guards CI/dev from silent polyfill gaps.
Add at package top-level:
"engines": { "node": ">=18.17" }packages/sdk/tests/thor/thorest/transactions/TransactionDelegationAndVerification.unit.test.ts (2)
175-179: Forged check: same encoding concern forsigmaA.Use the
Signatureobject directly to keep behavior consistent and future-proof.- const isVerifiedForge = nc_secp256k1.verify( - sigmaA.toBytes(), + const isVerifiedForge = nc_secp256k1.verify( + sigmaA, hashB, gasPayerPublicKey );
142-146: Pass the Signature instance to verify — avoid sigmaA.toBytes()File: packages/sdk/tests/thor/thorest/transactions/TransactionDelegationAndVerification.unit.test.ts (lines 142–146)
nc_secp256k1.verify accepts a Signature instance; toBytes() is version-dependent—pass sigmaA directly, or if you need raw bytes use toCompactRawBytes()/toDERRawBytes() that match your pinned @noble/curves/@noble/secp256k1 version.
- const isVerifiedA = nc_secp256k1.verify( - sigmaA.toBytes(), + const isVerifiedA = nc_secp256k1.verify( + sigmaA, hashA, gasPayerPublicKey );packages/sdk/src/thor/thorest/node/methods/RetrieveExpandedTransactionsFromTransactionPool.ts (4)
57-69: Wrap network/transport errors to ThorError via the centralized handler for consistency.
httpClient.get(...)exceptions (network, abort, timeouts) currently bypass ThorError mapping; the JSDoc promises ThorError on failures. Please wrap the whole request in a try/catch and route through the shared HTTP error handler introduced in this PR (e.g.,handleHttpError(...)) so consumers consistently receive ThorError.Would you confirm the intended helper (name/signature/import) so we can align this method with the rest of thorest?
15-15: FQP string looks copy‑pasted; update to the correct file path.Current value references
GetTxPoolStatus.ts/.... Use the actual filename for accurate diagnostics.-const FQP = - 'packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts/RetrieveExpandedTransactionsFromTransactionPool.ts!'; +const FQP = + 'packages/sdk/src/thor/thorest/node/methods/RetrieveExpandedTransactionsFromTransactionPool.ts!';
145-148: URL‑encode origin in query string.Encode the optional
originto avoid malformed queries if the type changes or includes unexpected chars.-const origin = this.origin != null ? `&origin=${this.origin}` : ''; +const origin = this.origin != null ? `&origin=${encodeURIComponent(String(this.origin))}` : '';
18-20: Replace localhost doc link with a public or repo‑relative reference.Pointing to
http://localhost:8669/...in source is confusing for consumers.Please provide the canonical public doc URL (or repo path) for
/node/txpoolso we can replace it here.packages/sdk/tests/thor/thorest/blocks/RetrieveRawBlock.solo.test.ts (1)
30-35: Rethrow unexpected errors to avoid masking failures.If a non-ThorError is thrown, rethrow it for a clearer failure signal.
- } catch (error) { - expect(error).toBeInstanceOf(ThorError); - // Now we expect status 0 for network errors (when server is not running) - // or status 400 for HTTP errors (when server is running but bad revision) - const thorError = error as ThorError; - expect([0, 400]).toContain(thorError.status); - } + } catch (error) { + // If it's not a ThorError, surface the original error. + if (!(error instanceof ThorError)) throw error; + // Now we expect status 0 for network errors (when server is not running) + // or status 400 for HTTP errors (when server is running but bad revision) + const thorError = error as ThorError; + expect([0, 400]).toContain(thorError.status); + }packages/sdk/tests/thor/thorest/fork/ForkDetector.unit.test.ts (1)
113-115: Avoid brittle string match; assert type/status instead.Match on status property rather than the exact error message.
- await expect(detector.isGalacticaForked('best')).rejects.toThrow( - 'HTTP request failed with status 400' - ); + await expect(detector.isGalacticaForked('best')).rejects.toHaveProperty('status', 400);packages/sdk/tests/thor/thorest/blocks/RetrieveRegularBlock.solo.test.ts (1)
25-25: Fix stray indentation flagged by Prettier.Minor formatting nit.
- try { + try {packages/sdk/src/common/http/HttpNetworkException.ts (1)
11-16: Add a type alias and set error name; also resolves Prettier union wrapping.Reduces duplication, makes instanceof/name clearer, and satisfies formatting rules.
import { VeChainSDKError } from '@common/errors'; +type NetworkErrorType = 'timeout' | 'connection' | 'no_response' | 'abort'; + /** * Represents a network error for connection issues, timeouts, or no response. * * @remarks * This error class extends the `VeChainSDKError` and denotes network errors encountered * when there are connection issues, timeouts, or when no response is received from the server. * It includes additional information about the network error type and request details. */ class HttpNetworkException extends VeChainSDKError { /** * The type of network error that occurred. */ - readonly networkErrorType: 'timeout' | 'connection' | 'no_response' | 'abort'; + readonly networkErrorType: NetworkErrorType; /** * The URL that was requested. */ readonly url: string; @@ constructor( fqn: string, message: string, - networkErrorType: 'timeout' | 'connection' | 'no_response' | 'abort', + networkErrorType: NetworkErrorType, url: string, args?: Record<string, unknown>, cause?: Error ) { super(fqn, message, args, cause); + this.name = 'HttpNetworkException'; this.networkErrorType = networkErrorType; this.url = url; } }If URLs can include tokens/PII in query strings, consider redacting before storing/logging them (can add a sanitize helper).
Also applies to: 32-43
packages/sdk/tests/MockHttpClient.ts (4)
10-12: Break long condition for readability and TS safety.Avoids lint error and clarifies intent.
- const isError = typeof response === 'object' && response !== null && 'error' in response; + const isError = + typeof response === 'object' && + response !== null && + 'error' in (response as Record<string, unknown>);
28-35: Simplify async helpers; avoid usingsatisfiesat runtime.Less noise, same behavior, fewer lint complaints.
- const mockResponse = { + const mockResponse = { ok: true, status: 200, statusText: 'OK', url: 'https://mock-url', - json: async () => await Promise.resolve(response satisfies T), - text: async () => await Promise.resolve(JSON.stringify(response)) + json: async () => response as T, + text: async () => JSON.stringify(response) };
48-51: Apply the same multilineisErrorpattern here.- const isError = typeof response === 'object' && response !== null && 'error' in response; + const isError = + typeof response === 'object' && + response !== null && + 'error' in (response as Record<string, unknown>);
42-69: Deduplicate mock implementations.
mockHttpClientandmockHttpClientForDebugshare most logic; consider a single factory with amode: 'response' | 'json'option to reduce drift.packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts (1)
70-71: Minor: collapse wrapped assignment (Prettier).- const json = - (await response.json()) as GetTxResponseJSON | null; + const json = (await response.json()) as GetTxResponseJSON | null;packages/sdk/src/thor/thorest/logs/methods/QueryVETTransferEvents.ts (1)
66-75: Unify error handling via handleHttpError and standardize message formatCurrent catch uses
error.messageand customThorError. Elsewhere the PR centralizes withhandleHttpError, and tests expect richer args (status/statusText). Consider switching to the shared helper for consistency and to avoid diverging message formatting across modules (some tests assert a quoted message, which is undesirable).Apply this refactor pattern:
import { type HttpClient, type HttpPath } from '@common/http'; import { TransferLogFilterRequest, TransferLogsResponse } from '@thor/thorest'; import { type TransferLogsResponseJSON } from '@thor/thorest/json'; -import { ThorError, type ThorRequest, type ThorResponse } from '@thor/thorest'; +import { ThorError, type ThorRequest, type ThorResponse } from '@thor/thorest'; +import { handleHttpError } from '@thor/thorest/utils'; @@ - const response = await httpClient.post( - QueryVETTransferEvents.PATH, - { query: '' }, - this.request.toJSON() - ); - if (response.ok) { - const json = (await response.json()) as TransferLogsResponseJSON; - try { - return { - request: this, - response: new TransferLogsResponse(json) - }; - } catch (error) { - throw new ThorError( - fqp, - error instanceof Error ? error.message : 'Bad response.', - { - url: response.url, - body: json - }, - error instanceof Error ? error : undefined, - response.status - ); - } - } - throw new ThorError( - fqp, - await response.text(), - { - url: response.url - }, - undefined, - response.status - ); + try { + const response = await httpClient.post( + QueryVETTransferEvents.PATH, + { query: '' }, + this.request.toJSON() + ); + const json = (await response.json()) as TransferLogsResponseJSON; + return { request: this, response: new TransferLogsResponse(json) }; + } catch (error) { + throw handleHttpError(fqp, error); + }Additionally, standardize on raw, unquoted
messagevalues across the SDK and tests. IfhandleHttpErrorJSON-stringifies messages, please remove that behavior and update tests accordingly.packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts (3)
9-11: Fix FQP format (duplicated filename, missing delimiter)
FQPcurrently reads"GetTxPoolStatus.ts/GetTxPoolStatus.ts". Other modules use a trailing!once. Align for consistency and for tests that may assert exact FQN.-const FQP = - 'packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts/GetTxPoolStatus.ts'; +const FQP = + 'packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts!';
36-37: Normalize FQN method signature stringThe current string has odd
<Promise<...> >formatting. Align with the pattern used elsewhere.- const fqp = `${FQP}askTo(httpClient: HttpClient)<Promise<ThorResponse<GetTxPoolStatus, Status>> >`; + const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<GetTxPoolStatus, Status>>`;
48-58: Consider centralizing with handleHttpError for consistencyThis file still hand-rolls
ThorErrorwhile other modules adopthandleHttpError. For parity and to ensure uniformargs(status/statusText/url), consider the same pattern as inTraceTransactionClause.+import { handleHttpError } from '@thor/thorest/utils'; @@ - const response = await httpClient.get(GetTxPoolStatus.PATH, { - query: '' - }); - if (response.ok) { - const json = (await response.json()) as StatusJSON; - try { - return { - request: this, - response: new Status(json) - }; - } catch (error) { - throw new ThorError( - fqp, - error instanceof Error ? error.message : 'Bad response.', - { - url: response.url, - body: json - }, - error instanceof Error ? error : undefined, - response.status - ); - } - } else { - throw new ThorError( - fqp, - await response.text(), - { - url: response.url - }, - undefined, - response.status - ); - } + try { + const response = await httpClient.get(GetTxPoolStatus.PATH, { query: '' }); + const json = (await response.json()) as StatusJSON; + return { request: this, response: new Status(json) }; + } catch (error) { + throw handleHttpError(fqp, error); + }Ensure
HttpClientthrows on non-2xx. If it returns non-okResponseobjects instead, keep a decode-specifictry/catchand convert non-ok responses via the centralized helper.packages/sdk/src/thor/thorest/debug/methods/TraceTransactionClause.ts (1)
51-64: Confirm non-OK HTTP responses are surfaced as exceptionsThe implementation relies on
httpClient.postthrowing for non-2xx; otherwise a non-ok response would be parsed and returned as success, contradicting the docstring (“@throw If the response status is not OK.”).
- If
HttpClientthrows on non-2xx: good—consider adding a brief comment to document this contract.- If it returns
Responsewithok=false: add an explicit check and route tohandleHttpError.Example (only if needed):
const response = await httpClient.post( TraceTransactionClause.PATH, { query: '' }, this.request.toJSON() ); + // If HttpClient does not throw on non-2xx, guard here. + // if (!response.ok) throw handleHttpError(fqp, response); const json: unknown = await response.json();packages/sdk/tests/thor/thorest/debug/TraceCall.solo.test.ts (2)
14-14: Prettier violation: indentationRemove the extra indentation to satisfy Prettier.
- const revision = Revision.of(Hex.of('0xBADC0FFEE')); + const revision = Revision.of(Hex.of('0xBADC0FFEE'));
34-38: Broadened status assertion is fine; consider determinismAccepting [0, 400] is pragmatic, but the test depends on environment state (node up vs down). Prefer mocking to deterministically assert each branch (network vs HTTP) to avoid flakes.
packages/sdk/tests/thor/thorest/accounts/RetrieveAccountDetails.unit.test.ts (1)
161-172: Quoted error message looks unintended
thorError.messageequals '"Network error"' (with quotes). That suggests the message is being JSON-stringified upstream. Either:
- Fix the implementation to use the raw
error.message, or- Relax the test to assert content instead of exact quoted string.
Apply this minimal test tweak if keeping current impl:
- expect(thorError.message).toBe('"Network error"'); + expect(thorError.message).toContain('Network error');The additions to
args(url, status, statusText) andcauseassertions look good.If the quotes are unintentional, I can prepare a small patch in
handleHttpErrorto avoid stringifying messages. Want that?packages/sdk/tests/common/http/FetchHttpClient.unit.test.ts (2)
563-563: Prefer asserting on error type/props over message; also satisfy PrettierMessages are brittle. Assert
HttpExceptionand itsstatus. This also resolves the Prettier complaint by splitting lines.+import { HttpException } from '@common/http'; @@ - await expect(client.post({ path: 'transactions' })).rejects.toThrow('HTTP request failed with status 400'); + await expect(client.post({ path: 'transactions' })) + .rejects.toBeInstanceOf(HttpException); + await expect(client.post({ path: 'transactions' })) + .rejects.toMatchObject({ status: 400 });
645-649: Remove trailing whitespaceWhitespace is flagged by ESLint/Prettier; trim the blank padding here.
- - - // Mock fetch to return a valid response + // Mock fetch to return a valid responsepackages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts (1)
79-79: Propagate richer context in error argsIncluding
statusandstatusTextin theThorError.argsimproves parity with other thorest methods and matches updated tests elsewhere.packages/sdk/src/thor/thorest/debug/methods/TraceCall.ts (2)
55-55: Fix FQP signature text to match actual genericString says TraceCall, undefined but method returns unknown. Align for clearer diagnostics.
Apply:
-const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<TraceCall, undefined>>`; +const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<TraceCall, unknown>>`;
49-51: Update JSDoc to reflect new error behaviorNow errors are thrown (not “undefined data on error”). Update return/throws text.
- * @return A Promise that resolves to a ThorResponse containing the request and parsed response on success, or with undefined data on error. - * @throw If the response status is not OK. + * @return A Promise that resolves to a ThorResponse on success. + * @throws ThorError when the request fails or returns a non‑OK HTTP status.packages/sdk/tests/thor/thorest/debug/TraceCall.unit.test.ts (2)
61-65: Tighten error assertions once mocks are stableAccepting either Error or ThorError is broad and may mask regressions. Consider asserting ThorError specifically when using mockHttpClientForDebug.
Do you plan to make mockHttpClientForDebug consistently throw HttpException/HttpNetworkException so tests can assert ThorError deterministically?
Also applies to: 97-101
165-165: Prettier formatting nitFix suggested line breaks.
- .askTo(mockHttpClientForDebug(mockResponse(expected, 200), 'post')) + .askTo( + mockHttpClientForDebug(mockResponse(expected, 200), 'post') + )(Apply similarly at Lines 198 and 231.)
Also applies to: 198-198, 231-231
packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts (2)
16-18: Wrong FQP pathFQP references RetrieveTransactionByID.ts; should be RetrieveTransactionReceipt.ts. This impairs diagnostics.
-const FQP = - 'packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts!'; +const FQP = + 'packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts!';
51-56: JSDoc fixesTypos/outdated types (“receiptusing”, wrong generic).
- * Asynchronously fetches and processes a transaction receiptusing the provided HTTP client. + * Asynchronously fetches and processes a transaction receipt using the provided HTTP client. - * @return {Promise<ThorResponse<RetrieveRawBlock, GetTxReceiptResponse>>} + * @return {Promise<ThorResponse<RetrieveTransactionReceipt, GetTxReceiptResponse | null>>}packages/sdk/tests/thor/thorest/debug/TraceTransactionClause.unit.test.ts (2)
44-45: Prettier formatting nitApply the suggested line break for the second argument.
- mockHttpClientForDebug(mockResponse('Invalid target', status), 'post') + mockHttpClientForDebug( + mockResponse('Invalid target', status), + 'post' + )
49-53: Broadened assertions may hide regressionsSame as other test: prefer asserting ThorError once mocks are aligned.
Would you like help to refactor the mock to always produce HttpException/HttpNetworkException?
packages/sdk/src/thor/thorest/logs/methods/QuerySmartContractEvents.ts (1)
52-90: Adopt centralized HTTP error handling for consistencyOther thorest methods use try/catch + handleHttpError. Align this file to avoid divergent patterns and unreachable else when HttpClient throws on non‑OK.
+import { handleHttpError } from '@thor/thorest/utils'; @@ - async askTo( - httpClient: HttpClient - ): Promise<ThorResponse<QuerySmartContractEvents, EventLogsResponse>> { - const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<QuerySmartContractEvents, EventLogsResponse>>`; - const response = await httpClient.post( - QuerySmartContractEvents.PATH, - { query: '' }, - this.request.toJSON() - ); - if (response.ok) { - const json = (await response.json()) as EventLogsResponseJSON; - try { - return { - request: this, - response: new EventLogsResponse(json) - }; - } catch (error) { - throw new ThorError( - fqp, - error instanceof Error ? error.message : 'Bad response.', - { - url: response.url, - body: json - }, - error instanceof Error ? error : undefined, - response.status - ); - } - } - throw new ThorError( - fqp, - await response.text(), - { - url: response.url - }, - undefined, - response.status - ); + async askTo( + httpClient: HttpClient + ): Promise<ThorResponse<QuerySmartContractEvents, EventLogsResponse>> { + const fqp = `${FQP}askTo(httpClient: HttpClient): Promise<ThorResponse<QuerySmartContractEvents, EventLogsResponse>>`; + try { + const response = await httpClient.post( + QuerySmartContractEvents.PATH, + { query: '' }, + this.request.toJSON() + ); + const json = (await response.json()) as EventLogsResponseJSON; + try { + return { + request: this, + response: new EventLogsResponse(json) + }; + } catch (error) { + throw new ThorError( + fqp, + error instanceof Error ? error.message : 'Bad response.', + { + url: response.url, + body: json + }, + error instanceof Error ? error : undefined, + response.status + ); + } + } catch (error) { + throw handleHttpError(fqp, error); + } }packages/sdk/src/thor/thorest/accounts/methods/RetrieveContractBytecode.ts (2)
46-52: JSDoc wordingRefers to “block response”; this method returns contract bytecode.
- * Asynchronously fetches and processes a block response using the provided HTTP client. + * Asynchronously fetches and processes a contract bytecode response using the provided HTTP client. - * Returns a promise that resolves to a ThorResponse containing the requested block. + * Returns a promise that resolves to a ThorResponse containing the requested bytecode.
1-1: Type‑only import for HttpQueryUse type import to avoid unnecessary value import at runtime.
-import { HttpQuery, type HttpClient, type HttpPath } from '@common/http'; +import { type HttpQuery, type HttpClient, type HttpPath } from '@common/http';packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts (2)
46-52: JSDoc wordingMentions “block response”; this API returns storage position value.
- * Asynchronously fetches and processes a block response using the provided HTTP client. + * Asynchronously fetches and processes the storage value using the provided HTTP client. - * Returns a promise that resolves to a ThorResponse containing the requested block. + * Returns a promise that resolves to a ThorResponse containing the requested storage value.
1-1: Type‑only import for HttpQueryPrefer type import to avoid runtime footprint.
-import { HttpQuery, type HttpClient, type HttpPath } from '@common/http'; +import { type HttpQuery, type HttpClient, type HttpPath } from '@common/http';packages/sdk/src/thor/thorest/utils/HttpErrorHandler.ts (2)
6-6: Trim trailing spaces (ESLint/prettier)Run Prettier to remove trailing spaces flagged by linters.
If you want, I can add a lint‑fix script to CI to prevent this.
Also applies to: 27-27, 41-41
15-25: Ensure responseBody is stringifiedIf HttpException.responseBody can be non‑string (e.g., JSON), ThorError message may become “[object Object]”. Consider stringifying or picking a safe excerpt.
- error.responseBody, + typeof error.responseBody === 'string' + ? error.responseBody + : JSON.stringify(error.responseBody),packages/sdk/tests/thor/thorest/fees/RetrieveHistoricalFeeData.solo.test.ts (2)
25-29: Fix formatting issues flagged by Prettier.The code logic is correct - properly handling both
HttpExceptionandThorErrortypes with appropriate status extraction. However, there are formatting issues that need to be addressed.Apply this diff to fix the formatting issues:
- // Can receive either HttpException (direct from FetchHttpClient) or ThorError (wrapped) - expect([HttpException, ThorError]).toContain((error as Error).constructor); - const errorStatus = error instanceof HttpException ? error.status : (error as ThorError).status; - expect([0, 400]).toContain(errorStatus); + // Can receive either HttpException (direct from FetchHttpClient) or ThorError (wrapped) + expect([HttpException, ThorError]).toContain( + (error as Error).constructor + ); + const errorStatus = + error instanceof HttpException + ? error.status + : (error as ThorError).status; + expect([0, 400]).toContain(errorStatus);The same formatting fix should be applied to the second test case (lines 39-43).
Also applies to: 39-43
19-19: Remove extra whitespace.There are unnecessary extra spaces that should be removed for consistent formatting.
Apply this diff to fix the whitespace:
- const blockCount = 1.2; + const blockCount = 1.2;And:
- const blockCount = 0; + const blockCount = 0;Also applies to: 33-33
packages/sdk/src/thor/thorest/transactions/methods/SendTransaction.ts (1)
19-22: Doc link points to localhostThe inline link references a local Stoplight instance. Replace with public docs or a repo-relative reference.
packages/sdk/tests/thor/thorest/debug/RetrieveStorageRange.unit.test.ts (1)
57-61: Tighten the rejection assertion to the new error modelNow that errors are consistently mapped, assert
ThorErrordirectly and drop the constructor array check to reduce flakiness.Apply this diff:
- try { - await RetrieveStorageRange.of(request).askTo( - mockHttpClientForDebug<Response>( - mockResponse('body: invalid length', status), - 'post' - ) - ); - // noinspection ExceptionCaughtLocallyJS - throw new Error('Should not reach here.'); - } catch (error) { - // Can receive either Error (mock issues) or ThorError (proper error handling) - expect([Error, ThorError]).toContain((error as Error).constructor); - if (error instanceof ThorError) { - expect([0, 400]).toContain(error.status); - } - } + await expect( + RetrieveStorageRange + .of(request) + .askTo( + mockHttpClientForDebug<Response>( + mockResponse('body: invalid length', status), + 'post' + ) + ) + ).rejects.toBeInstanceOf(ThorError);If older mocks still bypass
handleHttpError, keep a fallbackinstanceof Errorbranch for that specific suite.packages/sdk/src/thor/thorest/accounts/methods/RetrieveAccountDetails.ts (1)
15-18: Doc link points to localhostUpdate to public docs or repo-relative reference.
packages/sdk/src/common/http/FetchHttpClient.ts (3)
109-125: Abort cleanup leaks the 'abort' listener; detach itYou add a listener but never remove it. Detach in
cleanupto avoid retaining closures in long‑lived clients.Apply this diff:
- private createAbortSignal(): AbortSignalWithCleanup | undefined { + private createAbortSignal(): AbortSignalWithCleanup | undefined { if (this.options.timeout === undefined) { return undefined; } const controller = new AbortController(); const timer = setTimeout(() => { controller.abort(); }, this.options.timeout); - const cleanup = (): void => { - clearTimeout(timer); - }; - controller.signal.addEventListener('abort', cleanup); + const onAbort = (): void => { + clearTimeout(timer); + }; + controller.signal.addEventListener('abort', onAbort); + const cleanup = (): void => { + clearTimeout(timer); + controller.signal.removeEventListener('abort', onAbort); + }; return { signal: controller.signal, cleanup }; }
356-396: Logging full request/response bodies can leak sensitive data and bloat logsGate or clamp logged bodies (e.g., redact by header, or limit to N KB). Consider an opt‑in
httpOptions.logBodiesormaxLogBodyBytes.I can draft a small redaction/clamping helper if you want it included here.
176-179: Nit: avoid passingnullforsignal
signalcan be omitted; passingnullis unnecessary.Apply this diff:
- signal: abortSignal?.signal ?? null + signal: abortSignal?.signalAlso applies to: 275-278
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (55)
packages/sdk/package.json(1 hunks)packages/sdk/src/common/http/FetchHttpClient.ts(5 hunks)packages/sdk/src/common/http/HttpException.ts(1 hunks)packages/sdk/src/common/http/HttpNetworkException.ts(1 hunks)packages/sdk/src/common/http/index.ts(1 hunks)packages/sdk/src/thor/index.ts(1 hunks)packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts(4 hunks)packages/sdk/src/thor/thorest/accounts/methods/RetrieveAccountDetails.ts(4 hunks)packages/sdk/src/thor/thorest/accounts/methods/RetrieveContractBytecode.ts(4 hunks)packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts(4 hunks)packages/sdk/src/thor/thorest/blocks/methods/RetrieveExpandedBlock.ts(4 hunks)packages/sdk/src/thor/thorest/blocks/methods/RetrieveRawBlock.ts(4 hunks)packages/sdk/src/thor/thorest/blocks/methods/RetrieveRegularBlock.ts(4 hunks)packages/sdk/src/thor/thorest/debug/methods/RetrieveStorageRange.ts(3 hunks)packages/sdk/src/thor/thorest/debug/methods/TraceCall.ts(2 hunks)packages/sdk/src/thor/thorest/debug/methods/TraceTransactionClause.ts(2 hunks)packages/sdk/src/thor/thorest/fees/methods/RetrieveHistoricalFeeData.ts(1 hunks)packages/sdk/src/thor/thorest/fees/methods/SuggestPriorityFee.ts(1 hunks)packages/sdk/src/thor/thorest/logs/methods/QuerySmartContractEvents.ts(1 hunks)packages/sdk/src/thor/thorest/logs/methods/QueryVETTransferEvents.ts(1 hunks)packages/sdk/src/thor/thorest/node/methods/GetTxPoolStatus.ts(1 hunks)packages/sdk/src/thor/thorest/node/methods/RetrieveConnectedPeers.ts(4 hunks)packages/sdk/src/thor/thorest/node/methods/RetrieveExpandedTransactionsFromTransactionPool.ts(1 hunks)packages/sdk/src/thor/thorest/node/methods/RetrieveTransactionsFromTransactionPool.ts(1 hunks)packages/sdk/src/thor/thorest/transactions/methods/RetrieveRawTransactionByID.ts(4 hunks)packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts(4 hunks)packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts(4 hunks)packages/sdk/src/thor/thorest/transactions/methods/SendTransaction.ts(4 hunks)packages/sdk/src/thor/thorest/utils/HttpErrorHandler.ts(1 hunks)packages/sdk/src/thor/thorest/utils/index.ts(1 hunks)packages/sdk/tests/MockHttpClient.ts(2 hunks)packages/sdk/tests/common/http/FetchHttpClient.logging.unit.test.ts(2 hunks)packages/sdk/tests/common/http/FetchHttpClient.unit.test.ts(2 hunks)packages/sdk/tests/common/logging/prettylogger.unit.test.ts(2 hunks)packages/sdk/tests/thor/thorest/accounts/RetrieveAccountDetails.unit.test.ts(1 hunks)packages/sdk/tests/thor/thorest/accounts/RetrieveContractBytecode.unit.test.ts(1 hunks)packages/sdk/tests/thor/thorest/accounts/RetrieveStoragePositionValue.unit.test.ts(1 hunks)packages/sdk/tests/thor/thorest/blocks/RetrieveExpandedBlock.solo.test.ts(1 hunks)packages/sdk/tests/thor/thorest/blocks/RetrieveRawBlock.solo.test.ts(1 hunks)packages/sdk/tests/thor/thorest/blocks/RetrieveRegularBlock.solo.test.ts(1 hunks)packages/sdk/tests/thor/thorest/debug/RetrieveStorageRange.solo.test.ts(1 hunks)packages/sdk/tests/thor/thorest/debug/RetrieveStorageRange.unit.test.ts(3 hunks)packages/sdk/tests/thor/thorest/debug/TraceCall.solo.test.ts(2 hunks)packages/sdk/tests/thor/thorest/debug/TraceCall.unit.test.ts(7 hunks)packages/sdk/tests/thor/thorest/debug/TraceTransactionClause.solo.test.ts(1 hunks)packages/sdk/tests/thor/thorest/debug/TraceTransactionClause.unit.test.ts(3 hunks)packages/sdk/tests/thor/thorest/fees/RetrieveHistoricalFeeData.solo.test.ts(2 hunks)packages/sdk/tests/thor/thorest/fork/ForkDetector.unit.test.ts(1 hunks)packages/sdk/tests/thor/thorest/transactions/RetrieveRawTransactionByID.unit.test.ts(5 hunks)packages/sdk/tests/thor/thorest/transactions/RetrieveTransactionByID.unit.test.ts(5 hunks)packages/sdk/tests/thor/thorest/transactions/RetrieveTransactionReceipt.unit.test.ts(3 hunks)packages/sdk/tests/thor/thorest/transactions/SendTransaction.unit.test.ts(2 hunks)packages/sdk/tests/thor/thorest/transactions/TransactionDelegationAndVerification.unit.test.ts(2 hunks)packages/sdk/tests/viem/clients/publicClient/events.solo.test.ts(1 hunks)packages/solo-setup/package.json(1 hunks)
🧰 Additional context used
🪛 ESLint
packages/sdk/src/thor/thorest/utils/HttpErrorHandler.ts
[error] 6-6: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 6-6: Delete ·
(prettier/prettier)
[error] 27-27: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 27-27: Delete ····
(prettier/prettier)
[error] 41-41: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 41-41: Delete ····
(prettier/prettier)
packages/sdk/tests/thor/thorest/blocks/RetrieveRegularBlock.solo.test.ts
[error] 25-25: Delete ········
(prettier/prettier)
packages/sdk/src/common/http/HttpException.ts
[error] 45-45: Constructor has too many parameters (8). Maximum allowed is 3.
(sonarjs/sonar-max-params)
packages/sdk/tests/common/http/FetchHttpClient.unit.test.ts
[error] 563-563: Replace 'HTTP·request·failed·with·status·400' with ⏎················'HTTP·request·failed·with·status·400'⏎············
(prettier/prettier)
[error] 645-645: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 645-645: Delete ············
(prettier/prettier)
packages/sdk/tests/thor/thorest/blocks/RetrieveExpandedBlock.solo.test.ts
[error] 25-25: Delete ········
(prettier/prettier)
packages/sdk/src/common/http/HttpNetworkException.ts
[error] 15-15: Replace ·'timeout'·|·'connection'·|·'no_response' with ⏎········|·'timeout'⏎········|·'connection'⏎········|·'no_response'⏎·······
(prettier/prettier)
packages/sdk/tests/common/logging/prettylogger.unit.test.ts
[error] 6-6: 'LogItem' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 6-6: Remove this unused import of 'LogItem'.
(sonarjs/unused-import)
packages/sdk/tests/thor/thorest/fees/RetrieveHistoricalFeeData.solo.test.ts
[error] 19-19: Delete ········
(prettier/prettier)
[error] 26-26: Replace (error·as·Error).constructor with ⏎················(error·as·Error).constructor⏎············
(prettier/prettier)
[error] 27-27: Replace ·error·instanceof·HttpException·?·error.status with ⏎················error·instanceof·HttpException⏎····················?·error.status⏎···················
(prettier/prettier)
[error] 33-33: Delete ········
(prettier/prettier)
[error] 40-40: Replace (error·as·Error).constructor with ⏎················(error·as·Error).constructor⏎············
(prettier/prettier)
[error] 41-41: Replace ·error·instanceof·HttpException·?·error.status with ⏎················error·instanceof·HttpException⏎····················?·error.status⏎···················
(prettier/prettier)
packages/sdk/tests/common/http/FetchHttpClient.logging.unit.test.ts
[error] 49-49: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 49-49: Delete ············
(prettier/prettier)
packages/sdk/tests/thor/thorest/debug/TraceCall.solo.test.ts
[error] 14-14: Delete ········
(prettier/prettier)
packages/sdk/tests/thor/thorest/debug/TraceCall.unit.test.ts
[error] 165-165: Replace mockHttpClientForDebug(mockResponse(expected,·200),·'post') with ⏎····················mockHttpClientForDebug(mockResponse(expected,·200),·'post')⏎················
(prettier/prettier)
[error] 198-198: Replace mockHttpClientForDebug(mockResponse(expected,·200),·'post') with ⏎····················mockHttpClientForDebug(mockResponse(expected,·200),·'post')⏎················
(prettier/prettier)
[error] 231-231: Replace mockHttpClientForDebug(mockResponse(expected,·200),·'post') with ⏎····················mockHttpClientForDebug(mockResponse(expected,·200),·'post')⏎················
(prettier/prettier)
packages/sdk/tests/thor/thorest/debug/TraceTransactionClause.unit.test.ts
[error] 44-44: Replace mockResponse('Invalid·target',·status),·'post' with ⏎····················mockResponse('Invalid·target',·status),⏎····················'post'⏎················
(prettier/prettier)
packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts
[error] 70-71: Delete ⏎···············
(prettier/prettier)
packages/sdk/tests/MockHttpClient.ts
[error] 10-10: Replace ·typeof·response·===·'object'·&&·response·!==·null·&& with ⏎········typeof·response·===·'object'·&&⏎········response·!==·null·&&⏎·······
(prettier/prettier)
[error] 26-26: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 26-26: Delete ············
(prettier/prettier)
[error] 34-34: Insert ⏎···················
(prettier/prettier)
[error] 36-36: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 36-36: Delete ············
(prettier/prettier)
[error] 48-48: Replace ·typeof·response·===·'object'·&&·response·!==·null·&& with ⏎········typeof·response·===·'object'·&&⏎········response·!==·null·&&⏎·······
(prettier/prettier)
[error] 64-64: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 64-64: Delete ············
(prettier/prettier)
[error] 77-78: Delete ⏎············
(prettier/prettier)
[error] 79-79: Delete ····
(prettier/prettier)
[error] 80-80: Replace ···················· with ················
(prettier/prettier)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 82-82: Replace ···················· with ················
(prettier/prettier)
[error] 83-83: Delete ····
(prettier/prettier)
[error] 84-84: Delete ····
(prettier/prettier)
[error] 85-85: Replace ···················· with ················
(prettier/prettier)
[error] 86-86: Delete ····
(prettier/prettier)
[error] 87-88: Replace ············}⏎········ with ········}
(prettier/prettier)
packages/sdk/src/common/http/FetchHttpClient.ts
[error] 185-185: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 185-185: Delete ············
(prettier/prettier)
[error] 203-203: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 203-203: Delete ············
(prettier/prettier)
[error] 213-213: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 213-213: Delete ············
(prettier/prettier)
[error] 230-230: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 230-230: Delete ············
(prettier/prettier)
[error] 234-234: Replace ·?·error.message with ⏎····················?·error.message⏎···················
(prettier/prettier)
[error] 284-284: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 284-284: Delete ············
(prettier/prettier)
[error] 303-303: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 303-303: Delete ············
(prettier/prettier)
[error] 313-313: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 313-313: Delete ············
(prettier/prettier)
[error] 331-331: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 331-331: Delete ············
(prettier/prettier)
[error] 335-335: Replace ·?·error.message with ⏎····················?·error.message⏎···················
(prettier/prettier)
🪛 GitHub Actions: CI-V3
packages/sdk/src/common/http/FetchHttpClient.ts
[error] 232-232: FetchError: request to http://localhost:8669/blocks/1?raw=false failed, reason: socket hang up.
packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/thor/thorest/accounts/methods/RetrieveAccountDetails.ts
Show resolved
Hide resolved
packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts
Outdated
Show resolved
Hide resolved
packages/sdk/src/thor/thorest/transactions/methods/SendTransaction.ts
Outdated
Show resolved
Hide resolved
packages/sdk/tests/thor/thorest/transactions/RetrieveTransactionReceipt.unit.test.ts
Show resolved
Hide resolved
giuliamorg92
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Giulia Argiolas <155436416+giuliamorg92@users.noreply.github.com>
…PositionValue.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Giulia Argiolas <155436416+giuliamorg92@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts (2)
68-75: Preserve more response diagnostics in error meta.Include statusText alongside url/body; helps triage without altering the constructor signature.
- { - url: response.url, - body: json - }, + { + url: response.url, + body: json, + statusText: (response as any).statusText + },
163-165: Prefer URLSearchParams for query construction (encoding, future‑proofing).Safer for non‑string revisions and avoids manual string concat.
- get query(): string { - return this.revision != null ? `?revision=${this.revision}` : ''; - } + get query(): string { + return this.revision != null + ? `?${new URLSearchParams({ revision: String(this.revision) }).toString()}` + : ''; + }packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts (3)
70-86: Avoid consuming the body twice; parse from text first (or use clone).
response.json()may consume the body; subsequentresponse.text()can be empty/unavailable, losing diagnostics. Parse fromtext()first so you can always attach the raw body to the error (or useresponse.clone()before parsing).Apply either of the following.
Option A (parse from text first — safest with custom HttpClient):
- let json: ExecuteCodesResponseJSON; - try { - json = (await response.json()) as ExecuteCodesResponseJSON; - } catch (parseErr) { - const body = await response.text().catch(() => undefined); - throw new ThorError( - fqp, - parseErr instanceof Error ? parseErr.message : 'Bad response.', - { - url: response.url, - status: response.status, - statusText: response.statusText, - body - }, - parseErr instanceof Error ? parseErr : undefined, - response.status - ); - } + const raw = await response.text(); + let json: ExecuteCodesResponseJSON; + try { + json = JSON.parse(raw) as ExecuteCodesResponseJSON; + } catch (parseErr) { + throw new ThorError( + fqp, + parseErr instanceof Error ? parseErr.message : 'Bad response.', + { + url: response.url, + status: response.status, + statusText: response.statusText, + body: raw + }, + parseErr instanceof Error ? parseErr : undefined, + response.status + ); + }Option B (if
Response.clone()is supported):- let json: ExecuteCodesResponseJSON; - try { - json = (await response.json()) as ExecuteCodesResponseJSON; - } catch (parseErr) { - const body = await response.text().catch(() => undefined); + const clone = response.clone(); + let json: ExecuteCodesResponseJSON; + try { + json = (await response.json()) as ExecuteCodesResponseJSON; + } catch (parseErr) { + const body = await clone.text().catch(() => undefined); throw new ThorError( fqp, parseErr instanceof Error ? parseErr.message : 'Bad response.', { url: response.url, status: response.status, statusText: response.statusText, body }, parseErr instanceof Error ? parseErr : undefined, response.status ); }
76-76: Fix Prettier violation.Format the conditional on a new line to satisfy
prettier/prettier.- parseErr instanceof Error ? parseErr.message : 'Bad response.', + parseErr instanceof Error + ? parseErr.message + : 'Bad response.',
106-107: Make handleHttpError idempotent for ThorError.Add an early guard in packages/sdk/src/thor/thorest/utils/HttpErrorHandler.ts: if (error instanceof ThorError) return error; — prevents double-wrapping and preserves the original status when callers use
throw handleHttpError(fqp, error);.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/sdk/src/thor/index.ts(1 hunks)packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts(3 hunks)packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts(3 hunks)packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts(2 hunks)packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts(3 hunks)packages/sdk/src/thor/thorest/transactions/methods/SendTransaction.ts(2 hunks)packages/sdk/tests/common/http/FetchHttpClient.logging.unit.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionByID.ts
- packages/sdk/tests/common/http/FetchHttpClient.logging.unit.test.ts
- packages/sdk/src/thor/thorest/transactions/methods/RetrieveTransactionReceipt.ts
🧰 Additional context used
🪛 ESLint
packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts
[error] 76-76: Replace ·?·parseErr.message with ⏎························?·parseErr.message⏎·······················
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: SonarCloud / Sonar Scan
🔇 Additional comments (8)
packages/sdk/src/thor/thorest/transactions/methods/SendTransaction.ts (4)
10-10: LGTM!The import of
handleHttpErrorcentralizes error handling across the Thor REST clients, promoting consistency.
57-64: LGTM!The HTTP client call is correctly structured within the try-catch block for centralized error handling.
65-85: This addresses the previous review concern.The implementation correctly handles JSON parsing errors by reading the response as raw text first, then parsing JSON within the inner try-catch block. This ensures that malformed JSON responses are properly surfaced as
ThorErrorrather than being misclassified as network errors.
86-88: LGTM!The outer catch block appropriately delegates to
handleHttpErrorfor consistent HTTP error handling across the codebase.packages/sdk/src/thor/index.ts (1)
3-8: Public API reshuffle — verify re‑exports, stop bypassing barrels, add migration noteFindings: rg output shows many internal files still import direct paths from @thor/thor-client/... and @thor/thorest (e.g. packages/sdk/src/viem/clients/PublicClient.ts, packages/sdk/src/thor/signer/Signer.ts, packages/sdk/src/thor/signer/PrivateKeySigner.ts).
Action: ensure packages/sdk/src/thor/index.ts does not produce duplicate/conflicting names; either normalize internal imports to the new barrels (recommended) or keep explicit paths and publish a clear release/migration snippet listing the recommended import paths and any breaking changes.
packages/sdk/src/thor/thorest/accounts/methods/InspectClauses.ts (3)
2-2: Centralized HTTP error mapping import — good.Importing
handleHttpErrorhere aligns this method with the new unified error strategy.
63-68: POST + outer try/catch structure looks good.This cleanly funnels transport/non‑OK errors to the handler.
87-105: Good: enriches domain error with HTTP context.Including
status,statusText, andurlimproves debuggability when buildingExecuteCodesResponsefails.
packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts
Show resolved
Hide resolved
packages/sdk/src/thor/thorest/accounts/methods/RetrieveStoragePositionValue.ts
Show resolved
Hide resolved
…PositionValue.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Giulia Argiolas <155436416+giuliamorg92@users.noreply.github.com>
giuliamorg92
deleted the
2413-sdkv3---replicate-the-feature-in-sdkv3
branch
…messages - 2329 porting
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Summary by CodeRabbit