refactor: simplify rebuildShares
#197
Conversation
Codecov Report
@@ Coverage Diff @@
## master #197 +/- ##
==========================================
- Coverage 81.36% 80.80% -0.57%
==========================================
Files 7 7
Lines 526 500 -26
==========================================
- Hits 428 404 -24
+ Misses 58 57 -1
+ Partials 40 39 -1
|
| @@ -249,7 +247,6 @@ func (eds *ExtendedDataSquare) solveCrosswordCol( | |||
| // 2. Whether the original shares could be decoded from the shares parameter. | |||
| // 3. [Optional] an error. | |||
| func (eds *ExtendedDataSquare) rebuildShares( | |||
There was a problem hiding this comment.
do we need this function at all if its just decoding?
There was a problem hiding this comment.
good question, I can explore deleting it entirely in a FLUP issue
There was a problem hiding this comment.
TBH I'm only creating a new issue rather than tackling in this PR b/c I don't want to dismiss existing approvals 😞 #199
| @@ -259,25 +256,6 @@ func (eds *ExtendedDataSquare) rebuildShares( | |||
| return nil, false, nil | |||
| } | |||
|
|
|||
| if isExtendedPartIncomplete { | |||
There was a problem hiding this comment.
[Question] I suppose there have not been any tests associated with this part of the code, as no test file has been updated in this PR, right?
There was a problem hiding this comment.
codecov claims these lines were covered: https://app.codecov.io/gh/celestiaorg/rsmt2d/blob/master/extendeddatacrossword.go
I assume via https://github.com/rootulp/rsmt2d/blob/1f1904acc114b41dff838ad1d530feacc1d9f199/extendeddatacrossword_test.go#L21
| @@ -259,25 +256,6 @@ func (eds *ExtendedDataSquare) rebuildShares( | |||
| return nil, false, nil | |||
| } | |||
|
|
|||
| if isExtendedPartIncomplete { | |||
| // If needed, rebuild the parity shares too. | |||
| rebuiltExtendedShares, err := eds.codec.Encode(rebuiltShares[0:eds.originalDataWidth]) | |||
There was a problem hiding this comment.
[Question] The only potential issue could arise if the parity portion is incomplete and the rebuilt extended shares obtained from rebuiltShares, err := eds.codec.Decode(shares) (the second half of rebuiltShares) and rebuiltExtendedShares, err := eds.codec.Encode(rebuiltShares[0:eds.originalDataWidth]) turn out to be different. Have we conducted any tests to verify this?
In the original code, we initially decode the entire row, and if the extended part (before decoding) is incomplete, we discard that part from the decoded version and replace it with the encoding result. However, in the new version, we retain whatever the decode function provides us with. If we are confident that these two have the same result, then everything looks good to me! (I think they should be the same)
There was a problem hiding this comment.
The only potential issue could arise if the parity portion is incomplete
I don't expect this to happen because Decode calls Reconstruct here and the godoc for Reconstruct claims:
// If there are too few shards to reconstruct the missing
// ones, ErrTooFewShards will be returned.
Closes #196
Since
Decodereturns original + parity shares, there is no need to separately rebuild the parity shares. This PR removes an unnecessary conditional insiderebuildShareswhich let us also remove a param to that function. After the refactor, a few helper methods were no longer used so they were also removed.