feedback

dot/core/messages.go

@@ -70,34 +70,32 @@ func (s *Service) HandleTransactionMessage(peerID peer.ID, msg *network.Transact
 		return false, err
 	}
 
-	allTxsAreValid := true
+	allTxnsAreValid := true
 	for _, tx := range txs {
-		isValidTxn := true
+		txnIsValid := true
 		validity, err := s.validateTransaction(head, rt, tx)
 		if err != nil {
+			txnIsValid = false
+			allTxnsAreValid = false
 			switch err.(type) {
 			case runtime.InvalidTransaction:
-				isValidTxn = false
-				allTxsAreValid = false
 				s.net.ReportPeer(peerset.ReputationChange{
 					Value:  peerset.BadTransactionValue,
 					Reason: peerset.BadTransactionReason,
 				}, peerID)
 			case runtime.UnknownTransaction:
-				isValidTxn = false
-				allTxsAreValid = false
 			default:
-				return false, fmt.Errorf("failed validating transaction for peerID %s: %w", peerID, err)
+				return false, fmt.Errorf("validating transaction from peerID %s: %w", peerID, err)
 			}
 		}
 
-		if isValidTxn && validity.Propagate {
+		if txnIsValid && validity.Propagate {
 			// find tx(s) that should propagate
 			toPropagate = append(toPropagate, tx)
 		}
 	}
 
-	if allTxsAreValid {
+	if allTxnsAreValid {
 		s.net.ReportPeer(peerset.ReputationChange{
 			Value:  peerset.GoodTransactionValue,
 			Reason: peerset.GoodTransactionReason,

dot/core/messages_test.go

@@ -230,7 +230,7 @@ func TestServiceHandleTransactionMessage(t *testing.T) {
 				},
 			},
 			expErr: errDummyErr,
-			expErrMsg: "failed validating transaction for peerID D1KeRhQ: cannot get trie state from storage" +
+			expErrMsg: "validating transaction from peerID D1KeRhQ: cannot get trie state from storage" +
 				" for root 0x0000000000000000000000000000000000000000000000000000000000000000: dummy error for testing",
 		},
 		{
@@ -262,7 +262,7 @@ func TestServiceHandleTransactionMessage(t *testing.T) {
 				setContextStorage: &mockSetContextStorage{trieState: &storage.TrieState{}},
 				validateTxn: &mockValidateTxn{
 					input: types.Extrinsic(append([]byte{byte(types.TxnExternal)}, testExtrinsic[0]...)),
-					err:   invalidTransaction,
+					err:   *invalidTransaction,
 				},
 			},
 			args: args{

dot/core/service_integration_test.go

@@ -554,6 +554,7 @@ func TestService_HandleSubmittedExtrinsic(t *testing.T) {
 	require.NoError(t, err)
 
 	extBytes := createExtrinsic(t, rt, genHeader.Hash(), 0)
+
 	err = s.HandleSubmittedExtrinsic(extBytes)
 	require.NoError(t, err)
 }

dot/rpc/modules/author_integration_test.go

@@ -218,7 +218,6 @@ func TestAuthorModule_SubmitExtrinsic_AlreadyInPool(t *testing.T) {
 	// creating an extrisinc to the System.remark call using a sample argument
 	extHex := runtime.NewTestExtrinsic(t,
 		integrationTestController.runtime, genesisHash, genesisHash, 0, "System.remark", []byte{})
-	fmt.Println(extHex)
 	extBytes := common.MustHexToBytes(extHex)
 
 	integrationTestController.storageState = &state.StorageState{}

lib/runtime/invalid_transaction.go

@@ -48,13 +48,14 @@ func (i InvalidTransaction) Error() string {
 }
 
 // NewInvalidTransaction is constructor for InvalidTransaction
-func NewInvalidTransaction() InvalidTransaction {
+func NewInvalidTransaction() *InvalidTransaction {
 	vdt, err := scale.NewVaryingDataType(Call{}, Payment{}, Future{}, Stale{}, BadProof{}, AncientBirthBlock{},
 		ExhaustsResources{}, InvalidCustom(0), BadMandatory{}, MandatoryDispatch{})
 	if err != nil {
 		panic(err)
 	}
-	return InvalidTransaction(vdt)
+	invalidTxnVdr := InvalidTransaction(vdt)
+	return &invalidTxnVdr
 }
 
 // Call The call of the transaction is not expected
@@ -140,7 +141,7 @@ type InvalidCustom uint8
 // Index returns the VDT index
 func (InvalidCustom) Index() uint { return 7 }
 
-// Error returns the error message associated with the Call
+// Error returns the error message associated with the InvalidCustom
 func (i InvalidCustom) Error() string {
 	return newUnknownError(i).Error()
 }

lib/runtime/transaction_validity.go

@@ -50,7 +50,7 @@ func (tve TransactionValidityError) Error() string {
 
 // NewTransactionValidityError is constructor for TransactionValidityError
 func NewTransactionValidityError() *TransactionValidityError {
-	vdt, err := scale.NewVaryingDataType(NewInvalidTransaction(), NewUnknownTransaction())
+	vdt, err := scale.NewVaryingDataType(*NewInvalidTransaction(), NewUnknownTransaction())
 	if err != nil {
 		panic(err)
 	}
@@ -66,28 +66,28 @@ func UnmarshalTransactionValidity(res []byte) (*transaction.Validity, error) {
 	txnValidityResult := scale.NewResult(validTxn, *txnValidityErrResult)
 	err := scale.Unmarshal(res, &txnValidityResult)
 	if err != nil {
-		return nil, fmt.Errorf("scale decoding transaction validity result: %w", err)
+		return nil, fmt.Errorf("scale decoding transaction validity result: %s", err)
 	}
 	txnValidityRes, err := txnValidityResult.Unwrap()
 	if err != nil {
 		scaleWrappedErr, ok := err.(scale.WrappedErr)
-		if ok {
-			txnValidityErr, ok := scaleWrappedErr.Err.(TransactionValidityError)
-			if !ok {
-				return nil, fmt.Errorf("%w: %T", errors.New("invalid type cast"), scaleWrappedErr.Err)
-			}
+		if !ok {
+			return nil, fmt.Errorf("unwrapping transaction validity result: %s", err)
+		}
+		txnValidityErr, ok := scaleWrappedErr.Err.(TransactionValidityError)
+		if !ok {
+			return nil, fmt.Errorf("%w: %T", errors.New("invalid type cast"), scaleWrappedErr.Err)
+		}
 
-			switch val := txnValidityErr.Value().(type) {
-			// TODO use custom result issue #2780
-			case InvalidTransaction:
-				return nil, val
-			case UnknownTransaction:
-				return nil, val
-			default:
-				return nil, fmt.Errorf("unsupported transaction validity error: %T", txnValidityErr.Value())
-			}
+		switch val := txnValidityErr.Value().(type) {
+		// TODO use custom result issue #2780
+		case InvalidTransaction:
+			return nil, val
+		case UnknownTransaction:
+			return nil, val
+		default:
+			return nil, fmt.Errorf("unsupported transaction validity error: %T", txnValidityErr.Value())
 		}
-		return nil, fmt.Errorf("%w: %T", errors.New("invalid error value"), err)
 	}
 	validity, ok := txnValidityRes.(transaction.Validity)
 	if !ok {

lib/runtime/transaction_validity_test.go

@@ -4,25 +4,70 @@
 package runtime
 
 import (
-	"errors"
 	"testing"
 
+	"github.com/ChainSafe/gossamer/lib/transaction"
 	"github.com/ChainSafe/gossamer/pkg/scale"
 
 	"github.com/stretchr/testify/require"
 )
 
-func Test_ErrorsAs_Function(t *testing.T) {
-	transactionValidityErr := NewTransactionValidityError()
-	unknownTransaction := NewUnknownTransaction()
-	err := unknownTransaction.Set(NoUnsignedValidator{})
+func Test_UnmarshalTransactionValidity(t *testing.T) {
+	t.Parallel()
+
+	txtValidity := transaction.Validity{Priority: 1}
+	txnValidityResult := scale.NewResult(transaction.Validity{}, NewTransactionValidityError())
+	err := txnValidityResult.Set(scale.OK, txtValidity)
 	require.NoError(t, err)
-	err = transactionValidityErr.Set(unknownTransaction)
+	encResult, err := scale.Marshal(txnValidityResult)
 	require.NoError(t, err)
-
-	var txnValErr *TransactionValidityError
-	isTxnValErr := errors.As(transactionValidityErr, &txnValErr)
-	require.True(t, isTxnValErr)
+	testCases := []struct {
+		name        string
+		encodedData []byte
+		expErr      bool
+		expErrMsg   string
+		expValidity *transaction.Validity
+	}{
+		{
+			name:        "ancient birth block",
+			encodedData: []byte{1, 0, 5},
+			expErrMsg:   "ancient birth block",
+			expErr:      true,
+		},
+		{
+			name:        "lookup failed",
+			encodedData: []byte{1, 1, 0},
+			expErrMsg:   "lookup failed",
+			expErr:      true,
+		},
+		{
+			name:        "unmarshal error",
+			encodedData: []byte{1},
+			expErrMsg:   "scale decoding transaction validity result: EOF",
+			expErr:      true,
+		},
+		{
+			name:        "valid case",
+			encodedData: encResult,
+			expValidity: &txtValidity,
+		},
+	}
+
+	for _, c := range testCases {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			t.Parallel()
+			validity, err := UnmarshalTransactionValidity(c.encodedData)
+			if !c.expErr {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.EqualError(t, err, c.expErrMsg)
+			}
+
+			require.Equal(t, c.expValidity, validity)
+		})
+	}
 }
 
 func Test_InvalidTransactionValidity(t *testing.T) {

lib/runtime/unknown_transaction.go

@@ -9,7 +9,7 @@ import (
 	"github.com/ChainSafe/gossamer/pkg/scale"
 )
 
-// UnknownTransaction is child VDT of TransactionValidityError
+// UnknownTransaction is the child VDT of TransactionValidityError
 type UnknownTransaction scale.VaryingDataType
 
 // Index returns the VDT index
@@ -28,7 +28,7 @@ func (u *UnknownTransaction) Set(val scale.VaryingDataTypeValue) (err error) {
 	return nil
 }
 
-// Value will return value from underying VaryingDataType
+// Value will return value from the underying VaryingDataType
 func (u *UnknownTransaction) Value() (val scale.VaryingDataTypeValue) {
 	vdt := scale.VaryingDataType(*u)
 	return vdt.Value()

lib/runtime/wasmer/exports.go

@@ -13,11 +13,9 @@ import (
 )
 
 // ValidateTransaction runs the extrinsic through the runtime function
-// TaggedTransactionQueue_validate_transaction and returns *Validity. Two types of errors
-// are returned here: 1) *txnvalidity.transactionValidityError is a VDT containing
-// VDTs of the types of transaction validity errors. The whole VDT is returned so
-// the caller can handle as it seems fit, as this will vary per use case. 2) normal error
-// returned if something fails in the process i.e. unmarshalling error
+// TaggedTransactionQueue_validate_transaction and returns **transaction.Validity. The error can
+// be a VDT of either transaction.InvalidTransaction or transaction.UnknownTransaction, or can represent
+// a normal error i.e. unmarshalling error
 func (in *Instance) ValidateTransaction(e types.Extrinsic) (
 	*transaction.Validity, error) {
 	ret, err := in.Exec(runtime.TaggedTransactionQueueValidateTransaction, e)