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

fix: pay for blob typo(s) and redundant import #1127

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

rahulghangas
Copy link
Contributor

As title

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

LGTM

shares "github.com/celestiaorg/celestia-app/pkg/shares"
appshares "github.com/celestiaorg/celestia-app/pkg/shares"
Copy link
Member

Choose a reason for hiding this comment

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

[not blocking]
unrelated to this PR, but imo we should use a consistent import repo wide. Are there other shares packages that could be confusing?

personally would just prefer to use the package name, but I don't actually care that much

cc @rootulp

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong preference either.

My impression is that it is preferable to use the package name by default. If there is a conflict between two package names, then ideally we use a consistent import alias but that doesn't seem easily enforceable with linters so it's kinda at code maintainers discretion. In this case, we could remove the package alias by:

  1. Remove the package alias and duplicate package import
import (
	"github.com/celestiaorg/celestia-app/pkg/shares"
)
  1. Rename the variable shares => blobShares to avoid conflicting with the import name
func CreateCommitment(namespace []byte, blobData []byte, shareVersion uint8) ([]byte, error) {
	blob := coretypes.Blob{
		NamespaceID:  namespace,
		Data:         blobData,
		ShareVersion: shareVersion,
	}

	// split into shares that are length delimited and include the namespace in
	// each share
	blobShares, err := shares.SplitBlobs(0, nil, []coretypes.Blob{blob}, false)
	if err != nil {
		return nil, err
	}

	// the commitment is the root of a merkle mountain range with max tree size
	// equal to the minimum square size the blob can be included in. See
	// https://github.com/celestiaorg/celestia-app/blob/fbfbf111bcaa056e53b0bc54d327587dee11a945/docs/architecture/adr-008-blocksize-independent-commitment.md
	minSquareSize := BlobMinSquareSize(len(blobData))
	treeSizes := merkleMountainRangeSizes(uint64(len(blobShares)), uint64(minSquareSize))
	leafSets := make([][][]byte, len(treeSizes))
	cursor := uint64(0)
	for i, treeSize := range treeSizes {
		leafSets[i] = shares.ToBytes(blobShares[cursor : cursor+treeSize])
		cursor = cursor + treeSize
	}

shares "github.com/celestiaorg/celestia-app/pkg/shares"
appshares "github.com/celestiaorg/celestia-app/pkg/shares"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong preference either.

My impression is that it is preferable to use the package name by default. If there is a conflict between two package names, then ideally we use a consistent import alias but that doesn't seem easily enforceable with linters so it's kinda at code maintainers discretion. In this case, we could remove the package alias by:

  1. Remove the package alias and duplicate package import
import (
	"github.com/celestiaorg/celestia-app/pkg/shares"
)
  1. Rename the variable shares => blobShares to avoid conflicting with the import name
func CreateCommitment(namespace []byte, blobData []byte, shareVersion uint8) ([]byte, error) {
	blob := coretypes.Blob{
		NamespaceID:  namespace,
		Data:         blobData,
		ShareVersion: shareVersion,
	}

	// split into shares that are length delimited and include the namespace in
	// each share
	blobShares, err := shares.SplitBlobs(0, nil, []coretypes.Blob{blob}, false)
	if err != nil {
		return nil, err
	}

	// the commitment is the root of a merkle mountain range with max tree size
	// equal to the minimum square size the blob can be included in. See
	// https://github.com/celestiaorg/celestia-app/blob/fbfbf111bcaa056e53b0bc54d327587dee11a945/docs/architecture/adr-008-blocksize-independent-commitment.md
	minSquareSize := BlobMinSquareSize(len(blobData))
	treeSizes := merkleMountainRangeSizes(uint64(len(blobShares)), uint64(minSquareSize))
	leafSets := make([][][]byte, len(treeSizes))
	cursor := uint64(0)
	for i, treeSize := range treeSizes {
		leafSets[i] = shares.ToBytes(blobShares[cursor : cursor+treeSize])
		cursor = cursor + treeSize
	}

math "math"
"math"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@evan-forbes evan-forbes merged commit 67eecdd into celestiaorg:main Dec 19, 2022
@rahulghangas rahulghangas deleted the fix/payforblob-typo branch December 19, 2022 13:45
rootulp pushed a commit to rootulp/celestia-app that referenced this pull request Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants