chore!: remove excess blobTx file and add more test coverage (#99)

I forgot to remove the blobTx file when moving it to it's own folder.
This PR deletes it and tidies up the imports. I also added a few tests
to the marshalling/unmarshalling which weren't previously covered

Author: cmwaters

builder.go

@@ -50,8 +50,8 @@ func NewBuilder(maxSquareSize int, subtreeRootThreshold int, txs ...[]byte) (*Bu
 		PfbCounter:           share.NewCompactShareCounter(),
 	}
 	seenFirstBlobTx := false
-	for idx, tx := range txs {
-		blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx)
+	for idx, txBytes := range txs {
+		blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes)
 		if err != nil && isBlobTx {
 			return nil, fmt.Errorf("unmarshalling blob tx at index %d: %w", idx, err)
 		}
@@ -64,7 +64,7 @@ func NewBuilder(maxSquareSize int, subtreeRootThreshold int, txs ...[]byte) (*Bu
 			if seenFirstBlobTx {
 				return nil, fmt.Errorf("normal tx at index %d can not be appended after blob tx", idx)
 			}
-			if !builder.AppendTx(tx) {
+			if !builder.AppendTx(txBytes) {
 				return nil, fmt.Errorf("not enough space to append tx at index %d", idx)
 			}
 		}
@@ -88,7 +88,7 @@ func (b *Builder) AppendTx(tx []byte) bool {
 
 // AppendBlobTx attempts to allocate the blob transaction to the square. It returns false if there is not
 // enough space in the square to fit the transaction.
-func (b *Builder) AppendBlobTx(blobTx *share.BlobTx) bool {
+func (b *Builder) AppendBlobTx(blobTx *tx.BlobTx) bool {
 	iw := tx.NewIndexWrapper(blobTx.Tx, worstCaseShareIndexes(len(blobTx.Blobs))...)
 	size := proto.Size(iw)
 	pfbShareDiff := b.PfbCounter.Add(size)

builder_test.go

@@ -86,7 +86,7 @@ func TestBuilderRejectsBlobTransactions(t *testing.T) {
 			require.NoError(t, err)
 			txs := generateBlobTxsWithNamespaces(ns1.Repeat(len(tc.blobSize)), [][]int{tc.blobSize})
 			require.Len(t, txs, 1)
-			blobTx, isBlobTx, err := share.UnmarshalBlobTx(txs[0])
+			blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txs[0])
 			require.NoError(t, err)
 			require.True(t, isBlobTx)
 			require.Equal(t, tc.added, builder.AppendBlobTx(blobTx))
@@ -119,11 +119,11 @@ func TestBuilderFindTxShareRange(t *testing.T) {
 	size := dataSquare.Size() * dataSquare.Size()
 
 	var lastEnd int
-	for idx, tx := range blockTxs {
-		blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx)
+	for idx, txBytes := range blockTxs {
+		blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes)
 		if isBlobTx {
 			require.NoError(t, err)
-			tx = blobTx.Tx
+			txBytes = blobTx.Tx
 		}
 		shareRange, err := builder.FindTxShareRange(idx)
 		require.NoError(t, err)
@@ -138,7 +138,7 @@ func TestBuilderFindTxShareRange(t *testing.T) {
 		txShares := dataSquare[shareRange.Start : shareRange.End+1]
 		parsedShares, err := rawData(txShares)
 		require.NoError(t, err)
-		require.True(t, bytes.Contains(parsedShares, tx))
+		require.True(t, bytes.Contains(parsedShares, txBytes))
 		lastEnd = shareRange.End
 	}
 }
@@ -294,8 +294,8 @@ func TestSquareBlobPostions(t *testing.T) {
 		t.Run(fmt.Sprintf("case%d", i), func(t *testing.T) {
 			builder, err := square.NewBuilder(tt.squareSize, defaultSubtreeRootThreshold)
 			require.NoError(t, err)
-			for _, tx := range tt.blobTxs {
-				blobTx, isBlobTx, err := share.UnmarshalBlobTx(tx)
+			for _, txBytes := range tt.blobTxs {
+				blobTx, isBlobTx, err := tx.UnmarshalBlobTx(txBytes)
 				require.NoError(t, err)
 				require.True(t, isBlobTx)
 				_ = builder.AppendBlobTx(blobTx)

inclusion/commitment.go

@@ -107,7 +107,7 @@ func MerkleMountainRangeSizes(totalSize, maxTreeSize uint64) ([]uint64, error) {
 	return treeSizes, nil
 }
 
-// SplitBlobs splits the provided blobs into shares.
+// splitBlobs splits the provided blobs into shares.
 func splitBlobs(blobs ...*sh.Blob) ([]sh.Share, error) {
 	writer := sh.NewSparseShareSplitter()
 	for _, blob := range blobs {

internal/test/factory.go

@@ -7,6 +7,7 @@ import (
 	"math/rand"
 
 	"github.com/celestiaorg/go-square/v2/share"
+	"github.com/celestiaorg/go-square/v2/tx"
 )
 
 var DefaultTestNamespace = share.MustNewV0Namespace([]byte("test"))
@@ -52,7 +53,7 @@ func GenerateBlobTxWithNamespace(namespaces []share.Namespace, blobSizes []int,
 			panic(err)
 		}
 	}
-	blobTx, err := share.MarshalBlobTx(MockPFB(toUint32(blobSizes)), blobs...)
+	blobTx, err := tx.MarshalBlobTx(MockPFB(toUint32(blobSizes)), blobs...)
 	if err != nil {
 		panic(err)
 	}

share/blob.go

@@ -62,6 +62,18 @@ func UnmarshalBlob(blob []byte) (*Blob, error) {
 	return NewBlobFromProto(pb)
 }
 
+// Marshal marshals the blob to the proto encoded bytes
+func (b *Blob) Marshal() ([]byte, error) {
+	pb := &v1.BlobProto{
+		NamespaceId:      b.namespace.ID(),
+		NamespaceVersion: uint32(b.namespace.Version()),
+		ShareVersion:     uint32(b.shareVersion),
+		Data:             b.data,
+		Signer:           b.signer,
+	}
+	return proto.Marshal(pb)
+}
+
 // NewBlobFromProto creates a new blob from the proto generated type
 func NewBlobFromProto(pb *v1.BlobProto) (*Blob, error) {
 	if pb.NamespaceVersion > NamespaceVersionMax {
@@ -102,16 +114,9 @@ func (b *Blob) Data() []byte {
 	return b.data
 }
 
-// Marshal marshals the blob to the proto encoded bytes
-func (b *Blob) Marshal() ([]byte, error) {
-	pb := &v1.BlobProto{
-		NamespaceId:      b.namespace.ID(),
-		NamespaceVersion: uint32(b.namespace.Version()),
-		ShareVersion:     uint32(b.shareVersion),
-		Data:             b.data,
-		Signer:           b.signer,
-	}
-	return proto.Marshal(pb)
+// DataLen returns the length of the data of the blob
+func (b *Blob) DataLen() int {
+	return len(b.data)
 }
 
 // Compare is used to order two blobs based on their namespace

share/blob_test.go

@@ -0,0 +1,142 @@
+package share
+
+import (
+	"bytes"
+	"crypto/rand"
+	"testing"
+
+	v1 "github.com/celestiaorg/go-square/v2/proto/blob/v1"
+	"github.com/stretchr/testify/require"
+)
+
+func TestProtoEncoding(t *testing.T) {
+	signer := make([]byte, 20)
+	_, err := rand.Read(signer)
+	require.NoError(t, err)
+	blob, err := NewBlob(RandomNamespace(), []byte{1, 2, 3, 4, 5}, 1, signer)
+	require.NoError(t, err)
+
+	blobBytes, err := blob.Marshal()
+	require.NoError(t, err)
+
+	newBlob, err := UnmarshalBlob(blobBytes)
+	require.NoError(t, err)
+
+	require.Equal(t, blob, newBlob)
+}
+
+func TestBlobConstructor(t *testing.T) {
+	signer := make([]byte, 20)
+	_, err := rand.Read(signer)
+	require.NoError(t, err)
+
+	ns := RandomNamespace()
+	data := []byte{1, 2, 3, 4, 5}
+
+	// test all invalid cases
+	_, err = NewBlob(ns, data, 0, signer)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "share version 0 does not support signer")
+
+	_, err = NewBlob(ns, nil, 0, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "data can not be empty")
+
+	_, err = NewBlob(ns, data, 1, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "share version 1 requires signer of size")
+
+	_, err = NewBlob(ns, data, 128, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "share version can not be greater than MaxShareVersion")
+}
+
+func TestNewBlobFromProto(t *testing.T) {
+	namespace := RandomNamespace()
+	testCases := []struct {
+		name        string
+		proto       *v1.BlobProto
+		expectedErr string
+	}{
+		{
+			name: "valid blob",
+			proto: &v1.BlobProto{
+				NamespaceId:      namespace.ID(),
+				NamespaceVersion: uint32(namespace.Version()),
+				ShareVersion:     0,
+				Data:             []byte{1, 2, 3, 4, 5},
+			},
+			expectedErr: "",
+		},
+		{
+			name: "invalid namespace version",
+			proto: &v1.BlobProto{
+				NamespaceId:      namespace.ID(),
+				NamespaceVersion: 256,
+				ShareVersion:     0,
+				Data:             []byte{1, 2, 3, 4, 5},
+			},
+			expectedErr: "namespace version can not be greater than MaxNamespaceVersion",
+		},
+		{
+			name: "empty data",
+			proto: &v1.BlobProto{
+				NamespaceId:      namespace.ID(),
+				NamespaceVersion: 0,
+				ShareVersion:     0,
+				Data:             []byte{},
+			},
+			expectedErr: "blob data can not be empty",
+		},
+		{
+			name: "invalid namespace ID length",
+			proto: &v1.BlobProto{
+				NamespaceId:      []byte{1, 2, 3},
+				NamespaceVersion: 0,
+				ShareVersion:     0,
+				Data:             []byte{1, 2, 3, 4, 5},
+			},
+			expectedErr: "invalid namespace",
+		},
+		{
+			name: "valid blob with signer",
+			proto: &v1.BlobProto{
+				NamespaceId:      namespace.ID(),
+				NamespaceVersion: 0,
+				ShareVersion:     1,
+				Data:             []byte{1, 2, 3, 4, 5},
+				Signer:           bytes.Repeat([]byte{1}, SignerSize),
+			},
+			expectedErr: "",
+		},
+		{
+			name: "invalid signer length",
+			proto: &v1.BlobProto{
+				NamespaceId:      namespace.ID(),
+				NamespaceVersion: 0,
+				ShareVersion:     1,
+				Data:             []byte{1, 2, 3, 4, 5},
+				Signer:           []byte{1, 2, 3},
+			},
+			expectedErr: "share version 1 requires signer of size",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			blob, err := NewBlobFromProto(tc.proto)
+			if tc.expectedErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectedErr)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, blob)
+				require.Equal(t, tc.proto.NamespaceId, blob.Namespace().ID())
+				require.Equal(t, uint8(tc.proto.NamespaceVersion), blob.Namespace().Version())
+				require.Equal(t, uint8(tc.proto.ShareVersion), blob.ShareVersion())
+				require.Equal(t, tc.proto.Data, blob.Data())
+				require.Equal(t, tc.proto.Signer, blob.Signer())
+			}
+		})
+	}
+}