Skip to content

Conversation

@DeVikingMark
Copy link
Contributor

Description:

Enhance TestNafDecomposition with comprehensive test cases:

  • Add test table with various input numbers
  • Add boundary cases (0, 1)
  • Add interesting cases requiring NAF conversion
  • Add property checks for NAF representation
  • Add reconstruction validation
  • Improve error messages with detailed information

This commit removes the TODO comment and provides proper test coverage for the NafDecomposition function, ensuring correct NAF properties and accurate number reconstruction.

Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DeVikingMark for the test!
There are though two sign errors in 15 and 31 and endianness error in 7. You should also run go fmt utils_test.go to pass CI.

@yelhousni
Copy link
Collaborator

Thanks for the test! There are though two sign errors in 15 and 31 and endianness error in 7. You should also run go fmt utils_test.go to pass CI.

Here is the suggested diff:

diff --git a/ecc/utils_test.go b/ecc/utils_test.go
index fed45efe5..187d760df 100644
--- a/ecc/utils_test.go
+++ b/ecc/utils_test.go
@@ -12,12 +12,12 @@ func TestNafDecomposition(t *testing.T) {
 		input    string // large number in decimal form
 		expected []int8 // expected NAF representation
 	}{
-		{"13", []int8{1, 0, -1, 0, 1}},          // existing test case
-		{"0", []int8{}},                          // edge case - zero
-		{"1", []int8{1}},                         // edge case - one
-		{"7", []int8{1, 0, 0, -1}},              // 7 = 2³ - 2⁰ (8 - 1)
-		{"15", []int8{1, 0, 0, 0, 1}},           // 15 = 2⁴ - 2⁰
-		{"31", []int8{1, 0, 0, 0, 0, 1}},        // 31 = 2⁵ - 2⁰
+		{"13", []int8{1, 0, -1, 0, 1}},    // existing test case
+		{"0", []int8{}},                   // edge case - zero
+		{"1", []int8{1}},                  // edge case - one
+		{"7", []int8{-1, 0, 0, 1}},        // 7 = 2³ - 2⁰ (8 - 1)
+		{"15", []int8{-1, 0, 0, 0, 1}},    // 15 = 2⁴ - 2⁰
+		{"31", []int8{-1, 0, 0, 0, 0, 1}}, // 31 = 2⁵ - 2⁰
 	}
 
 	for i, test := range tests {
@@ -33,7 +33,7 @@ func TestNafDecomposition(t *testing.T) {
 
 		// Length check
 		if len(naf) != len(test.expected) {
-			t.Errorf("Test %d: Incorrect length for input %s. Got %d, want %d", 
+			t.Errorf("Test %d: Incorrect length for input %s. Got %d, want %d",
 				i, test.input, len(naf), len(test.expected))
 			continue
 		}
@@ -69,7 +69,7 @@ func TestNafDecomposition(t *testing.T) {
 			power.Mul(power, big.NewInt(2))
 		}
 		if reconstructed.Cmp(input) != 0 {
-			t.Errorf("Test %d: NAF reconstruction failed for input %s. Got %s", 
+			t.Errorf("Test %d: NAF reconstruction failed for input %s. Got %s",
 				i, test.input, reconstructed.String())
 		}
 	}

and a python code to cross check values:

def NAF(x):
	if x == 0:
		return []
	z = 0 if x % 2 == 0 else 2 - (x % 4)
	return NAF( (x-z) // 2 ) + [z]

@DeVikingMark
Copy link
Contributor Author

@yelhousni fixed, is it fine?

@ivokub ivokub requested a review from yelhousni February 13, 2025 17:06
@yelhousni
Copy link
Collaborator

You should also run go fmt utils_test.go to pass CI.

@DeVikingMark thanks for the correction. You should also run go fmt utils_test.go to pass CI.

@yelhousni
Copy link
Collaborator

You should also run go fmt utils_test.go to pass CI.

@DeVikingMark thanks for the correction. You should also run go fmt utils_test.go to pass CI.

@DeVikingMark CI still does not pass. Did you run go fmt or modified the file by hand?

@DeVikingMark
Copy link
Contributor Author

You should also run go fmt utils_test.go to pass CI.

@DeVikingMark thanks for the correction. You should also run go fmt utils_test.go to pass CI.

@DeVikingMark CI still does not pass. Did you run go fmt or modified the file by hand?

I run go fmt and modified it by hand still cand understand whats wrong

@DeVikingMark
Copy link
Contributor Author

I ran go fmt and it only provided minor improvement with alignments of text, and I suppose that will not solve the problem

@yelhousni yelhousni merged commit 7c1592e into Consensys:master Mar 6, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants