From fed16378e5b055bdbc3637bd4125d943f142a0a1 Mon Sep 17 00:00:00 2001 From: sweexordious Date: Tue, 12 Nov 2024 11:42:16 +0400 Subject: [PATCH 1/2] chore: require error from included --- blob/service_test.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/blob/service_test.go b/blob/service_test.go index 0ce48685c2..68ac77e95e 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -327,13 +327,7 @@ func TestBlobService_Get(t *testing.T) { ) }, expectedResult: func(res interface{}, err error) { - // Question for reviwers: for the case of included, it's here made not to return an error - // when the inclusion proof is invalid. For the implementation that we do, we return (false, error) - // where error is the reason why it failed. For example, if the data root does not commit to the - // row roots, we return that in the error message. - // is it fine if Included also does the same? returning the reason why it failed in the error? - // given that this will change how this method was behaving previously, i.e, return (false, nil) - // for a proof that does not commit to the data.require.NoError(t, err) + require.Error(t, err) included, ok := res.(bool) require.True(t, ok) require.False(t, included) From 6d2dc800af26af882e494e83104bebcda50edc8c Mon Sep 17 00:00:00 2001 From: sweexordious Date: Tue, 12 Nov 2024 11:45:12 +0400 Subject: [PATCH 2/2] chore: wrap err invalid proof --- blob/service.go | 3 ++- blob/service_test.go | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/blob/service.go b/blob/service.go index 1d128422ec..8ceb453881 100644 --- a/blob/service.go +++ b/blob/service.go @@ -316,7 +316,8 @@ func (s *Service) Included( // verify that the blob subtree roots match the proof subtree roots if proofCommitment := proof.GenerateCommitment(); !commitment.Equal(proofCommitment) { return false, fmt.Errorf( - `unequal blob commitment %s and proof commitment %s`, + "%w: unequal blob commitment %s and proof commitment %s", + ErrInvalidProof, hex.EncodeToString(commitment), hex.EncodeToString(proofCommitment), ) diff --git a/blob/service_test.go b/blob/service_test.go index 68ac77e95e..7728b64d6b 100644 --- a/blob/service_test.go +++ b/blob/service_test.go @@ -288,9 +288,7 @@ func TestBlobService_Get(t *testing.T) { }, expectedResult: func(res interface{}, err error) { require.Error(t, err) - // Question for reviewers: same for here, we're returning exactly, ErrInvalidProof. - // is it fine to wrap it? - // require.ErrorIs(t, err, ErrInvalidProof) + require.ErrorIs(t, err, ErrInvalidProof) included, ok := res.(bool) require.True(t, ok) require.False(t, included)