Repository navigation
Dedup test coverage, rate limit decorator, and stellar-sdk version compatibility checking - #885
Merged
Merged
Conversation
…ecovery (Sorokit#863) The issue names a ContractEventStream class, which does not exist anywhere in this codebase; the relevant code is src/soroban/subscribeContractEvents.ts's subscribeContractEvents() function. Reading it closely: the described bug (no deduplication, duplicates corrupt consumers) does not match the current implementation either. A seen-event-id Map with TTL-bounded eviction (deduplicationTtlMs/deduplicationMaxSize options) already deduplicates both the current poll's events and gap-recovery events before either set reaches the caller's callback, and this logic already predates this issue. What genuinely did not exist: any test file for subscribeContractEvents.ts at all, despite this substantial dedup mechanism. Added src/soroban/subscribeContractEvents.test.ts covering the two scenarios this issue's acceptance criteria actually describe: - an event id reported again on a later poll (simulating a reconnect or retry that re-reports an already-seen event) must be delivered exactly once, while a genuinely new event on that same poll still gets through; - a gap-recovery fetch and the subsequent live poll can legitimately report the same event id (a real overlap window), and that must still only be delivered once. Both self-verified: temporarily replaced deduplicateEvents with an identity function, confirmed both tests fail with a clear diff showing the duplicate ids, then restored. npm run typecheck and npm run lint both clean.
createRateLimiter already existed (a sliding-window limiter with queueing, retry/backoff, and a bounded-queue RATE_LIMITED error) but was never applied to any actual SDK method, just exported unused from the public entry point. Added withRateLimit, which wraps an object of async methods so each one is routed through its own independent RateLimiter. Configurable per method (perMethod) or globally (default); a method with neither applicable is returned unwrapped and uncounted rather than silently falling back to some implicit limit. Each configured method gets its own limiter and queue, so one method being saturated cannot starve another's. A call that exceeds a configured maxQueueSize throws SorokitRateLimitError (carrying the original SorokitResult, so error.result.error.code is inspectable) rather than returning a SorokitResult, since the wrapped methods preserve their original throwing call signature instead of changing every method's return shape to SorokitResult. Deliberately built as a standalone decorator rather than modifying createSorokitClient.ts (2861 lines) directly: the issue's own wording is 'add optional rate limiting middleware/decorator to SDK methods,' and wrapping is safely additive against the existing client without touching its internals. Wiring it into createSorokitClient's own default method set is left to a follow-up. Exported withRateLimit, SorokitRateLimitError, AsyncMethod, MethodRateLimitConfig, and RateLimitDecoratorConfig from the public entry point (src/index.ts), alongside the already-exported createRateLimiter and RateLimiterOptions/RateLimiter types. Tests: per-method config overriding the global default, a method with no applicable config staying completely unlimited, the global default applying when no per-method entry exists, and two configured methods having fully independent limiters (saturating one does not affect the other). Self-verified by breaking the per-method config resolution (forcing it to always use the global default) and confirming the override test fails with a clear diff before restoring. An @example JSDoc block on withRateLimit shows configuring a tighter per-method limit for a more expensive call alongside a looser global default, and catching SorokitRateLimitError, per the issue's own 'Examples show how to configure limits' acceptance criterion. npm run typecheck, npm run lint, npm run test (3869 passed, 80 skipped, no regressions), and npm run build all clean.
…Sorokit#865) The issue names @stellar/js-sdk, which is not this SDK's actual dependency; the real peer dependency is @stellar/stellar-sdk, pinned to an exact version (13.3.0, not a range) in package.json. npm itself would normally catch an install-time mismatch given that exact pin, but that protection is silently bypassed by --legacy-peer-deps (which this very repo's own CI pipeline uses) or a nested duplicate install of a different major version elsewhere in the dependency tree, so a runtime check still has real value. Added src/client/sdkVersionCompatibility.ts: - parseSemverCore: parses major.minor.patch, ignoring prerelease/build suffixes. - compareStellarSdkVersion: compares an installed version against the version this SDK was built against (SUPPORTED_STELLAR_SDK_VERSION, kept in sync with package.json's peer dependency). A major version difference, or an installed version strictly older than supported, is incompatible; a newer minor/patch within the same major is incompatible but framed as lower-severity (same-major releases are usually additive). - checkStellarSdkVersionCompatibility: runs the comparison and logs a warning for a non-compatible result via a SorokitLogger. Never throws; a mismatch is a diagnostic aid, not a hard failure. Does not attempt to auto-detect the installed version from node_modules: @stellar/stellar-sdk's own package.json confirms ./package.json is absent from its exports map, so require(@stellar/stellar-sdk/package.json) throws under strict Node ESM/CJS resolution and an equivalent dynamic import() fails identically, confirmed directly against the installed package, not assumed. The caller supplies the installed version however suits their own module resolution (documented with a @example showing createRequire for a plain Node/CJS entry point). Wired into createSorokitClient via a new optional SorokitClientConfig.stellarSdkVersion field, checked right after the client's logger is constructed. Omitting it (the default) skips the check entirely rather than silently failing or guessing. Exported checkStellarSdkVersionCompatibility, compareStellarSdkVersion, parseSemverCore, SUPPORTED_STELLAR_SDK_VERSION, and the StellarSdkCompatibility/ParsedVersion types from the public entry point. Tests: parseSemverCore (plain, prerelease-suffixed, and unparseable strings), compareStellarSdkVersion (compatible, unknown for missing or unparseable input, major-mismatch in both directions, older-than and newer-than within the same major), checkStellarSdkVersionCompatibility (logs only on incompatible, structured log context, no logger supplied, no installed version supplied), and an integration test on createSorokitClient itself confirming the config field actually wires through to a real warning. Self-verified twice: disabled the major-mismatch branch in the comparator and confirmed 3 tests fail with a clear diff; separately removed the wiring call in createSorokitClient.ts and confirmed the integration test fails. Both restored and reconfirmed clean afterward. npm run typecheck, npm run lint, npm run test (3886 passed, 80 skipped, no regressions), and npm run build all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Processed 4 assigned issues. 3 got genuine, tested implementations; the
4th is disclosed below as already resolved on main, not closed by this PR.
closes #863
closes #864
closes #865
Changes
ContractEventStreamdoes not deduplicate events — same event can be emitted multiple times #863 - ContractEventStream event deduplication: the namedContractEventStream class does not exist; the relevant code is
src/soroban/subscribeContractEvents.ts's subscribeContractEvents()
function. Reading it closely, a seen-event-id Map with TTL-bounded
eviction already deduplicates both the current poll's events and
gap-recovery events before either reaches the caller's callback, and
predates this issue. What genuinely did not exist: any test file for
this function at all. Added subscribeContractEvents.test.ts covering an
event id re-reported on a later poll (simulating reconnect/retry) and a
gap-recovery fetch overlapping the next live poll's report of the same
event, both must be delivered exactly once.
SDK does not provide built-in rate limiting for user-facing APIs — callers can DOS their own app #864 - optional rate limiting for SDK methods: createRateLimiter
(a sliding-window limiter with queueing, retry/backoff, and a
bounded-queue RATE_LIMITED error) already existed but was never applied
to any actual method. Added withRateLimit, a decorator that wraps an
object of async methods so each one is routed through its own
independent limiter, configurable per method or globally. A method with
no applicable config is returned unwrapped and uncounted. A call
exceeding a configured queue size throws SorokitRateLimitError (carrying
the original SorokitResult for structured inspection). Built as a
standalone decorator rather than modifying createSorokitClient.ts (2861
lines) directly, matching the issue's own wording ("add optional rate
limiting middleware/decorator"); wiring it into the client's own default
method set is left to a follow-up. Includes an @example JSDoc block
showing per-method vs. global configuration and catching the error type.
@stellar/js-sdkversion mismatch detection is missing — SDK and dependency may diverge #865 - @stellar/stellar-sdk version compatibility checking: theissue names @stellar/js-sdk, which is not this SDK's actual dependency
(the real one is @stellar/stellar-sdk, pinned to an exact version in
package.json, not a range). Added sdkVersionCompatibility.ts:
parseSemverCore, compareStellarSdkVersion (major mismatch in either
direction is incompatible; same-major newer is incompatible but framed
as lower severity; same-major older-than-supported is incompatible), and
checkStellarSdkVersionCompatibility, which logs a warning (never throws)
for a non-compatible result. Wired into createSorokitClient via a new
optional stellarSdkVersion config field, checked right after the
client's logger is constructed. Does not attempt to auto-detect the
installed version: confirmed directly that
@stellar/stellar-sdk's package.json is absent from its own exports map,
so reading it via require or a dynamic import both throw under strict
Node resolution; the caller supplies the version however suits their own
module resolution instead.
Test plan
ContractEventStreamdoes not deduplicate events — same event can be emitted multiple times #863:npx vitest run src/soroban/subscribeContractEvents.test.ts(2 new tests). Self-verified by replacing the dedup filter with an
identity function and confirming both tests fail with a clear diff
showing the duplicate ids, then restoring.
npx vitest run src/utility/rateLimiter.test.ts(5 new testsalongside the 5 pre-existing ones, all pass). Self-verified by forcing
per-method config resolution to always fall back to the global default
and confirming the override test fails, then restoring.
@stellar/js-sdkversion mismatch detection is missing — SDK and dependency may diverge #865:npx vitest run src/client/sdkVersionCompatibility.test.ts(15 new tests) plus 2 new integration tests in
createSorokitClient.test.tsconfirming the config field actually wiresthrough to a real logger call. Self-verified twice: disabled the
major-mismatch branch in the comparator (3 tests failed with a clear
diff) and separately removed the wiring call in createSorokitClient.ts
(the integration test failed), both restored and reconfirmed clean.
npm run typecheck,npm run lint,npm run test(3886passed, 80 skipped, no regressions from a clean baseline), and
npm run buildall clean, per this repo's required CI commands.Disclosures
StellarTomlResolverdoes not support.well-known/stellar.tomlfallback — fails if TOML is at legacy path #862 (StellarTomlResolver.well-knownfallback) is already fullyresolved on
main, not closed by this PR. Readsrc/integration/sep1Toml.tsin full: there is noStellarTomlResolverclass anywhere in this codebase (confirmed via repo-wide grep), and the
actual
fetchStellarTomlfunction already fetches/.well-known/stellar.tomlexclusively and has never had a root-pathfetch to fall back from, confirmed via this file's own git history back
to its original implementation. The existing test suite
(
sep1Toml.test.ts) already asserts the.well-known/path is the onlyone used. No code change was needed; the issue's described bug does not
match this version of the code.