Fix bug that causes history summary to grow incorrectly

Signed-off-by: Matthew Whitehead <[email protected]>

Author: matthew1001

pkg/txhistory/txhistory.go

@@ -129,8 +129,10 @@ func (h *manager) AddSubStatusAction(ctx context.Context, mtx *apitypes.ManagedT
 	// See if this action exists in the list already since we only want to update the single entry, not
 	// add a new one
 	currentSubStatus := mtx.History[len(mtx.History)-1]
+	alreadyRecordedAction := false
 	for _, entry := range currentSubStatus.Actions {
 		if entry.Action == action {
+			alreadyRecordedAction = true
 			entry.Count++
 			entry.LastOccurrence = fftypes.Now()
 
@@ -142,41 +144,39 @@ func (h *manager) AddSubStatusAction(ctx context.Context, mtx *apitypes.ManagedT
 			if info != nil {
 				entry.LastInfo = jsonOrString(info)
 			}
-
-			// As we have a possibly indefinite list of sub-status records (which might be a long list and early entries
-			// get purged at some point) we keep a separate list of all the discrete types of sub-status
-			// and action we've we've ever seen for this transaction along with a count of them. This means an early sub-status
-			// (e.g. "queued") with an action that never happens again (e.g. "assignNonce") followed by 100s of different sub-status
-			// types will still be recorded
-			for _, actionType := range mtx.HistorySummary {
-				if actionType.Action == action {
-					// Just increment the counter and last timestamp
-					actionType.LastOccurrence = fftypes.Now()
-					actionType.Count++
-					break
-				}
-			}
-			return
+			break
 		}
 	}
 
-	// If this is an entirely new status add it to the list
-	newAction := &apitypes.TxHistoryActionEntry{
-		Time:           fftypes.Now(),
-		Action:         action,
-		LastOccurrence: fftypes.Now(),
-		Count:          1,
-	}
+	if !alreadyRecordedAction {
+		// If this is an entirely new action for this status entry, add it to the list
+		newAction := &apitypes.TxHistoryActionEntry{
+			Time:           fftypes.Now(),
+			Action:         action,
+			LastOccurrence: fftypes.Now(),
+			Count:          1,
+		}
 
-	if err != nil {
-		newAction.LastError = jsonOrString(err)
-		newAction.LastErrorTime = fftypes.Now()
-	}
+		if err != nil {
+			newAction.LastError = jsonOrString(err)
+			newAction.LastErrorTime = fftypes.Now()
+		}
 
-	if info != nil {
-		newAction.LastInfo = jsonOrString(info)
+		if info != nil {
+			newAction.LastInfo = jsonOrString(info)
+		}
+
+		currentSubStatus.Actions = append(currentSubStatus.Actions, newAction)
 	}
 
-	currentSubStatus.Actions = append(currentSubStatus.Actions, newAction)
+	// Check if the history summary needs updating
+	for _, actionType := range mtx.HistorySummary {
+		if actionType.Action == action {
+			// Just increment the counter and last timestamp
+			actionType.LastOccurrence = fftypes.Now()
+			actionType.Count++
+			return
+		}
+	}
 	mtx.HistorySummary = append(mtx.HistorySummary, &apitypes.TxHistorySummaryEntry{Action: action, Count: 1, FirstOccurrence: fftypes.Now(), LastOccurrence: fftypes.Now()})
 }

pkg/txhistory/txhistory_test.go

@@ -102,7 +102,7 @@ func TestManagedTXSubStatusAction(t *testing.T) {
 	assert.Equal(t, 1, len(mtx.History[0].Actions))
 	assert.Nil(t, mtx.History[0].Actions[0].LastErrorTime)
 
-	// Add an action
+	// Add another action
 	h.AddSubStatusAction(ctx, mtx, apitypes.TxActionRetrieveGasPrice, nil, fftypes.JSONAnyPtr(`{"gasError":"Acme Gas Oracle RC=12345"}`))
 	assert.Equal(t, 2, len(mtx.History[0].Actions))
 	assert.Equal(t, (*mtx.History[0].Actions[1].LastError).String(), `{"gasError":"Acme Gas Oracle RC=12345"}`)
@@ -139,6 +139,17 @@ func TestManagedTXSubStatusAction(t *testing.T) {
 	// History is the complete list of unique sub-status types and actions
 	assert.Equal(t, 5, len(mtx.HistorySummary))
 
+	// Add some new sub-status and actions to check max lengths are correct
+	// Seen one of these before - should increase summary length by 1
+	h.SetSubStatus(ctx, mtx, apitypes.TxSubStatusConfirmed)
+	h.AddSubStatusAction(ctx, mtx, apitypes.TxActionReceiveReceipt, fftypes.JSONAnyPtr(`{"receiptId":"`+receiptId+`"}`), nil)
+	assert.Equal(t, 6, len(mtx.HistorySummary))
+
+	// Seen both of these before - no change expected
+	h.SetSubStatus(ctx, mtx, apitypes.TxSubStatusReceived)
+	h.AddSubStatusAction(ctx, mtx, apitypes.TxActionAssignNonce, nil, nil)
+	assert.Equal(t, 6, len(mtx.HistorySummary))
+
 	// Sanity check the history summary entries
 	for _, historyEntry := range mtx.HistorySummary {
 		assert.NotNil(t, historyEntry.FirstOccurrence)