Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signer: EIP 712, parse bytes and bytesX as hex strings + correct padding #21307

Merged
merged 6 commits into from
Aug 3, 2020
Merged

signer: EIP 712, parse bytes and bytesX as hex strings + correct padding #21307

merged 6 commits into from
Aug 3, 2020

Conversation

natsukagami
Copy link
Contributor

@natsukagami natsukagami commented Jul 7, 2020

I've come across a lot of input TypedData where bytes and bytesX is encoded as a hex string (0x...).

This adds support for parsing those inputs in the TypedData.EncodePritimitiveValue method.

Tests are included.


This change is Reviewable

@holiman
Copy link
Contributor

holiman commented Jul 23, 2020

I tried to add the following, but you seem to have disabled maintainer-commit to your repo:

diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go
index 4e6e177084..53f7d8dc86 100644
--- a/signer/core/signed_data_internal_test.go
+++ b/signer/core/signed_data_internal_test.go
@@ -33,6 +33,7 @@ func TestParseBytes(t *testing.T) {
                {"0x1234", []byte{0x12, 0x34}},
                {[]byte{12, 34}, []byte{12, 34}},
                {hexutil.Bytes([]byte{12, 34}), []byte{12, 34}},
+               {"1234", nil}, // not a proper hex-string
                {"not a hex string", nil},
                {15, nil},
                {nil, nil},

Another testcase to verify that we don't treat ambiguous strings as hexdata.
Aside from that, the PR looks good to me. Could you add the padding-fix in this PR too?

signer/core/signed_data_internal_test.go Outdated Show resolved Hide resolved
signer/core/signed_data_internal_test.go Outdated Show resolved Hide resolved
signer/core/signed_data_internal_test.go Outdated Show resolved Hide resolved
signer/core/signed_data_internal_test.go Show resolved Hide resolved
signer/core/signed_data.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Jul 23, 2020

Oh, and thanks for digging into this!

@holiman
Copy link
Contributor

holiman commented Aug 3, 2020

Thanks, it's starting to looks good, we're almost there now. Could you please apply this diff? (some nitpicks plus a couple of more testcases)

diff --git a/signer/core/signed_data_internal_test.go b/signer/core/signed_data_internal_test.go
index be55bff3d6..9768ee0b3f 100644
--- a/signer/core/signed_data_internal_test.go
+++ b/signer/core/signed_data_internal_test.go
@@ -41,6 +41,11 @@ func TestBytesPadding(t *testing.T) {
 			Input:  []byte{1},
 			Output: []byte{1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},
 		},
+		{
+			Type:   "bytes1",
+			Input:  []byte{1, 2},
+			Output: nil,
+		},
 		{
 			Type:   "bytes7",
 			Input:  []byte{1, 2, 3, 4, 5, 6, 7},
@@ -51,24 +56,29 @@ func TestBytesPadding(t *testing.T) {
 			Input:  []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32},
 			Output: []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32},
 		},
+		{
+			Type:   "bytes32",
+			Input:  []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33},
+			Output: nil,
+		},
 	}
 
 	d := TypedData{}
-	for _, test := range tests {
+	for i, test := range tests {
 		val, err := d.EncodePrimitiveValue(test.Type, test.Input, 1)
 		if test.Output == nil {
 			if err == nil {
-				t.Errorf("expected error, got nil error with result %v: case %v", val, test)
+				t.Errorf("test %d: expected error, got no error (result %x)", i, val)
 			}
 		} else {
 			if err != nil {
-				t.Errorf("expected nil error, got %v: case %v", err, test)
+				t.Errorf("test %d: expected no error, got %v", i, err)
 			}
 			if len(val) != 32 {
-				t.Errorf("expected len 32, got %v: case %v", len(val), test)
+				t.Errorf("test %d: expected len 32, got %d", i, len(val))
 			}
-			if fmt.Sprint(val) != fmt.Sprint(test.Output) {
-				t.Errorf("expected %x, got %x: case %v", test.Output, val, test)
+			if !bytes.Equal(val, test.Output) {
+				t.Errorf("test %d: expected %x, got %x", i, test.Output, val)
 			}
 		}
 	}
@@ -92,15 +102,15 @@ func TestParseBytes(t *testing.T) {
 		out, ok := parseBytes(tt.v)
 		if tt.exp == nil {
 			if ok || out != nil {
-				t.Errorf("Case %d: expecting !ok, got ok = %v with out = %x", i, ok, out)
+				t.Errorf("test %d: expected !ok, got ok = %v with out = %x", i, ok, out)
 			}
 			continue
 		}
 		if !ok {
-			t.Errorf("Case %d: expecting ok got !ok", i)
+			t.Errorf("test %d: expected ok got !ok", i)
 		}
 		if !bytes.Equal(out, tt.exp) {
-			t.Errorf("Case %d: expecting %x got %x", i, tt.exp, out)
+			t.Errorf("test %d: expected %x got %x", i, tt.exp, out)
 		}
 	}
 }

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@holiman holiman added this to the 1.9.19 milestone Aug 3, 2020
@holiman holiman changed the title TypedData: Attempt to parse bytes and bytesX types as a hex string signer: EIP 712, parse bytes and bytesX as hex strings + correct padding Aug 3, 2020
@holiman holiman merged commit 90dedea into ethereum:master Aug 3, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
…padding (ethereum#21307)

* Handle hex strings for bytesX types

* Add tests for parseBytes

* Improve tests

* Return nil bytes if error is non-nil

* Right-pad instead of left-pad bytes

* More tests
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.

3 participants