Merge pull request #1858 from OpenBazaar/1839-moderator-info-validation

(#1839) Validate moderator fee schedule

Author: placer14

api/jsonapi_test.go

@@ -129,7 +129,7 @@ func TestProfile(t *testing.T) {
 	})
 }
 
-func TestProfileCurrencyUpdate(t *testing.T) {
+func TestPatchProfileCurrencyUpdate(t *testing.T) {
 	var (
 		postProfile = `{
 	"handle": "test",
@@ -154,7 +154,7 @@ func TestProfileCurrencyUpdate(t *testing.T) {
 	},
 	"currencies": ["LTC"]
 }`
-		putProfile      = `{"currencies": ["ETH"]}`
+		patchProfile    = `{"currencies": ["ETH"]}`
 		validateProfile = func(testRepo *test.Repository) error {
 			m, err := schema.NewCustomSchemaManager(schema.SchemaContext{
 				DataPath:        testRepo.Path,
@@ -189,10 +189,76 @@ func TestProfileCurrencyUpdate(t *testing.T) {
 
 	runAPITestsWithSetup(t, apiTests{
 		{"POST", "/ob/profile", postProfile, 200, anyResponseJSON},
-		{"PATCH", "/ob/profile", putProfile, 200, anyResponseJSON},
+		{"PATCH", "/ob/profile", patchProfile, 200, anyResponseJSON},
 	}, nil, validateProfile)
 }
 
+func TestPatchProfileCanBeInvalid(t *testing.T) {
+	var (
+		// init profile for patch
+		postProfile = `{
+	"handle": "test",
+	"name": "Test User",
+	"location": "Internet",
+	"about": "The test fixture",
+	"shortDescription": "Fixture",
+	"contactInfo": {
+		"website": "internet.com",
+		"email": "[email protected]",
+		"phoneNumber": "687-5309"
+	},
+	"nsfw": false,
+	"vendor": false,
+	"moderator": false,
+	"colors": {
+		"primary": "#000000",
+		"secondary": "#FFD700",
+		"text": "#ffffff",
+		"highlight": "#123ABC",
+		"highlightText": "#DEAD00"
+	},
+	"currencies": ["LTC"]
+}`
+		// test valid patch
+		patchProfile = `{
+	"moderator": true,
+	"moderatorInfo": {
+		"description": "Fix plus percentage. Test moderator account. DO NOT USE.",
+		"fee": {
+			"feeType": "FIXED_PLUS_PERCENTAGE",
+			"fixedFee": {
+				"amountCurrency": {
+					"code": "USD",
+					"divisibility": 2
+				},
+				"bigAmount": "2"
+			},
+			"percentage": 0.1
+		},
+		"languages": [
+			"en-US"
+		],
+		"termsAndConditions": "Test moderator account. DO NOT USE."
+	}
+}`
+		// test invalid patch: percentage must be greater than 0
+		invalidPatchProfile = `{
+	"moderatorInfo": {
+		"fee": {
+			"percentage": -1
+		}
+	}
+}`
+	)
+
+	expectedErr := fmt.Errorf("invalid profile: %s", repo.ErrModeratorFeeHasNegativePercentage)
+	runAPITests(t, apiTests{
+		{"POST", "/ob/profile", postProfile, 200, anyResponseJSON},
+		{"PATCH", "/ob/profile", patchProfile, 200, anyResponseJSON},
+		{"PATCH", "/ob/profile", invalidPatchProfile, 500, errorResponseJSON(expectedErr)},
+	})
+}
+
 func TestAvatar(t *testing.T) {
 	// Setting an avatar fails if we don't have a profile
 	runAPITests(t, apiTests{

core/moderation.go

@@ -221,12 +221,12 @@ func (n *OpenBazaarNode) GetModeratorFee(transactionTotal *big.Int, txCurrencyCo
 		}
 		f := big.NewFloat(float64(profile.ModeratorInfo.Fee.Percentage))
 		f.Mul(f, big.NewFloat(0.01))
-		t.Mul(t, f)
-		total, _ := t.Int(transactionTotal)
-		if fixed.Add(fixed, total).Cmp(transactionTotal) > 0 {
+		percentAmt, _ := new(big.Float).Mul(t, f).Int(nil)
+		feeTotal := new(big.Int).Add(fixed, percentAmt)
+		if feeTotal.Cmp(transactionTotal) > 0 {
 			return big.NewInt(0), errors.New("Fixed moderator fee exceeds transaction amount")
 		}
-		return fixed.Add(fixed, total), nil
+		return feeTotal, nil
 	default:
 		return big.NewInt(0), errors.New("Unrecognized fee type")
 	}

core/profile.go

@@ -204,7 +204,6 @@ func (n *OpenBazaarNode) PatchProfile(patch map[string]interface{}) error {
 		return err
 	}
 
-	// Execute UpdateProfile with new profile
 	newProfile, err := json.Marshal(patch)
 	if err != nil {
 		return err
@@ -213,6 +212,16 @@ func (n *OpenBazaarNode) PatchProfile(patch map[string]interface{}) error {
 	if err := jsonpb.Unmarshal(bytes.NewReader(newProfile), p); err != nil {
 		return err
 	}
+
+	repoProfile, err := repo.ProfileFromProtobuf(p)
+	if err != nil {
+		return fmt.Errorf("building profile for validation: %s", err.Error())
+	}
+
+	if err := repoProfile.Valid(); err != nil {
+		return fmt.Errorf("invalid profile: %s", err.Error())
+	}
+
 	return n.UpdateProfile(p)
 }
 

repo/currency.go

@@ -17,10 +17,11 @@ const (
 )
 
 var (
-	ErrCurrencyValueInsufficientPrecision = errors.New("unable to accurately represent value as int64")
-	ErrCurrencyValueNegativeRate          = errors.New("conversion rate must be greater than zero")
-	ErrCurrencyValueAmountInvalid         = errors.New("invalid amount")
-	ErrCurrencyValueDefinitionInvalid     = errors.New("invalid currency definition")
+	ErrCurrencyValueInsufficientPrecision         = errors.New("unable to accurately represent value as int64")
+	ErrCurrencyValueNegativeRate                  = errors.New("conversion rate must be greater than zero")
+	ErrCurrencyValueAmountInvalid                 = errors.New("invalid amount")
+	ErrCurrencyValueDefinitionInvalid             = errors.New("invalid currency definition")
+	ErrCurrencyValueInvalidCmpDifferentCurrencies = errors.New("unable to compare two different currencies")
 )
 
 // CurrencyValue represents the amount and variety of currency
@@ -245,3 +246,26 @@ func (v *CurrencyValue) ConvertTo(final CurrencyDefinition, exchangeRatio float6
 	roundedAcc := big.Accuracy(roundedAmount.Cmp(newAmount))
 	return &CurrencyValue{Amount: roundedInt, Currency: final}, roundedAcc, nil
 }
+
+// Cmp exposes the (*big.Int).Cmp behavior after verifying currency and adjusting
+// for different currency divisibilities.
+func (v *CurrencyValue) Cmp(other *CurrencyValue) (int, error) {
+	if v.Currency.Code.String() != other.Currency.Code.String() {
+		return 0, ErrCurrencyValueInvalidCmpDifferentCurrencies
+	}
+	if v.Currency.Equal(other.Currency) {
+		return v.Amount.Cmp(other.Amount), nil
+	}
+	if v.Currency.Divisibility > other.Currency.Divisibility {
+		adjOther, _, err := other.AdjustDivisibility(v.Currency.Divisibility)
+		if err != nil {
+			return 0, fmt.Errorf("adjusting other divisibility: %s", err.Error())
+		}
+		return v.Amount.Cmp(adjOther.Amount), nil
+	}
+	selfAdj, _, err := v.AdjustDivisibility(other.Currency.Divisibility)
+	if err != nil {
+		return 0, fmt.Errorf("adjusting self divisibility: %s", err.Error())
+	}
+	return selfAdj.Amount.Cmp(other.Amount), nil
+}

repo/currency_test.go

@@ -483,3 +483,51 @@ func TestCurrencyValueAdjustDivisibility(t *testing.T) {
 		}
 	}
 }
+
+func TestCurrencyValueCmp(t *testing.T) {
+	var examples = []struct {
+		subject     *repo.CurrencyValue
+		other       *repo.CurrencyValue
+		expected    int
+		expectedErr error
+	}{
+		{ // success case
+			subject:  factory.MustNewCurrencyValue("1234", "USD"),
+			other:    factory.MustNewCurrencyValue("12345", "USD"),
+			expected: -1, // subject.Amount.Cmp(other.Amount) == -1
+		},
+		{ // different divisibilities handled
+			subject:  factory.MustNewCurrencyValueUsingDiv("1234", "USD", 2),
+			other:    factory.MustNewCurrencyValueUsingDiv("123456", "USD", 4),
+			expected: -1, // subject.AdjustDivisibility(4).Amount.Cmp(other.Amount) == -1
+		},
+		{ // different divisibilities (reverse suject/other) handled
+			subject:  factory.MustNewCurrencyValueUsingDiv("123456", "USD", 4),
+			other:    factory.MustNewCurrencyValueUsingDiv("1234", "USD", 2),
+			expected: 1, // subject.AdjustDivisibility(4).Amount.Cmp(other.Amount) == 1
+		},
+		{ // different currencies return error
+			subject:     factory.MustNewCurrencyValue("1234", "USD"),
+			other:       factory.MustNewCurrencyValue("1234", "NOTUSD"),
+			expectedErr: repo.ErrCurrencyValueInvalidCmpDifferentCurrencies,
+		},
+	}
+
+	for _, e := range examples {
+		t.Logf("subject (%+v), other (%+v)", e.subject, e.other)
+		actual, err := e.subject.Cmp(e.other)
+		if e.expectedErr != nil {
+			if err != e.expectedErr {
+				t.Errorf("expected err (%s), but was (%s)", e.expectedErr.Error(), err.Error())
+			}
+		} else {
+			if err != nil {
+				t.Errorf("unexpected err: %s", err)
+				continue
+			}
+		}
+		if e.expected != actual {
+			t.Errorf("expected comparison result (%d), but was (%d)", e.expected, actual)
+		}
+	}
+}

repo/dispute_resolution.go

@@ -10,16 +10,25 @@ import (
 // turns them into their v5 counterpart.
 func ToV5DisputeResolution(disputeResolution *pb.DisputeResolution) *pb.DisputeResolution {
 	newDisputeResolution := proto.Clone(disputeResolution).(*pb.DisputeResolution)
+	if disputeResolution.Payout == nil {
+		return newDisputeResolution
+	}
 
-	if disputeResolution.Payout.BuyerOutput.Amount != 0 && disputeResolution.Payout.BuyerOutput.BigAmount == "" {
+	if disputeResolution.Payout.BuyerOutput != nil &&
+		disputeResolution.Payout.BuyerOutput.Amount != 0 &&
+		disputeResolution.Payout.BuyerOutput.BigAmount == "" {
 		newDisputeResolution.Payout.BuyerOutput.BigAmount = big.NewInt(int64(disputeResolution.Payout.BuyerOutput.Amount)).String()
 		newDisputeResolution.Payout.BuyerOutput.Amount = 0
 	}
-	if disputeResolution.Payout.VendorOutput.Amount != 0 && disputeResolution.Payout.VendorOutput.BigAmount == "" {
+	if disputeResolution.Payout.VendorOutput != nil &&
+		disputeResolution.Payout.VendorOutput.Amount != 0 &&
+		disputeResolution.Payout.VendorOutput.BigAmount == "" {
 		newDisputeResolution.Payout.VendorOutput.BigAmount = big.NewInt(int64(disputeResolution.Payout.VendorOutput.Amount)).String()
 		newDisputeResolution.Payout.VendorOutput.Amount = 0
 	}
-	if disputeResolution.Payout.ModeratorOutput.Amount != 0 && disputeResolution.Payout.ModeratorOutput.BigAmount == "" {
+	if disputeResolution.Payout.ModeratorOutput != nil &&
+		disputeResolution.Payout.ModeratorOutput.Amount != 0 &&
+		disputeResolution.Payout.ModeratorOutput.BigAmount == "" {
 		newDisputeResolution.Payout.ModeratorOutput.BigAmount = big.NewInt(int64(disputeResolution.Payout.ModeratorOutput.Amount)).String()
 		newDisputeResolution.Payout.ModeratorOutput.Amount = 0
 	}

repo/dispute_resolution_test.go

@@ -1,8 +1,10 @@
-package repo
+package repo_test
 
 import (
-	"github.com/OpenBazaar/openbazaar-go/pb"
 	"testing"
+
+	"github.com/OpenBazaar/openbazaar-go/pb"
+	"github.com/OpenBazaar/openbazaar-go/repo"
 )
 
 func TestToV5DisputeResolution(t *testing.T) {
@@ -20,7 +22,7 @@ func TestToV5DisputeResolution(t *testing.T) {
 				},
 			},
 		}
-		newDisputeResolution = ToV5DisputeResolution(disputeResolution)
+		newDisputeResolution = repo.ToV5DisputeResolution(disputeResolution)
 		expected             = "10000"
 	)
 
@@ -44,3 +46,52 @@ func TestToV5DisputeResolution(t *testing.T) {
 		t.Errorf("Expected Amount of 0, got %d", newDisputeResolution.Payout.ModeratorOutput.Amount)
 	}
 }
+
+func TestToV5DisputeResolutionHandlesMissingOutputs(t *testing.T) {
+	var (
+		examples = []*pb.DisputeResolution{
+			{ // missing BuyerOutput
+				Payout: &pb.DisputeResolution_Payout{
+					BuyerOutput: nil,
+					VendorOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+					ModeratorOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+				},
+			},
+			{ // missing VendorOutput
+				Payout: &pb.DisputeResolution_Payout{
+					BuyerOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+					VendorOutput: nil,
+					ModeratorOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+				},
+			},
+			{ // missing ModeratorOutput
+				Payout: &pb.DisputeResolution_Payout{
+					BuyerOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+					VendorOutput: &pb.DisputeResolution_Payout_Output{
+						Amount: 10000,
+					},
+					ModeratorOutput: nil,
+				},
+			},
+		}
+	)
+
+	for _, e := range examples {
+		repo.ToV5DisputeResolution(e)
+	}
+}
+
+func TestToV5DisputeResolutionHandlesMissingPayout(t *testing.T) {
+	var example = &pb.DisputeResolution{Payout: nil}
+	repo.ToV5DisputeResolution(example)
+}