TransactionFactory - Fix fee percentage calculation for self spend #11507
Conversation
| // In this scenario since the amount changes as the fee changes, we need to compare against the total sum / 2, | ||
| // as with this, we will make sure the fee cannot be higher than the amount. | ||
| decimal inputSumDecimal = spentCoins.Sum(x => x.Amount.ToDecimal(MoneyUnit.BTC)); | ||
| feePercentage = 100 * (feeDecimal / (inputSumDecimal / 2)); |
There was a problem hiding this comment.
I don't understand the change. Why divided by 2?
There was a problem hiding this comment.
Currently, on master, we don't allow the build of a transaction where the fee is higher than the amount to be sent.
To have the same functionality in the case of ChangeStrategy.AllRemainingCustom we have to compare against the input sum / 2 because that is the maximum fee that we allow.
Examples:
Input: 1 BTC
Fee: 0.2 BTC
Output: 0.8 BTC
feePercentage: 100 * (0.2 / (1 BTC / 2) = 40%
Input: 1 BTC
Fee: 0.5 BTC
Output: 0.5 BTC
feePercentage: 100 * (0.5 / (1 BTC / 2) = 100% // It pays the maximum that is allowed
Input: 1 BTC
Fee: 0.6 BTC
Output: 0.4 BTC
feePercentage: 100 * (0.6 / (1 BTC / 2) = 120% // Overpays as the fee is higher than the amount.
Note: In the case of self spend the total outgoing amount is 0, so the fee PC calculation allowed a transaction like this when the ChangeStrategy was AllRemainingCustom
|
@soosr @molnard I am not sure how to test this. |
It is not really possible to test it on master as be1645c commit was an alternative way on how to send whole coins. But with that commit, I would have touched other files too to make it work properly in every case, so I just dropped the idea. Currently, Full Privacy and Better Privacy suggestions could make a transaction with So just rely on the test I added. |
|
Well done. 🏅 |
During #11120 I found that the calculation is wrong in the case of self spend.
I added a test for it.
Issue: #11120 (review)
Explanation: #11120 (comment)
ping for review @zkSNACKs/code-team