SDK review improvements: fixes, hardening, performance and test coverage#92
Merged
Conversation
- Reject challenge transactions without time bounds, as required by SEP-10 - Verify time bounds and the server signature once per challenge instead of once per operation - Return a clearer error for challenges with infinite (zero) maximum time bounds - Fix the home-domain validation error to report an invalid home domain instead of an invalid source account
Cover the challenge-submission error responses, fromDomain misconfiguration, and challenge validation rejections (invalid base64, missing memo, memo with muxed account).
The hint computation threw a TypeError on payloads shorter than the 4-byte hint. Right-pad the payload with zeros and XOR byte-for-byte with the key hint, matching CAP-40.
…coding Correct the restore-footprint and extend-footprint-TTL "missing operation" error messages, and rename two misspelled private helpers in AbstractOperation::fromXdr.
Memoize the stroop scale, maximum, and zero values as lazily-initialized static BigIntegers instead of re-parsing them on every StellarAmount construction. Behavior is unchanged. Add a boundary test that rejects exactly one stroop above the maximum.
Memoize the native-endianness check in a static field instead of recomputing it on every signed integer encode, matching XdrDecoder. Behavior is unchanged.
Read the event stream in chunks and extract complete lines from a buffer instead of reading one byte at a time, and reconnect on a closed stream instead of looping. Behavior is unchanged. Add unit tests for the line extraction/SSE parsing and end-to-end tests for getAndStream dispatch, handshake handling, and byebye reconnect.
Avoid a redundant decode in XdrDecoder::boolean, read unpack results by key instead of mutating the array, compute opaqueFixed length once, and use an integer literal for the max-length bound. Behavior is unchanged. Add codec primitive tests, including the previously-untested boolean rejection path.
Query each payment destination account at most once instead of issuing a separate request per operation. Behavior is unchanged. Add unit tests for checkMemoRequired, including memo-required detection and destination de-duplication.
Relocate the testanchor.stellar.org authentication tests out of the unit suite into Integration/SEP010Test.php so CI no longer makes live network calls. The tests are unchanged.
Analyse the hand-written SDK source at level 6 (generated XDR types excluded), with a baseline capturing existing findings so CI blocks new issues. Add a composer analyse script and a static-analysis workflow.
Run composer audit on production dependencies on every push and pull request so newly disclosed advisories in shipped dependencies fail CI.
Reconstruct the signed payload by stripping at the signature parameter marker rather than matching the re-encoded signature value, so verification is independent of the signature's URL-encoding. Add tests for special-character signatures, tampered URIs, invalid base64 signatures, and signed pay URIs.
Reject public and private keys that are not exactly 32 bytes with a clear error instead of silently producing a corrupt account id or failing deep inside key derivation.
Correct Asset::TYPE_POOL_SHARE to "liquidity_pool_shares", matching the asset type string used by Horizon. Code comparing Horizon data against the constant could previously never match. Asset::create() now reports clearly that pool share assets must be constructed via AssetTypePoolShare.
Cover contract client construction, method invocation, transaction building, install, and deploy flows with mocked RPC responses, so the client is exercised by the unit suite.
Query getTransaction right after submission instead of sleeping three seconds first, and retry with exponential backoff bounded by the configured timeout. Confirmed transactions now resolve without the fixed delay. Add a test for the retry path.
Rewrite the AssembledTransaction sign-and-send test with mocked RPC responses and concrete assertions on the signed transaction and final status, and use an unroutable test RPC URL so unmocked requests fail locally instead of reaching a live server.
…ding Cover not-found, malformed-response, server-error and validation-rejection paths for Federation, CrossBorderPaymentsService and StellarToml::fromDomain with mocked HTTP.
ClientOptions, InstallRequest and DeployRequest accept an optional preconfigured SorobanServer instance, created automatically from the RPC URL when omitted. This enables custom HTTP client configuration and makes the contract client flows fully unit-testable; the tests now inject mocked servers directly and cover forClientOptions.
- SEP-01 (stellar.toml): currencyFromUrl accepts an optional HTTP client, like fromDomain - SEP-02 (federation): resolveStellarAddress uses the provided HTTP client for the stellar.toml fetch as well - SEP-31 (cross-border payments): tolerate a transaction_info_needed error response without a fields key
Memo ids above PHP_INT_MAX previously failed to decode and could not be created. Memo::id now also accepts unsigned decimal strings, decoding exposes large ids as decimal strings, and getIdAsString provides a uniform string representation. Also correct the liquidity pool fee field comments (fee_bp is an int32 in basis points, not a big integer).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
============================================
+ Coverage 92.00% 92.63% +0.63%
- Complexity 20778 20803 +25
============================================
Files 987 987
Lines 49651 49706 +55
============================================
+ Hits 45680 46044 +364
+ Misses 3971 3662 -309
🚀 New features to boost your workflow:
|
christian-rogobete
added a commit
that referenced
this pull request
Jun 11, 2026
Large chunked reads block on PHP http stream wrappers until the internal buffer fills, stalling sparse event streams. Read lines from the detached resource with a 1-byte chunk size instead. Corrects the unreleased streaming rework; released versions are not affected.
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
1 participant
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.
Improvements from a multi-perspective review of the hand-written SDK core (architecture, security, performance, code quality, testing). All changes are covered by unit tests and were reviewed individually; the full unit suite and PHPStan pass on every commit.
Fixes
Memo::idnow also accepts unsigned decimal strings, andgetIdAsStringprovides a uniform string representation.Asset::TYPE_POOL_SHAREwas misspelled (liquidty_pool_shares) and could never match the asset type string Horizon returns; corrected toliquidity_pool_shares.AbstractOperation::fromXdr.SEP services
currencyFromUrlaccepts an optional HTTP client, likefromDomain.resolveStellarAddressuses the provided HTTP client for the stellar.toml fetch as well.transaction_info_needederror response without afieldskey is tolerated.Soroban contract client
ClientOptions,InstallRequestandDeployRequestaccept an optional preconfiguredSorobanServer, enabling custom HTTP client configuration (mirrors the JS SDK'sClientOptions.server).checkMemoRequiredqueries each payment destination account at most once.Performance
StellarAmountreuses shared BigInteger constants instead of re-parsing them on every construction;XdrEncodercaches the endianness probe; XDR primitive codec hot paths avoid redundant decodes and allocations.CI
composer auditchecks production dependencies for known security advisories on every push and pull request.Tests
SorobanClientis now unit tested (was integration-only): construction, invocation, transaction building, install, deploy and spec loading with mocked RPC responses.