cluster: refactor EIP712 domain data #1169

xenowits · 2022-09-26T11:51:25Z

Refactor EIP712 Domain data to add support for different chains ids.

category: refactor
ticket: #1203

codecov · 2022-09-26T11:54:05Z

Codecov Report

Base: 52.81% // Head: 52.97% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (576f5ca) compared to base (fad6f93).
Patch coverage: 72.72% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
+ Coverage   52.81%   52.97%   +0.15%     
==========================================
  Files         133      134       +1     
  Lines       15648    15860     +212     
==========================================
+ Hits         8265     8402     +137     
- Misses       6187     6230      +43     
- Partials     1196     1228      +32

Impacted Files	Coverage Δ
cluster/definition.go	`53.35% <50.00%> (-0.43%)`	⬇️
cluster/helpers.go	`62.65% <73.21%> (-2.31%)`	⬇️
cluster/test_cluster.go	`86.95% <100.00%> (+0.34%)`	⬆️
p2p/sender.go	`43.95% <0.00%> (-12.95%)`	⬇️
core/consensus/msg.go	`53.97% <0.00%> (-7.64%)`	⬇️
core/consensus/transport.go	`56.09% <0.00%> (-4.96%)`	⬇️
core/consensus/component.go	`50.23% <0.00%> (-3.42%)`	⬇️
core/scheduler/scheduler.go	`73.13% <0.00%> (-0.40%)`	⬇️
app/retry/retry.go	`76.56% <0.00%> (-0.19%)`	⬇️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

xenowits · 2022-09-26T11:57:59Z

Discussion: https://docs.google.com/document/d/1lYP1IU56JLtadrdaZTA9J1edGJjEaG8hLesyQlNxjCc/edit#heading=h.6ef9urxlm0ng

corverroos

This breaks backwards compatibility with previous signatures.

Also, you are not only changing the chainID, you are also changing name and salt. We said that the DV launchpad is going to sign like charon does, then we are going to get product to come up a proper next iteration.

Lets rather hold off on this until we know what the plan is, how we are going to do backwards compatibility stuff, etc.

corverroos · 2022-09-30T09:11:35Z

cluster/helpers.go

@@ -165,14 +163,14 @@ func digestEIP712(address string, message []byte, nonce int) ([32]byte, error) {
 		},
 		PrimaryType: "Challenge",
 		Domain: apitypes.TypedDataDomain{
-			Name:    "ETHChallenger",
+			Name:    "Obol",


remove these structure changes for now

corverroos · 2022-09-30T09:12:27Z

cluster/cluster_internal_test.go

@@ -28,21 +28,25 @@ import (
 )

 func TestDefinitionVerify(t *testing.T) {
+	forkVersion := "0x80000069"


make these constants in helpers.go

xenowits · 2022-09-30T10:37:50Z

cluster/helpers.go

@@ -191,6 +174,68 @@ func digestEIP712(address string, message []byte, nonce int) ([32]byte, error) {
 	return crypto.Keccak256Hash(rawData), nil
 }

+// configHashSig returns the EIP712 (https://eips.ethereum.org/EIPS/eip-712) signature of the config hash.
+func configHashSig(secret *ecdsa.PrivateKey, configHash string, chainID int64) ([]byte, error) {


will add tests for these new functions in the next PR

corverroos · 2022-09-30T10:56:50Z

cluster/helpers.go

+}
+
+// eip712Digest returns the EIP712 digest of the input data.
+func eip712Digest(signerData apitypes.TypedData) ([32]byte, error) {


can replace eip712Digest function with apitypes.TypedDataAndHash

corverroos · 2022-09-30T10:59:10Z

cluster/helpers.go

+
+// configHashDigest returns the EIP712 digest of the config hash.
+func configHashDigest(configHash string, chainID int64) ([32]byte, error) {
+	signer := eip712SignerData(chainID)


this isn't a signer, it is more data

corverroos · 2022-10-01T05:58:23Z

cluster/helpers.go

-// See reference https://medium.com/alpineintel/issuing-and-verifying-eip-712-challenges-with-go-32635ca78aaf.
-func digestEIP712(address string, message []byte, nonce int) ([32]byte, error) {
+// eip712SignerData returns an EIP712 structured data containing the signing domain and the required types.
+func eip712SignerData(chainID int64) apitypes.TypedData {


Why not replace multiple functions with just a single function:

// digestEIP712 returns an EIP712 structured data containing the signing domain and the required types. func digestEIP712(primaryType, fieldName, fieldValue string, chainID int64) ([]byte, error) { data := apitypes.TypedData{ Types: apitypes.Types{ "EIP712Domain": []apitypes.Type{ {Name: "name", Type: "string"}, {Name: "version", Type: "string"}, {Name: "chainId", Type: "uint256"}, }, primaryType: []apitypes.Type{ {Name: fieldName, Type: "string"}, }, }, PrimaryType: primaryType, Message: apitypes.TypedDataMessage{ fieldName: fieldValue, }, Domain: apitypes.TypedDataDomain{ Name: "Obol", Version: "1", ChainId: ethmath.NewHexOrDecimal256(chainID), }, } resp, _ , err := apitypes.TypedDataAndHash(data) return resp, err }

cluster/helpers.go

corverroos · 2022-10-02T06:19:40Z

cluster/helpers_internal_test.go

@@ -68,3 +68,16 @@ func TestVerifySig(t *testing.T) {
 		require.True(t, ok)
 	})
 }
+
+func TestForkVersionToChainID(t *testing.T) {
+	gnosisForkVersion := []byte{0, 0, 0, 100}


use sepolia or make this a constant?

corverroos · 2022-10-02T06:20:01Z

cluster/helpers.go

+		return 100, nil
+	case "0x80000069": // Ropsten
+		return 3, nil
+	case "0x90000069": // Sepolia


I suggest make these all constants

corverroos · 2022-10-02T06:23:21Z

cluster/helpers.go

+	// Fork version of sepolia testnet.
+	sepoliaForkVersion = "0x90000069"
+	// EIP712 values.
+	configHashPrimaryType = "ConfigHash"


suggest identical prefixes:

eip712TypeConfigHash = "ConfigHash" eip712FieldConfigHash = "config_hash" eip712TypeENR = "ENR" eip712FieldENR = "enr"

another option would be to make this a type, this couples the two things better:

type eip712Type struct { PrimaryType string Field string } var ( eip712TypeConfigHash = eip712Type {"ConfigHash","config_hash} eip712TypeENR = eip712Type {"ENR","enr} ) func signEIP712(secret *ecdsa.PrivateKey, typ eip712Type, fieldValue string, chainID int64) ([]byte, error) {

corverroos · 2022-10-02T06:27:33Z

cluster/cluster_test.go

@@ -44,7 +44,7 @@ func TestEncode(t *testing.T) {
 				3,
 				testutil.RandomETHAddress(),
 				testutil.RandomETHAddress(),
-				"0x00000002",
+				"0x90000069", // Sepolia testnet.


use the constant

will raise a PR to combine network chainID, name and forkVersion and replace relevant constants across the codebase

corverroos suggested changes Sep 26, 2022

View reviewed changes

corverroos approved these changes Sep 30, 2022

View reviewed changes

xenowits added 4 commits September 30, 2022 15:55

refactoring done and updated tests

8829843

cleanup

2f49467

add tests

ed19257

refactoring done

a34724b

xenowits force-pushed the xenowits/fix-eip712-domain branch from 5d7463d to a34724b Compare September 30, 2022 10:25

add sepolia

c5ddc66

xenowits commented Sep 30, 2022

View reviewed changes

corverroos reviewed Sep 30, 2022

View reviewed changes

corverroos suggested changes Oct 1, 2022

View reviewed changes

cleanup

3df706c

corverroos approved these changes Oct 2, 2022

View reviewed changes

corverroos reviewed Oct 2, 2022

View reviewed changes

xenowits added 2 commits October 3, 2022 19:36

cleanup

576f5ca

cleanup

df59ede

xenowits self-assigned this Oct 3, 2022

xenowits added the merge when ready Indicates bulldozer bot may merge when all checks pass label Oct 3, 2022

obol-bulldozer bot merged commit 9486b6d into main Oct 3, 2022

obol-bulldozer bot deleted the xenowits/fix-eip712-domain branch October 3, 2022 15:15

xenowits mentioned this pull request Oct 4, 2022

Support dynamic chainID in EIP712 sigs #1201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: refactor EIP712 domain data #1169

cluster: refactor EIP712 domain data #1169

xenowits commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

xenowits commented Sep 26, 2022

corverroos left a comment

corverroos Sep 30, 2022

corverroos Sep 30, 2022

xenowits Sep 30, 2022

corverroos Sep 30, 2022 •

edited

Loading

corverroos Sep 30, 2022

corverroos Oct 1, 2022

corverroos Oct 2, 2022

corverroos Oct 2, 2022

corverroos Oct 2, 2022

corverroos Oct 2, 2022 •

edited

Loading

corverroos Oct 2, 2022

xenowits Oct 3, 2022

cluster: refactor EIP712 domain data #1169

cluster: refactor EIP712 domain data #1169

Conversation

xenowits commented Sep 26, 2022 • edited Loading

codecov bot commented Sep 26, 2022 • edited Loading

Codecov Report

xenowits commented Sep 26, 2022

corverroos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos Oct 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenowits commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

corverroos Sep 30, 2022 •

edited

Loading

corverroos Oct 2, 2022 •

edited

Loading