op-challenger: Tidy up game status logging (#6917)

* op-challenger: Tidy up game status logging

Removes redundant call to get game status.

* op-challenger: Fix caller_test.

---------
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

op-challenger/fault/caller.go

@@ -4,13 +4,11 @@ import (
 	"context"
 	"math/big"
 
+	"github.com/ethereum-optimism/optimism/op-bindings/bindings"
+	"github.com/ethereum-optimism/optimism/op-challenger/fault/types"
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/ethclient"
-	"github.com/ethereum/go-ethereum/log"
-
-	"github.com/ethereum-optimism/optimism/op-bindings/bindings"
-	"github.com/ethereum-optimism/optimism/op-challenger/fault/types"
 )
 
 type FaultDisputeGameCaller interface {
@@ -19,62 +17,39 @@ type FaultDisputeGameCaller interface {
 }
 
 type FaultCaller struct {
-	FaultDisputeGameCaller
-	log log.Logger
+	contract FaultDisputeGameCaller
 }
 
-func NewFaultCaller(caller FaultDisputeGameCaller, log log.Logger) *FaultCaller {
+func NewFaultCaller(caller FaultDisputeGameCaller) *FaultCaller {
 	return &FaultCaller{
 		caller,
-		log,
 	}
 }
 
-func NewFaultCallerFromBindings(fdgAddr common.Address, client *ethclient.Client, log log.Logger) (*FaultCaller, error) {
+func NewFaultCallerFromBindings(fdgAddr common.Address, client *ethclient.Client) (*FaultCaller, error) {
 	caller, err := bindings.NewFaultDisputeGameCaller(fdgAddr, client)
 	if err != nil {
 		return nil, err
 	}
 	return &FaultCaller{
 		caller,
-		log,
 	}, nil
 }
 
-// LogGameInfo logs the game info.
-func (fc *FaultCaller) LogGameInfo(ctx context.Context) {
-	status, err := fc.GetGameStatus(ctx)
-	if err != nil {
-		fc.log.Error("failed to get game status", "err", err)
-		return
-	}
-	claimLen, err := fc.GetClaimDataLength(ctx)
-	if err != nil {
-		fc.log.Error("failed to get claim count", "err", err)
-		return
-	}
-	fc.log.Info("Game info", "claims", claimLen, "status", status)
-}
-
 // GetGameStatus returns the current game status.
 // 0: In Progress
 // 1: Challenger Won
 // 2: Defender Won
 func (fc *FaultCaller) GetGameStatus(ctx context.Context) (types.GameStatus, error) {
-	status, err := fc.Status(&bind.CallOpts{Context: ctx})
+	status, err := fc.contract.Status(&bind.CallOpts{Context: ctx})
 	return types.GameStatus(status), err
 }
 
-// GetClaimDataLength returns the number of claims in the game.
-func (fc *FaultCaller) GetClaimDataLength(ctx context.Context) (*big.Int, error) {
-	return fc.ClaimDataLen(&bind.CallOpts{Context: ctx})
-}
-
-func (fc *FaultCaller) LogClaimDataLength(ctx context.Context) {
-	claimLen, err := fc.GetClaimDataLength(ctx)
+// GetClaimCount returns the number of claims in the game.
+func (fc *FaultCaller) GetClaimCount(ctx context.Context) (uint64, error) {
+	count, err := fc.contract.ClaimDataLen(&bind.CallOpts{Context: ctx})
 	if err != nil {
-		fc.log.Error("failed to get claim count", "err", err)
-		return
+		return 0, err
 	}
-	fc.log.Info("Number of claims", "length", claimLen)
+	return count.Uint64(), nil
 }

op-challenger/fault/caller_test.go

@@ -64,7 +64,7 @@ func TestFaultCaller_GetGameStatus(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			fc := NewFaultCaller(test.caller, nil)
+			fc := NewFaultCaller(test.caller)
 			status, err := fc.GetGameStatus(context.Background())
 			require.Equal(t, test.expectedStatus, status)
 			require.Equal(t, test.expectedErr, err)
@@ -72,11 +72,11 @@ func TestFaultCaller_GetGameStatus(t *testing.T) {
 	}
 }
 
-func TestFaultCaller_GetClaimDataLength(t *testing.T) {
+func TestFaultCaller_GetClaimCount(t *testing.T) {
 	tests := []struct {
 		name                 string
 		caller               FaultDisputeGameCaller
-		expectedClaimDataLen *big.Int
+		expectedClaimDataLen uint64
 		expectedErr          error
 	}{
 		{
@@ -84,7 +84,7 @@ func TestFaultCaller_GetClaimDataLength(t *testing.T) {
 			caller: &mockFaultDisputeGameCaller{
 				claimDataLen: big.NewInt(1),
 			},
-			expectedClaimDataLen: big.NewInt(1),
+			expectedClaimDataLen: 1,
 			expectedErr:          nil,
 		},
 		{
@@ -92,15 +92,15 @@ func TestFaultCaller_GetClaimDataLength(t *testing.T) {
 			caller: &mockFaultDisputeGameCaller{
 				errClaimDataLen: true,
 			},
-			expectedClaimDataLen: nil,
+			expectedClaimDataLen: 0,
 			expectedErr:          errMock,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			fc := NewFaultCaller(test.caller, nil)
-			claimDataLen, err := fc.GetClaimDataLength(context.Background())
+			fc := NewFaultCaller(test.caller)
+			claimDataLen, err := fc.GetClaimCount(context.Background())
 			require.Equal(t, test.expectedClaimDataLen, claimDataLen)
 			require.Equal(t, test.expectedErr, err)
 		})

op-challenger/fault/game.go → op-challenger/fault/player.go

@@ -21,7 +21,7 @@ type Actor interface {
 
 type GameInfo interface {
 	GetGameStatus(context.Context) (types.GameStatus, error)
-	LogGameInfo(ctx context.Context)
+	GetClaimCount(context.Context) (uint64, error)
 }
 
 type GamePlayer struct {
@@ -80,7 +80,7 @@ func NewGamePlayer(
 		return nil, fmt.Errorf("failed to create the responder: %w", err)
 	}
 
-	caller, err := NewFaultCallerFromBindings(addr, client, logger)
+	caller, err := NewFaultCallerFromBindings(addr, client)
 	if err != nil {
 		return nil, fmt.Errorf("failed to bind the fault contract: %w", err)
 	}
@@ -100,21 +100,32 @@ func (g *GamePlayer) ProgressGame(ctx context.Context) bool {
 	}
 	if status, err := g.caller.GetGameStatus(ctx); err != nil {
 		g.logger.Warn("Unable to retrieve game status", "err", err)
-	} else if status != 0 {
-		var expectedStatus types.GameStatus
-		if g.agreeWithProposedOutput {
-			expectedStatus = types.GameStatusChallengerWon
-		} else {
-			expectedStatus = types.GameStatusDefenderWon
-		}
-		if expectedStatus == status {
-			g.logger.Info("Game won", "status", status)
-		} else {
-			g.logger.Error("Game lost", "status", status)
-		}
-		return true
 	} else {
-		g.caller.LogGameInfo(ctx)
+		g.logGameStatus(ctx, status)
+		return status != types.GameStatusInProgress
 	}
 	return false
 }
+
+func (g *GamePlayer) logGameStatus(ctx context.Context, status types.GameStatus) {
+	if status == types.GameStatusInProgress {
+		claimCount, err := g.caller.GetClaimCount(ctx)
+		if err != nil {
+			g.logger.Error("Failed to get claim count for in progress game", "err", err)
+			return
+		}
+		g.logger.Info("Game info", "claims", claimCount, "status", status)
+		return
+	}
+	var expectedStatus types.GameStatus
+	if g.agreeWithProposedOutput {
+		expectedStatus = types.GameStatusChallengerWon
+	} else {
+		expectedStatus = types.GameStatusDefenderWon
+	}
+	if expectedStatus == status {
+		g.logger.Info("Game won", "status", status)
+	} else {
+		g.logger.Error("Game lost", "status", status)
+	}
+}

op-challenger/fault/game_test.go → op-challenger/fault/player_test.go

@@ -11,27 +11,23 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestProgressGameAndLogState(t *testing.T) {
-	_, game, actor, gameInfo := setupProgressGameTest(t, true)
-	done := game.ProgressGame(context.Background())
-	require.False(t, done, "should not be done")
-	require.Equal(t, 1, actor.callCount, "should perform next actions")
-	require.Equal(t, 1, gameInfo.logCount, "should log latest game state")
-}
-
 func TestProgressGame_LogErrorFromAct(t *testing.T) {
-	handler, game, actor, gameInfo := setupProgressGameTest(t, true)
+	handler, game, actor, _ := setupProgressGameTest(t, true)
 	actor.err = errors.New("boom")
 	done := game.ProgressGame(context.Background())
 	require.False(t, done, "should not be done")
 	require.Equal(t, 1, actor.callCount, "should perform next actions")
-	require.Equal(t, 1, gameInfo.logCount, "should log latest game state")
 	errLog := handler.FindLog(log.LvlError, "Error when acting on game")
 	require.NotNil(t, errLog, "should log error")
 	require.Equal(t, actor.err, errLog.GetContextValue("err"))
+
+	// Should still log game status
+	msg := handler.FindLog(log.LvlInfo, "Game info")
+	require.NotNil(t, msg)
+	require.Equal(t, uint64(1), msg.GetContextValue("claims"))
 }
 
-func TestProgressGame_LogErrorWhenGameLost(t *testing.T) {
+func TestProgressGame_LogGameStatus(t *testing.T) {
 	tests := []struct {
 		name            string
 		status          types.GameStatus
@@ -67,16 +63,23 @@ func TestProgressGame_LogErrorWhenGameLost(t *testing.T) {
 			logLevel:        log.LvlInfo,
 			logMsg:          "Game won",
 		},
+		{
+			name:            "GameInProgress",
+			status:          types.GameStatusInProgress,
+			agreeWithOutput: true,
+			logLevel:        log.LvlInfo,
+			logMsg:          "Game info",
+		},
 	}
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
-			handler, game, _, gameInfo := setupProgressGameTest(t, test.agreeWithOutput)
+			handler, game, actor, gameInfo := setupProgressGameTest(t, test.agreeWithOutput)
 			gameInfo.status = test.status
 
 			done := game.ProgressGame(context.Background())
-			require.True(t, done, "should be done")
-			require.Equal(t, 0, gameInfo.logCount, "should not log latest game state")
+			require.Equal(t, 1, actor.callCount, "should perform next actions")
+			require.Equal(t, test.status != types.GameStatusInProgress, done, "should be done when not in progress")
 			errLog := handler.FindLog(test.logLevel, test.logMsg)
 			require.NotNil(t, errLog, "should log game result")
 			require.Equal(t, test.status, errLog.GetContextValue("status"))
@@ -91,7 +94,7 @@ func setupProgressGameTest(t *testing.T, agreeWithProposedRoot bool) (*testlog.C
 	}
 	logger.SetHandler(handler)
 	actor := &stubActor{}
-	gameInfo := &stubGameInfo{}
+	gameInfo := &stubGameInfo{claimCount: 1}
 	game := &GamePlayer{
 		agent:                   actor,
 		agreeWithProposedOutput: agreeWithProposedRoot,
@@ -112,15 +115,15 @@ func (a *stubActor) Act(ctx context.Context) error {
 }
 
 type stubGameInfo struct {
-	status   types.GameStatus
-	err      error
-	logCount int
+	status     types.GameStatus
+	claimCount uint64
+	err        error
 }
 
 func (s *stubGameInfo) GetGameStatus(ctx context.Context) (types.GameStatus, error) {
 	return s.status, s.err
 }
 
-func (s *stubGameInfo) LogGameInfo(ctx context.Context) {
-	s.logCount++
+func (s *stubGameInfo) GetClaimCount(ctx context.Context) (uint64, error) {
+	return s.claimCount, s.err
 }

op-challenger/fault/types/game.go

@@ -60,7 +60,7 @@ func NewGameState(agreeWithProposedOutput bool, root Claim, depth uint64) *gameS
 	}
 }
 
-// AgreeWithLevel returns if the game state agrees with the provided claim level.
+// AgreeWithClaimLevel returns if the game state agrees with the provided claim level.
 func (g *gameState) AgreeWithClaimLevel(claim Claim) bool {
 	isOddLevel := claim.Depth()%2 == 1
 	// If we agree with the proposed output, we agree with odd levels