Merged
Conversation
|
Size Change: +4.18 kB (+0.24%) Total Size: 1.77 MB 📦 View Changed
|
There was a problem hiding this comment.
Pull request overview
This PR continues the “various fixes” series by tightening input validation and error handling around numeric parsing/encoding and transaction fee computation, with accompanying unit tests and changelog updates.
Changes:
- Add stricter numeric validation paths (e.g.,
toXDRPrice,nativeToScValstring→u32/i32,Memo.idstring format checks). - Add transaction builder protections (fee overflow checks;
cloneFromguard for zero-operation transactions). - Add range validation for
XdrLargeInt.toI128()/toI256()to prevent silent signed overflow behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/transaction_builder.test.ts | Updates/extends tests for fee overflow handling and cloneFrom zero-op guard. |
| test/unit/scval.test.ts | Adds tests for stricter string parsing behavior in nativeToScVal for u32/i32. |
| test/unit/operation.test.ts | Adds tests asserting earlier/clearer toXDRPrice validation errors. |
| test/unit/numbers/xdr_large_int.test.ts | Adds tests for signed-range rejection in toI128()/toI256(). |
| test/unit/memo.test.ts | Adds coverage for rejecting non-plain-digit Memo.id() strings; minor formatting cleanups. |
| src/util/operations.ts | Implements early numeric validation for toXDRPrice before calling best_r(). |
| src/transaction_builder.ts | Adds uint32 fee overflow checks and cloneFrom() zero-op guard. |
| src/scval.ts | Replaces parseInt() with BigInt() for string→u32/i32 conversion with bounds checks. |
| src/numbers/xdr_large_int.ts | Adds signed-range checks for toI128() and toI256() using BigInt.asIntN. |
| src/memo.ts | Rejects Memo.id() non-digit-only strings early to avoid BigInt() serialization crashes. |
| src/keypair.ts | Clarifies payload hint padding calculation (hint.length vs dataBuffer.length). |
| CHANGELOG.md | Documents the behavioral changes introduced in this PR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ryang-21
reviewed
Apr 3, 2026
Comment on lines
+168
to
+170
| if (BigInt.asIntN(128, v) !== v) { | ||
| throw RangeError(`value too large for i128: ${v}`); | ||
| } |
Contributor
There was a problem hiding this comment.
These checks are redundant since they are now done in js-xdr
Ryang-21
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.
Fixed issues from the doc.
nativeToScValstring-to-u32/i32 — strict parsing: ReplacedparseInt()withBigInt()for the string path. Rejects trailing junk ("123abc"), correctly parses hex/octal/binary prefixes (e.g.,"0x10"→ 16 instead of 0), and applies the same range validation as the number/bigint path.Memo.id()— reject non-plain-digit strings: Added a/^[0-9]+$/regex guard to reject scientific notation ("1e18") and decimal strings ("1.0") that passedBigNumbervalidation but crashed inBigInt()during XDR serialization.TransactionBuilder.build()— fee overflow protection: Total fee (baseFee * operations, plusresourceFeefor Soroban) is now checked against the uint32 maximum before XDR construction. Previously, overflow produced an invalid XDR value caught only at serialization with a confusing error.TransactionBuilder.cloneFrom()— zero-operation guard: Rejects transactions with no operations upfront, instead of silently producing a builder withbaseFeeset toInfinity(from division by zero).toXDRPrice— early validation for invalid prices: Zero, negative,NaN, andInfinityvalues are now rejected with"price must be positive"before reachingbest_r(), instead of producing confusing downstream errors.Keypair.signPayloadDecorated— clarity refactor: Changed4 - dataBuffer.lengthto4 - hint.lengthin the padding calculation. No behavior change — the values are always equal in context — but makes the intent ("pad hint to 4 bytes") self-evident.