Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor!: remove builder return param from init #2165

Merged
merged 6 commits into from
Jul 27, 2023
Merged

refactor!: remove builder return param from init #2165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0eea0e5
refactor!: remove builder return param from init
rootulp Jul 25, 2023
16fbdfe
fix: lint
rootulp Jul 25, 2023
cb86e48
refactor: call init() from NewBuilder
rootulp Jul 26, 2023
1deec50
fix godoc
rootulp Jul 26, 2023
109bff6
more consise error handling
rootulp Jul 26, 2023
6f32b96
Merge branch 'main' into rp/refactor-init
rootulp Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/shares/padding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
// provided should be the namespace and shareVersion of the blob that precedes
// this padding in the data square.
func NamespacePaddingShare(ns appns.Namespace, shareVersion uint8) (Share, error) {
b, err := NewBuilder(ns, shareVersion, true).Init()
b, err := NewBuilder(ns, shareVersion, true)
if err != nil {
return Share{}, err
}
Expand Down
24 changes: 11 additions & 13 deletions pkg/shares/share_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,26 @@ func NewEmptyBuilder() *Builder {
}
}

// Init() needs to be called right after this method
func NewBuilder(ns appns.Namespace, shareVersion uint8, isFirstShare bool) *Builder {
return &Builder{
// NewBuilder returns a new share builder.
func NewBuilder(ns appns.Namespace, shareVersion uint8, isFirstShare bool) (*Builder, error) {
b := Builder{
namespace: ns,
shareVersion: shareVersion,
isFirstShare: isFirstShare,
isCompactShare: isCompactShare(ns),
}
if err := b.init(); err != nil {
return nil, err
}
return &b, nil
}

func (b *Builder) Init() (*Builder, error) {
// init initializes the share builder by populating rawShareData.
func (b *Builder) init() error {
if b.isCompactShare {
if err := b.prepareCompactShare(); err != nil {
return nil, err
}
} else {
if err := b.prepareSparseShare(); err != nil {
return nil, err
}
return b.prepareCompactShare()
}

return b, nil
return b.prepareSparseShare()
}

func (b *Builder) AvailableBytes() int {
Expand Down
58 changes: 29 additions & 29 deletions pkg/shares/share_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,58 +23,56 @@ func TestShareBuilderIsEmptyShare(t *testing.T) {
testCases := []testCase{
{
name: "first compact share empty",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, true),
data: nil,
want: true,
},
{
name: "first compact share not empty",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, true),
data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
want: false,
},
{
name: "first sparse share empty",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, true),
data: nil,
want: true,
},
{
name: "first sparse share not empty",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, true),
data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
want: false,
},
{
name: "continues compact share empty",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, false),
data: nil,
want: true,
},
{
name: "continues compact share not empty",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, false),
data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
want: false,
},
{
name: "continues sparse share not empty",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, false),
data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
want: false,
},
{
name: "continues sparse share empty",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, false),
data: nil,
want: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, err := tc.builder.Init()
require.NoError(t, err)
tc.builder.AddData(tc.data)
assert.Equal(t, tc.want, tc.builder.IsEmptyShare())
})
Expand All @@ -93,31 +91,31 @@ func TestShareBuilderWriteSequenceLen(t *testing.T) {
testCases := []testCase{
{
name: "first share",
builder: NewBuilder(ns1, 1, true),
builder: mustNewBuilder(t, ns1, 1, true),
wantLen: 10,
wantErr: false,
},
{
name: "first share with long sequence",
builder: NewBuilder(ns1, 1, true),
builder: mustNewBuilder(t, ns1, 1, true),
wantLen: 323,
wantErr: false,
},
{
name: "continuation sparse share",
builder: NewBuilder(ns1, 1, false),
builder: mustNewBuilder(t, ns1, 1, false),
wantLen: 10,
wantErr: true,
},
{
name: "compact share",
builder: NewBuilder(appns.TxNamespace, 1, true),
builder: mustNewBuilder(t, appns.TxNamespace, 1, true),
wantLen: 10,
wantErr: false,
},
{
name: "continuation compact share",
builder: NewBuilder(ns1, 1, false),
builder: mustNewBuilder(t, ns1, 1, false),
wantLen: 10,
wantErr: true,
},
Expand All @@ -131,8 +129,6 @@ func TestShareBuilderWriteSequenceLen(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, err := tc.builder.Init()
require.NoError(t, err)
if err := tc.builder.WriteSequenceLen(tc.wantLen); tc.wantErr {
assert.Error(t, err)
return
Expand Down Expand Up @@ -162,65 +158,62 @@ func TestShareBuilderAddData(t *testing.T) {
testCases := []testCase{
{
name: "small share",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, true),
data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
want: nil,
},
{
name: "exact fit first compact share",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, true),
data: bytes.Repeat([]byte{1}, appconsts.ShareSize-appconsts.NamespaceSize-appconsts.ShareInfoBytes-appconsts.CompactShareReservedBytes-appconsts.SequenceLenBytes),
want: nil,
},
{
name: "exact fit first sparse share",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, true),
data: bytes.Repeat([]byte{1}, appconsts.ShareSize-appconsts.NamespaceSize-appconsts.SequenceLenBytes-1 /*1 = info byte*/),
want: nil,
},
{
name: "exact fit continues compact share",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, false),
data: bytes.Repeat([]byte{1}, appconsts.ShareSize-appconsts.NamespaceSize-appconsts.CompactShareReservedBytes-1 /*1 = info byte*/),
want: nil,
},
{
name: "exact fit continues sparse share",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, false),
data: bytes.Repeat([]byte{1}, appconsts.ShareSize-appconsts.NamespaceSize-1 /*1 = info byte*/),
want: nil,
},
{
name: "oversize first compact share",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, true),
data: bytes.Repeat([]byte{1}, 1 /*1 extra byte*/ +appconsts.ShareSize-appconsts.NamespaceSize-appconsts.CompactShareReservedBytes-appconsts.SequenceLenBytes-1 /*1 = info byte*/),
want: []byte{1},
},
{
name: "oversize first sparse share",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, true),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, true),
data: bytes.Repeat([]byte{1}, 1 /*1 extra byte*/ +appconsts.ShareSize-appconsts.NamespaceSize-appconsts.SequenceLenBytes-1 /*1 = info byte*/),
want: []byte{1},
},
{
name: "oversize continues compact share",
builder: NewBuilder(appns.TxNamespace, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, appns.TxNamespace, appconsts.ShareVersionZero, false),
data: bytes.Repeat([]byte{1}, 1 /*1 extra byte*/ +appconsts.ShareSize-appconsts.NamespaceSize-appconsts.CompactShareReservedBytes-1 /*1 = info byte*/),
want: []byte{1},
},
{
name: "oversize continues sparse share",
builder: NewBuilder(ns1, appconsts.ShareVersionZero, false),
builder: mustNewBuilder(t, ns1, appconsts.ShareVersionZero, false),
data: bytes.Repeat([]byte{1}, 1 /*1 extra byte*/ +appconsts.ShareSize-appconsts.NamespaceSize-1 /*1 = info byte*/),
want: []byte{1},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, err := tc.builder.Init()
require.NoError(t, err)

got := tc.builder.AddData(tc.data)
assert.Equal(t, tc.want, got)
})
Expand Down Expand Up @@ -318,3 +311,10 @@ func TestShareBuilderImportRawData(t *testing.T) {
})
}
}

// mustNewBuilder returns a new builder with the given parameters. It fails the test if an error is encountered.
func mustNewBuilder(t *testing.T, ns appns.Namespace, shareVersion uint8, isFirstShare bool) *Builder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart idea

b, err := NewBuilder(ns, shareVersion, isFirstShare)
require.NoError(t, err)
return b
}
6 changes: 3 additions & 3 deletions pkg/shares/split_compact_shares.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type CompactShareSplitter struct {
// NewCompactShareSplitter returns a CompactShareSplitter using the provided
// namespace and shareVersion.
func NewCompactShareSplitter(ns appns.Namespace, shareVersion uint8) *CompactShareSplitter {
sb, err := NewBuilder(ns, shareVersion, true).Init()
sb, err := NewBuilder(ns, shareVersion, true)
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -105,7 +105,7 @@ func (css *CompactShareSplitter) stackPending() error {
css.shares = append(css.shares, *pendingShare)

// Now we need to create a new builder
css.shareBuilder, err = NewBuilder(css.namespace, css.shareVersion, false).Init()
css.shareBuilder, err = NewBuilder(css.namespace, css.shareVersion, false)
return err
}

Expand Down Expand Up @@ -163,7 +163,7 @@ func (css *CompactShareSplitter) writeSequenceLen(sequenceLen uint32) error {
}

// We may find a more efficient way to write seqLen
b, err := NewBuilder(css.namespace, css.shareVersion, true).Init()
b, err := NewBuilder(css.namespace, css.shareVersion, true)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/shares/split_sparse_shares.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (sss *SparseShareSplitter) Write(blob coretypes.Blob) error {
}

// First share
b, err := NewBuilder(blobNamespace, blob.ShareVersion, true).Init()
b, err := NewBuilder(blobNamespace, blob.ShareVersion, true)
if err != nil {
return err
}
Expand All @@ -57,7 +57,7 @@ func (sss *SparseShareSplitter) Write(blob coretypes.Blob) error {
}
sss.shares = append(sss.shares, *share)

b, err = NewBuilder(blobNamespace, blob.ShareVersion, false).Init()
b, err = NewBuilder(blobNamespace, blob.ShareVersion, false)
if err != nil {
return err
}
Expand Down
Loading