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

feat: params.ChainConfig extra payload can use root JSON #8

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Sep 10, 2024

Why this should be merged

Adds support for backwards compatibility with JSON format of ava-labs/{subnet-evm,coreth}/params.ChainConfig.

How this works

The params.Extras.ReuseJSONRoot bool flag is added to allow the end user to signal the need for backwards compatibility. This isn't the default because of the need for multi-pass (un)marshalling.

For extra payload:

type ChainConfigExtra struct {
  Foo string `json:"foo"`
}
//...
params.RegisterExtras(Extras[ChainConfigExtra,...]{
  ReuseJSONRoot: ...
})

ReuseJSONRoot == false =>

{
  "chainId": 42,
  "extra": {"foo": "bar"}
}

ReuseJSONRoot == true =>

{
  "chainId": 42,
  "foo": "bar"
}

How this was tested

Unit tests for JSON (un)marshalling for all cases; i.e when:

  1. No registered extras: plain JSON (un)marshalling;
  2. Flag not set: as before this PR, "promoting" the extra field to be exported with the json: "extra" tag; and
  3. Flag set: feature added in this PR.

@ARR4N ARR4N marked this pull request as ready for review September 10, 2024 18:36
@ARR4N ARR4N requested review from darioush and ceyonur September 10, 2024 18:37
params/json.libevm_test.go Outdated Show resolved Hide resolved
}

// MarshalJSON implements the [json.Marshaler] interface.
func (c *ChainConfig) MarshalJSON() ([]byte, error) {
Copy link
Collaborator

@darioush darioush Sep 10, 2024

Choose a reason for hiding this comment

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

I noticed in a couple cases that MarshalJSON() being defined on the value vs. pointer makes a difference.
Since it is only a couple instances so far I switched the uses to & to move on, but perhaps this should be defined on the value to avoid invoking the default MarshalJSON when the receiver is non-pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting! Do you remember where this was, so I can take a look before I merge this? I defined it on the pointer because that's what tends to be carried around, e.g. in vm.EVM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to leave it as defined on the pointer for now. In the geth code base there are only two instances where a non-pointer ChainConfig is used (note that most of the results are either new(ChainConfig) or immediately address the value with &).

This is in contrast to the 65 files with a *ChainConfig.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The *ChainConfig instances will still work if the receiver is value. We can leave it as is for now.

@ARR4N ARR4N added the Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state label Sep 10, 2024
@ARR4N
Copy link
Collaborator Author

ARR4N commented Sep 11, 2024

Sorry about pushing the refactor in a9250c6 after your review; I didn't realise that you'd already approved. The code is functionally identical and much simpler, plus there are no changes to the tests so I'll continue to merge it.

@ARR4N ARR4N removed the Status: 🔴 DO NOT MERGE This PR is not meant to be merged in its current state label Sep 11, 2024
@ARR4N ARR4N merged commit d9991bb into libevm Sep 11, 2024
1 check passed
@ARR4N ARR4N deleted the arr4n/json-root branch September 11, 2024 10:27
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