Skip to content

Account: allow negative sequence number#948

Merged
quietbits merged 1 commit intotypescript-migrationfrom
account-remove-neg-seq
Mar 26, 2026
Merged

Account: allow negative sequence number#948
quietbits merged 1 commit intotypescript-migrationfrom
account-remove-neg-seq

Conversation

@quietbits
Copy link
Contributor

@quietbits quietbits commented Mar 24, 2026

There is a use case for a negative sequence number. For example, we use it in WebAuth.buildChallengeTx (code).

Note

This was introduced during migration. This check does not exist in the current version.

@github-actions
Copy link

Size Change: -283 B (-0.02%)

Total Size: 1.75 MB

Filename Size Change
dist/stellar-base.cjs.js 695 kB -102 B (-0.01%)
dist/stellar-base.js 714 kB -105 B (-0.01%)
dist/stellar-base.min.js 345 kB -76 B (-0.02%)

compressed-size-action

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 updates the Account constructor to permit negative sequence numbers, enabling workflows that need to construct transactions with sequence "0" (e.g., by starting from "-1" and relying on TransactionBuilder’s increment-on-build behavior).

Changes:

  • Removed the Account constructor check that rejected negative sequence numbers.
  • Updated unit tests by removing the expectation that negative sequences throw, and added an eslint suppression for an invalid-type test case.

Reviewed changes

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

File Description
src/account.ts Removes the “non-negative sequence” validation to allow negative sequence inputs.
test/unit/account.test.ts Removes the negative-sequence rejection test and adds an eslint suppression for an invalid-type test.
Comments suppressed due to low confidence (1)

test/unit/account.test.ts:33

  • This PR changes the contract to allow negative sequence numbers, but the unit tests no longer assert the new behavior. Add a test that new Account(ACCOUNT, "-1") succeeds (and ideally that TransactionBuilder built from it produces a tx sequence of "0" / increments the source to "0"), so the intended SEP-10-style use case stays covered.
  it("fails to create Account object from an empty string sequence", () => {
    expect(() => new Account(ACCOUNT, "")).toThrow(
      /sequence is not a valid number/,
    );
  });

  it("creates an Account object", () => {
    const account = new Account(ACCOUNT, "100");
    expect(account.accountId()).toBe(ACCOUNT);
    expect(account.sequenceNumber()).toBe("100");
  });

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

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

This doesn't seem like a good idea to me tbh, I think any valid use-case should be able to use a sequence number of zero, instead 🤔

@quietbits
Copy link
Contributor Author

@Shaptic , should we update WebAuth.buildChallengeTx in JS SDK instead? My fear is that others might be using the same pattern. We set the initial sequence number "-1" and then it's increased to be 0.

@quietbits quietbits requested a review from Ryang-21 March 24, 2026 19:12
Copy link
Contributor

@Ryang-21 Ryang-21 left a comment

Choose a reason for hiding this comment

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

Nice catch

@Shaptic
Copy link
Contributor

Shaptic commented Mar 24, 2026

@quietbits we should definitely change that code to not rely on a negative value (seqnum of 0 is enough as an "invalid" check if that's the purpose), and we can re-introduce "no negative numbers" later to avoid the "breaking" (arguably a bugfix) change.

@quietbits quietbits merged commit 07b74b7 into typescript-migration Mar 26, 2026
11 checks passed
@quietbits quietbits deleted the account-remove-neg-seq branch March 26, 2026 13:06
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