Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

txnbuild: add SequenceNumber func to Transaction #3616

Merged
merged 7 commits into from
May 24, 2021
Merged

txnbuild: add SequenceNumber func to Transaction #3616

merged 7 commits into from
May 24, 2021

Conversation

leighmcculloch
Copy link
Member

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Add SequenceNumber function to Transaction.

Why

Stellar transactions have a sequence number. If a transaction is valid and it is
accepted into a ledger the source account of the transaction is updated such
that its sequence number becomes the sequence number of the transaction.

In the Go SDK a parsed transaction has no sequence number field or function
directly on it. For a developer to get the sequence number they must look at the
source account object. This is not intuitive and not consistent conceptually
because a transaction has a sequence number and it should be a first class
attribute of the transaction.

In my own development using the Go SDK I spent time trying to figure out where
the sequence was stored and how to get at it. I work on Stellar everyday, so if
this is not intuitive to me, I suspect it isn't intuitive to others.

Other Stellar SDKs, such as the JS SDK, expose it as an attribute on a
Transaction.

This is likely to become even less intuitive as we plan to change the sequence
number rules where the sequence number on an account does not need to be
sequential. Regardless of this, it isn't intuitive today.

Known limitations

N/A

### What
Add `SequenceNumber` function to `Transaction`.

### Why
Stellar transactions have a sequence number. If a transaction is valid and it is
accepted into a ledger the source account of the transaction is updated such
that its sequence number becomes the sequence number of the transaction.

In the Go SDK a parsed transaction has no sequence number field or function
directly on it. For a developer to get the sequence number they must look at the
source account object. This is not intuitive and not consistent conceptually
because a transaction has a sequence number and it should be a first class
attribute of the transaction.

In my own development using the Go SDK I spent time trying to figure out where
the sequence was stored and how to get at it. I work on Stellar everyday, so if
this is not intuitive to me, I suspect it isn't intuitive to others.

This is likely to become even less intuitive as we plan to change the sequence
number rules where the sequence number on an account does not need to be
sequential. Regardless of this, it isn't intuitive today.
@leighmcculloch leighmcculloch requested review from a team May 20, 2021 16:26
@leighmcculloch leighmcculloch self-assigned this May 20, 2021
Copy link
Contributor

@acharb acharb left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

Shouldn't the transaction sequence number depend on the IncrementSequenceNumber flag as well?

txnbuild/transaction.go Show resolved Hide resolved
Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

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

Great, this looks really helpful, especially for any non-trivial application (i.e. one that assumes validity, and tracks sequence numbers locally rather than looking up Horizon each time).

@leighmcculloch leighmcculloch merged commit a5ddb4e into stellar:master May 24, 2021
@leighmcculloch leighmcculloch deleted the txnbuild>|<->|<get-seq-num branch May 24, 2021 18:49
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