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

Canonicalise Namespace and Share types #66

Closed
4 tasks
cmwaters opened this issue Jun 10, 2024 · 1 comment
Closed
4 tasks

Canonicalise Namespace and Share types #66

cmwaters opened this issue Jun 10, 2024 · 1 comment
Assignees

Comments

@cmwaters
Copy link
Collaborator

cmwaters commented Jun 10, 2024

Summary

We have different constructions of the Share and Namespace types across celestia repos. We should converge on types and have them standardised across repos.

https://github.com/celestiaorg/celestia-node/blob/762bd93b1b6242bdd5660172d1b32fcade144a8c/share/namespace.go#L34

type Namespace struct {
Version uint8
ID []byte
}

https://github.com/celestiaorg/celestia-node/blob/762bd93b1b6242bdd5660172d1b32fcade144a8c/share/share.go#L29

type Share struct {
data []byte
}

Proposal

Agreed upon types:

type Namespace struct {
    data []byte
}

type Share struct {
    data []byte
}

We have decided to encapsulate the types as this completely prevents users from misusing them (i.e. incorrect length).

Encapsulation also allows us to make changes in the future that don't break users such as caching certain bytes in the struct i.e. namespace in the share struct.

The other option considered was to use type Namespace []byte

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters cmwaters added the enhancement New feature or request label Jun 10, 2024
@cmwaters cmwaters self-assigned this Jun 10, 2024
@cmwaters cmwaters removed the enhancement New feature or request label Jun 10, 2024
@cmwaters
Copy link
Collaborator Author

cmwaters commented Aug 5, 2024

This has now been resolved (in v2)

@cmwaters cmwaters closed this as completed Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant