chore!: refactor NewMsgPayForBlob to accept blobs

[evan-forbes]

Overview

part of #388 and split off from #1154. This is just a minor refactor for NewMsgPayForBlob to accept a tmproto.Blob, instead of the individual components.

Checklist

• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[codecov-commenter]

Codecov Report

Merging #1197 (f2b2a57) into main (f4f6a3a) will increase coverage by 0.08%.
The diff coverage is 25.55%.

@@            Coverage Diff             @@
##             main    #1197      +/-   ##
==========================================
+ Coverage   48.08%   48.17%   +0.08%     
==========================================
  Files          72       72              
  Lines        4082     4081       -1     
==========================================
+ Hits         1963     1966       +3     
+ Misses       1947     1943       -4     
  Partials      172      172              

Impacted Files	Coverage Δ
app/process_proposal.go	0.00% <0.00%> (ø)
x/blob/payforblob.go	0.00% <0.00%> (ø)
x/blob/types/blob_tx.go	19.69% <6.89%> (+3.03%)	⬆️
x/blob/types/payforblob.go	81.81% <94.11%> (ø)
testutil/testnode/node_interaction_api.go	57.85% <100.00%> (+1.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[cmwaters]

I don't know if this is a good idea. Now users need to import the tendermint repo as well which they might not have been doing. Blob also has the share version which users might be confused as to whether it's necessary or where to get it. Also what if they do provide a share version and it's different to appconsts.ShareVersionZero

[evan-forbes]

Now users need to import the tendermint repo as well which they might not have been doing.

true! would a referenced type make this any better? added in 9dfae25

type Blob = tmproto.Blob

Blob also has the share version which users might be confused as to whether it's necessary or where to get it.

If the NewBlob function is used, do you think this is more palatable? There we add the default share version, at least until we have multiple share versions. I have also added a DefaultShareVersion constant with docs that specifically say to use that if someone is not sure which one to use. 9dfae25

Those are fair criticisms, but the alternative for multiple blobs is

func NewMsgPayForBlob(signer string, namespaces [][]byte, data [][]byte, shareversions []uint32)

and that feels really clunky to me

[cmwaters]

and that feels really clunky to me

Yeah I agree. I saw that in your other PR and was like meh.

Yeah I like the idea

type Blob = tmproto.Blob

func NewMsgPayForBlobs(signer string, blobs Blob...)

func NewBlob(namespace, data []byte) // uses default

// and together
msg := NewMsgPayForBlobs("callum", NewBlob("chat", "gm"), NewBlob("chat", "gn"))

[evan-forbes]

Do we also want to refactor

celestia-app/x/blob/types/payforblob.go

Lines 143 to 157 in f4f6a3a

	// CreateMultiShareCommitment generates a commitment over multiple blobs at
	// arbitrary points in the square. It uses the normal commitment creation
	// function per blob, and then creates a merkle root of those commitments.
	func CreateMultiShareCommitment(nsids [][]byte, blobs [][]byte, shareVersions []uint32) ([]byte, error) {
	commitments := make([][]byte, len(nsids))
	for i := range nsids {
	c, err := CreateCommitment(nsids[i], blobs[i], uint8(shareVersions[i]))
	if err != nil {
	return nil, err
	}
	commitments[i] = c
	}
	return merkle.HashFromByteSlices(commitments), nil
	}

in a similar way?

[rootulp]

👍

[cmwaters]

LGTM