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

Clean up change computation in CreateTransaction. #5924

Merged
merged 1 commit into from Oct 1, 2015

Conversation

domob1812
Copy link
Contributor

@domob1812 domob1812 commented Mar 18, 2015

Compute the change directly as difference between the "requested" and the actual value returned by SelectCoins. This removes a duplication of the fee logic code.

@gavinandresen
Copy link
Contributor

gavinandresen commented Mar 18, 2015

How did you test this?

@domob1812
Copy link
Contributor Author

domob1812 commented Mar 18, 2015

Unit tests and wallet.py (which seems to be the relevant one).

Compute the change directly as difference between the "requested" and
the actual value returned by SelectCoins.  This removes a duplication of
the fee logic code.
if (nSubtractFeeFromAmount == 0)
nChange -= nFeeRet;

const CAmount nChange = nValueIn - nValueToSelect;
Copy link
Member

@laanwj laanwj Oct 1, 2015

Choose a reason for hiding this comment

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

Before

nChange = nValueIn - nValue - nSubtractFeeFromAmount?0:nFeeRet 

After:

nChange = nValueIn - nValueToSelect 
        = nValueIn - (nValue + nSubtractFeeFromAmount?0:nFeeRet) 
        = nValueIn - nValue - nSubtractFeeFromAmount?0:nFeeRet

Looks good to me!

@laanwj
Copy link
Member

laanwj commented Oct 1, 2015

ut/code review ACK

@laanwj laanwj merged commit 835c122 into bitcoin:master Oct 1, 2015
laanwj added a commit that referenced this pull request Oct 1, 2015
835c122 Clean up change computation in CreateTransaction. (Daniel Kraft)
@domob1812 domob1812 deleted the change-cleanup branch Oct 1, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants