code review updates

Signed-off-by: Merlin Ran <[email protected]>

README.md

@@ -185,7 +185,7 @@ Do you think `bidbot` can have other commands that would make your life easier?
 [CID gravity](https://www.cidgravity.com) is a tool for storage providers to manage clients and price tiers. If integrated, bidbot can bid based on the configuration there, rather than locally configured `--ask-price` and `--verified-ask-price`. There are only two parameters involved.
 
 * `--cid-gravity-key`. You should be able to generate one by clicking the "Integrations" menu item from the CID gravity console.
-* `--cid-gravity-default-reject`. By default, if bidbot can not reach the CID gravity API for some reason, it bids based on the locally configured price. If you want it to stop bidding in that case, set this to true.
+* `--cid-gravity-strict`. By default, if bidbot can not reach the CID gravity API for some reason, it bids based on the locally configured price. If you want it to stop bidding in that case, set this to true.
 
 ## Contributing
 

main.go

@@ -173,7 +173,7 @@ Zero means no limits`,
 		},
 
 		{
-			Name:        "cid-gravity-default-reject",
+			Name:        "cid-gravity-strict",
 			DefValue:    false,
 			Description: "When CID gravity is enabled, stop bidding if there's any problem loading cid-gravity pricing rules.",
 		},
@@ -381,11 +381,11 @@ var daemonCmd = &cobra.Command{
 					Max: v.GetUint64("deal-size-max"),
 				},
 			},
-			BytesLimiter:              bytesLimiter,
-			ConcurrentImports:         v.GetInt("concurrent-imports-limit"),
-			SealingSectorsLimit:       v.GetInt("sealing-sectors-limit"),
-			PricingRules:              pricing.EmptyRules{},
-			PricingRulesDefaultReject: v.GetBool("cid-gravity-default-reject"),
+			BytesLimiter:        bytesLimiter,
+			ConcurrentImports:   v.GetInt("concurrent-imports-limit"),
+			SealingSectorsLimit: v.GetInt("sealing-sectors-limit"),
+			PricingRules:        pricing.EmptyRules{},
+			PricingRulesStrict:  v.GetBool("cid-gravity-strict"),
 		}
 		if cidGravityKey := v.GetString("cid-gravity-key"); cidGravityKey != "" {
 			config.PricingRules = pricing.NewCIDGravityRules(cidGravityKey)

service/limiter/running_total_limiter_test.go

@@ -45,7 +45,7 @@ func TestRequestCommit(t *testing.T) {
 }
 
 func TestSecure(t *testing.T) {
-	shortExp := time.Millisecond
+	shortExp := 10 * time.Millisecond
 	rl := NewRunningTotalLimiter(5, 50*time.Millisecond)
 	require.True(t, rl.Request("id1", 3, shortExp))
 	require.False(t, rl.Request("id2", 3, shortExp))

service/pricing/cid_gravity.go

@@ -26,8 +26,7 @@ var (
 	cidGravityCachePeriod = time.Minute
 )
 
-// CIDGravityRules is the format CID gravity API returns. Needs to be public to unmarshal JSON payload. Do not use.
-type CIDGravityRules struct {
+type rawRules struct {
 	Blocked         bool
 	MaintenanceMode bool
 	PricingRules    []struct {
@@ -41,24 +40,24 @@ type CIDGravityRules struct {
 }
 
 type cidGravityRules struct {
-	apiKey      string
-	clientRules map[string]*clientRules
-	lkRules     sync.Mutex
+	apiKey         string
+	perClientRules map[string]*clientRules
+	lkRules        sync.Mutex
 }
 
 // NewCIDGravityRules returns PricingRules based on CID gravity configuration for the storage provider.
 func NewCIDGravityRules(apiKey string) PricingRules {
-	return &cidGravityRules{apiKey: apiKey, clientRules: make(map[string]*clientRules)}
+	return &cidGravityRules{apiKey: apiKey, perClientRules: make(map[string]*clientRules)}
 }
 
 // PricesFor looks up prices for the auction based on its client address.
 func (cg *cidGravityRules) PricesFor(auction *pb.Auction) (prices ResolvedPrices, valid bool) {
 	cg.lkRules.Lock()
-	rules, exists := cg.clientRules[auction.ClientAddress]
+	rules, exists := cg.perClientRules[auction.ClientAddress]
 	if !exists {
 		rules = newClientRulesFor(cg.apiKey, auction.ClientAddress)
 	}
-	cg.clientRules[auction.ClientAddress] = rules
+	cg.perClientRules[auction.ClientAddress] = rules
 	cg.lkRules.Unlock()
 	return rules.PricesFor(auction)
 }
@@ -95,16 +94,16 @@ func (cg *clientRules) PricesFor(auction *pb.Auction) (prices ResolvedPrices, va
 		valid = false
 		return
 	}
-	if rules.(*CIDGravityRules).Blocked {
+	if rules.(*rawRules).Blocked {
 		return
 	}
-	if rules.(*CIDGravityRules).MaintenanceMode {
+	if rules.(*rawRules).MaintenanceMode {
 		return
 	}
 	// rules are checked in sequence and the first match wins.
-	for _, r := range rules.(*CIDGravityRules).PricingRules {
-		if auction.DealSize >= r.MinSize && auction.DealSize < r.MaxSize &&
-			auction.DealDuration >= r.MinDuration && auction.DealDuration < r.MaxDuration {
+	for _, r := range rules.(*rawRules).PricingRules {
+		if auction.DealSize >= r.MinSize && auction.DealSize <= r.MaxSize &&
+			auction.DealDuration >= r.MinDuration && auction.DealDuration <= r.MaxDuration {
 			if r.Verified && !prices.VerifiedPriceValid {
 				prices.VerifiedPriceValid, prices.VerifiedPrice = true, r.Price
 			} else if !prices.UnverifiedPriceValid {
@@ -162,12 +161,12 @@ func (cg *clientRules) maybeReloadRules(url string, timeout time.Duration, cache
 			if err != nil {
 				return fmt.Errorf("reading http response: %v", err)
 			}
-			var rules CIDGravityRules
+			var rules rawRules
 			if err := json.Unmarshal(b, &rules); err != nil {
 				return fmt.Errorf("unmarshalling rules: %v", err)
 			}
-			cg.rulesLastUpdated.Store(time.Now())
 			cg.rules.Store(&rules)
+			cg.rulesLastUpdated.Store(time.Now())
 			return nil
 		}()
 		log.Errorf("loading rules from API: %v", err)

service/pricing/cid_gravity_test.go

@@ -16,7 +16,7 @@ import (
 func TestPriceFor(t *testing.T) {
 	cidGravityAPIUrl = "http://localhost:invalid" // do not care about rules loading
 	cidGravityCachePeriod = time.Second
-	rules := &CIDGravityRules{
+	rules := &rawRules{
 		PricingRules: []struct {
 			Verified    bool
 			MinSize     uint64
@@ -27,32 +27,32 @@ func TestPriceFor(t *testing.T) {
 		}{
 			{
 				Verified: false, Price: 100,
-				MinSize: 2 << 20, MaxSize: 2 << 30,
+				MinSize: 2 << 20, MaxSize: 2<<30 - 1,
 				MinDuration: 1, MaxDuration: 2 << 10,
 			},
 			{
 				Verified: false, Price: 10,
-				MinSize: 2 << 30, MaxSize: 2 << 40,
+				MinSize: 2 << 30, MaxSize: 2<<40 - 1,
 				MinDuration: 1, MaxDuration: 2 << 10,
 			},
 			{
 				Verified: false, Price: 1000,
-				MinSize: 2 << 30, MaxSize: 2 << 40,
+				MinSize: 2 << 30, MaxSize: 2<<40 - 1,
 				MinDuration: 2 >> 10, MaxDuration: 2 << 30,
 			},
 			{
 				Verified: true, Price: 1,
-				MinSize: 1, MaxSize: 2 << 30,
+				MinSize: 1, MaxSize: 2<<30 - 1,
 				MinDuration: 1, MaxDuration: 2 << 10,
 			},
 			{
 				Verified: true, Price: 0,
-				MinSize: 2 << 30, MaxSize: 2 << 40,
+				MinSize: 2 << 30, MaxSize: 2<<40 - 1,
 				MinDuration: 1, MaxDuration: 2 << 10,
 			},
 			{
 				Verified: true, Price: 100,
-				MinSize: 2 << 30, MaxSize: 2 << 40,
+				MinSize: 2 << 30, MaxSize: 2<<40 - 1,
 				MinDuration: 2 >> 10, MaxDuration: 2 << 30,
 			},
 		},
@@ -165,17 +165,17 @@ func TestMaybeReloadRules(t *testing.T) {
 	t.Run("timeout", func(t *testing.T) {
 		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 			time.Sleep(100 * time.Millisecond)
-			response, _ := json.Marshal(CIDGravityRules{MaintenanceMode: true})
+			response, _ := json.Marshal(rawRules{MaintenanceMode: true})
 			_, _ = rw.Write(response)
 		}))
 		cg.maybeReloadRules(server.URL, 10*time.Millisecond, 0)
-		rules := cg.rules.Load().(*CIDGravityRules)
+		rules := cg.rules.Load().(*rawRules)
 		assert.False(t, rules.MaintenanceMode, "should have loaded the cached rules")
 		assert.Len(t, rules.PricingRules, 1)
 		assert.Equal(t, uint64(1), rules.PricingRules[0].MinDuration)
 		time.Sleep(100 * time.Millisecond)
 		assert.True(t,
-			cg.rules.Load().(*CIDGravityRules).MaintenanceMode,
+			cg.rules.Load().(*rawRules).MaintenanceMode,
 			"should have loaded the new rules")
 	})
 }

service/service.go

@@ -50,14 +50,14 @@ var (
 
 // Config defines params for Service configuration.
 type Config struct {
-	Peer                      peer.Config
-	BidParams                 BidParams
-	AuctionFilters            AuctionFilters
-	BytesLimiter              limiter.Limiter
-	ConcurrentImports         int
-	SealingSectorsLimit       int
-	PricingRules              pricing.PricingRules
-	PricingRulesDefaultReject bool
+	Peer                peer.Config
+	BidParams           BidParams
+	AuctionFilters      AuctionFilters
+	BytesLimiter        limiter.Limiter
+	ConcurrentImports   int
+	SealingSectorsLimit int
+	PricingRules        pricing.PricingRules
+	PricingRulesStrict  bool
 }
 
 // BidParams defines how bids are made.
@@ -151,12 +151,12 @@ type Service struct {
 	store      *bidstore.Store
 	subscribed bool
 
-	bidParams                 BidParams
-	auctionFilters            AuctionFilters
-	bytesLimiter              limiter.Limiter
-	sealingSectorsLimit       int
-	pricingRules              pricing.PricingRules
-	pricingRulesDefaultReject bool
+	bidParams           BidParams
+	auctionFilters      AuctionFilters
+	bytesLimiter        limiter.Limiter
+	sealingSectorsLimit int
+	pricingRules        pricing.PricingRules
+	pricingRulesStrict  bool
 
 	ctx       context.Context
 	finalizer *finalizer.Finalizer
@@ -218,18 +218,18 @@ func New(
 	}
 
 	srv := &Service{
-		peer:                      p,
-		fc:                        fc,
-		lc:                        lc,
-		store:                     s,
-		bidParams:                 conf.BidParams,
-		auctionFilters:            conf.AuctionFilters,
-		bytesLimiter:              conf.BytesLimiter,
-		sealingSectorsLimit:       conf.SealingSectorsLimit,
-		pricingRules:              conf.PricingRules,
-		pricingRulesDefaultReject: conf.PricingRulesDefaultReject,
-		ctx:                       ctx,
-		finalizer:                 fin,
+		peer:                p,
+		fc:                  fc,
+		lc:                  lc,
+		store:               s,
+		bidParams:           conf.BidParams,
+		auctionFilters:      conf.AuctionFilters,
+		bytesLimiter:        conf.BytesLimiter,
+		sealingSectorsLimit: conf.SealingSectorsLimit,
+		pricingRules:        conf.PricingRules,
+		pricingRulesStrict:  conf.PricingRulesStrict,
+		ctx:                 ctx,
+		finalizer:           fin,
 	}
 	if srv.pricingRules == nil {
 		srv.pricingRules = pricing.EmptyRules{}
@@ -397,7 +397,7 @@ func (s *Service) makeBid(a *pb.Auction, from core.ID) error {
 
 	prices, valid := s.pricingRules.PricesFor(a)
 	log.Infof("pricing engine result valid for auction %s?: %v, details: %+v", a.Id, valid, prices)
-	if !valid && s.pricingRulesDefaultReject {
+	if !valid && s.pricingRulesStrict {
 		return nil
 	}
 	if !prices.UnverifiedPriceValid {