fix(dot/rpc/modules): grandpa.proveFinality update parameters, fix bug (#2576)

Author: edwardmack

dot/rpc/modules/api_mocks.go

@@ -27,8 +27,8 @@ func NewMockStorageAPI() *modulesmocks.StorageAPI {
 	return m
 }
 
-// NewMockBlockAPI creates and return an rpc BlockAPI interface mock
-func NewMockBlockAPI() *modulesmocks.BlockAPI {
+// NewMockeryBlockAPI creates and return an rpc BlockAPI interface mock
+func NewMockeryBlockAPI() *modulesmocks.BlockAPI {
 	m := new(modulesmocks.BlockAPI)
 	m.On("GetHeader", mock.AnythingOfType("common.Hash")).Return(nil, nil)
 	m.On("BestBlockHash").Return(common.Hash{})

dot/rpc/modules/grandpa.go

@@ -4,6 +4,7 @@
 package modules
 
 import (
+	"fmt"
 	"net/http"
 
 	"github.com/ChainSafe/gossamer/lib/common"
@@ -48,39 +49,35 @@ type RoundStateResponse struct {
 
 // ProveFinalityRequest request struct
 type ProveFinalityRequest struct {
-	blockHashStart common.Hash
-	blockHashEnd   common.Hash
-	authorityID    uint64
+	BlockNumber uint32 `json:"blockNumber"`
 }
 
 // ProveFinalityResponse is an optional SCALE encoded proof array
-type ProveFinalityResponse [][]byte
+type ProveFinalityResponse []string
 
-// ProveFinality for the provided block range. Returns NULL if there are no known finalised blocks in the range.
-// If no authorities set is provided, the current one will be attempted.
+// ProveFinality for the provided block number, the Justification for the last block in the set is written to the
+//  response. The response is a SCALE encoded proof array.  The proof array is empty if the block number is
+//  not finalized.
+//  Returns error which are included in the response if they occur.
 func (gm *GrandpaModule) ProveFinality(r *http.Request, req *ProveFinalityRequest, res *ProveFinalityResponse) error {
-	blocksToCheck, err := gm.blockAPI.SubChain(req.blockHashStart, req.blockHashEnd)
+	blockHash, err := gm.blockAPI.GetHashByNumber(uint(req.BlockNumber))
 	if err != nil {
 		return err
 	}
-
-	// Leaving check in for linter
-	if req.authorityID != uint64(0) {
-		// TODO: Check if functionality relevant (#1404)
+	hasJustification, err := gm.blockAPI.HasJustification(blockHash)
+	if err != nil {
+		return fmt.Errorf("checking for justification: %w", err)
 	}
 
-	for _, block := range blocksToCheck {
-		hasJustification, _ := gm.blockAPI.HasJustification(block)
-		if !hasJustification {
-			continue
-		}
-
-		justification, err := gm.blockAPI.GetJustification(block)
-		if err != nil {
-			continue
-		}
-		*res = append(*res, justification)
+	if !hasJustification {
+		*res = append(*res, "GRANDPA prove finality rpc failed: Block not covered by authority set changes")
+		return nil
+	}
+	justification, err := gm.blockAPI.GetJustification(blockHash)
+	if err != nil {
+		return fmt.Errorf("getting justification: %w", err)
 	}
+	*res = append(*res, common.BytesToHex(justification))
 
 	return nil
 }

dot/rpc/modules/grandpa_integration_test.go

@@ -6,14 +6,14 @@
 package modules
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/ChainSafe/gossamer/dot/state"
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/lib/crypto/ed25519"
 	"github.com/ChainSafe/gossamer/lib/grandpa"
 	"github.com/ChainSafe/gossamer/lib/keystore"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	rpcmocks "github.com/ChainSafe/gossamer/dot/rpc/modules/mocks"
@@ -36,22 +36,18 @@ func TestGrandpaProveFinality(t *testing.T) {
 	testStateService.Block.SetJustification(bestBlock.Header.ParentHash, make([]byte, 10))
 	testStateService.Block.SetJustification(bestBlock.Header.Hash(), make([]byte, 11))
 
-	var expectedResponse ProveFinalityResponse
-	expectedResponse = append(expectedResponse, make([]byte, 10), make([]byte, 11))
+	expectedResponse := &ProveFinalityResponse{"0x0000000000000000000000"}
 
 	res := new(ProveFinalityResponse)
 	err = gmSvc.ProveFinality(nil, &ProveFinalityRequest{
-		blockHashStart: bestBlock.Header.ParentHash,
-		blockHashEnd:   bestBlock.Header.Hash(),
+		BlockNumber: uint32(bestBlock.Header.Number),
 	}, res)
 
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	if !reflect.DeepEqual(*res, expectedResponse) {
-		t.Errorf("Fail: expected: %+v got: %+v\n", res, &expectedResponse)
-	}
+	assert.Equal(t, *expectedResponse, *res)
 }
 
 func TestRoundState(t *testing.T) {

dot/rpc/modules/grandpa_test.go

@@ -14,126 +14,99 @@ import (
 	"github.com/ChainSafe/gossamer/lib/crypto/ed25519"
 	"github.com/ChainSafe/gossamer/lib/grandpa"
 	"github.com/ChainSafe/gossamer/lib/keystore"
-
+	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestGrandpaModule_ProveFinality(t *testing.T) {
-	testHash := common.NewHash([]byte{0x01, 0x02})
-	testHashSlice := []common.Hash{testHash, testHash, testHash}
-
-	mockBlockFinalityAPI := new(mocks.BlockFinalityAPI)
-	mockBlockAPI := new(mocks.BlockAPI)
-	mockBlockAPI.On("SubChain", testHash, testHash).Return(testHashSlice, nil)
-	mockBlockAPI.On("HasJustification", testHash).Return(true, nil)
-	mockBlockAPI.On("GetJustification", testHash).Return([]byte("test"), nil)
+	t.Parallel()
 
-	mockBlockAPIHasJustErr := new(mocks.BlockAPI)
-	mockBlockAPIHasJustErr.On("SubChain", testHash, testHash).Return(testHashSlice, nil)
-	mockBlockAPIHasJustErr.On("HasJustification", testHash).Return(false, nil)
+	mockError := errors.New("test mock error")
 
-	mockBlockAPIGetJustErr := new(mocks.BlockAPI)
-	mockBlockAPIGetJustErr.On("SubChain", testHash, testHash).Return(testHashSlice, nil)
-	mockBlockAPIGetJustErr.On("HasJustification", testHash).Return(true, nil)
-	mockBlockAPIGetJustErr.On("GetJustification", testHash).Return(nil, errors.New("GetJustification error"))
-
-	mockBlockAPISubChainErr := new(mocks.BlockAPI)
-	mockBlockAPISubChainErr.On("SubChain", testHash, testHash).Return(nil, errors.New("SubChain error"))
-
-	grandpaModule := NewGrandpaModule(mockBlockAPISubChainErr, mockBlockFinalityAPI)
-	type fields struct {
-		blockAPI         BlockAPI
-		blockFinalityAPI BlockFinalityAPI
-	}
-	type args struct {
-		r   *http.Request
-		req *ProveFinalityRequest
-	}
-	tests := []struct {
-		name   string
-		fields fields
-		args   args
-		expErr error
-		exp    ProveFinalityResponse
+	tests := map[string]struct {
+		blockAPIBuilder func(ctrl *gomock.Controller) BlockAPI
+		request         *ProveFinalityRequest
+		expErr          error
+		exp             ProveFinalityResponse
 	}{
-		{
-			name: "SubChain Err",
-			fields: fields{
-				grandpaModule.blockAPI,
-				grandpaModule.blockFinalityAPI,
+		"error during get hash by number": {
+			blockAPIBuilder: func(ctrl *gomock.Controller) BlockAPI {
+				mockBlockAPI := NewMockBlockAPI(ctrl)
+				mockBlockAPI.EXPECT().GetHashByNumber(uint(1)).Return(common.Hash{}, mockError)
+				return mockBlockAPI
 			},
-			args: args{
-				req: &ProveFinalityRequest{
-					blockHashStart: testHash,
-					blockHashEnd:   testHash,
-					authorityID:    uint64(21),
-				},
+			request: &ProveFinalityRequest{
+				BlockNumber: 1,
 			},
-			expErr: errors.New("SubChain error"),
+			expErr: mockError,
 		},
-		{
-			name: "OK Case",
-			fields: fields{
-				mockBlockAPI,
-				mockBlockFinalityAPI,
+		"error during has justification": {
+			blockAPIBuilder: func(ctrl *gomock.Controller) BlockAPI {
+				mockBlockAPI := NewMockBlockAPI(ctrl)
+				mockBlockAPI.EXPECT().GetHashByNumber(uint(2)).Return(common.Hash{2}, nil)
+				mockBlockAPI.EXPECT().HasJustification(common.Hash{2}).Return(false, mockError)
+				return mockBlockAPI
 			},
-			args: args{
-				req: &ProveFinalityRequest{
-					blockHashStart: testHash,
-					blockHashEnd:   testHash,
-					authorityID:    uint64(21),
-				},
+			request: &ProveFinalityRequest{
+				BlockNumber: 2,
 			},
-			exp: ProveFinalityResponse{
-				[]uint8{0x74, 0x65, 0x73, 0x74},
-				[]uint8{0x74, 0x65, 0x73, 0x74},
-				[]uint8{0x74, 0x65, 0x73, 0x74}},
+			expErr: mockError,
 		},
-		{
-			name: "HasJustification Error",
-			fields: fields{
-				mockBlockAPIHasJustErr,
-				mockBlockFinalityAPI,
+		"has justification is false": {
+			blockAPIBuilder: func(ctrl *gomock.Controller) BlockAPI {
+				mockBlockAPI := NewMockBlockAPI(ctrl)
+				mockBlockAPI.EXPECT().GetHashByNumber(uint(2)).Return(common.Hash{2}, nil)
+				mockBlockAPI.EXPECT().HasJustification(common.Hash{2}).Return(false, nil)
+				return mockBlockAPI
 			},
-			args: args{
-				req: &ProveFinalityRequest{
-					blockHashStart: testHash,
-					blockHashEnd:   testHash,
-					authorityID:    uint64(21),
-				},
+			request: &ProveFinalityRequest{
+				BlockNumber: 2,
 			},
-			exp: ProveFinalityResponse(nil),
+			exp: ProveFinalityResponse{"GRANDPA prove finality rpc failed: Block not covered by authority set changes"},
 		},
-		{
-			name: "GetJustification Error",
-			fields: fields{
-				mockBlockAPIGetJustErr,
-				mockBlockFinalityAPI,
+		"error during getJustification": {
+			blockAPIBuilder: func(ctrl *gomock.Controller) BlockAPI {
+				mockBlockAPI := NewMockBlockAPI(ctrl)
+				mockBlockAPI.EXPECT().GetHashByNumber(uint(3)).Return(common.Hash{3}, nil)
+				mockBlockAPI.EXPECT().HasJustification(common.Hash{3}).Return(true, nil)
+				mockBlockAPI.EXPECT().GetJustification(common.Hash{3}).Return(nil, mockError)
+				return mockBlockAPI
 			},
-			args: args{
-				req: &ProveFinalityRequest{
-					blockHashStart: testHash,
-					blockHashEnd:   testHash,
-					authorityID:    uint64(21),
-				},
+			request: &ProveFinalityRequest{
+				BlockNumber: 3,
+			},
+			expErr: mockError,
+		},
+		"happy path": {
+			blockAPIBuilder: func(ctrl *gomock.Controller) BlockAPI {
+				mockBlockAPI := NewMockBlockAPI(ctrl)
+				mockBlockAPI.EXPECT().GetHashByNumber(uint(4)).Return(common.Hash{4}, nil)
+				mockBlockAPI.EXPECT().HasJustification(common.Hash{4}).Return(true, nil)
+				mockBlockAPI.EXPECT().GetJustification(common.Hash{4}).Return([]byte(`justification`), nil)
+				return mockBlockAPI
+			},
+			request: &ProveFinalityRequest{
+				BlockNumber: 4,
 			},
-			exp: ProveFinalityResponse(nil),
+			exp: ProveFinalityResponse{common.BytesToHex([]byte(`justification`))},
 		},
 	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+	for name, tt := range tests {
+		tt := tt
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			ctrl := gomock.NewController(t)
 			gm := &GrandpaModule{
-				blockAPI:         tt.fields.blockAPI,
-				blockFinalityAPI: tt.fields.blockFinalityAPI,
+				blockAPI: tt.blockAPIBuilder(ctrl),
 			}
 			res := ProveFinalityResponse(nil)
-			err := gm.ProveFinality(tt.args.r, tt.args.req, &res)
+			err := gm.ProveFinality(nil, tt.request, &res)
+			assert.Equal(t, tt.exp, res)
 			if tt.expErr != nil {
-				assert.EqualError(t, err, tt.expErr.Error())
+				assert.ErrorContains(t, err, tt.expErr.Error())
 			} else {
 				assert.NoError(t, err)
 			}
-			assert.Equal(t, tt.exp, res)
 		})
 	}
 }

dot/rpc/modules/mocks_generate_test.go

@@ -0,0 +1,6 @@
+// Copyright 2021 ChainSafe Systems (ON)
+// SPDX-License-Identifier: LGPL-3.0-only
+
+package modules
+
+//go:generate mockgen -destination=mocks_test.go -package=$GOPACKAGE . BlockAPI