Code review of the payment module

code is not well structured, most of the methods are in x/payment/types not really grouped based on functionality

Code quality

Transaction Lifecycle

CheckTx

Note: Although the desired outcome is only for MsgWirePayForData messages to be submitted to the application via CheckTx, there is nothing stopping MsgPayForData messages to be in the mempool.

Message	Source code	Brief description	UTs	Issues found
`MsgWirePayForData`	ValidateBasic	Stateless checks of `MsgWirePayForData`.	TestWirePayForData_ValidateBasic	- CreateCommitment could be expensive for CheckTx <br /> - WirePayForData with empty Message
MsgPayForData	See DeliverTx section below

CreateCommitment

func CreateCommitment(k uint64, namespace, message []byte) {
    // ...
    shares := msg.SplitIntoShares().RawShares()
    // SplitIntoShares() => list of shares: (nid|len|partial-message1, nid), (nid|len|partial-message2, nid), ...
    // .RawShares() => [nid|len|partial-message1, nid|len|partial-message2 ... ]
    if uint64(len(shares)) > (k*k)-1 {
		return nil, fmt.Errorf("message size exceeds max shares for square size %d: max %d taken %d", k, (k*k)-1, len(shares))
	}
    // TODO this check assumes that only this message gets in the square (no txs or evidence)

    // split shares into leafSets, each of size k or a power of two < k
    //  - create an ErasuredNamespacedMerkleTree subtree for every set in leafSets
    //      - for every leaf in set, nsLeaf=namespace|leaf is added as a leaf in the subtree
    //      via ErasuredNamespacedMerkleTree.Push(nsLeaf)
    //          - prefix share with another namespace !!!
    //      - compute root of subtree
    //  - compute hash of all the subtree roots 
    return merkle.HashFromByteSlices(subTreeRoots)
}

func (msgs Messages) SplitIntoShares() NamespacedShares {
    shares := make([]NamespacedShare, 0)
    for _, m := range msgs.MessagesList {
        rawData, err := m.MarshalDelimited()
        // rawData: len | data
        shares = AppendToShares(shares, m.NamespaceID, rawData)
    }
    return shares
}

func AppendToShares(shares []NamespacedShare, nid namespace.ID, rawData []byte) []NamespacedShare {
    if len(rawData) <= 248 {
        // rawShare: nid | len | data
        // paddedShare: zeroPadIfNecessary up to 256
        share := NamespacedShare{paddedShare, nid}
        // !!! a share has the nid twice !!!
        shares = append(shares, share)
    } else {
        shares = append(shares, splitMessage(rawData, nid)...)
	}
	return shares
}

// splitMessage breaks the data in a message into the minimum number of
// namespaced shares
func splitMessage(rawData []byte, nid namespace.ID) NamespacedShares {
    // result: (nid|len|partialData1, nid), (nid|len|partialData2, nid), ...
}

PrepareProposal

Method	Invoked by	Brief description	UTs	Issues found
parsedTxs()	`PrepareProposal`	Parse TXs in proposed block.	estimate_square_size_test.go	- MsgWirePayForData messages are validate twice by the proposer
estimateSquareSize()	`PrepareProposal`	Estimate square size using the data in the proposed block.	Test_estimateSquareSize	- nextPowerOfTwo vs. NextHigherPowerOf2
rawShareCount()	`estimateSquareSize`	Calculates the number of shares needed by the block data.	NA	- [Optimization] MsgSharesUsed requires remainder operation <br /> - The number of TX shares is incremented twice <br /> - Unit length missing from estimating txBytes in rawShareCount <br /> - Number of share estimated by rawShareCount is inaccurate
FitsInSquare	`estimateSquareSize`	Uses the non interactive default rules to see if messages of some lengths will fit in a square of squareSize starting at share index cursor.	TestFitsInSquare	- FitsInSquare is incorrect for edge case
prune	`PrepareProposal`	Removes txs until the set of txs will fit in the square.	Test_pruning	- Inconsistency while pruning compact shares <br> - Redundancy in PrepareProposal
malleateTxs	`PrepareProposal`	Process any MsgWirePayForData transactions into MsgPayForData and their respective messages.	NA	NA
malleate	`malleateTxs`	Split `MsgWirePayForData` txs into `MsgPayForData` txs and data.	NA	- Complex logic for estimating the square size
ProcessWirePayForData	`malleate`	Parses the MsgWirePayForData to produce the components needed to create a single MsgPayForData.	TestProcessMessage	NA
calculateCompactShareCount	`malleateTxs`	calculates the exact number of compact shares used	Test_calculateCompactShareCount	- ShareIndex offset in calculateCompactShareCount
NewCompactShareSplitter	`calculateCompactShareCount`	Creates a CompactShareSplitter.	TestCompactShareWriter	NA
Split	`PrepareProposal`	converts block data into encoded shares	NA	- SparseShareSplitter writer can be simplified
ExtendShares	`PrepareProposal`	Erasure the data square. Relies on the rsmt2d library.	TestExtendShares	NA
NewDataAvailabilityHeader	`PrepareProposal`	Generates a `DataAvailability` header using the provided square size and shares.	TestNewDataAvailabilityHeader	NA

ProcessProposal

Method	Invoked by	Brief description	UTs	Issues found
ProcessProposal	ABCI++		TestMessageInclusionCheck	- Minor: Inconsistency re. number of messages in PFDs <br /> - Commitments checked only in ProcessProposal
Split	`ProcessProposal`	converts block data into encoded shares	NA	See PrepareProposal
NewDataAvailabilityHeader	`ProcessProposal`	Generates a `DataAvailability` header using the provided square size and shares.	TestNewDataAvailabilityHeader	See PrepareProposal
GetCommit	`ProcessProposal`	Get PFD commitment	NA	NA
calculateCommitPaths	`GetCommit`	Calculates all of the paths to subtree roots needed to create the commitment for a given message.	Test_calculateSubTreeRootCoordinates	NA
getSubTreeRoot	`GetCommit`	Traverses the nmt of the selected row and returns the subtree root.	TestEDSSubRootCacher	NA

DeliverTx

Note: Only MsgPayForData are delivered to the payment module.

Message	Source code	Brief description	UTs	Issues found
MsgPayForData	ValidateBasic	Stateless checks of `MsgPayForData`.	TestValidateBasic	- MsgPayForData.ValidateBasic doesn't invalidate reserved namespaces
MsgPayForData	MsgServer.PayForData	Execute `MsgPayForData`: Consume `msg.MessageSize` amount of gas.	TestPayForDataGas	None

← Back to Documentation