celestiaorg / go-square

A library for encoding blobs into a 2D square of evenly sized chunks designed for sampling and reconstruction
Apache License 2.0
13 stars 16 forks source link

chore!: merge namespace package into shares package #89

Closed cmwaters closed 2 months ago

cmwaters commented 2 months ago

Second part of the merge

Wondertan commented 2 months ago

Another step would be moving Blob, Share, and Namespace to the root, leaving the business(parsing, splitting, etc.) logic behind(in current or new pkg).

rach-id commented 2 months ago

Another step would be moving Blob, Share, and Namespace to the root, leaving the business(parsing, splitting, etc.) logic behind(in current or new pkg).

I like this. I would even go to put the all the types in a package with a separate go.mod so that it can be imported from all repos without having any logic dependencies that might be undesirable in some repos

cmwaters commented 2 months ago

I would even go to put the all the types in a package with a separate go.mod so that it can be imported from all repos without having any logic dependencies that might be undesirable in some repos

Why would the logic be undesirable? I don't usually like having a types only package. It's counter to object oriented languages. You shouldn't separate types from their function

rach-id commented 2 months ago

Why would the logic be undesirable? I don't usually like having a types only package. It's counter to object oriented languages. You shouldn't separate types from their function

That depends on the logic. If it requires some specific packages that we might not want to import in some projects, then we can separate that.

My motivation for having a separate types project is to be able to import in core, app, node, etc, without having to worrying about a dependency hell. This would allow defining the types in a single place instead of having 2 or 3 definitions just because we can't import the package containing them.

Wondertan commented 2 months ago

I don't usually like having a types only package. It's counter to object oriented languages

That's a bold statement because this is how most of the Golang std library is designed. The low-level types and interfaces are separated in a sovereign(kek) pkg while specialized logic/implementations are in sub or other pkgs. Examples:

Wondertan commented 2 months ago

@rach-id, I am on the same page as you and want to note that the goal can be achieved even without separate go.mod due to lazy loading.

cmwaters commented 2 months ago

My motivation for having a separate types project is to be able to import in core, app, node, etc, without having to worrying about a dependency hell. This would allow defining the types in a single place instead of having 2 or 3 definitions just because we can't import the package containing them.

There won't be any dependency hell. app, core and node will all import go-square. We also hopefully won't need multiple definitions

cmwaters commented 2 months ago

The low-level types and interfaces are separated in a sovereign(kek) pkg while specialized logic/implementations are in sub or other pkgs. Examples:

Interfaces with multiple implementations in sub packages is different from a single implementation and splitting the raw type from the business logic that uses it

rootulp commented 2 months ago

Interfaces with multiple implementations in sub packages is different from a single implementation and splitting the raw type from the business logic that uses it

+100. Given we don't have multiple implementations of namespace, shares, or blobs, I'm in favor of defining the types and methods on those types in the same package.

rach-id commented 2 months ago

Just to be clear, I am in favour of that as long as we don't have any weird dependencies. Especially, having dependencies on app for example, core, cosmos-sdk or... in the logic of the types

Wondertan commented 2 months ago

Interfaces with multiple implementations in sub packages is different from a single implementation and splitting the raw type from the business logic that uses it

Its both the interfaces with implementations as well as core types and specialized logic

Wondertan commented 2 months ago

Given we don't have multiple implementations of namespace, shares, or blobs, I'm in favor of defining the types and methods on those types in the same package.

I am not proposing splitting methods from types, but separating higher level businnes logic like splitting data into shares from the types:

Anyway, I honestly don't want to block you on this, but I would like to know why coupling these makes sense to you. It's not a hill I am willing to die on, and time will show us if there are issues with the current approach, like it showed when we tried keeping every type in its own package.

cmwaters commented 2 months ago

@Wondertan, I'm not totally opposed to what you're saying. I just feel like this is more an avenue to bike shed then something that brings tangible benefits so I just don't want to allocate too much time to it. I think when you're dealing with blobs or shares it's because you're trying to construct a square or read parts of a square thus I feel that keeping them together makes sense. There all part of the same logical realm. Blob -> Shares and Shares -> Blobs.

FYI: I do think some of the structs, like the Builder and maybe even the SparseShareSplitter should be made private.

cmwaters commented 2 months ago

There will also be some logic separated from the share package, namely the "compact shares" which is it's own encoding protocol i.e. [][]bytes -> Blob