Internal agent bug fixes 2#1373
Merged
Merged
Conversation
- pollTransaction: change loop condition from < to <= so attempts=N makes N iterations instead of N-1. Fixes crash on attempts=1. - requestAirdrop: access error.response.data?.detail instead of error.response.detail to match feaxios response structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ?? operator has lower precedence than >, so sim.results?.length ?? 0 > 0 was parsed as sim.results?.length ?? (0 > 0). Added parentheses for correctness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- structToJsonSchema: move additionalProperties to schema level - typeRef: handle scSpecTypeResult by returning okType ref - nativeToScVal: use Number(val) for u32/i32 to handle BigInt inputs - unionToNative: add missing $ in template literal interpolations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restructure loop so offset = start + sectionLength always executes at the bottom, preventing offset corruption when nameLen is 0 or exceeds section bounds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Validate domain format in resolve() to prevent SSRF via userinfo@host injection in federation addresses - Clone serverURL before query() in all three resolve methods to prevent permanent destruction of existing query parameters Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use a cloned URL for all streaming operations so that X-Client-Name, X-Client-Version, cursor, and app headers don't leak into subsequent call() invocations on the same builder. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When buildWithOp is used (e.g. Client.deploy), store the original operation so the restore-and-rebuild path reuses it instead of incorrectly calling contract.call() which fails with invalid contractId. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use hostname:port as the map key instead of hostname-only so that servers on different ports of the same host don't overwrite each other's time data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Import Err from rust_result and check for scvError ScVal type before unconditionally wrapping as Ok. Error variants now return Err with the serialized ScError instead of crashing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR aggregates several internal bug fixes across Soroban RPC, Horizon, contract spec tooling, federation resolution, and transaction assembly logic, aiming to prevent subtle state corruption and shared-URL mutation issues while improving correctness of parsing/serialization.
Changes:
- Fixes multiple off-by-one / precedence / property-path bugs in RPC polling, simulation parsing, and Friendbot error handling.
- Prevents shared
URImutation in streaming and federation requests by cloning URLs before adding query params. - Improves contract tooling correctness (WASM custom section parsing, JSON-schema generation,
Result<ok, err>handling, and restore-path rebuild operation reuse).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/server/soroban/request_airdrop.test.ts | Updates test fixture to match Friendbot error shape (response.data.detail). |
| src/rpc/server.ts | Fixes poll retry count and Friendbot error detail access path. |
| src/rpc/parsers.ts | Fixes boolean precedence when checking results/stateChanges length. |
| src/horizon/server.ts | Uses host:port key when reading cached server time for timebounds. |
| src/horizon/horizon_axios_client.ts | Stores server time cache entries keyed by host:port (from response URL). |
| src/horizon/call_builder.ts | Prevents CallBuilder.stream() from mutating the shared URL by using a clone. |
| src/horizon/account_response.ts | Replaces dynamic field assignment with explicit constructor assignments. |
| src/federation/server.ts | Adds domain validation for federation addresses and prevents serverURL query mutation via clone. |
| src/contract/utils.ts | Fixes WASM custom section parsing to always advance to end-of-section to avoid offset corruption. |
| src/contract/spec.ts | Improves Result handling, root additionalProperties, bigint->U32/I32 conversion, and error message interpolation. |
| src/contract/assembled_transaction.ts | Stores original operation from buildWithOp and reuses it during restore-path rebuilds. |
Comments suppressed due to low confidence (1)
src/rpc/server.ts:733
pollTransactionnow performsmaxAttemptscalls (good), but it also sleeps after the final attempt when the transaction is still NOT_FOUND. That makes the total wait time longer than necessary and contradicts the idea of "sleep between attempts".
Suggestion: only sleep when attempt < maxAttempts (or restructure as a do/while) so there is no post-final sleep before returning the last-seen status.
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
foundInfo = await this.getTransaction(hash);
if (foundInfo.status !== Api.GetTransactionStatus.NOT_FOUND) {
return foundInfo;
}
await Utils.sleep((opts?.sleepStrategy ?? BasicSleepStrategy)(attempt));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
805
to
811
| case "bigint": { | ||
| switch (value) { | ||
| case xdr.ScSpecType.scSpecTypeU32().value: | ||
| return xdr.ScVal.scvU32(val as number); | ||
| return xdr.ScVal.scvU32(Number(val)); | ||
| case xdr.ScSpecType.scSpecTypeI32().value: | ||
| return xdr.ScVal.scvI32(val as number); | ||
| return xdr.ScVal.scvI32(Number(val)); | ||
| case xdr.ScSpecType.scSpecTypeU64().value: |
There was a problem hiding this comment.
Number(val) allows bigint inputs for U32/I32, but it can silently lose precision for bigints > 2^53-1. Even though valid U32/I32 ranges are below 2^53, a caller can still pass an out-of-range bigint and get an incorrect/scary conversion rather than a clear error.
Consider validating that bigint values are within the allowed numeric range (and are integral) before converting to Number, and throw a TypeError on overflow.
|
Size Change: +171 kB (+0.38%) Total Size: 45.6 MB 📦 View Changed
|
quietbits
approved these changes
Apr 3, 2026
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
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.
What
This fixes a collective of bugs documented here
AssembledTransaction.simulaterestore path rebuilds wrong operationAccountResponsefields in the constructorstructToJsonSchemanow correctly returnsadditionalPropertiesin the root objecttypeRefnow handlesResult<ok, err>typepollTransactionretry being off-by-oneCallBuilder.stream()no longer mutates the shared urlFederationServer.serverUrlis now cloned before adding query's to prevent url mutation