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

refactor: params extra types are zero values not nil pointers by default #13

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Sep 11, 2024

Why this should be merged

Adds support for type-safe setting of ChainConfig and Rules extras via the ExtraPayloadGetter, which can now return pointers. This is demonstrated to work with zero-values of both types.

getter := params.RegisterExtras(Extras[Foo,...]{})

var x ChainConfig
fmt.Println(getter.FromChainConfig(&x)) // prints a zero Foo
*getter.PointerFromChainConfig(&x) = Foo{...} // returned pointer is guaranteed non-nil so this is always safe

How this works

I suspect that zero-value extras via x := new(ChainConfig) or var x ChainConfig have always worked this way, but the behaviour is now locked in with a test that also demonstrates shallow copying.

Note

As the extras are wrapped in a *pseudo.Type, shallow copying only copies the address of the wrapper even if the registered type is not a pointer.

Modification via pointers is achieved by extending the pseudo package to allow for "addressing" via the new PointerTo() function. This is then exposed via the ExtraPayloadGetter.PointerFromChainConfig() *C and PointerFromRules() *R methods to provide type safety to external users. The guarantees of correctness that avoid MustNewPointer() panicking are the same as those for the original FromChainConfig() and FromRules() methods.

The other changes to C and R plumbing (i.e never using *C and *R) were to clean up a code smell; this made the rest of this PR much easier to reason about. I relaxed the constraint on C and R being structs, allowing for pointers to structs, also because it's a smell (that I think can actually be removed entirely but in another PR).

How this was tested

New unit test for pseudo.PointerTo() and integration test for ExtraPaloadGetter.PointerFrom{ChainConfig,Rules}().

@ARR4N ARR4N marked this pull request as ready for review September 11, 2024 19:41
@ARR4N ARR4N requested review from a team, darioush, ceyonur and michaelkaplan13 and removed request for a team September 11, 2024 19:41
@ARR4N ARR4N merged commit d31803a into libevm Sep 12, 2024
1 check passed
@ARR4N ARR4N deleted the arr4n/extra-type-normalisation branch September 12, 2024 06:54
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