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: fix bug where base fee is not allowed to be zero #3622

Merged
merged 5 commits into from
May 22, 2021
Merged

txnbuild: fix bug where base fee is not allowed to be zero #3622

merged 5 commits into from
May 22, 2021

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented May 21, 2021

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

Allow the base fee to be zero when building transactions.

Why

The txnbuild.NewTransaction function errors when creating a transaction with a base fee of zero.

This is a bug.

Transactions with a zero base fee are accepted to the Stellar network when they are wrapped in a fee bump transaction envelope that pays the fee of the transaction. Applications use a zero base fee on the inner transaction to support fee sponsorship by an account that isn't the transaction source account.

This is required for payment channels, wallets like Vibrant, and other uses.

Known limitations

N/A

@leighmcculloch leighmcculloch requested review from a team May 21, 2021 21:18
@leighmcculloch leighmcculloch self-assigned this May 21, 2021
@leighmcculloch leighmcculloch changed the title txnbuild: fix bug where base fee was not allowed to be zero txnbuild: fix bug where base fee is not allowed to be zero May 21, 2021
Copy link
Contributor

@Zagan202 Zagan202 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

@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

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.

Nice 👍

txnbuild/transaction_fee_test.go Outdated Show resolved Hide resolved
txnbuild/CHANGELOG.md Outdated Show resolved Hide resolved
@leighmcculloch leighmcculloch requested a review from Shaptic May 22, 2021 02:06
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.

🎉

@leighmcculloch leighmcculloch merged commit eee7b9a into stellar:master May 22, 2021
@leighmcculloch leighmcculloch deleted the txnbuild-(゚ヮ゚)-allow-zero-base-fee branch May 22, 2021 06:28
@leighmcculloch leighmcculloch restored the txnbuild-(゚ヮ゚)-allow-zero-base-fee branch May 22, 2021 06:28
@leighmcculloch leighmcculloch deleted the txnbuild-(゚ヮ゚)-allow-zero-base-fee branch May 22, 2021 06:28
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