chore!: improve start/end row proof validation #1476
Conversation
rach-id
requested review from
rootulp and
evan-forbes
and removed request for
a team
types/row_proof.go
Outdated
| if len(rp.Proofs) != 0 && | ||
| (int64(rp.StartRow) != rp.Proofs[0].Index || | ||
| int64(rp.StartRow) != rp.Proofs[len(rp.Proofs)-1].Index) { | ||
| return fmt.Errorf("invalid start/end row") | ||
| } |
There was a problem hiding this comment.
I'm a bit lost on why this conditional is an invalid start or end row. EndRow isn't used in the conditional so how can it be deemed invalid?
Can this PR please include a unit test?
types/row_proof.go
Outdated
| if len(rp.Proofs) != 0 && | ||
| (int64(rp.StartRow) != rp.Proofs[0].Index || | ||
| int64(rp.EndRow) != rp.Proofs[len(rp.Proofs)-1].Index) { | ||
| return fmt.Errorf("invalid start/end row") | ||
| } |
There was a problem hiding this comment.
[nit] this could be split into two separate checks with two separate errors: one for start row and one for end row. That way the error message is precise.
| return proof | ||
| }(), | ||
| root: root, | ||
| wantErr: true, |
There was a problem hiding this comment.
nit: this test could be improved by verifying that the error is fmt.Errorf("invalid start/end row") in case some other validation check is erroring before the expected error is encountered.
There was a problem hiding this comment.
I think that's fine as long as we're attesting that some error happens. Changing this would mean refactoring the whole test + it's not something that's being done consistently in this repo, most tests are just wanting an error.
If you feel strongly about it, I can refactor, no issue
There was a problem hiding this comment.
Not blocking on it. It's just a nice to have
There was a problem hiding this comment.
Thanks for addressing #1475; however, I noticed there's one more item in that issue that isn't covered in the PR:
However, I think it is still a valid check (and orthogonal to
StartRowandEndRow) to see if theTotalfield is the same across all the proofs inProofs. Again, will leave the decision to you.
If you plan to add it in another PR, then perhaps the Closes in the PR description can be rephrased.
rach-id
changed the title
|
This is becoming a breaking change since we're adding more constraints on the proofs. I'm not sure if it's worth merging or not |
I don't have a strong opinion on this, but if introducing a breaking change is not desirable, I'm fine with holding off on this PR. |
Description
Closes #1475