In commit “gui: remove unreachable AmountWithFeeExceedsBalance error” (355199c7a6c58eb812dd7ef8e204e6e068c142ae)
This is a nice catch noticing that this code doesn’t work. But I think the fact that it doesn’t work is a pretty unfortunate problem, and the current PR will make fixing it harder.
It seems like bitcoin/bitcoin#20640 unintentionally introduced a bug where the more accurate error message “The total exceeds your balance when the %1 transaction fee is included” can never trigger, and instead only less informative “Insufficient funds” or “The amount exceeds your balance” errors will be shown.
It looks like there are few ways this could be fixed:
-
An easy way to fix it would be to restore the CAmount& fee
output parameter removed by #20640 so it is always returned and this code can work the way it was originally intended.
-
A slightly more complicated but cleaner fix would be to use CreatedTransactionResult
as a return type instead of util::Result<CreatedTransactionResult>
and add a bilingual_str error
field to CreatedTransactionResult
so fee information and an error string can be returned together.
-
Another alternative fix could be to build on bitcoin/bitcoin#25665 and keep using util::Result
, but return the fee in the new util::Result
failure field.
-
And the best but probably most complicated fix would be to remove error message code from the GUI and instead generate detailed error messages in wallet code. This would require bigger changes in the GUI code and might require changing the interface to provide the wallet with more information about transactions.
Against that background, I think this PR (as of edc3f9a733aaa1a1cc2547013d09c27828151e8a) makes some nice cleanups, but I don’t like the current changes because I think they make all of the fixes above except maybe the last one harder to implement.
So I don’t think I would ACK this PR currently, I’d happily ACK it if it either (1) fixed the problem, maybe using one of the easier fixes suggested above, or (2) did not fix the problem, but at least did not make it harder to fix in the future. This could be implemented by keeping most of the changes in the PR, but not deleting this GUI code in this commit and not deleting the CAmount& fee
output parameter from the wallet interface after this commit.