pool: round default min chan amt to nearest unit #151
Conversation
cmd/pool/order.go
Outdated
| @@ -82,6 +82,14 @@ func promptForConfirmation(msg string) bool { | |||
| } | |||
| } | |||
|
|
|||
| // roundUp rounds up `n` to the nearest multiple. | |||
| func roundUp(n, multiple btcutil.Amount) btcutil.Amount { | |||
There was a problem hiding this comment.
Algo looks sound at a glance. If we want to clamp things down further, it shouldn't be too involved to create a testing/quick test to ensure the output of this function, given arbitrary inputs (within our range bounds) should always return something that's a multiple of our unit size. This can likely live where the unit is defined.
There was a problem hiding this comment.
Could make this a method on the SupplyUnit itself for easy testing.
cmd/pool/order.go
Outdated
| @@ -82,6 +82,14 @@ func promptForConfirmation(msg string) bool { | |||
| } | |||
| } | |||
|
|
|||
| // roundUp rounds up `n` to the nearest multiple. | |||
| func roundUp(n, multiple btcutil.Amount) btcutil.Amount { | |||
There was a problem hiding this comment.
Could make this a method on the SupplyUnit itself for easy testing.
| @@ -21,6 +21,12 @@ func NewSupplyFromSats(sats btcutil.Amount) SupplyUnit { | |||
| return SupplyUnit(uint64(sats) / uint64(BaseSupplyUnit)) | |||
| } | |||
|
|
|||
| // RoundToNextSupplyUnit computes and rounds to the next whole number of supply | |||
There was a problem hiding this comment.
Can we remove the TODO on L19 or is there still something more to do, @Roasbeef?
There was a problem hiding this comment.
Nope this should be good, was just a hunch to revisit to ensure things are consistent. I think we need to do a sweep w.r.t where we use either of these methods, we may want to unify and ensure we always round up.
| @@ -21,6 +21,12 @@ func NewSupplyFromSats(sats btcutil.Amount) SupplyUnit { | |||
| return SupplyUnit(uint64(sats) / uint64(BaseSupplyUnit)) | |||
| } | |||
|
|
|||
| // RoundToNextSupplyUnit computes and rounds to the next whole number of supply | |||
There was a problem hiding this comment.
Nope this should be good, was just a hunch to revisit to ensure things are consistent. I think we need to do a sweep w.r.t where we use either of these methods, we may want to unify and ensure we always round up.
| @@ -10,7 +10,7 @@ type SupplyUnit uint64 | |||
| const ( | |||
| // BaseSupplyUnit is the smallest channel that can be bought or sold in | |||
| // the system. These units are expressed in satoshis. | |||
| BaseSupplyUnit SupplyUnit = 100_000 | |||
| BaseSupplyUnit btcutil.Amount = 100_000 | |||
There was a problem hiding this comment.
Why change? To cutdown on casting somewhere else?
There was a problem hiding this comment.
It just seemed strange to define it as SupplyUnit as you could then invoke ToSatoshis on it resulting in 100000^2.
Fee estimation for batch, deposit and withdrawal transactions
Fixes #145.