feat!: refactor blob, namespace and shares into single package #2778
Conversation
|
|
|
Overall LGTM. Not actually approving b/c looks like merge conflicts. One remaining question: should this be marked as API breaking? I assume consumers who imported the previous packages will have to change their import path. |
|
Yeah this is definitely API breaking. I'll add the exclamation mark in the commit just to be clear |
rootulp
changed the title
|
So there's talk about just having this as a separate repo. I don't mind doing that but if that is the case we probably don't need to merge this PR. |
| @@ -41,7 +41,7 @@ func TestInsufficientMinGasPriceIntegration(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| fee := sdk.NewCoins(sdk.NewCoin(app.BondDenom, sdk.NewInt(feeAmount))) | |||
| b, err := blob.NewBlob(namespace.RandomNamespace(), []byte("hello world"), 0) | |||
| b, err := blob.NewBlob(shares.RandomNamespace(), []byte("hello world"), 0) | |||
There was a problem hiding this comment.
[Question] Is there any plan on isolating test related functionalities to their own package as well?
| {1, 385, []int{128, 128, 128}, []uint32{2, 130, 258}}, | ||
| {1024, 32, []int{32}, []uint32{1024}}, | ||
| } | ||
| for i, tt := range tests { | ||
| res, indexes := BlobSharesUsedNonInteractiveDefaults(tt.cursor, appconsts.DefaultSubtreeRootThreshold, tt.blobLens...) | ||
| res, indexes := BlobSharesUsedNonInteractiveDefaults(tt.cursor, 64, tt.blobLens...) |
There was a problem hiding this comment.
Mind elaborating why this const is replaced with a hardcoded value?
| @@ -26,12 +25,12 @@ func TestBlobSharesUsedNonInteractiveDefaults(t *testing.T) { | |||
| {0, 10, []int{10}, []uint32{0}}, | |||
| {1, 20, []int{10, 10}, []uint32{1, 11}}, | |||
| {0, 1000, []int{1000}, []uint32{0}}, | |||
| {0, appconsts.DefaultSquareSizeUpperBound + 1, []int{appconsts.DefaultSquareSizeUpperBound + 1}, []uint32{0}}, | |||
| {0, 129, []int{129}, []uint32{0}}, | |||
There was a problem hiding this comment.
Mind elaborating why these are replaced with a hardcoded value?
| @@ -0,0 +1,59 @@ | |||
| package shares | |||
There was a problem hiding this comment.
This file seems to contain only test utilities, would be good to consider updating the file's name with some 'test' suffix.
| // NamespaceVersionMaxValue is the maximum value a namespace version can be. | ||
| // This const must be updated if NamespaceVersionSize is changed. | ||
| NamespaceVersionMaxValue = math.MaxUint8 | ||
|
|
||
| // NamespaceIDSize is the size of a namespace ID in bytes. | ||
| NamespaceIDSize = ns.NamespaceIDSize | ||
| NamespaceIDSize = shares.NamespaceIDSize |
There was a problem hiding this comment.
[suggestion] Now that the share package will have its own go.mod, it might be more logical to define these constants e.g., NamespaceIDSize at the app level (where the share package is used and where the core logic of app resides). We can then pass them as parameters to the relevant structs and methods within the share package. If we decide to that, it may not fall within the scope of this PR though, could be part of another PR.
|
Converting to draft b/c we may not merge this based on
|
|
Assuming this is safe to close b/c we're moving most of this code to https://github.com/celestiaorg/go-square |
Overview
This is an incredibly broad change in terms of files changed but it chiefly consists of merging
pkg/blob,pkg/namespaceandpkg/sharesinto asharespackage and updating all the import paths accordingly.This is part of the move to create a separate go module for the shares package that can be used by downstream dependents like rollkit without needing to import tendermint or the cosmos-sdk.
Logically,
Share,NamespaceandBlobare all part of the same encoding protocol that marshals/unmarshals blobs into fixed length shares.Checklist