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

Don't ignore the fee when it's explicitly specified. #1214

Merged
merged 5 commits into from
May 11, 2015

Conversation

dskloet
Copy link
Contributor

@dskloet dskloet commented May 9, 2015

Make sure a specified transaction fee and outputs add up to the sum of the inputs.

…f the inputs. Don't ignore the fee when it's explicitly specified.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 97.67% when pulling 16dc489 on dskloet:fix/fee into baf39f3 on bitpay:master.

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

I'm looking at how the coverage could have decreased.

@maraoz
Copy link
Contributor

maraoz commented May 9, 2015

LGTM

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

I found a problem when investigating the coverage. I'll write a test and fix it. Is there a way to amend the pull request or do I have to make a new pull request?

@martindale
Copy link
Contributor

@dskloet just push new commits to the same branch and it'll automatically associate them with this pull request! :)

…ses. Fix the issue uncovered by this, which is that getFee might not be the actual fee, but only an estimate, if a change address is specified but there isn't enough to pay a fee and have change.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.72% when pulling d1eb190 on dskloet:fix/fee into baf39f3 on bitpay:master.

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

Thanks!
The problem wasn't caught by the tests because a test that expected a FeeError passed because it got a FeeError for a different reason than intended. I'm fixing it by introducing some different kinds of fee errors to distinguish the different cases.

I've pushed the fix.

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

FYI: This pull requests addresses part of issue #1174.

message: 'Fee is too large: {0}',
}, {
name: 'DifferentFeeError',
message: 'Unspent value is different from specified fee: {0}',
}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could make these additional errors part of a general FeeError:

if (err instanceof errors.Transaction.FeeError) {
  //...
}

And if interested in a more specific exception:

if (err instanceof errors.Transaction.FeeError.TooSmall) {
  //...
}

@braydonf
Copy link
Contributor

braydonf commented May 9, 2015

Is there a use case for disabling the check if fees are different?

If so, the related Transaction serialization docs need to be updated with the additional serialization option.

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

The check wasn't there before so I'm mostly just being defensive. Would you prefer I remove the ability to turn it off or add it to the docs?

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

I realized that if it can be disabled, I also need to add a test that it can be disabled.

@@ -85,8 +85,18 @@ module.exports = [{
name: 'InvalidSatoshis',
message: 'Output satoshis are invalid',
}, {
name: 'FeeError',
message: 'Fees are not correctly set {0}',
name: 'Fee',
Copy link
Contributor

Choose a reason for hiding this comment

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

For backwards compatibility, we can keep this as FeeError

@braydonf
Copy link
Contributor

braydonf commented May 9, 2015

If the fee is specified and the actual fee would differ, I can't think of a use case to disable the check, as by not specifying an exact fee would be the means to do such. (I'll verify though).

@dskloet
Copy link
Contributor Author

dskloet commented May 9, 2015

Makes sense. Let me know when you've verified it and I'll remove it.

@braydonf
Copy link
Contributor

braydonf commented May 9, 2015

Yep, works as expected.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.72% when pulling 056f171 on dskloet:fix/fee into baf39f3 on bitpay:master.

braydonf pushed a commit that referenced this pull request May 11, 2015
Don't ignore the fee when it's explicitly specified.
@braydonf braydonf merged commit 698625c into bitpay:master May 11, 2015
@dskloet dskloet deleted the fix/fee branch May 12, 2015 21:54
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.

5 participants