Remove the Nonce field in MsgWirePayForMessage

[evan-forbes]

Currently, the nonce in MsgWirePayForMessage is not being used. Instead, the sdk handles the AccountNumber SequenceNumber for the app. The app should be modified so that it follows the spec and handles the nonce inside of MsgWirePayForMessage, instead of relying on the sdk to handle the AccountNumber SequenceNumber.

[liamsi]

is this critical to do before mainnet @adlerjohn? Or in other words: what is wrong with the AccountNumber SequenceNumber?

[adlerjohn]

Wait the linked issue doesn't help at all! 😂

What does AccountNumber do? How does it affect replay protection, etc?

[evan-forbes]

Whoops, apparently the issue linked confused me as well 😅 . The sdk uses a SequeceNumber instead of a nonce. AccountNumber has something to do with reducing state bloat. Both are handled implicitly by the default sdk.Tx building tools, found here. We are currently using those tools for convenience, but I think we should look into what needs to be done in order to use the most important pieces of those tools, the key handling/signing, while also organizing transactions the way the spec does. This would give us the best of both worlds, the explicitness of the spec, with the safety of battle tested key signing tools.

[evan-forbes]

The sdk wraps all the transactions to get around protobuf being nondeterministic, so I think the plan is to now remove the Nonce field from WirePayForMessage and use the AccountNumber already implemented in the auth module/default sdk.Tx

[liamsi]

Looking at MsgWirePayForMessage I think it should not even have any fields other than those that commit to a blob (or multiple blobs for different square sizes). Fee payment could be handled by the auth module I think.

So instead of:
https://github.com/celestiaorg/lazyledger-app/blob/edaba2924a595141d5aa2d23ff12daacf9bfb134/x/payment/types/tx.pb.go#L34-L42

this can probably be just reduced to these fields only:

MessageNameSpaceId     []byte                    `protobuf:"bytes,3,opt,name=message_name_space_id,json=messageNameSpaceId,proto3" json:"message_name_space_id,omitempty"`
MessageSize            uint64                    `protobuf:"varint,4,opt,name=message_size,json=messageSize,proto3" json:"message_size,omitempty"`
Message                []byte                    `protobuf:"bytes,5,opt,name=message,proto3" json:"message,omitempty"`
MessageShareCommitment []ShareCommitAndSignature `protobuf:"bytes,6,rep,name=message_share_commitment,json=messageShareCommitment,proto3" json:"message_share_commitment"`

The PublicKey would already part of the wrapping Tx, specifically in the AuthInfo:
https://github.com/cosmos/cosmos-sdk/blob/94377f1cb3df94ad79e8a741b237be03c9716426/types/tx/tx.pb.go#L38

We can mybe even remove the signatures and use the ones included in Tx?
https://github.com/cosmos/cosmos-sdk/blob/94377f1cb3df94ad79e8a741b237be03c9716426/types/tx/tx.pb.go#L42

The last point might require a modified AnteDecorator if the signatures are for committing to blobs 🤔
https://github.com/cosmos/cosmos-sdk/blob/78c3c4e92d38c00b07fd888e231a91372189fa67/x/auth/ante/sigverify.go#L231

[liamsi]

related to above comment: #59, #48 (and has implications on #49)

[evan-forbes]

This issue should have been closed with the merge of #129