Skip to content

Feat/telemetry#26

Open
Arshia-r-m wants to merge 19 commits into
masterfrom
feat/telemetry
Open

Feat/telemetry#26
Arshia-r-m wants to merge 19 commits into
masterfrom
feat/telemetry

Conversation

@Arshia-r-m
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Member

@moakilodash moakilodash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@moakilodash
Copy link
Copy Markdown
Member

though i suggest using v0.2.0 as this has breaking changes.

Copy link
Copy Markdown
Collaborator

@bitwalt bitwalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Telemetry review — SDK #26

Blocker

1. Version — breaking change shipped as patch. create()/from_config going async is a hard breaking change, but the version stays 0.1.6 and the X-Kaleido-SDK header too. Every existing caller breaks on a patch bump. → 0.2.0 + sync the header.

Majors

2. http-client — no TLS guard on attribution headers. Authorization: Bearer kld_live_... + install/session headers are attached regardless of scheme. A http:// base URL leaks the live API key in cleartext. Refuse to attach Authorization over non-https (allow an explicit allow_insecure opt-out for localhost).

3. identity.ts — browser install-id is a persistent supercookie. localStorage["kld_install_id"] is a stable cross-session device identifier sent on every request with no consent — privacy/GDPR concern for a partner-embedded SDK. Make persistence opt-in or default to in-memory in browsers.

4. _identity.py — TOCTOU on install-id file. write_text then chmod 0600 leaves the file briefly world-readable. Use os.open(..., O_CREAT|O_EXCL, 0o600). Also: the TS random fallback to Math.random() when crypto is absent should warn/throw.

Identity design itself is reasonable; the async create() conversion is clean. Main fixes: semver bump and the cleartext-key TLS guard before this leaves Draft.

@Arshia-r-m Arshia-r-m marked this pull request as ready for review May 19, 2026 17:59
Copilot AI review requested due to automatic review settings May 19, 2026 17:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds telemetry/attribution support to both the Python and TypeScript SDKs. The Maker API now receives persistent install IDs, per-client session IDs, and an SDK version header, with HTTPS enforcement (and a localhost/explicit allowInsecure escape hatch) for any request that carries an API key or attribution. To support filesystem/browser-storage lookups during identity resolution, KaleidoClient.create() / from_config() becomes async in both SDKs — a documented breaking change.

Changes:

  • New identity module in each SDK that generates and persists install IDs (Node file at ~/.kaleido/install_id with 0600; browser memory-only by default, with opt-in localStorage persistence) and per-session UUIDs.
  • HttpClient now emits Authorization, X-Kaleido-Install-Id, X-Kaleido-Session-Id, and X-Kaleido-SDK headers for Maker requests, refusing apiKey over non-HTTPS unless local or allowInsecure.
  • KaleidoClient.create() becomes async; examples, READMEs, and tests are updated accordingly; CHANGELOG documents the breaking change.

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
typescript-sdk/src/identity.ts New install/session ID generation and pluggable storage (memory/localStorage/Node FS)
typescript-sdk/src/http-client.ts Adds Maker attribution headers + HTTPS guard with localhost/allowInsecure exemptions
typescript-sdk/src/client.ts create() is now async; resolves install + session IDs and passes them to HttpClient; hard-codes SDK_VERSION = '0.1.6'
typescript-sdk/src/types/config.ts New allowInsecure, installId, installIdStore, persistInstallId config fields
typescript-sdk/src/index.ts, src/logging.ts Exports identity helpers; doc snippets updated to await
typescript-sdk/tests/** New identity unit tests; HttpClient header/HTTPS tests; integration/type-safety tests adapted to async create()
typescript-sdk/examples/*.ts Updated to await KaleidoClient.create(...) and read optional KALEIDO_API_KEY; prettier reformatting
typescript-sdk/README.md Documents attribution behavior, local-dev override, and browser persistence opt-in
python-sdk/kaleido_sdk/_identity.py New install/session ID helpers with atomic 0o600 file creation and asyncio.to_thread wrapper
python-sdk/kaleido_sdk/_http_client.py Splits default vs maker headers; adds HTTPS/localhost/allow-insecure validation; emits attribution headers per request
python-sdk/kaleido_sdk/client.py create() and from_config() are now async; resolve install/session IDs
python-sdk/kaleido_sdk/types.py Adds allow_insecure, install_id, session_id fields to KaleidoConfig
python-sdk/tests/** New identity/header tests; fixtures updated to bypass async create() by constructing the client directly
python-sdk/examples/*.py Updated to await KaleidoClient.create(...) and read optional KALEIDO_API_KEY
python-sdk/README.md, kaleido_sdk/init.py Documents async usage and attribution behavior
CHANGELOG.md Records the new attribution features and the async breaking change
Comments suppressed due to low confidence (1)

python-sdk/kaleido_sdk/client.py:141

  • The replace(config, install_id=..., session_id=...) here unconditionally overwrites the caller's config.install_id with whatever load_or_create_install_id returns. That branch already handles the override case (returning it unchanged), so behavior is correct today, but it does mean any explicit install_id passed via the dataclass goes through file-IO resolution on a worker thread. More importantly, this path silently ignores the new allow_insecure setting at validation time — the only validation happens lazily on the first Maker request inside _build_maker_headers. Consider validating (base_url, api_key, allow_insecure) once during client construction so misconfigurations fail fast rather than on the first attributed call.
        resolved_config = replace(
            config,
            install_id=await load_or_create_install_id(config.install_id),
            session_id=config.session_id or generate_session_id(),
        )
        return cls(resolved_config)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread typescript-sdk/src/identity.ts
Comment thread typescript-sdk/src/client.ts Outdated
Comment thread python-sdk/kaleido_sdk/_http_client.py
Comment thread python-sdk/kaleido_sdk/client.py
Copy link
Copy Markdown
Member

@moakilodash moakilodash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great, idk if the same could be done for the python sdk as well or not @Arshia-r-m?

Arshia-r-m and others added 8 commits May 20, 2026 18:07
…opment support

- Added `allow_insecure` option for non-HTTPS Maker URLs.
- Implemented browser `persistInstallId` for stable attribution.
- Hardened Python install ID file creation for security.
- Updated documentation for attribution and local development.
- Added tests for secure API key handling and install ID persistence.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.

Comment thread python-sdk/README.md
Comment thread python-sdk/README.md
Comment on lines +46 to +58
## Attribution and local development

When `api_key` is configured, the SDK sends Maker attribution headers over HTTPS only.
HTTP is allowed automatically for local development hosts such as `localhost` and
`127.0.0.1`. To use a non-local HTTP Maker URL intentionally, pass
`allow_insecure=True`.

```python
client = await KaleidoClient.create(
base_url="http://dev-maker.internal:8000",
api_key="kld_live_c_...",
allow_insecure=True,
)
Comment on lines 28 to +33

_log = get_logger("http")
_SDK_HEADER = "python/0.1.6"
_LOCAL_HTTP_HOSTS = {"localhost", "127.0.0.1", "::1"}


Comment on lines +187 to +195
export async function loadOrCreateInstallId({
override,
store,
persistBrowser,
}: {
override?: string;
store?: InstallIdStore;
persistBrowser?: boolean;
} = {}): Promise<string> {
Comment thread typescript-sdk/src/client.ts Outdated
Comment on lines +10 to +17
import { LogState, applyLogLevel, setComponentLogLevel, setLogger } from './logging.js';
import { generateSessionId, loadOrCreateInstallId } from './identity.js';
import packageJson from '../package.json';
import type { LogLevel, LogLevelName, SdkLogger } from './logging.js';
import type { KaleidoConfig } from './types/config.js';

const SDK_VERSION = packageJson.version;

Arshia-r-m and others added 4 commits May 21, 2026 14:15
A1 — python: forward access_token through wait_for_swap_completion
  - Add optional access_token: str | None to SwapCompletionOptions
  - Propagate it onto SwapOrderStatusRequest only when set, keeping
    anonymous polling byte-identical on the wire
  - Authenticated callers can now reach their real order state instead
    of silently seeing an empty/stale order
  - New TestWaitForSwapCompletionAccessToken unit class covers both
    omitted-token and supplied-token paths; SwapCompletionOptions
    dataclass tests assert the new default and custom value

A2 — typescript: port the python retry loop to HttpClient
  - Add maxRetries (default 3 via KaleidoClient, 0 on bare HttpClient)
    and retryBaseDelayMs (default 1000) to HttpClientConfig and
    KaleidoConfig
  - Replace _createFetchWithTimeout with _createFetchWithRetry that
    combines per-attempt timeout and exponential backoff
    (baseDelayMs * 2 ** attempt), mirroring python-sdk/_http_client.py
  - Retry on network errors, per-attempt timeouts, HTTP >= 500, and
    HTTP 429; do not retry on other 4xx or upstream-aborted requests
  - 6 new unit tests under "Retry behaviour" exercising each case

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes 7 of the 9 medium-severity parity items found in the audit. All
changes are additive — no existing call sites break.

Python additions:
  - has_maker() method on KaleidoClient mirroring the TS counterpart
  - SdkLogger Protocol + _SdkLoggerHandler bridge in _logging, plus a
    new KaleidoConfig.logger field and create(logger=...) kwarg. Any
    object with debug/info/warning/error methods can now collect SDK
    log records (loguru, structlog wrappers, test recorders).
  - _utils/asset_pair_mapper.py — full port of the TS AssetPairMapper
    utility (AssetPairMappedAsset TypedDict, find_by_ticker / find_by_id
    / can_trade / get_trading_partners / find_pair_by_tickers /
    get_active_pairs). Exported from the package root.

TypeScript additions:
  - KaleidoClient.fromConfig() static factory mirroring Python's
    from_config; useful when config is loaded from a file
  - NodeInstallIdStore now honours the KALEIDO_INSTALL_ID_PATH env var
    (matching the Python override mechanism) and writes the install ID
    with flag: 'wx' (O_EXCL semantics) so a concurrent writer can never
    clobber an existing value
  - WSClient gains a userId config; MakerClient.enableWebSocket(url,
    userId?) propagates it, mirroring python-sdk enable_websocket
  - importNodeModule rewritten to use await import(/* @vite-ignore */ ...)
    with a new Function fallback. Fixes ERR_VM_DYNAMIC_IMPORT_CALLBACK_
    MISSING under vitest while keeping browser-bundler safety.

Symmetric test coverage:
  - Python: TestHasMaker (2), TestCustomLogger (2), TestAssetPairMapper
    (3), TestHttpRetryBehaviour (6, mirroring TS retry tests),
    TestEnableWebsocketUserId (3), TestInstallIdRaceSafety (2)
  - TypeScript: waitForSwapCompletion accessToken forwarding (2),
    hasMaker (2), custom logger forwarding + idempotent swap (2),
    AssetPairMapper case-insensitive lookup / trading-partner / pair-
    direction tests (3), env-var override (1), race-safety (1),
    userId precedence (2), fromConfig (2)

Newly surfaced (added to audit ledger as R8/R9, not fixed here):
  - Python RateLimitError.is_retryable() returns False so 429 is NOT
    retried; TS retries 429 alongside 5xx
  - Python WSClient lets user_id override an embedded URL client ID;
    TS gives the embedded ID precedence

Tests covering both surface each SDK's current behaviour with comments
flagging the divergence so neither side drifts further before the
reconciliation batch.

Intentionally skipped:
  - cache_ttl in TS — Python's cache_ttl is declared but never read
    anywhere; ported nothing rather than carry dead config across
  - multipart in TS — postAssetMedia already uses FormData via
    bodySerializer, functionally equivalent to Python's node_post_
    multipart

Test results:
  - Python: 115 unit tests pass (up from 97 post-Batch-A)
  - TypeScript: 144 unit tests pass (up from 129 post-Batch-A)
  - tsc build: clean

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes the 5 type-name drifts surfaced in the audit. Aliases are
non-deprecated additions so call sites written against either spelling
keep compiling on both SDKs.

Aliases added:
  - EstimateFeesRequest <-> EstimateLspFeesRequest
  - EstimateFeesResponse <-> EstimateLspFeesResponse
  - ChannelOrderResponse <-> GetLspOrderResponse
  - LNInvoiceResponse <-> CreateLNInvoiceResponse
  - EmptyResponse <-> MakerExecuteResponse
  - NetworkInfoResponse <-> NodeNetworkInfoResponse (Python rln submodule)

TypeScript additions:
  - EstimateFeesRequest re-exported from api-types-ext
  - LNInvoiceResponse re-exported from node-types-ext

Python additions:
  - EstimateLspFeesRequest / EstimateLspFeesResponse / GetLspOrderResponse
    re-exported from kaleido_sdk.types and the package root
  - CreateLNInvoiceResponse / MakerExecuteResponse / NodeNetworkInfoResponse
    re-exported from kaleido_sdk.rln

Symmetric tests:
  - Python: TestCrossSdkTypeNameAliases asserts `is` identity for all 5
    aliases — catches future regenerations that rebind or drop the alias
  - TypeScript: public type-name aliases (Batch F) uses compile-time
    assignment compatibility (type aliases evaporate at runtime, so
    mutual assignment is the only honest check)

Test results:
  - Python: 120 unit tests pass (up from 115)
  - TypeScript: 147 unit tests pass (up from 144)
  - tsc build: clean

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ch H)

The single test_client.py file had grown to 1080 lines covering 19 test
classes across at least 6 distinct concerns (client lifecycle, HTTP
transport, identity/telemetry, MakerClient methods, RlnClient methods,
WebSocket, public exports). Splitting it into focused files mirrors the
layout the TypeScript SDK already uses and makes each concern easier to
find and extend.

New files:
  - test_http_client.py — TestConnectionErrorHandling, TestHttpRetryBehaviour
  - test_identity.py — TestIdentity, TestInstallIdRaceSafety
  - test_maker_client.py — TestWaitForSwapCompletionAccessToken
  - test_rln_client.py — TestCreateUtxosFeeRate, TestDecodeRgbInvoiceType,
    TestMakerExecuteType, TestSyncRgbWalletRequest,
    TestListAssetsEnumSerialization, TestListAssetsIfaParsing,
    TestUnlockWalletTimeoutHandling
  - test_public_exports.py — TestAssetPairMapper,
    TestCrossSdkTypeNameAliases

Existing files modified:
  - test_client.py slimmed from 1080 to 192 lines; keeps TestKaleidoClient,
    TestUtilityFunctions, TestHasMaker, TestCustomLogger (the bits that
    genuinely belong to the KaleidoClient surface)
  - test_websocket.py absorbs the user_id parity tests instead of
    introducing a parallel test_ws_client.py

No test was removed or renamed; the split is purely structural. The
suite still reports 120/120 passing unit tests, and the TypeScript SDK
layout is unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Arshia-r-m and others added 6 commits May 21, 2026 15:19
BREAKING CHANGES (target 0.2.0):

B1 — typescript: sendBtc now returns SendBtcResponse instead of void
  Previously the response body (which carries the broadcast txid) was
  swallowed by assertResponse() and discarded. Callers can now read the
  txid directly:
      const { txid } = await rln.sendBtc({ ... });
  Matches the Python SDK's send_btc(...) contract.

B2 — python: connect_peer now returns EmptyResponse instead of None
  The /connectpeer endpoint returns an empty JSON body per the OpenAPI
  spec, so the model is intentionally empty — but surfacing it (rather
  than None) keeps the return-type semantics consistent with the
  TypeScript SDK and with other empty-bodied endpoints such as
  maker_execute(). A ConnectPeerResponse = EmptyResponse alias is also
  re-exported from kaleido_sdk.rln so call sites written against the
  TypeScript name keep compiling.

Migration:
  - Callers that ignored the return value of either method keep working.
  - The only callers that break are those doing
      `isinstance(result, type(None))` (Python) or
      `if ((await sendBtc(...)) === undefined)` (TypeScript)
    which are vanishingly rare.

Symmetric test coverage:
  - python: TestSendBtcReturnType (locks in the existing contract that
    send_btc returns SendBtcResponse with a txid); TestConnectPeerReturnType
    (asserts isinstance EmptyResponse + alias identity)
  - typescript: rln-client.test.ts gains sendBtc-returns-txid and
    connectPeer-returns-empty cases

Test results:
  - Python: 123 unit tests pass (up from 120)
  - TypeScript: 149 unit tests pass (up from 147)
  - tsc build: clean

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BREAKING CHANGES (target 0.2.0):

G1 — python: RateLimitError now extends APIError
  Was a direct subclass of KaleidoError, while the TypeScript SDK had
  it as APIError. Aligning the parent class lets generic API-error
  handlers also catch rate-limit failures via `isinstance(err, APIError)`.
  The .code attribute is intentionally re-pinned to "RATE_LIMIT_ERROR"
  after super().__init__() so existing consumers that match on the
  code string keep working.

R8 — python: 429 responses are now retried with exponential backoff
  RateLimitError.is_retryable() previously returned False, meaning rate-
  limit responses surfaced immediately even with max_retries > 0.
  Aligned with the TypeScript SDK and the HTTP Retry-After convention:
  the HTTP layer now retries 429 alongside 5xx. Callers who want the
  previous fail-fast behaviour should set max_retries=0.

G2 — typescript: KaleidoClient.rln getter throws NodeNotConfiguredError
  Previously the getter returned an RlnClient unconditionally and the
  underlying http.node call would throw lazily on first request. Now it
  fails fast at the point of access, matching the Python SDK's @Property
  and giving a clearer stack trace at the call site.

R9 — typescript: WSClient userId overrides embedded URL client ID
  Previously an embedded client ID in the WebSocket URL took precedence
  over the userId config; Python has always treated the caller-supplied
  id as authoritative. The new precedence is:
    1. Explicit userId (URL is rebuilt with this id appended)
    2. Embedded URL client id
    3. Fresh crypto.randomUUID()
  Callers who depend on the old behaviour should drop the userId config.

Test updates:
  - Symmetric tests already added in earlier batches (TestEnableWebsocket-
    UserId, retry-on-429) had their assertions flipped to reflect the new
    canonical behaviour, with comments referencing this batch.
  - New tests: TestRateLimitError extends_api_error (python),
    KaleidoClient.rln throws NodeNotConfiguredError (typescript),
    keeps embedded ID when no userId supplied (typescript),
    max_retries=0 disables 429 retries (python).

Test results:
  - Python: 125 unit tests pass (up from 123 after Batch B)
  - TypeScript: 152 unit tests pass (up from 149 after Batch B)
  - tsc build: clean

Closes audit ledger items: G1, G2, R8, R9.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e configs to seconds (Batches C + D)

BREAKING CHANGES (target 0.2.0):

C — route method canonicalization (Python + TypeScript)
  The OpenAPI spec exposes two distinct route operations
  (`getMarketPairRoutes` and `discoverMarketRoutes`); the SDKs had drifted
  to incompatible names and request/response shapes. Aligned both:
    - `PairRoutesRequest` / `PairRoutesResponse` ↔ pair routes endpoint
      (`POST /api/v1/market/pairs/routes`)
    - `RoutesRequest` / `RoutesResponse` ↔ market route discovery endpoint
      (`POST /api/v1/market/routes`)

  Python `MakerClient.get_pair_routes()` now accepts a spec-aligned
  `PairRoutesRequest` body and returns `PairRoutesResponse`. A pair-ticker
  string is still accepted as a temporary shorthand. A new
  `get_pair_routes_by_ticker(pair_ticker)` companion preserves the legacy
  `list[SwapRoute]` shape for callers who relied on it.

  TypeScript `getPairRoutes()` now takes `PairRoutesRequest`; the old
  `RoutesRequest` / `RoutesResponse` (which had previously aliased the
  pair-routes endpoint) are now bound to market route discovery, matching
  Python. `DiscoverRoutesRequest` / `DiscoverRoutesResponse` are kept as
  `@deprecated` aliases of the new `RoutesRequest` / `RoutesResponse` so
  TS callers compile through the migration.

D — TypeScript time configs normalized to seconds
  `SwapCompletionOptions.timeout` / `pollInterval` and
  `WSClientConfig.reconnectDelay` / `pingInterval` are now interpreted as
  SECONDS (matching the Python SDK) instead of milliseconds.

  A one-version migration aid: each field has a `@deprecated *Ms` companion
  (`timeoutMs`, `pollIntervalMs`, `reconnectDelayMs`, `pingIntervalMs`)
  that callers can use to keep passing millisecond values until they're
  ready to switch. A `configTimeToMilliseconds(seconds, deprecatedMs,
  defaultSeconds)` helper centralises the conversion in both client files.

  Internal WSClient fields renamed `reconnectDelay`/`pingInterval` →
  `reconnectDelayMs`/`pingIntervalMs` so the unit boundary is explicit in
  the codebase.

  `typescript-sdk/examples/04_create_swap_order.ts` updated to use the new
  seconds-based shape.

Symmetric test coverage:
  - python: TestPairRoutes (3 tests covering new request/response,
    by-ticker companion, and market routes); new
    test_treats_canonical_swap_completion_intervals_as_seconds and
    test_config_delays_are_seconds for D
  - typescript: route methods × 2 in maker-client.test.ts; "treats
    canonical swap completion intervals as seconds" and "accepts
    deprecated swap completion millisecond aliases" using fake timers;
    "normalizes WebSocket config delays from seconds to timer
    milliseconds" and "accepts deprecated WebSocket millisecond delay
    aliases" for the WS config
  - integration tests on both sides updated to assert the new response
    shape (`hasattr(routes, "routes")` / `expect(routes).toHaveProperty('routes')`)

Migration notes (also in CHANGELOG):
  - TS callers passing `timeout: 60000` should now pass `timeout: 60` (or
    keep using `timeoutMs: 60000` during the transition).
  - Python callers calling `get_pair_routes("BTC/USDT")` keep working but
    now receive `PairRoutesResponse` instead of `list[SwapRoute]`; switch
    to `get_pair_routes_by_ticker("BTC/USDT")` to keep the old shape.

Test results:
  - Python: 130 unit tests pass (up from 125)
  - TypeScript: 158 unit tests pass (up from 152)
  - tsc build: clean

Closes audit ledger items: R3 (getPairRoutes), R4 (getMarketRoutes),
R5 (wait_for_swap_completion unit drift — the access_token half shipped
in Batch A; this commit covers the ms-vs-seconds half).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants