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

39 changes: 24 additions & 15 deletions blob/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import (
"fmt"
"sort"

"github.com/celestiaorg/go-square/namespace"
ns "github.com/celestiaorg/go-square/namespace"
"google.golang.org/protobuf/proto"
)

// SupportedBlobNamespaceVersions is a list of namespace versions that can be specified by a user for blobs.
var SupportedBlobNamespaceVersions = []uint8{namespace.NamespaceVersionZero}
var SupportedBlobNamespaceVersions = []uint8{ns.NamespaceVersionZero}

// ProtoBlobTxTypeID is included in each encoded BlobTx to help prevent
// decoding binaries that are not actually BlobTxs.
Expand All @@ -28,35 +28,40 @@ const MaxShareVersion = 127

// New creates a new coretypes.Blob from the provided data after performing
// basic stateless checks over it.
func New(ns namespace.Namespace, blob []byte, shareVersion uint8) *Blob {
func New(ns ns.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() (ns.Namespace, error) {
return ns.NewFromBytes(b.RawNamespace())
}

// RawNamespace returns the namespace of the blob
func (b *Blob) RawNamespace() []byte {
namespace := make([]byte, ns.NamespaceSize)
namespace[ns.VersionIndex] = uint8(b.NamespaceVersion)
copy(namespace[ns.NamespaceVersionSize:], 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
func (b *Blob) Validate() error {
if b == nil {
return errors.New("nil blob")
}
if len(b.NamespaceId) != namespace.NamespaceIDSize {
return fmt.Errorf("namespace id must be %d bytes", namespace.NamespaceIDSize)
if len(b.NamespaceId) != ns.NamespaceIDSize {
return fmt.Errorf("namespace id must be %d bytes", ns.NamespaceIDSize)
}
if b.ShareVersion > MaxShareVersion {
return errors.New("share version can not be greater than MaxShareVersion")
}
if b.NamespaceVersion > namespace.NamespaceVersionMax {
if b.NamespaceVersion > ns.NamespaceVersionMax {
return errors.New("namespace version can not be greater than MaxNamespaceVersion")
}
if len(b.Data) == 0 {
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 All @@ -81,7 +90,7 @@ func UnmarshalBlobTx(tx []byte) (*BlobTx, bool) {
return &bTx, false
}
for _, b := range bTx.Blobs {
if len(b.NamespaceId) != namespace.NamespaceIDSize {
if len(b.NamespaceId) != ns.NamespaceIDSize {
return &bTx, false
}
}
Expand All @@ -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
14 changes: 6 additions & 8 deletions namespace/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const (
// NamespaceVersionSize is the size of a namespace version in bytes.
NamespaceVersionSize = 1

// VersionIndex is the index of the version in the namespace. This should
// always be the first byte
VersionIndex = 0

// NamespaceIDSize is the size of a namespace ID in bytes.
NamespaceIDSize = 28

Expand Down Expand Up @@ -69,15 +73,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))
}
91 changes: 53 additions & 38 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, NamespaceVersionSize+len(id))
data[VersionIndex] = version
copy(data[NamespaceVersionSize:], id)
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[VersionIndex])
if err != nil {
return Namespace{}, err
cristaloleg marked this conversation as resolved.
Show resolved Hide resolved
}

err = validateID(bytes[VersionIndex], bytes[NamespaceVersionSize:])
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 All @@ -47,16 +73,10 @@ func NewV0(subID []byte) (Namespace, error) {
return Namespace{}, fmt.Errorf("subID must be <= %v, but it was %v bytes", NamespaceVersionZeroIDSize, lenSubID)
}

subID = leftPad(subID, NamespaceVersionZeroIDSize)
id := make([]byte, NamespaceIDSize)
copy(id[NamespaceVersionZeroPrefixSize:], subID)

ns, err := New(NamespaceVersionZero, id)
if err != nil {
return Namespace{}, err
}
namespace := make([]byte, NamespaceSize)
copy(namespace[NamespaceSize-len(subID):], subID)

return ns, nil
return NewFromBytes(namespace)
}

// MustNewV0 returns a new namespace with version 0 and the provided subID. This
Expand All @@ -70,18 +90,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[VersionIndex]
}

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

// validateVersionSupported returns an error if the version is not supported.
Expand Down Expand Up @@ -147,7 +173,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 +193,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 +209,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
}
Loading
Loading