feat(params): `UnmarshalChainJSONConfig` and `MarshalChainJSONConfig` #92

qdm12 · 2024-12-17T18:16:49Z

Why this should be merged

Allow wallet and other clients to parse blocks from both coreth & subnet-evm, and also make their genesis/chain-configs.

How this works

🕐 Waiting on feedback from Arr4n to make sure this is how it's supposed to operate before updating the function comment and associated test(s)

UnmarshalChainConfig is a standalone function func UnmarshalChainConfigJSON[T any](data []byte, config *ChainConfig, extra *T) (err error) taking as input arguments:

data []byte the json data to decode
config *ChainConfig the chain config pointer to decode to, except the json "extra" key, if non-nil
extra *T the extra destination pointer to decode the "extra" key object to, if non-nil

How this was tested

New unit test Test_UnmarshalChainConfig

params/json.libevm.go

ARR4N · 2024-12-18T09:53:52Z

params/json.libevm_test.go

@@ -144,3 +145,60 @@ func TestChainConfigJSONRoundTrip(t *testing.T) {
 		})
 	}
 }
+
+func Test_UnmarshalChainConfig(t *testing.T) {


Please only include _ in identifiers when it's required by a general convention; the only instance I can think of off the top of my head is the godoc parsing of testable examples.

The _ right after Test can be removed indeed. However, if we test for example func (o *Object) myFunc() naming the test as TestObject_myFunc is better than TestObjectmyFunc readability-wise, would you agree or have another suggestion? 🤔

ARR4N · 2024-12-18T09:55:15Z

params/json.libevm_test.go

@@ -144,3 +145,60 @@ func TestChainConfigJSONRoundTrip(t *testing.T) {
 		})
 	}
 }
+
+func Test_UnmarshalChainConfig(t *testing.T) {


I suspect that the existing test cases will be sufficient if you refactor as I recommended above. That will also give us a greater degree of confidence between the situations where extras are explicitly registered vs merely passed to the function.

They are for the happy path for both the marshal and unmarshal functions. However, I prefer to have unit tests for each marshal and umarshal functions, on top of the existing test, to test unhappy paths/corner cases, especially given the any/generic thingies which are prone to error.

params/json.libevm.go

params/json.libevm_test.go

params/json.libevm.go

…raNotRegistered` and `unmarshalChainConfigJSONExtraRegistered`

…thing and remove functions

params/json.libevm.go

qdm12 · 2024-12-20T14:36:08Z

params/json.libevm.go

+	switch {
+	case err != nil:
+		return fmt.Errorf("decoding root chain config: %s", err)
+	case extra == nil: // ignore the extra, whether at the root JSON depth, or at the "extra" JSON field.


do we want to support this case where the user pass in a nil extra? 🤔 Or not really? Thinking about the case an extra set to nil is passed and we silently not decode the extra content

I changed to return an error extra pointer argument is nil because:

I don't think there is a point using this function at all if the extra argument is a (typed) nil

Discarding silently the extra field sounds like a dangerous path, better to return an error.

No, we don't want to support nil extras because that is never a valid use case. In the extras-registered scenario it will never be nil (by design), and in the non-registered scenario the caller should just use the regular JSON decoding mechanism of either json.Decode([]byte, *ChainConfig) or ChainConfig.UnmarshalJSON().

Yes I think returning an error in case the extra is nil makes sense in that case then.

…ace arguments

darioush

nice, let's add MarshalJSON, we can do it in this or a separate PR

params/json.libevm_test.go

darioush · 2024-12-20T16:13:11Z

params/json.libevm_test.go

+
+	testCases := map[string]struct {
+		jsonData       string // string for convenience
+		extra          any


nit: seems this is always set to &testExtra{} so we can remove it from the table struct and have the test instantiate it.
or if it is to keep things generic, there could be newExtra: func() any { return &testExtra{} }

I changed it a bit to be extra any to be able to throw different things at it.

ARR4N · 2024-12-23T10:19:35Z

params/json.libevm.go

 		return json.Unmarshal(data, (*chainConfigWithoutMethods)(c))
 	}
+	extraConstructors := registeredExtras.Get()


(style) Please don't use such verbose variable names when the scope is only 3 lines; they distract from reading of the code. I'd personally be happy with just r, but understand that others are not—as long as it's not 5 syllables.

See style guide

I've read a few style guides including that one, my view on it, unless it's a really obvious use (i.e. i for a range index), it's nice to have a meaningful name almost always. For example c instead of extraConstructors made me wonder what was c and had to lookup where it was created and what .Get() was returning, and, because it's a generic function, had to dig into extraConstructors struct. Not a big deal obviously, but it does make readability worst in my opinion.

Obviously that's my opinion on this, so I'll conform to the existing code and use shorter variable names 😉

EDIT: also one more reason is I do like to use a single letter for the receiving struct for methods, so using single letter variables in there makes things confusing imo

EDIT2: my brain is kind of very formed (malformed or ok-formed, debatable 😄) to use explicit non-single-letter variable names, so feel free to hammer with style comments in case you feel like it should be a smaller variable name. I got the extraConstructors -> ec but after going through the rest of the code I wrote, I'm feeling like:

params/json.libevm.go

ARR4N · 2024-12-23T10:27:18Z

params/json.libevm.go

+	switch {
+	case err != nil:
+		return fmt.Errorf("decoding root chain config: %s", err)
+	case extra == nil: // ignore the extra, whether at the root JSON depth, or at the "extra" JSON field.


No, we don't want to support nil extras because that is never a valid use case. In the extras-registered scenario it will never be nil (by design), and in the non-registered scenario the caller should just use the regular JSON decoding mechanism of either json.Decode([]byte, *ChainConfig) or ChainConfig.UnmarshalJSON().

ARR4N · 2024-12-23T10:28:33Z

params/json.libevm.go

+	case extra == nil: // ignore the extra, whether at the root JSON depth, or at the "extra" JSON field.
+	case reuseJSONRoot:
+		err = json.Unmarshal(data, extra)
+		if err != nil {


(style) When a variable is scoped to a block, please scope it as such where possible, e.g. by initialisation within a if or switch statement.

if err := json.Unmarshal(...); err != nil {

In my opinion, the short if forms makes it harder to read, but maybe that's just me eyes acting strange 😄
Regarding the scoping, we could change this to

err := json.Unmarshal(data, extra) if err != nil {

? 🤔

ARR4N · 2024-12-23T10:34:45Z

params/json.libevm.go

+//     `data` is decoded into `config` and the extra is ignored.
+func UnmarshalChainConfigJSON[T any](data []byte, config *ChainConfig, extra *T, reuseJSONRoot bool) (err error) {
+	err = json.Unmarshal(data, (*chainConfigWithoutMethods)(config))
+	switch {


Why is a switch needed? Either we are reusing the JSON root or we're not, and each of those may or may not return an error.

See my comment below about double unmarshalling into the extra field when not reusing the root, which may have influenced this.

The double unmarshaling is fixed, and switch converted to 2 ifs which are indeed perhaps clearer 😉

ARR4N · 2024-12-23T10:39:09Z

params/json.libevm_test.go

-				require.NoError(t, json.Unmarshal([]byte(tt.jsonInput), got))
-				require.Equal(t, tt.want, got)
-			})
+			expectedEncoded := bytes.NewBuffer(nil)


Why is this change to the original test needed?

I was just going over the existing test, and was wondering why we had subtests where it seems to me this makes it more convoluted? Maybe I missed something though. I can revert it of course, no worry.

ARR4N · 2024-12-23T10:41:06Z

params/json.libevm_test.go

+		"invalid_json": {
+			extra:         &testExtra{},
+			expectedExtra: &testExtra{},
+			errMessage:    "decoding root chain config: unexpected end of JSON input",


This is a change-detector test, which results in having to change tests when implementations change.

See: https://google.github.io/styleguide/go/decisions#test-error-semantics

If you want to confirm precise error-spawning pathways then please either compare against a set of error variables (probably most appropriate here as along as they're not exported) or, in more complex situations, with specific error types.

Indeed, I'm aware 😄 I do test error messages, since I have seen many error messages that were unclear and/or repeating themselves, and I found testing error strings do reduce a lot of this from happening.

I don't think testing error variables (except nil vs non nil) help really, especially since we are focusing on "human readable" and not programmatically handled errors - we don't wrap errors with %w. But I can change this to check for nil vs non-nil only.

ARR4N · 2024-12-23T10:45:55Z

params/json.libevm_test.go

+	}
+}
+
+func TestUnmarshalChainConfigJSON(t *testing.T) {


What cases aren't covered by the original tests? (Other than errors, that is)

Can the non-erroring cases just be added to the table above?

Re (1); while coverage isn't a perfect metric, I'm seeing full happy-path coverage on your branch with just the originals.

See #92 (comment) 😉

ARR4N · 2024-12-23T10:50:17Z

params/json.libevm_test.go

+		jsonData       string // string for convenience
+		extra          *testExtra
+		reuseJSONRoot  bool
+		expectedConfig ChainConfig


The rest of libevm uses "want" instead of "expected" and "got" instead of "actual".

Changed expected prefixes to want prefixes.
However - ha I want shorter variable names perhaps - regarding actual (and got), I think i.e. data, ok := MarshalXYZ in a test is better than actualData, actualOk := MarshalXYZ and there is no need for an actual prefix really? And if it's only one variable being checked, why not name it to what it actually is i.e. data instead of actual

qdm12 force-pushed the qdm12/params/unmarshal-config branch from 32afb0b to a41a78d Compare December 17, 2024 18:20

ARR4N requested changes Dec 18, 2024

View reviewed changes

qdm12 commented Dec 19, 2024

View reviewed changes

params/json.libevm.go Outdated Show resolved Hide resolved

params/json.libevm.go Outdated Show resolved Hide resolved

params/json.libevm_test.go Outdated Show resolved Hide resolved

qdm12 requested a review from ARR4N December 19, 2024 12:55

qdm12 marked this pull request as ready for review December 19, 2024 13:50

darioush reviewed Dec 19, 2024

View reviewed changes

params/json.libevm.go Outdated Show resolved Hide resolved

qdm12 added 16 commits December 20, 2024 12:16

feat(params): UnmarshalChainConfig function

97c5acf

UnmarshalChainConfig -> UnmarshalChainConfigJSON

a0ac003

Add extra field type to extra json decoding error

d6ef5d5

Do not wrap error and use %s instead of %w

d30ebd0

rework UnmarshalChainConfigJSON

de4b33a

update function comment

9fd0ecc

rework UnmarshalChainConfigJSON

0c8c1a0

reuseJSONRoot argument

59f755f

inline body of unmarshalJSONWithRegisteredExtra and remove it

3221420

use switch for shorter code

8f919e6

chore: simplify code

e14575d

move UnmarshalChainConfigJSON below UnmarshalJSON chainConfig method

a1d3f16

chore: split function into two functions `unmarshalChainConfigJSONExt…

8e96caa

…raNotRegistered` and `unmarshalChainConfigJSONExtraRegistered`

Update comment

b52253b

Test fix: .extra -> .Extra

dff2709

chore: merge encode/decode subtests in one

4586102

qdm12 force-pushed the qdm12/params/unmarshal-config branch 2 times, most recently from 06c7610 to a6ebb69 Compare December 20, 2024 11:41

qdm12 added 3 commits December 20, 2024 13:37

update function comment

8f41a39

Update TestUnmarshalChainConfigJSON

9807627

only use body of unmarshalChainConfigJSONExtraNotRegistered for every…

a46252d

…thing and remove functions

qdm12 force-pushed the qdm12/params/unmarshal-config branch from a6ebb69 to a46252d Compare December 20, 2024 14:29

qdm12 commented Dec 20, 2024

View reviewed changes

params/json.libevm.go Show resolved Hide resolved

qdm12 commented Dec 20, 2024

View reviewed changes

qdm12 added 2 commits December 20, 2024 15:43

Add test cases to TestUnmarshalChainConfigJSON

30ea0f4

Make UnmarshalChainConfigJSON non generic to support extra interf…

542c575

…ace arguments

darioush approved these changes Dec 20, 2024

View reviewed changes

qdm12 added 2 commits December 20, 2024 17:29

Revert function to be generic

d6cb53d

Return an error if extra argument is nil

51eb2c5

ARR4N reviewed Dec 23, 2024

View reviewed changes

qdm12 added 7 commits December 23, 2024 12:45

MarshalChainConfigJSON with test

9b18c99

Update ChainConfig MarshalJSON comment

5753371

Remove unneeded comments

59804b7

style: inline variables

73092a8

docs: fix typo in comment

d4e3d85

hotfix: fix TestUnmarshalChainConfigJSON

3c2a6c4

UnmarshalChainConfigJSON decodes only once for re-use root case

2172aa7

qdm12 force-pushed the qdm12/params/unmarshal-config branch from da45de2 to 2172aa7 Compare December 23, 2024 12:21

qdm12 added 2 commits December 23, 2024 13:22

Use want* for test case fields

ae7198b

style: UnmarshalChainConfigJSON does not use switch anymore

eb31aae

qdm12 changed the title ~~feat(params): UnmarshalChainConfig function~~ feat(params): UnmarshalChainJSONConfig and MarshalChainJSONConfig Dec 23, 2024

extraConstructors -> ec

a050836

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(params): `UnmarshalChainJSONConfig` and `MarshalChainJSONConfig` #92

feat(params): `UnmarshalChainJSONConfig` and `MarshalChainJSONConfig` #92

qdm12 commented Dec 17, 2024 •

edited

Loading

ARR4N Dec 18, 2024

qdm12 Dec 19, 2024

ARR4N Dec 18, 2024

qdm12 Dec 23, 2024

qdm12 Dec 20, 2024 •

edited

Loading

qdm12 Dec 23, 2024 •

edited

Loading

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

darioush left a comment

darioush Dec 20, 2024

qdm12 Dec 20, 2024 •

edited

Loading

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024 •

edited

Loading

ARR4N Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024

ARR4N Dec 23, 2024

qdm12 Dec 23, 2024 •

edited

Loading

feat(params): UnmarshalChainJSONConfig and MarshalChainJSONConfig #92

Are you sure you want to change the base?

feat(params): UnmarshalChainJSONConfig and MarshalChainJSONConfig #92

Conversation

qdm12 commented Dec 17, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

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

qdm12 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

qdm12 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Dec 23, 2024 • 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

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

qdm12 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

feat(params): `UnmarshalChainJSONConfig` and `MarshalChainJSONConfig` #92

feat(params): `UnmarshalChainJSONConfig` and `MarshalChainJSONConfig` #92

qdm12 commented Dec 17, 2024 •

edited

Loading

qdm12 Dec 20, 2024 •

edited

Loading

qdm12 Dec 23, 2024 •

edited

Loading

qdm12 Dec 20, 2024 •

edited

Loading

qdm12 Dec 23, 2024 •

edited

Loading

qdm12 Dec 23, 2024 •

edited

Loading