eth2util/deposit: refactor deposit data and add tests #521

xenowits · 2022-05-13T06:50:14Z

refactor deposit data to use types from eth2p0 (ex).
Test generated deposit data against a sample output generated using eth2 cli.

category: refactor
ticket: #481

codecov · 2022-05-13T06:51:33Z

Codecov Report

Merging #521 (34ab1c5) into main (7254daa) will increase coverage by 0.05%.
The diff coverage is 56.60%.

@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
+ Coverage   54.77%   54.82%   +0.05%     
==========================================
  Files          91       92       +1     
  Lines        8480     8574      +94     
==========================================
+ Hits         4645     4701      +56     
- Misses       3226     3248      +22     
- Partials      609      625      +16

Impacted Files	Coverage Δ
eth2util/deposit/deposit.go	`56.60% <56.60%> (ø)`
app/app.go	`63.31% <0.00%> (-0.89%)`	⬇️
core/qbft/qbft.go	`84.90% <0.00%> (-0.81%)`	⬇️
testutil/genchangelog/main.go	`15.38% <0.00%> (+0.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7254daa...34ab1c5. Read the comment docs.

corverroos · 2022-05-13T07:43:34Z

eth2util/deposit/testdata/sd_cli_keystore_json.golden

@@ -0,0 +1,34 @@
+{


store the privatekey as in the test code directly:

const privkey = "acb123adc4212367"

corverroos · 2022-05-13T07:44:04Z

eth2util/keystore/keystore.go

-// keystore json file representation as a Go struct.
-type keystore struct {
+// Keystore json file representation as a Go struct.
+type Keystore struct {


Best not refactor other packages just for testing.

it is better code and it is already done and very short. In general I agree of not getting into other stuff outside the PR goal

corverroos · 2022-05-13T07:46:58Z

eth2util/deposit/deposit_data_test.go

+	"testing"
+)
+
+func TestDepositData1Serialization(t *testing.T) {


As single test should be enough:

root, err := deposit.MessageRoot(...) sig := testSign(root) resp, err := deposit.MarshalDepositData(..., sig) testutil.RequireGoldenBytes(t, resp)

leolara · 2022-05-13T10:22:52Z

@xenowits I don't undestand why there is a feature flag called eth2util? is it eth2util a feature? the name does not suggest a feature

corverroos · 2022-05-13T11:24:20Z

Please rebase this on latest main 🙏

corverroos · 2022-05-13T11:24:36Z

.pre-commit-config.yaml

@@ -1,7 +1,7 @@
 repos:
  # First run code formatters
  - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.1.0
+    rev: v4.2.0


please move this a separate PR.

xenowits · 2022-05-13T15:49:44Z

@xenowits I don't undestand why there is a feature flag called eth2util? is it eth2util a feature? the name does not suggest a feature

I removed the feature_flag as it was not necessary.

corverroos · 2022-05-13T15:45:27Z

eth2util/deposit/deposit_data.go

+)
+
+// GetMessageRoot returns both the hash root and the signing root of the deposit message.
+func GetMessageRoot(pubkey eth2p0.BLSPubKey, withdrawalCreds [32]byte, forkVersion eth2p0.Version) (eth2p0.Root, eth2p0.Root, error) {


when returning identical types, it is best to use named parameters so it is clear which one is which.

Wouldn't a withdrawalAddr string be a more convenient format for users of this package?

| when returning identical types, it is best to use named parameters so it is clear which one is which.

very good!! let's make code self documenting, we should name return values unless they are super obvious

changed this to return only the hash root

corverroos · 2022-05-13T15:47:15Z

eth2util/deposit/deposit_data.go

-	idx := hh.Index()
+// NewDepositData returns the json serialized DepositData.
+func NewDepositData(pubkey eth2p0.BLSPubKey, withdrawalAddr string, sig eth2p0.BLSSignature, network string) ([]byte, error) {
+	forkVersion := NetworkToForkVersion(network)


I would unexport all functions and types, except MarshalDepositData and GetMessageRoot

rename NewDepositData to MarshalDepositData

Yes, I agree New should be reserved for constructors.

let's as you say unexport as default, and export only things that we need outside

corverroos · 2022-05-13T15:50:54Z

eth2util/deposit/deposit_data.go

-	// DepositMessageRoot is the hash tree root of DepositMessage.
-	DepositMessageRoot eth2p0.Root
+	// DOMAIN_DEPOSIT. See spec: https://benjaminion.xyz/eth2-annotated-spec/phase0/beacon-chain/#domain-types
+	depositDomainType = []byte{0x03, 0x00, 0x00, 0x00}


wrap in proper type = eth2p0.DomainType([4]byte{0x03, 0x00, 0x00, 0x00})

same for zeroBytes32

there isn't a proper type for zeroBytes32

corverroos · 2022-05-13T15:52:30Z

eth2util/deposit/deposit_data.go

+	}
+	root, err := forkData.HashTreeRoot()
+	if err != nil {
+		return eth2p0.Domain{}, errors.Wrap(err, "failed to calculate signature domain")


nit: can be more concise with error messages: hash fork data

corverroos · 2022-05-13T15:54:24Z

eth2util/deposit/deposit_data_test.go

+)
+
+func TestDepositData(t *testing.T) {
+	depositData, err := DepositDataFromFile(t, depositDataFile)


I do not think you need to ever read deposit data from file, just calculate the expected value and do golden file comparison.

corverroos · 2022-05-13T15:59:00Z

eth2util/deposit/deposit_data.go

+	case "mainnet":
+		fvBytes = []byte{0x00, 0x00, 0x00, 0x00}
+	case "prater":
+		fvBytes = []byte{0x00, 0x00, 0x10, 0x20}


can return proper types directly: return eth2p0.Version([4]byte{0x00, 0x00, 0x10, 0x20}

corverroos · 2022-05-13T16:01:19Z

eth2util/deposit/deposit_data.go

-func (d depositData) HashTreeRoot() ([32]byte, error) {
-	b, err := ssz.HashWithDefaultHasher(d)
+// GetDataRoot returns both the hash root and the signing root of the deposit data.
+func GetDataRoot(pubkey eth2p0.BLSPubKey, withdrawalCreds [32]byte, sig eth2p0.BLSSignature, forkVersion eth2p0.Version) (eth2p0.Root, eth2p0.Root, error) {


only one of the two responses is ever used, so suggest only retuning that one thing

yes corrected that

corverroos · 2022-05-14T07:13:34Z

eth2util/deposit/deposit.go

@@ -13,6 +13,7 @@
 // You should have received a copy of the GNU General Public License along with
 // this program.  If not, see <http://www.gnu.org/licenses/>.

+// Package deposit provides functions to create deposit data files.


package level godoc is important

corverroos · 2022-05-14T07:13:51Z

eth2util/deposit/deposit.go

-	// zeroes11 refers to a zeroed out 11 byte array.
-	zeroBytes11 = []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
-
-	// zeroes refers to a zeroed out 32 byte array.
-	zeroBytes32 = []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}


zero types not required

corverroos · 2022-05-14T07:14:01Z

eth2util/deposit/deposit.go

+// getMessageRoot returns a deposit message hash root created by the parameters.
+func getMessageRoot(pubkey eth2p0.BLSPubKey, withdrawalAddr string) (eth2p0.Root, error) {


made it private

corverroos · 2022-05-14T07:14:41Z

eth2util/deposit/deposit.go

 	creds, err := withdrawalCredsFromAddr(withdrawalAddr)
 	if err != nil {
-		return eth2p0.Root{}, errors.Wrap(err, "withdrawal credentials")


only wrap external packages, no need to wrap Charon packages or Charon functions (since the errors have proper stack traces)

corverroos · 2022-05-14T07:15:14Z

eth2util/deposit/deposit.go

@@ -66,101 +60,87 @@ func GetMessageRoot(pubkey eth2p0.BLSPubKey, withdrawalAddr string) (eth2p0.Root
 	return hashRoot, nil
 }

-// GetDataRoot returns the hash root of the deposit data.
-func GetDataRoot(pubkey eth2p0.BLSPubKey, withdrawalAddr string, sig eth2p0.BLSSignature) (eth2p0.Root, error) {


this function doesn't do much, so I just inlined it

corverroos · 2022-05-14T07:16:34Z

eth2util/deposit/deposit.go

-func GetSigningRoot(forkVersion eth2p0.Version, root eth2p0.Root) ([32]byte, error) {
-	domain, err := GetDomain(forkVersion, depositDomainType)
+// GetMessageSigningRoot returns the deposit message signing root created by the provided parameters.
+func GetMessageSigningRoot(pubkey eth2p0.BLSPubKey, withdrawalAddr string, network string) ([32]byte, error) {


this is the new "Step1" function of this package

corverroos · 2022-05-14T07:17:12Z

eth2util/deposit/deposit.go

+	var creds [32]byte
+	copy(creds[0:], eth1AddressWithdrawalPrefix) // Add 1 byte prefix.
+	copy(creds[12:], addrBytes)                  // Add 20 bytes of ethereum address suffix.


more succinct to work directly with array, then zero11 also not required.

yes, since the default zero value of a byte is 0

corverroos · 2022-05-14T07:17:37Z

eth2util/deposit/deposit.go

@@ -207,6 +174,8 @@ func networkToForkVersion(network string) eth2p0.Version {
 		return [4]byte{0x70, 0x00, 0x00, 0x69}
 	case "gnosis":
 		return [4]byte{0x00, 0x00, 0x00, 0x64}
+	case "mainnet": // Default to mainnet
+		fallthrough
 	default:
 		return [4]byte{0x00, 0x00, 0x00, 0x00}


it isn't obviously that this is mainnet

oh i get, this is the first usecase i have seen where fallthrough makes sense. thanks for pointing this!

corverroos · 2022-05-14T07:18:41Z

eth2util/deposit/deposit_test.go

-	msgRoot, err := deposit.GetMessageRoot(pubkey, withdrawalAddr)
-	require.NoError(t, err)
-
-	forkVersion := eth2p0.Version([4]byte{0x00, 0x00, 0x10, 0x20})
-	msgSigningRoot, err := deposit.GetSigningRoot(forkVersion, msgRoot)


now this is just a single step. Also aligned use of network between both functions. Also aligned function signatures

corverroos · 2022-05-14T07:19:11Z

eth2util/deposit/deposit_test.go

 	require.NoError(t, err)

-	testutil.RequireGoldenBytes(t, serializedDepositData)


golden files allow updating, this is not applicable in this usecase

got it. So, for test data files like this which are static, golden files are not required👍

yeah, golden files are generated, in this case the test file is a static external test file

xenowits marked this pull request as draft May 13, 2022 06:50

xenowits force-pushed the xenowits/refactor-deposit-data branch from 93dd5b6 to 5175627 Compare May 13, 2022 07:02

corverroos reviewed May 13, 2022

View reviewed changes

refactor deposit data code and add tests

a16e2c3

xenowits force-pushed the xenowits/refactor-deposit-data branch from 6cce1c8 to a16e2c3 Compare May 13, 2022 15:23

change test package to deposit_test

d16d941

xenowits marked this pull request as ready for review May 13, 2022 15:43

corverroos suggested changes May 13, 2022

View reviewed changes

corverroos reviewed May 13, 2022

View reviewed changes

address review comments

d7d4e31

xenowits linked an issue May 13, 2022 that may be closed by this pull request

dkg: generate deposit_data #481

Closed

clean

34ab1c5

corverroos reviewed May 14, 2022

View reviewed changes

corverroos approved these changes May 14, 2022

View reviewed changes

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

obol-bulldozer bot merged commit c9f8779 into main May 16, 2022

obol-bulldozer bot deleted the xenowits/refactor-deposit-data branch May 16, 2022 04:38

		// getMessageRoot returns a deposit message hash root created by the parameters.
		func getMessageRoot(pubkey eth2p0.BLSPubKey, withdrawalAddr string) (eth2p0.Root, error) {

		require.NoError(t, err)

		testutil.RequireGoldenBytes(t, serializedDepositData)

eth2util/deposit: refactor deposit data and add tests #521

eth2util/deposit: refactor deposit data and add tests #521

Conversation

xenowits commented May 13, 2022 • edited Loading

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

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

leolara commented May 13, 2022

corverroos commented May 13, 2022

Choose a reason for hiding this comment

xenowits commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenowits May 13, 2022 • edited Loading

Choose a reason for hiding this comment

corverroos May 13, 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 May 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos May 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenowits May 14, 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 May 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenowits commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

xenowits May 13, 2022 •

edited

Loading

corverroos May 13, 2022 •

edited

Loading

corverroos May 13, 2022 •

edited

Loading

corverroos May 14, 2022 •

edited

Loading

xenowits May 14, 2022 •

edited

Loading

xenowits May 14, 2022 •

edited

Loading