From d01d2a81d2a95110a89b45d9cf66946498a68bc9 Mon Sep 17 00:00:00 2001
From: roos <roos@anapaya.net>
Date: Mon, 22 Jul 2019 10:14:03 +0200
Subject: [PATCH] feedback

---
 go/lib/scrypto/trc/v2/keychanges.go  | 19 +++++---------
 go/lib/scrypto/trc/v2/update.go      | 39 ++++++++++------------------
 go/lib/scrypto/trc/v2/update_test.go |  5 ++--
 3 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/go/lib/scrypto/trc/v2/keychanges.go b/go/lib/scrypto/trc/v2/keychanges.go
index 58ba64a207..3849724974 100644
--- a/go/lib/scrypto/trc/v2/keychanges.go
+++ b/go/lib/scrypto/trc/v2/keychanges.go
@@ -59,12 +59,6 @@ func (c *KeyChanges) Sensitive() bool {
 	return len(c.Fresh[OfflineKey]) != 0 || len(c.Modified[OfflineKey]) != 0
 }
 
-func (c *KeyChanges) insertAllFresh(as addr.AS, next PrimaryAS) {
-	for keyType, meta := range next.Keys {
-		c.Fresh[keyType][as] = meta
-	}
-}
-
 func (c *KeyChanges) insertModifications(as addr.AS, prev, next PrimaryAS) error {
 	for keyType, meta := range next.Keys {
 		prevMeta, ok := prev.Keys[keyType]
@@ -89,14 +83,13 @@ func (c *KeyChanges) insertModifications(as addr.AS, prev, next PrimaryAS) error
 // return value indicates, whether the update is a modification.
 func ValidateKeyUpdate(prev, next KeyMeta) (bool, error) {
 	modified := next.Algorithm != prev.Algorithm || !bytes.Equal(next.Key, prev.Key)
-	// If the meta data has changed, expect a key version change.
-	expectedVersion := prev.KeyVersion
-	if modified {
-		expectedVersion = prev.KeyVersion + 1
-	}
-	if next.KeyVersion != expectedVersion {
+	switch {
+	case modified && next.KeyVersion != prev.KeyVersion+1:
+		return modified, common.NewBasicError(InvalidKeyVersion, nil, "modified", modified,
+			"expected", prev.KeyVersion+1, "actual", next.KeyVersion)
+	case !modified && next.KeyVersion != prev.KeyVersion:
 		return modified, common.NewBasicError(InvalidKeyVersion, nil, "modified", modified,
-			"expected", expectedVersion, "actual", next.KeyVersion)
+			"expected", prev.KeyVersion, "actual", next.KeyVersion)
 	}
 	return modified, nil
 }
diff --git a/go/lib/scrypto/trc/v2/update.go b/go/lib/scrypto/trc/v2/update.go
index 76a2b97e2e..e2eb978af1 100644
--- a/go/lib/scrypto/trc/v2/update.go
+++ b/go/lib/scrypto/trc/v2/update.go
@@ -137,12 +137,7 @@ func (v *UpdateValidator) sanity() error {
 func (v *UpdateValidator) keyChanges() (*KeyChanges, error) {
 	c := newKeyChanges()
 	for as, primary := range v.Next.PrimaryASes {
-		prev, ok := v.Prev.PrimaryASes[as]
-		if !ok {
-			c.insertAllFresh(as, primary)
-			continue
-		}
-		if err := c.insertModifications(as, prev, primary); err != nil {
+		if err := c.insertModifications(as, v.Prev.PrimaryASes[as], primary); err != nil {
 			return nil, err
 		}
 	}
@@ -153,17 +148,12 @@ func (v *UpdateValidator) attrChanges() AttributeChanges {
 	c := make(AttributeChanges)
 	// Check all attributes of all ASes that persist or are added.
 	for as, primary := range v.Next.PrimaryASes {
-		prev, ok := v.Prev.PrimaryASes[as]
-		if !ok {
-			c.insertAll(as, primary.Attributes, AttributeAdded)
-			continue
-		}
-		c.insertModifications(as, prev, primary)
+		c.insertModifications(as, v.Prev.PrimaryASes[as], primary)
 	}
 	// Check all attributes of all ASes that are removed.
 	for as, prev := range v.Prev.PrimaryASes {
-		if _, ok := v.Next.PrimaryASes[as]; !ok {
-			c.insertAll(as, prev.Attributes, AttributeRemoved)
+		if empty, ok := v.Next.PrimaryASes[as]; !ok {
+			c.insertModifications(as, prev, empty)
 		}
 	}
 	return c
@@ -185,12 +175,15 @@ func (v *UpdateValidator) checkProofOfPossesion(keyChanges *KeyChanges) error {
 }
 
 func (v *UpdateValidator) checkVotes(info UpdateInfo) error {
-	check := v.checkVotesSensitive
-	if info.Type == RegularUpdate {
-		check = v.checkVotesRegular
-	}
-	if err := check(info); err != nil {
-		return err
+	switch info.Type {
+	case RegularUpdate:
+		if err := v.checkVotesRegular(info); err != nil {
+			return err
+		}
+	default:
+		if err := v.checkVotesSensitive(info); err != nil {
+			return err
+		}
 	}
 	if len(v.Next.Votes) < v.Prev.VotingQuorum() {
 		return common.NewBasicError(QuorumUnmet, nil,
@@ -266,12 +259,6 @@ func (c AttributeChanges) Sensitive() bool {
 	return len(c) != 0
 }
 
-func (c AttributeChanges) insertAll(as addr.AS, attrs Attributes, change AttributeChange) {
-	for _, attr := range attrs {
-		c.insert(as, attr, change)
-	}
-}
-
 func (c AttributeChanges) insertModifications(as addr.AS, prev, next PrimaryAS) {
 	for _, attr := range next.Attributes {
 		if !prev.Is(attr) {
diff --git a/go/lib/scrypto/trc/v2/update_test.go b/go/lib/scrypto/trc/v2/update_test.go
index 6238cab11f..3bacb2c9f2 100644
--- a/go/lib/scrypto/trc/v2/update_test.go
+++ b/go/lib/scrypto/trc/v2/update_test.go
@@ -35,8 +35,7 @@ func TestCommonUpdate(t *testing.T) {
 		"Trust reset": {
 			Modify: func(updated, _ *trc.TRC) {
 				*updated = *newBaseTRC()
-				updated.Version = 2
-				updated.BaseVersion = 2
+				updated.BaseVersion = updated.Version
 			},
 			ExpectedErrMsg: trc.ErrBaseNotUpdate.Error(),
 		},
@@ -48,7 +47,7 @@ func TestCommonUpdate(t *testing.T) {
 		},
 		"Wrong ISD": {
 			Modify: func(updated, _ *trc.TRC) {
-				updated.ISD = 2
+				updated.ISD = updated.ISD + 1
 			},
 			ExpectedErrMsg: trc.ImmutableISD,
 		},