Add sats_per_byte option to newAccounts cmd #288
Conversation
positiveblue
force-pushed
the
issue-191
branch
3 times, most recently
from
6ff83df to
cbb7d14
Compare
positiveblue
force-pushed
the
issue-191
branch
2 times, most recently
from
f5affb9 to
926abf0
Compare
account/manager.go
Outdated
|
|
||
| if feeRate == 0 { | ||
| return fmt.Errorf("unable to create transaction for "+ | ||
| "account with trader key %x, feeRate should be greater "+ |
There was a problem hiding this comment.
nit: wrap at 80 characters. You might need to update your editor's config to use 8 spaces for a tab character instead of just 4 or 2 to match everyone else's view of what 80 characters width is :)
There was a problem hiding this comment.
I wonder if we can use a lint step for this, like lll or something
There was a problem hiding this comment.
The problem is, some lines we can't really break nicely (for example the config with the tagged struct values). So we'd get a lot of false positives. And adding // nolint:lll to each of the long lines just makes them even longer 😅
account/manager.go
Outdated
|
|
||
| // We calculate the default fee rate that will be used | ||
| // for resuming accounts for which we haven't created and broadcast | ||
| // a transaction for yet |
There was a problem hiding this comment.
nit: not 100% sure but I think there's an extra "for" here.
account/manager.go
Outdated
| ctx, int32(DefaultFundingConfTarget), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to estimate default fees for resuming"+ |
There was a problem hiding this comment.
nit: add a space at the end after resuming as otherwise it'll concatenate with the next line. Also I think the error itself could just be: "unable to estimate default fees". I personally like long errors but overall we tend to not overexplain error cases and try to reduce to the actual cause.
rpcserver.go
Outdated
| return nil, fmt.Errorf("confirmation target must be " + | ||
| "greater than 0") | ||
| if req.GetFeeRateSatPerKw() > 0 && req.GetConfTarget() > 0 { | ||
| return nil, fmt.Errorf("you must set only one of the sat/vbyte " + |
There was a problem hiding this comment.
nit: in the error message I think you wanted to write sats/kw
rpcserver.go
Outdated
| "value=%v, conf_target=%v", feeRate, value, confTarget) | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("either sat/vbyte or confirmation target " + |
There was a problem hiding this comment.
Maybe I'd move the error case from the above if statement here too as a case.
There was a problem hiding this comment.
Let me add another small PR with this fix + adding the same check for expiryHeight
positiveblue
force-pushed
the
issue-191
branch
from
926abf0 to
884533e
Compare
Generally the confirmation target is much less granular than a direct sat/byte due to jumps in the confirmation confidence intervals. Changed the account manager to take the `feeRate` instead of the `confTarget`. The caller will be the one with the responsability of calculating the `freeRate`. If the caller does not provide a value (`freeRate=0`) we will calculate it from the default confTarget (6 blocks).
positiveblue
force-pushed
the
issue-191
branch
from
884533e to
e502ffd
Compare
cmd/pool/account.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("unable to estimate on-chain fees: "+ | ||
| "%v", err) | ||
| } | ||
|
|
||
| feeRate := chainfee.SatPerKWeight(resp.MinerFeeRateSatPerKw) | ||
| satPerVByte := float64(feeRate.FeePerKVByte()) / 1000 | ||
|
|
||
| feePerKVByte := float64(feeRate.FeePerKVByte()) / 1000 |
There was a problem hiding this comment.
nit: we still divide by 1000 so it's feePerVByte. Meaning, we can just overwrite the value of the existing variable here.
| if confTarget < 1 { | ||
| return nil, fmt.Errorf("confirmation target must be " + | ||
| "greater than 0") | ||
| if req.GetFeeRateSatPerKw() > 0 && req.GetConfTarget() > 0 { |
There was a problem hiding this comment.
nit: this could just be another case in the switch statement below.
positiveblue
force-pushed
the
issue-191
branch
from
e502ffd to
c90f158
Compare
Add ability to specify raw sat/byte when creating an account
#191