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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 15 additions & 41 deletions params/config.libevm.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package params

import (
"encoding/json"
"fmt"
"math/big"
"reflect"
Expand All @@ -14,6 +13,15 @@ import (
// Extras are arbitrary payloads to be added as extra fields in [ChainConfig]
// and [Rules] structs. See [RegisterExtras].
type Extras[C ChainConfigHooks, R RulesHooks] struct {
// ReuseJSONRoot, if true, signals that JSON unmarshalling of a
// [ChainConfig] MUST reuse the root JSON input when unmarshalling the extra
// payload. If false, it is assumed that the extra JSON payload is nested in
// the "extra" key.
//
// *NOTE* this requires multiple passes for both marshalling and
// unmarshalling of JSON so is inefficient and should be used as a last
// resort.
ReuseJSONRoot bool
// NewRules, if non-nil is called at the end of [ChainConfig.Rules] with the
// newly created [Rules] and other context from the method call. Its
// returned value will be the extra payload of the [Rules]. If NewRules is
Expand Down Expand Up @@ -51,10 +59,11 @@ func RegisterExtras[C ChainConfigHooks, R RulesHooks](e Extras[C, R]) ExtraPaylo

getter := e.getter()
registeredExtras = &extraConstructors{
chainConfig: pseudo.NewConstructor[C](),
rules: pseudo.NewConstructor[R](),
newForRules: e.newForRules,
getter: getter,
chainConfig: pseudo.NewConstructor[C](),
rules: pseudo.NewConstructor[R](),
reuseJSONRoot: e.ReuseJSONRoot,
newForRules: e.newForRules,
getter: getter,
}
return getter
}
Expand Down Expand Up @@ -87,6 +96,7 @@ var registeredExtras *extraConstructors

type extraConstructors struct {
chainConfig, rules pseudo.Constructor
reuseJSONRoot bool
newForRules func(_ *ChainConfig, _ *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) *pseudo.Type
// use top-level hooksFrom<X>() functions instead of these as they handle
// instances where no [Extras] were registered.
Expand Down Expand Up @@ -158,42 +168,6 @@ func (e ExtraPayloadGetter[C, R]) hooksFromRules(r *Rules) RulesHooks {
return NOOPHooks{}
}

// UnmarshalJSON implements the [json.Unmarshaler] interface.
func (c *ChainConfig) UnmarshalJSON(data []byte) error {
type raw ChainConfig // doesn't inherit methods so avoids recursing back here (infinitely)
cc := &struct {
*raw
Extra *pseudo.Type `json:"extra"`
}{
raw: (*raw)(c), // embedded to achieve regular JSON unmarshalling
}
if e := registeredExtras; e != nil {
cc.Extra = e.chainConfig.NilPointer() // `c.extra` is otherwise unexported
}

if err := json.Unmarshal(data, cc); err != nil {
return err
}
c.extra = cc.Extra
return nil
}

// MarshalJSON implements the [json.Marshaler] interface.
func (c *ChainConfig) MarshalJSON() ([]byte, error) {
// See UnmarshalJSON() for rationale.
type raw ChainConfig
cc := &struct {
*raw
Extra *pseudo.Type `json:"extra"`
}{raw: (*raw)(c), Extra: c.extra}
return json.Marshal(cc)
}

var _ interface {
json.Marshaler
json.Unmarshaler
} = (*ChainConfig)(nil)

// addRulesExtra is called at the end of [ChainConfig.Rules]; it exists to
// abstract the libevm-specific behaviour outside of original geth code.
func (c *ChainConfig) addRulesExtra(r *Rules, blockNum *big.Int, isMerge bool, timestamp uint64) {
Expand Down
2 changes: 0 additions & 2 deletions params/config.libevm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ func TestRegisterExtras(t *testing.T) {
require.NoError(t, json.Unmarshal(buf, got))
assert.Equal(t, tt.ccExtra.Interface(), got.extraPayload().Interface())
assert.Equal(t, in, got)
// TODO: do we need an explicit test of the JSON output, or is a
// Marshal-Unmarshal round trip sufficient?

gotRules := got.Rules(nil, false, 0)
assert.Equal(t, tt.wantRulesExtra, gotRules.extraPayload().Interface())
Expand Down
119 changes: 119 additions & 0 deletions params/json.libevm.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package params

import (
"encoding/json"
"fmt"

"github.com/ethereum/go-ethereum/libevm/pseudo"
)

var _ interface {
json.Marshaler
json.Unmarshaler
} = (*ChainConfig)(nil)

// chainConfigWithoutMethods avoids infinite recurion into
// [ChainConfig.UnmarshalJSON].
type chainConfigWithoutMethods ChainConfig

// chainConfigWithExportedExtra supports JSON (un)marshalling of a [ChainConfig]
// while exposing the `extra` field as the "extra" JSON key.
type chainConfigWithExportedExtra struct {
*chainConfigWithoutMethods // embedded to achieve regular JSON unmarshalling
Extra *pseudo.Type `json:"extra"` // `c.extra` is otherwise unexported
}

// UnmarshalJSON implements the [json.Unmarshaler] interface.
func (c *ChainConfig) UnmarshalJSON(data []byte) error {
extras := registeredExtras

if extras != nil && !extras.reuseJSONRoot {
return c.unmarshalJSONWithExtra(data)
}

if err := json.Unmarshal(data, (*chainConfigWithoutMethods)(c)); err != nil {
return err
}
if extras == nil {
return nil
}

// Invariants if here:
// - reg.reuseJSONRoot == true
// - Non-extra ChainConfig fields already unmarshalled

c.extra = extras.chainConfig.NilPointer()
if err := json.Unmarshal(data, c.extra); err != nil {
c.extra = nil
return err
}
return nil
}

// unmarshalJSONWithExtra unmarshals JSON under the assumption that the
// registered [Extras] payload is in the JSON "extra" key. All other
// unmarshalling is performed as if no [Extras] were registered.
func (c *ChainConfig) unmarshalJSONWithExtra(data []byte) error {
cc := &chainConfigWithExportedExtra{
chainConfigWithoutMethods: (*chainConfigWithoutMethods)(c),
Extra: registeredExtras.chainConfig.NilPointer(),
}
if err := json.Unmarshal(data, cc); err != nil {
return err
}
c.extra = cc.Extra
return nil
}

// 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.

switch extras := registeredExtras; {
case extras == nil:
return json.Marshal((*chainConfigWithoutMethods)(c))

case !extras.reuseJSONRoot:
return c.marshalJSONWithExtra()

default:
// The inverse of reusing the JSON root is merging two JSON buffers,
// which isn't supported by the native package. So we use
// map[string]json.RawMessage intermediates.
geth, err := toJSONRawMessages((*chainConfigWithoutMethods)(c))
if err != nil {
return nil, err
}
extra, err := toJSONRawMessages(c.extra)
if err != nil {
return nil, err
}

for k, v := range extra {
if _, ok := geth[k]; ok {
return nil, fmt.Errorf("duplicate JSON key %q in both %T and registered extra", k, c)
}
geth[k] = v
}
return json.Marshal(geth)
}
}

// marshalJSONWithExtra is the inverse of unmarshalJSONWithExtra().
func (c *ChainConfig) marshalJSONWithExtra() ([]byte, error) {
cc := &chainConfigWithExportedExtra{
chainConfigWithoutMethods: (*chainConfigWithoutMethods)(c),
Extra: c.extra,
}
return json.Marshal(cc)
}

func toJSONRawMessages(v any) (map[string]json.RawMessage, error) {
buf, err := json.Marshal(v)
if err != nil {
return nil, err
}
msgs := make(map[string]json.RawMessage)
if err := json.Unmarshal(buf, &msgs); err != nil {
return nil, err
}
return msgs, nil
}
99 changes: 99 additions & 0 deletions params/json.libevm_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package params

import (
"bytes"
"encoding/json"
"math/big"
"testing"

"github.com/ethereum/go-ethereum/libevm/pseudo"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

type nestedChainConfigExtra struct {
NestedFoo string `json:"foo"`

NOOPHooks
}

type rootJSONChainConfigExtra struct {
TopLevelFoo string `json:"foo"`

NOOPHooks
}

func TestChainConfigJSONRoundTrip(t *testing.T) {
tests := []struct {
name string
register func()
jsonInput string
want *ChainConfig
}{
{
name: "no registered extras",
register: func() {},
jsonInput: `{
"chainId": 1234
}`,
want: &ChainConfig{
ChainID: big.NewInt(1234),
},
},
{
name: "reuse top-level JSON",
register: func() {
RegisterExtras(Extras[rootJSONChainConfigExtra, NOOPHooks]{
ReuseJSONRoot: true,
})
},
jsonInput: `{
"chainId": 5678,
"foo": "hello"
}`,
want: &ChainConfig{
ChainID: big.NewInt(5678),
extra: pseudo.From(&rootJSONChainConfigExtra{TopLevelFoo: "hello"}).Type,
},
},
{
name: "nested JSON",
register: func() {
RegisterExtras(Extras[nestedChainConfigExtra, NOOPHooks]{
ReuseJSONRoot: false, // explicit zero value only for tests
})
},
jsonInput: `{
"chainId": 42,
"extra": {"foo": "world"}
}`,
want: &ChainConfig{
ChainID: big.NewInt(42),
extra: pseudo.From(&nestedChainConfigExtra{NestedFoo: "world"}).Type,
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
TestOnlyClearRegisteredExtras()
t.Cleanup(TestOnlyClearRegisteredExtras)
tt.register()

t.Run("json.Unmarshal()", func(t *testing.T) {
got := new(ChainConfig)
require.NoError(t, json.Unmarshal([]byte(tt.jsonInput), got))
assert.Equal(t, tt.want, got)
})

t.Run("json.Marshal()", func(t *testing.T) {
var want bytes.Buffer
require.NoError(t, json.Compact(&want, []byte(tt.jsonInput)), "json.Compact()")

got, err := json.Marshal(tt.want)
require.NoError(t, err)
assert.Equal(t, want.String(), string(got))
ARR4N marked this conversation as resolved.
Show resolved Hide resolved
})
})
}
}
Loading