Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pool: round default min chan amt to nearest unit #151

Merged
merged 1 commit into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions cmd/pool/order.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,31 +105,29 @@ func parseCommonParams(ctx *cli.Context, blockDuration uint32) (*poolrpc.Order,
}

// If the minimum channel amount flag wasn't provided, use a default of
// 10%, but make sure it doesn't dip below the minimum allowed.
minOrderChanAmt := btcutil.Amount(order.BaseSupplyUnit)
// 10% and round to the nearest unit.
minChanAmt := btcutil.Amount(ctx.Uint64("min_chan_amt"))
if minChanAmt == 0 {
minChanAmt = btcutil.Amount(params.Amt) / 10
if minChanAmt < minOrderChanAmt {
minChanAmt = minOrderChanAmt
}
minChanAmt = order.RoundToNextSupplyUnit(
btcutil.Amount(params.Amt) / 10,
).ToSatoshis()
}

// Verify the minimum channel amount flag has been properly set.
switch {
case minChanAmt%minOrderChanAmt != 0:
case minChanAmt%order.BaseSupplyUnit != 0:
return nil, fmt.Errorf("minimum channel amount %v must be "+
"a multiple of %v", minChanAmt, minOrderChanAmt)
"a multiple of %v", minChanAmt, order.BaseSupplyUnit)

case minChanAmt < minOrderChanAmt:
case minChanAmt < order.BaseSupplyUnit:
return nil, fmt.Errorf("minimum channel amount %v is below "+
"required value of %v", minChanAmt, minOrderChanAmt)
"required value of %v", minChanAmt, order.BaseSupplyUnit)

case minChanAmt > btcutil.Amount(params.Amt):
return nil, fmt.Errorf("minimum channel amount %v is above "+
"order amount %v", minChanAmt, btcutil.Amount(params.Amt))
}
params.MinUnitsMatch = uint32(minChanAmt / minOrderChanAmt)
params.MinUnitsMatch = uint32(minChanAmt / order.BaseSupplyUnit)

var err error
params.TraderKey, err = parseAccountKey(ctx, args)
Expand Down
8 changes: 7 additions & 1 deletion order/supplyunit.go
Original file line number Diff line number Diff line change
Expand 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change? To cutdown on casting somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seemed strange to define it as SupplyUnit as you could then invoke ToSatoshis on it resulting in 100000^2.

)

// NewSupplyFromSats calculates the number of supply units that can be bought or
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the TODO on L19 or is there still something more to do, @Roasbeef?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// units from the given amount in satoshis.
func RoundToNextSupplyUnit(sats btcutil.Amount) SupplyUnit {
return SupplyUnit((sats + BaseSupplyUnit - 1) / BaseSupplyUnit)
}

// ToSatoshis maps a set number of supply units to the corresponding number of
// satoshis.
func (s SupplyUnit) ToSatoshis() btcutil.Amount {
Expand Down
34 changes: 34 additions & 0 deletions order/supplyunit_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package order_test

import (
"math/rand"
"reflect"
"testing"
"testing/quick"

"github.com/btcsuite/btcutil"
"github.com/lightninglabs/pool/order"
"github.com/stretchr/testify/require"
)

// TestRoundToNextSupplyUnit runs a quick check scenario to ensure the
// correctness of RoundToNextSupplyUnit.
func TestRoundToNextSupplyUnit(t *testing.T) {
t.Parallel()

f := func(sats btcutil.Amount) bool {
units := order.RoundToNextSupplyUnit(sats)
if sats%order.BaseSupplyUnit == 0 {
return units == order.NewSupplyFromSats(sats)
}
return units == order.NewSupplyFromSats(sats)+1
}

cfg := &quick.Config{
Values: func(v []reflect.Value, r *rand.Rand) {
v[0] = reflect.ValueOf(btcutil.Amount(r.Int63()))
},
}

require.NoError(t, quick.Check(f, cfg))
}