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

Remove unused TransactionError constants #15408

Merged
merged 1 commit into from Feb 22, 2019

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Feb 14, 2019

Fixup to #14978, which introduced a bunch of unused enum values, such as UNKNOWN_ERROR, ERROR_COUNT and TRANSACTION_ERR_LAST. None of those have a meaning in the context of an enum class, where the compiler can infer if all cases have been covered in a switch-case.

Also, move the global ::maxTxFee back to the rpc caller, so it can be set on a per call basis (in the future).

Show resolved Hide resolved src/node/transaction.h Outdated
Show resolved Hide resolved src/wallet/psbtwallet.h Outdated
@practicalswift

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Concept ACK

Nice cleanup!

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1902-TransactionErrorConst branch from fadc1b2 to fa1b275 Feb 14, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1902-TransactionErrorConst branch from fa1b275 to fa9b60c Feb 14, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@gwillen

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

utACK

The only thing I feel weird about here is the caller of BroadcastTransaction needing to know that there is a maxTxFee they are supposed to be using. Would it be possible to default that parameter to maxTxFee? (So that calls to BroadcastTransaction automatically obey the -maxtxfee command line flag if not overridden deliberately?)

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

The goal in the future is to have it as a per-call default that can be overwritten by each individual rpc call. So the default should really live in the scope of the rpc method.

@MarcoFalke MarcoFalke merged commit fa9b60c into bitcoin:master Feb 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Feb 22, 2019

Merge #15408: Remove unused TransactionError constants
fa9b60c Remove unused TransactionError constants (MarcoFalke)

Pull request description:

  Fixup to #14978, which introduced a bunch of unused enum values, such as `UNKNOWN_ERROR`, `ERROR_COUNT` and `TRANSACTION_ERR_LAST`. None of those have a meaning in the context of an `enum class`, where the compiler can infer if all cases have been covered in a switch-case.

  Also, move the global `::maxTxFee` back to the rpc caller, so it can be set on a per call basis (in the future).

Tree-SHA512: 7f1e2d795f1c1278ecd54ddab2b92c2a862f3c637b482d1d008208925befa1c9dd4b3c4bb1bfcbc5ca4b66a41004aaf01ea96ea95236f944250b8a6cf99ff173

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1902-TransactionErrorConst branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.