Go: txnbuild: allow for custom sequence numbers

[tomerweller]

The Transaction.Build() function uses and consumes the current sequence number of supplied Account. This makes it tricky to build multiple transactions with the same seq number, or future seq numbers. These are crucial for writing smart contract functionality.

In addition, this build step has a side effect of mutating the seq number of the given account which is not obvious and conflates account state with the construction of a transaction.

[tomerweller]

One possible solution: replace Account in the Transaction with a simple account strkey and seqnum. Transaction.Build() can use these without mutating any state.

In addition, there can be a BuildTx() function on Account which takes in a transaction and builds with the account strkey, seqnum, and consumes that.

[tomerweller]

Also, I know that current functionality mimics that of other sdks (js, for example) but I don't think we should allow that baggage set the tone.

[ire-and-curses]

In the new Go SDK, I implemented it this way because

• I assume most users are following a workflow where they need to submit a transaction now, not writing smart contracts for the future. Auto-incrementing + passing an Account provides the simplest API for this main use case.
• This is the behaviour of the JS SDK build().
• This is the behaviour of the Java SDK build().

This is also the same behaviour as the Python SDK, the .NET SDK, the Scala SDK, and so on.

Universally the approach taken by all the SDKs I am aware of has been to allow power users to modify the sequence number manually, while defaulting to incrementing. This approach is summed up well in the Scala docs:

The sequence number for a new account is the id of the ledger in which is was created. The id increments by one with every submitted transaction. In simple architectures, it is possible to keep track of the next sequence number without querying the network. However, if this is not possible, or the number is unknown, you can directly substitute the response from the @ref:account query into the Transaction constructor.

Changing the design now to something like this proposal has benefits:
1) An arguably more elegant design (no side-effects) that encourages explicitness.
2) Future/parallel transactions are simpler to write.
3) Adding a second build path allows the best of both worlds.

It also has downsides:
1) It breaks with the convention established by every other SDK in the ecosystem.
2) It is a major breaking change to this API. No existing code will function after the change.
3) It adds complexity to the majority use case, since users have to understand the second build path and recognise that it applies to them.
4) We would need to consider making the same change to the other official SDKs, since presumably the same logic applies. Similarly, we might want to agitate for the same change across community SDKs. This would be a big job.

Note that the current build method does not disallow these future transactions - it just makes them more fiddly to write. A caller simply sets a custom sequence number in their Account struct before calling build.

My suggestion: we modify Transaction.Build() to take an optional argument indicating the sequence number not be incremented. We expand the Account interface with a third method, GetSequenceNumber(), and modify the existing implementations in protocols/horizon and friendbot. This preserves the current default behaviour, while allowing more convenient usage for smart contracts.

[tomerweller]

@ire-and-curses thanks for that great breakdown of pros and cons. I think your suggestion does a good job in addressing the issue while keeping the ecosystem in sync.

[poliha]

This will bring a breaking change. To be implemented in v2.0

[aanupam23]

This is very urgent and required since ideally the flow should be like below
tx := txnbuild.Transaction{ SourceAccount: &hAccount0, Operations: []txnbuild.Operation{&op}, Timebounds: timebounds, Network: phrase, Sequence: sequenceNo }

The current code for txnbuild creates tx with fixed sequence number for a new transaction (https://github.com/stellar/go/blob/8cd5bd9555680f008b9ce6f6e21715fe155c0c74/txnbuild/transaction.go#L92)

Currently are using the build method to supply the sequence number.

[poliha]

@aanupam23 SequenceNumber is a feature of an account, not the transaction so it is set on the SourceAccount. Typically when you get the details of an account from horizon, it already contains the sequence number so you do not need to specify this.
However, if you need to have more control over what sequence number is used, you can create a txnbuild.SimpleAccount and use that as the source account of the transaction. It has the following shape.

type SimpleAccount struct {
    AccountID string
    Sequence  int64
}

[aanupam23]

@poliha : Yes I knew about sequence number is feature of account. SimpleAccount works very well, yay :+1:

Here is the code to control sequence number if anyone needs this

    accountRequest := horizonclient.AccountRequest{AccountID: Address}
    Account, err := client.AccountDetail(accountRequest)

    seqno, _ := Account.GetSequenceNumber()
    // Do anything with sequence number

    tx := txnbuild.Transaction{
        SourceAccount: &txnbuild.SimpleAccount{AccountID: Address, Sequence: int64(seqno)},
        Operations:    []txnbuild.Operation{&op},
        Timebounds:    txnbuild.NewInfiniteTimeout(),
        Network: phrase,
    }

NOTE Whatever sequence number you will provide will get increment automatically so make sure to consider it in your calculations (Sequence number of transaction envelope should always be greater than account sequence number)
https://github.com/stellar/go/blob/d39a0aac6abc94f92cb50d3bc430c65c7be8dbf1/txnbuild/transaction.go#L94

[leighmcculloch]

💯 The auto-incrementing of sequence numbers in our SDKs is an unexpected side-effect which requires a developer to track in their head how the internals of the SDK works, can cause bugs, and can be a burden.

I think in principle our SDKs favor mutating objects which cause side-effects in a few ways, some which could be dangerous side-effects, others are inconvenient, and if we're thinking about a v2 of our SDKs we should consider how we can remove them. The cases I know about:

1. Calling Build increments sequence number.
2. You cannot call Hash without calling Build first.
3. If a transaction is changed after calling Build then call Hash, the hash returned is wrong. ❗️
4. You cannot call Sign or Encode without calling Build first, but the API doesn't enforce it.
5. You cannot call Build again if you've called Sign.

Some ways we could change the SDK APIs to reduce unexpected side-effects and make it easier to understand:

1. Encourage setting the sequence number manually. As a developer who used the Go SDK before I joined SDF, I don't think we get enough win from this auto-incrementing behavior.
2. Specifically in the Go SDK, remove the Hash, Sign, and Base64/encode operations from the builder class. Have Build return an immutable value that has the Sign and Encode functions on it (similar to the JS SDK).

[bartekn]

Presumably fixed by https://github.com/stellar/go/pull/2495 (used fix tag with this issue ID). Can you confirm? If not, please reopen.