Skip to content

Some fixes 2#892

Merged
quietbits merged 16 commits intomasterfrom
some-fixes-2
Mar 4, 2026
Merged

Some fixes 2#892
quietbits merged 16 commits intomasterfrom
some-fixes-2

Conversation

@quietbits
Copy link
Contributor

  • TransactionBuilder
    • build() timebounds — / 1000 → Math.floor(/ 1000)
    • cloneFrom() unscaledFee — integer division → Math.floor
    • extraSigners — XDR objects → StrKey strings
  • Transaction: extraSigners getter — JSDoc @type {string[]}{xdr.SignerKey[]}
  • Keypair: verify() — bare call → try-catch returning false
  • Memo: _validateIdValue() — add negative, decimal, uint64 overflow guards

@github-actions
Copy link

github-actions bot commented Mar 3, 2026

Size Change: +6.13 kB (+0.17%)

Total Size: 3.53 MB

Filename Size Change
dist/stellar-base.js 2.61 MB +4.53 kB (+0.17%)
dist/stellar-base.min.js 922 kB +1.61 kB (+0.17%)

compressed-size-action

@quietbits quietbits requested review from Ryang-21 and Shaptic March 3, 2026 21:35
throw error;
}

// Exceeds uint64 max (2^64 - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it int64 not uint64?

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed the xdr says uin64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking, @Ryang-21!

// negative
expect(() => StellarBase.Memo.id('-1')).to.throw(/Expects a int64/);
// decimal
expect(() => StellarBase.Memo.id('1.5')).to.throw(/Expects a int64/);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should correct the error message error to uint64

Base automatically changed from some-fixes-1 to master March 4, 2026 14:00
Copilot AI review requested due to automatic review settings March 4, 2026 14:09
Copy link

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

This PR tightens correctness and edge-case handling across classic transactions and Soroban utilities in stellar-base, with accompanying regression tests to prevent previously observed rounding/encoding/validation failures.

Changes:

  • Fix fee/timebounds rounding behaviors in TransactionBuilder (Math.floor for sub-second Dates and non-even fee splits in cloneFrom()), and normalize cloneFrom() extraSigners to StrKey strings.
  • Harden parsing/encoding and validation: signed-payload SignerKey payload extraction, Memo.id() uint64 validation, Soroban.parseTokenAmount() decimal-place limits, and Keypair.verify() returning false on invalid signatures.
  • Add regression tests for immutability, nonce generation, signed payload padding, uint sizing, and price fraction handling.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/unit/transaction_test.js Adds regression test asserting Transaction.networkPassphrase is immutable.
test/unit/transaction_builder_test.js Adds tests for floored Date timebounds and cloneFrom() fee/extraSigners behavior.
test/unit/soroban_test.js Adds test for “too many decimal places” parsing error.
test/unit/signerkey_test.js Adds roundtrip tests for signed-payload signer keys with padding edge cases.
test/unit/scint_test.js Extends coverage for string inputs at uint sizing boundaries (e.g. 2^64).
test/unit/operations/classic_ops_test.js Adds test for price fraction objects with zero numerator.
test/unit/memo_test.js Expands Memo.id() tests for uint64 max, negatives/decimals, and overflow.
test/unit/keypair_test.js Adds tests validating verify() returns false for invalid signatures/lengths.
test/unit/auth_test.js Adds regression tests ensuring nonce generation consumes all 8 bytes.
src/transaction_builder.js Floors unscaled fee in cloneFrom(), encodes extraSigners to StrKey, floors sub-second Date timebounds.
src/transaction_base.js Makes networkPassphrase setter consistent with other immutable setters (throws).
src/transaction.js Updates extraSigners JSDoc to reflect XDR objects.
src/soroban.js Adds explicit decimal-place limit enforcement in parseTokenAmount().
src/signerkey.js Fixes signed-payload decoding to slice payload by declared length (ignoring XDR padding).
src/operation.js Accepts price fraction objects even when numerator is 0.
src/numbers/sc_int.js Normalizes inputs through BigInt() to improve sizing decisions for string values.
src/memo.js Strengthens Memo.id() validation to enforce uint64, integer-only, and no overflow.
src/keypair.js Wraps verify() in try/catch to return false instead of throwing on invalid signatures.
src/auth.js Fixes nonce derivation to use all 8 bytes and return a signed Int64 BigInt.
.gitignore Ignores common local IDE/tooling dirs and TS build info output.
Comments suppressed due to low confidence (1)

src/operation.js:540

  • The new price.n !== undefined && price.d !== undefined check allows { n: 1, d: 0 } (or null values) to be treated as an explicit fraction, producing an XDR Price with a zero/invalid denominator. Since the subsequent validation only rejects negatives, this would now slip through. Please add validation that n/d are integers and that d > 0 (while still allowing n = 0).
  static _toXDRPrice(price) {
    let xdrObject;
    if (price.n !== undefined && price.d !== undefined) {
      xdrObject = new xdr.Price(price);
    } else {
      const approx = best_r(price);
      xdrObject = new xdr.Price({
        n: parseInt(approx[0], 10),
        d: parseInt(approx[1], 10)
      });
    }

    if (xdrObject.n() < 0 || xdrObject.d() < 0) {
      throw new Error('price must be positive');
    }

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

You can also share your feedback on Copilot code review. Take the survey.

@quietbits quietbits merged commit c872f75 into master Mar 4, 2026
8 checks passed
@quietbits quietbits deleted the some-fixes-2 branch March 4, 2026 18:01
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.

4 participants