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.

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

feat!: refactor Namespace type #68

Merged
merged 11 commits into from
Jun 14, 2024
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
go.work
go.work.sum
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this, considering #67 was merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, else it's going to see this as a diff between the users branch and main and will want to check it in

25 changes: 17 additions & 8 deletions blob/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,24 @@ const MaxShareVersion = 127
// basic stateless checks over it.
func New(ns namespace.Namespace, blob []byte, shareVersion uint8) *Blob {
return &Blob{
NamespaceId: ns.ID,
NamespaceId: ns.ID(),
Copy link
Member

Choose a reason for hiding this comment

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

This is a nit but I think it should be NamespaceID

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the proto generated struct and I don't intend to break that but I will keep your suggestion for when we create a go-native Blob implementation

Data: blob,
ShareVersion: uint32(shareVersion),
NamespaceVersion: uint32(ns.Version),
NamespaceVersion: uint32(ns.Version()),
walldiss marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Namespace returns the namespace of the blob
func (b *Blob) Namespace() namespace.Namespace {
return namespace.Namespace{
Version: uint8(b.NamespaceVersion),
ID: b.NamespaceId,
}
func (b *Blob) Namespace() (namespace.Namespace, error) {
return namespace.NewFromBytes(b.RawNamespace())
}
walldiss marked this conversation as resolved.
Show resolved Hide resolved

// Namespace returns the namespace of the blob
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
func (b *Blob) RawNamespace() []byte {
namespace := make([]byte, namespace.NamespaceSize)
namespace[0] = uint8(b.NamespaceVersion)
cristaloleg marked this conversation as resolved.
Show resolved Hide resolved
copy(namespace[1:], b.NamespaceId)
return namespace
}

// Validate runs a stateless validity check on the form of the struct.
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -65,6 +70,10 @@ func (b *Blob) Validate() error {
return nil
}

func (b *Blob) Compare(other *Blob) int {
return bytes.Compare(b.RawNamespace(), other.RawNamespace())
}
Comment on lines +73 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Usually, such methods are called Equals with the bool return value, but as long as this is not PR targeting Blob we can omit that and this method altogether

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thats improvement over that comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually thinking comparing is more important because it's more common to want to order these things then see if the namespaces are equal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course nothing wrong with having both


// UnmarshalBlobTx attempts to unmarshal a transaction into blob transaction. If an
// error is thrown, false is returned.
func UnmarshalBlobTx(tx []byte) (*BlobTx, bool) {
Expand Down Expand Up @@ -105,7 +114,7 @@ func MarshalBlobTx(tx []byte, blobs ...*Blob) ([]byte, error) {
// Sort sorts the blobs by their namespace.
func Sort(blobs []*Blob) {
sort.SliceStable(blobs, func(i, j int) bool {
return bytes.Compare(blobs[i].Namespace().Bytes(), blobs[j].Namespace().Bytes()) < 0
return blobs[i].Compare(blobs[j]) < 0
})
}

Expand Down
5 changes: 4 additions & 1 deletion inclusion/commitment.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ func CreateCommitment(blob *blob.Blob, merkleRootFn MerkleRootFn, subtreeRootThr
if err := blob.Validate(); err != nil {
return nil, err
}
namespace := blob.Namespace()
namespace, err := blob.Namespace()
if err != nil {
return nil, err
}

shares, err := sh.SplitBlobs(blob)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions inclusion/commitment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,10 @@ func TestCreateCommitment(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
blob := &blob.Blob{
NamespaceId: tt.namespace.ID,
NamespaceId: tt.namespace.ID(),
Data: tt.blob,
ShareVersion: uint32(tt.shareVersion),
NamespaceVersion: uint32(tt.namespace.Version),
NamespaceVersion: uint32(tt.namespace.Version()),
}
res, err := inclusion.CreateCommitment(blob, twoLeafMerkleRoot, defaultSubtreeRootThreshold)
if tt.expectErr {
Expand Down
10 changes: 2 additions & 8 deletions namespace/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,9 @@ var (
)

func primaryReservedNamespace(lastByte byte) Namespace {
return Namespace{
Version: NamespaceVersionZero,
ID: append(bytes.Repeat([]byte{0x00}, NamespaceIDSize-1), lastByte),
}
return newNamespace(NamespaceVersionZero, append(bytes.Repeat([]byte{0x00}, NamespaceIDSize-1), lastByte))
Copy link
Member

Choose a reason for hiding this comment

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

nit but NamespaceVersionZero vs NamespaceVersionMax ? Can we make NamespaceVersionZero --> NamespaceVersionMin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we were to go back we would probably start user namespaces at v1 and have NamespaceVersionMin as 0 for the primary reserved namespaces. For now v0 is both used by external users and by us. I think it would be more confusing to call it version min

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunate but ok

}

func secondaryReservedNamespace(lastByte byte) Namespace {
return Namespace{
Version: NamespaceVersionMax,
ID: append(bytes.Repeat([]byte{0xFF}, NamespaceIDSize-1), lastByte),
}
return newNamespace(NamespaceVersionMax, append(bytes.Repeat([]byte{0xFF}, NamespaceIDSize-1), lastByte))
}
79 changes: 50 additions & 29 deletions namespace/namespace.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import (
)

type Namespace struct {
Version uint8
ID []byte
data []byte
Copy link
Member

Choose a reason for hiding this comment

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

One option we haven't discussed yet is making Namespace map friendly by keeping this byte slice as a string(basically string(data)). I haven't seen this being needed that often, but intuitively one would wanna map namespaces to Blobs or else sometimes.

No actionable here, but a discussion point we haven't touched previously.

}

// New returns a new namespace with the provided version and id.
Wondertan marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -23,10 +22,16 @@ func New(version uint8, id []byte) (Namespace, error) {
return Namespace{}, err
}

return newNamespace(version, id), nil
}

func newNamespace(version uint8, id []byte) Namespace {
data := make([]byte, 1+len(id))
Copy link
Member

Choose a reason for hiding this comment

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

nit but do you want to point at NamespaceVersionSize + len(id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I missed these parts when going through it again

data[0] = version
copy(data[1:], id)
Copy link
Member

Choose a reason for hiding this comment

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

nit: NamespaceVersionIndex? for readability

return Namespace{
Version: version,
ID: id,
}, nil
data: data,
}
}

// MustNew returns a new namespace with the provided version and id. It panics
Expand All @@ -39,6 +44,27 @@ func MustNew(version uint8, id []byte) Namespace {
return ns
}

// NewFromBytes returns a new namespace from the provided byte slice.
func NewFromBytes(bytes []byte) (Namespace, error) {
if len(bytes) != NamespaceSize {
return Namespace{}, fmt.Errorf("invalid namespace length: %v must be %v", len(bytes), NamespaceSize)
}

err := validateVersionSupported(bytes[0])
if err != nil {
return Namespace{}, err
cristaloleg marked this conversation as resolved.
Show resolved Hide resolved
}

err = validateID(bytes[0], bytes[1:])
walldiss marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return Namespace{}, err
}

return Namespace{
data: bytes,
}, nil
}

// NewV0 returns a new namespace with version 0 and the provided subID. subID
// must be <= 10 bytes. If subID is < 10 bytes, it will be left-padded with 0s
// to fill 10 bytes.
Expand Down Expand Up @@ -70,18 +96,24 @@ func MustNewV0(subID []byte) Namespace {
}

// From returns a namespace from the provided byte slice.
// Deprecated: Please use NewFromBytes instead.
func From(b []byte) (Namespace, error) {
if len(b) != NamespaceSize {
return Namespace{}, fmt.Errorf("invalid namespace length: %v must be %v", len(b), NamespaceSize)
}
rawVersion := b[0]
rawNamespace := b[1:]
return New(rawVersion, rawNamespace)
return NewFromBytes(b)
}

// Bytes returns this namespace as a byte slice.
func (n Namespace) Bytes() []byte {
return append([]byte{n.Version}, n.ID...)
return n.data
}

// Version return this namespace's version
func (n Namespace) Version() uint8 {
return n.data[0]
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
}

// ID returns this namespace's ID
func (n Namespace) ID() []byte {
return n.data[1:]
}

// validateVersionSupported returns an error if the version is not supported.
Expand Down Expand Up @@ -147,7 +179,7 @@ func (n Namespace) Repeat(times int) []Namespace {
}

func (n Namespace) Equals(n2 Namespace) bool {
return n.Version == n2.Version && bytes.Equal(n.ID, n2.ID)
return bytes.Equal(n.data, n2.data)
}

func (n Namespace) IsLessThan(n2 Namespace) bool {
Expand All @@ -167,14 +199,7 @@ func (n Namespace) IsGreaterOrEqualThan(n2 Namespace) bool {
}

func (n Namespace) Compare(n2 Namespace) int {
switch {
case n.Version == n2.Version:
return bytes.Compare(n.ID, n2.ID)
case n.Version < n2.Version:
return -1
default:
return 1
}
return bytes.Compare(n.data, n2.data)
}

// leftPad returns a new byte slice with the provided byte slice left-padded to the provided size.
Expand All @@ -190,14 +215,10 @@ func leftPad(b []byte, size int) []byte {
// deepCopy returns a deep copy of the Namespace object.
func (n Namespace) deepCopy() Namespace {
// Create a deep copy of the ID slice
copyID := make([]byte, len(n.ID))
copy(copyID, n.ID)
copyData := make([]byte, len(n.data))
copy(copyData, n.data)

// Create a new Namespace object with the copied fields
copyNamespace := Namespace{
Version: n.Version,
ID: copyID,
return Namespace{
data: copyData,
}

return copyNamespace
}
57 changes: 13 additions & 44 deletions namespace/namespace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@ func TestNew(t *testing.T) {
version: NamespaceVersionZero,
id: validID,
wantErr: false,
want: Namespace{
Version: NamespaceVersionZero,
ID: validID,
},
want: MustNew(NamespaceVersionZero, validID),
},
{
name: "unsupported version",
Expand Down Expand Up @@ -76,19 +73,6 @@ func TestNew(t *testing.T) {
}
}

// TestRepeatNonMutability ensures that the output of Repeat method is not mutated when the original namespace is mutated.
func TestRepeatNonMutability(t *testing.T) {
n := 10
namespace := Namespace{Version: NamespaceVersionMax, ID: []byte{1, 2, 3, 4}}
repeated := namespace.Repeat(n)
// mutate the original namespace
namespace.ID[0] = 5
// ensure the repeated namespaces are not mutated
for i := 0; i < n; i++ {
assert.NotEqual(t, repeated[i], namespace)
}
}

func TestNewV0(t *testing.T) {
type testCase struct {
name string
Expand All @@ -99,21 +83,15 @@ func TestNewV0(t *testing.T) {

testCases := []testCase{
{
name: "valid namespace",
subID: bytes.Repeat([]byte{1}, NamespaceVersionZeroIDSize),
want: Namespace{
Version: NamespaceVersionZero,
ID: append(NamespaceVersionZeroPrefix, bytes.Repeat([]byte{1}, NamespaceVersionZeroIDSize)...),
},
name: "valid namespace",
subID: bytes.Repeat([]byte{1}, NamespaceVersionZeroIDSize),
want: MustNew(NamespaceVersionZero, append(NamespaceVersionZeroPrefix, bytes.Repeat([]byte{1}, NamespaceVersionZeroIDSize)...)),
wantErr: false,
},
{
name: "left pads subID if too short",
subID: []byte{1, 2, 3, 4},
want: Namespace{
Version: NamespaceVersionZero,
ID: append(NamespaceVersionZeroPrefix, []byte{0, 0, 0, 0, 0, 0, 1, 2, 3, 4}...),
},
name: "left pads subID if too short",
subID: []byte{1, 2, 3, 4},
want: MustNew(NamespaceVersionZero, append(NamespaceVersionZeroPrefix, []byte{0, 0, 0, 0, 0, 0, 1, 2, 3, 4}...)),
wantErr: false,
},
{
Expand Down Expand Up @@ -153,19 +131,13 @@ func TestFrom(t *testing.T) {
name: "valid namespace",
bytes: validNamespace,
wantErr: false,
want: Namespace{
Version: NamespaceVersionZero,
ID: validID,
},
want: MustNew(NamespaceVersionZero, validID),
},
{
name: "parity namespace",
bytes: parityNamespace,
wantErr: false,
want: Namespace{
Version: NamespaceVersionMax,
ID: bytes.Repeat([]byte{0xFF}, NamespaceIDSize),
},
want: MustNew(NamespaceVersionMax, bytes.Repeat([]byte{0xFF}, NamespaceIDSize)),
},
{
name: "unsupported version",
Expand Down Expand Up @@ -285,10 +257,7 @@ func TestIsReserved(t *testing.T) {
want: true,
},
{
ns: Namespace{
Version: math.MaxUint8,
ID: append(bytes.Repeat([]byte{0xFF}, NamespaceIDSize-1), 1),
},
ns: MustNew(math.MaxUint8, append(bytes.Repeat([]byte{0xFF}, NamespaceIDSize-1), 1)),
want: true,
},
}
Expand All @@ -308,7 +277,7 @@ func Test_compareMethods(t *testing.T) {
}

vers := []byte{NamespaceVersionZero, NamespaceVersionMax}
ids := [][]byte{minID, maxID}
ids := [][]byte{append(NamespaceVersionZeroPrefix, minID...), append(NamespaceVersionZeroPrefix, maxID...)}

// collect all possible pairs: (ver1 ?? ver2) x (id1 ?? id2)
var testPairs [][2]Namespace
Expand All @@ -317,8 +286,8 @@ func Test_compareMethods(t *testing.T) {
for _, id1 := range ids {
for _, id2 := range ids {
testPairs = append(testPairs, [2]Namespace{
{Version: ver1, ID: id1},
{Version: ver2, ID: id2},
MustNew(ver1, id1),
MustNew(ver2, id2),
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion namespace/random_blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func isBlobNamespace(ns Namespace) bool {
return false
}

if !slices.Contains(SupportedBlobNamespaceVersions, ns.Version) {
if !slices.Contains(SupportedBlobNamespaceVersions, ns.Version()) {
return false
}

Expand Down
5 changes: 4 additions & 1 deletion shares/split_sparse_shares.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ func (sss *SparseShareSplitter) Write(blob *blob.Blob) error {
}

rawData := blob.Data
blobNamespace := blob.Namespace()
blobNamespace, err := blob.Namespace()
if err != nil {
return err
}

// First share (note by validating the blob we can safely cast the share version to uint8)
b, err := NewBuilder(blobNamespace, uint8(blob.ShareVersion), true)
Expand Down
4 changes: 3 additions & 1 deletion square/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ func (b *Builder) Export() (Square, error) {
// of blobs within a namespace because b.Blobs are already ordered by tx
// priority.
sort.SliceStable(b.Blobs, func(i, j int) bool {
return bytes.Compare(b.Blobs[i].Blob.Namespace().Bytes(), b.Blobs[j].Blob.Namespace().Bytes()) < 0
ns1 := append([]byte{byte(b.Blobs[i].Blob.NamespaceVersion)}, b.Blobs[i].Blob.NamespaceId...)
ns2 := append([]byte{byte(b.Blobs[j].Blob.NamespaceVersion)}, b.Blobs[j].Blob.NamespaceId...)
return bytes.Compare(ns1, ns2) < 0
})

// write all the regular transactions into compact shares
Expand Down
Loading