StaticAddr: Withdraw arbitrary amounts #860
Conversation
hieblmi
force-pushed
the
withdraw-amounts
branch
5 times, most recently
from
4f2ff5d to
f7af7b4
Compare
hieblmi
force-pushed
the
static-addr-staging
branch
from
5b68e42 to
fb9562b
Compare
hieblmi
force-pushed
the
withdraw-amounts
branch
from
f7af7b4 to
827a65b
Compare
hieblmi
force-pushed
the
static-addr-staging
branch
3 times, most recently
from
818562f to
9342b0f
Compare
| outpoints []wire.OutPoint, destAddr string, satPerVbyte int64) (string, | ||
| string, error) { | ||
| outpoints []wire.OutPoint, destAddr string, satPerVbyte int64, | ||
| amount int64) (string, string, error) { |
There was a problem hiding this comment.
I propose to add to godoc, that amount=0 means "withdraw everything".
staticaddr/withdraw/manager.go
Outdated
| } | ||
|
|
||
| addressParams, err := m.cfg.AddressManager.GetStaticAddressParameters( | ||
| ctx, | ||
| // Send change back to the static address. |
There was a problem hiding this comment.
I propose this change to make it more clear that address is the same.
s/to the static address/to the same static address/
staticaddr/withdraw/manager.go
Outdated
| // the withdrawal. | ||
| return nil, 0, 0, fmt.Errorf("the change doesn't " + | ||
| "cover for fees. Consider lowering the fee " + | ||
| "rate or increase the withdrawal amount") |
There was a problem hiding this comment.
s/increase/decrease/ IIUC
There was a problem hiding this comment.
you are correct, so that the remaining amount can cover for fees.
| withdrawalAmount = selectedWithdrawalAmount | ||
|
|
||
| default: | ||
| // If the fees eat into our withdrawal amount, we fail |
There was a problem hiding this comment.
Actually, the feeWithChange fees eat into our withdrawal amount, but this doesn't mean that feeWithoutChange do, since it is lower than feeWithChange. This means, that some amounts fail, even though it is possible to withdraw without change. And we fallback to withdrawing without change in general case when it is not enough funds for the change. So I think for consistency we should do the same here. Can we replace case change-feeWithChange >= 0: with case change-feeWithoutChange >= 0: above?
There was a problem hiding this comment.
This is a good catch 🙏 You are right in that the withdrawal shouldn't fail until the feeWithoutChange eats into our withdrawal amount.
Changed it.
|
Needs a rebase. |
|
Needs a rebase 🙏 |
hieblmi
force-pushed
the
withdraw-amounts
branch
2 times, most recently
from
f944edf to
87eb78f
Compare
|
Rebased and addressed Boris' comments, thanks for the improvement @starius. |
| }) | ||
|
|
||
| if hasChange { | ||
| changeScript, err := txscript.PayToAddrScript(changeAddress) |
There was a problem hiding this comment.
I think we should just get the change address here (so no need to pass it around and it is not generated if unused).
There was a problem hiding this comment.
I moved all change address data into this if-branch.
hieblmi
force-pushed
the
withdraw-amounts
branch
2 times, most recently
from
f931282 to
c5e4b52
Compare
|
@starius: review reminder |
|
|
||
| if changeAmount < 0 { | ||
| return nil, 0, 0, fmt.Errorf("change amount is negative") | ||
| } |
There was a problem hiding this comment.
Optional. Can we add a check that changeAmount is lower than each input UTXO? If it is larger, this means we could save on fees by not including that UTXO into the transaction. Can we show a warning to the user asking to remove some UTXOs or to confirm that they want to proceed.
There was a problem hiding this comment.
That's a neat tweak, I will add that.
hieblmi
force-pushed
the
withdraw-amounts
branch
from
c5e4b52 to
c7cc47b
Compare
This PR adds an amount field to the withdrawal rpc.
It allows to withdraw a fraction of the sum of selected deposits to withdraw. The change is sent back to the static address.
If the change is below the dust limit it goes towards miner fees, but warns the user beforehand.