Skip to content

Commit

Permalink
!fix(share/getters): return nil shares from all getters in non-inclus…
Browse files Browse the repository at this point in the history
…ion case (#3454)

Make return of shares consistent accross tall getter, when encountering
non-inclusion case
Improves unnecessary allocations of empty slice.

Resolves #3448
  • Loading branch information
walldiss authored and ramin committed Jun 6, 2024
1 parent f3dc94b commit ca3c8f1
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 5 deletions.
2 changes: 1 addition & 1 deletion share/getter.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type NamespacedShares []NamespacedRow

// Flatten returns the concatenated slice of all NamespacedRow shares.
func (ns NamespacedShares) Flatten() []Share {
shares := make([]Share, 0)
var shares []Share
for _, row := range ns {
shares = append(shares, row.Shares...)
}
Expand Down
4 changes: 2 additions & 2 deletions share/getters/getter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func TestStoreGetter(t *testing.T) {
randNamespace := sharetest.RandV0Namespace()
emptyShares, err := sg.GetSharesByNamespace(ctx, eh, randNamespace)
require.NoError(t, err)
require.Empty(t, emptyShares.Flatten())
require.Nil(t, emptyShares.Flatten())

// root not found
emptyRoot := da.MinDataAvailabilityHeader()
Expand Down Expand Up @@ -229,7 +229,7 @@ func TestIPLDGetter(t *testing.T) {
randNamespace := sharetest.RandV0Namespace()
emptyShares, err := sg.GetSharesByNamespace(ctx, eh, randNamespace)
require.NoError(t, err)
require.Empty(t, emptyShares.Flatten())
require.Nil(t, emptyShares.Flatten())

// nid doesn't exist in root
emptyRoot := da.MinDataAvailabilityHeader()
Expand Down
15 changes: 14 additions & 1 deletion share/getters/shrex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func TestShrexGetter(t *testing.T) {
Height: 1,
})

// namespace inside root range
nID, err := addToNamespace(maxNamespace, -1)
require.NoError(t, err)
// check for namespace to be between max and min namespace in root
Expand All @@ -118,7 +119,19 @@ func TestShrexGetter(t *testing.T) {
emptyShares, err := getter.GetSharesByNamespace(ctx, eh, nID)
require.NoError(t, err)
// no shares should be returned
require.Empty(t, emptyShares.Flatten())
require.Nil(t, emptyShares.Flatten())
require.Nil(t, emptyShares.Verify(dah, nID))

// namespace outside root range
nID, err = addToNamespace(maxNamespace, 1)
require.NoError(t, err)
// check for namespace to be not in root
require.Len(t, ipld.FilterRootByNamespace(dah, nID), 0)

emptyShares, err = getter.GetSharesByNamespace(ctx, eh, nID)
require.NoError(t, err)
// no shares should be returned
require.Nil(t, emptyShares.Flatten())
require.Nil(t, emptyShares.Verify(dah, nID))
})

Expand Down
5 changes: 4 additions & 1 deletion share/ipld/get_shares.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,17 @@ func GetSharesByNamespace(
}

leaves := data.Leaves()
if len(leaves) == 0 {
return nil, data.Proof(), nil
}

shares := make([]share.Share, len(leaves))
for i, leaf := range leaves {
if leaf != nil {
shares[i] = leafToShare(leaf)
}
}
return shares, data.Proof(), err
return shares, data.Proof(), nil
}

// leafToShare converts an NMT leaf into a Share.
Expand Down

0 comments on commit ca3c8f1

Please sign in to comment.