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: Make txnbuild create V1 transactions #2640

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Jun 1, 2020

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

Currently, txnbuild creates v0 transaction envelopes which are accepted by both protocol 12 and protocol 13. In this PR, I have configured txnbuild to only create v1 transactions which will only be accepted once protocol 13 is enabled.

This release should only be published after pubnet transitions to protocol 13. However, developers who would like to try out protocol 13 features in testnet such as fee bump transactions can use the txnbuild package from this PR to generate v1 transactions.

Why

It is not possible to create fee bump transactions using v0 transaction envelopes.

Known limitations

Once this PR is merged there will be no way to generate v0 transaction envelopes anymore. However, I don't think this is a big concern because we will only publish this release once protocol 13 is enabled.

@cla-bot cla-bot bot added the cla: yes label Jun 1, 2020
@tamirms tamirms force-pushed the generate-v1 branch 2 times, most recently from 7fa04a4 to 5f9cbba Compare June 1, 2020 15:11
@tamirms tamirms marked this pull request as ready for review June 1, 2020 15:20
@tamirms tamirms requested review from howardtw and a team June 1, 2020 15:20
Copy link
Contributor

@howardtw howardtw left a comment

Choose a reason for hiding this comment

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

LGTM. Question: what's the reason that we use SourceAccount as MuxedAccount in V1 and SourceAccountEd25519 in V0? And what exactly is a MuxedAccount?

Comment on lines +1090 to +1095
feeBumpTx, err := txnbuild.NewFeeBumpTransaction(txnbuild.FeeBumpTransactionParams{
Inner: tx,
FeeAccount: feeBumpKP.Address(),
BaseFee: txnbuild.MinBaseFee * 2,
})
feeBumpTx, err = feeBumpTx.Sign(network.TestNetworkPassphrase, feeBumpKP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you skip checking these two errors on purpose? Same question to the below example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is mistake, I'll push a fix

@tamirms
Copy link
Contributor Author

tamirms commented Jun 1, 2020

what's the reason that we use SourceAccount as MuxedAccount in V1 and SourceAccountEd25519 in V0?

The XDR definition for v1 transaction envelopes and v0 transaction envelopes have different source account types. The source account field is represented as a SourceAccountEd25519 in v0 because we want v0 transaction envelopes to be binary compatible with protocol 12 transaction envelopes. You can find more details about this here https://github.com/stellar/stellar-protocol/blob/master/core/cap-0015.md#how-does-the-transactionenvelope-transformation-work


And what exactly is a MuxedAccount?

This is another feature that is partially introduced in protocol 13. https://github.com/stellar/stellar-protocol/blob/master/core/cap-0027.md has the full explanation

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.

3 participants