Skip to content

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Dec 18, 2024

Why this should be merged

JSON equivalent of #89.

How this works

The check for registered extras, previously used in {En,De}codeRLP() methods is abstracted into a Header.hooks() HeaderHooks method that either returns (a) an instance of the registered type or (b) a NOOPHeaderHooks if no registration was performed. This is then used for all hooks, new (JSON) and old (RLP).

How this was tested

Extension of existing unit tests.

@ARR4N ARR4N marked this pull request as ready for review December 18, 2024 16:18
@ARR4N ARR4N requested review from a team, darioush, michaelkaplan13 and qdm12 and removed request for a team December 18, 2024 16:19
Comment on lines +40 to +42
if r := registeredExtras; r.Registered() {
return r.Get().hooks.hooksFromHeader(h)
}
Copy link

Choose a reason for hiding this comment

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

nit no need to assign to r:

Suggested change
if r := registeredExtras; r.Registered() {
return r.Get().hooks.hooksFromHeader(h)
}
if registeredExtras.Registered() {
return registeredExtras.Get().hooks.hooksFromHeader(h)
}

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 did it to avoid unnecessary verbosity and code stuttering. One of the core guiding principles that I follow (and recommend) is for code to get out of the way of the human seeing the meaning being communicated to the computer. Here there's a trade-off between distraction (a 5-syllable variable being unnecessarily repeated, with 3 of them repeated again in the method name) and a meat-RAM allocation (remembering what r is). I chose the latter because it only needs to be remembered for one line.

Copy link

Choose a reason for hiding this comment

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

I was actually saying this for readability more than for memory usage 😄
Assigning to r made me think initially we were copying the variable because the method calls were perhaps modifying it. I feel it could hide something eventually if you see what I mean?

hdr := new(Header)
stub := &stubHeaderHooks{
setHeaderToOnUnmarshalOrDecode: Header{
Extra: []byte("can you solve this puzzle? 0xbda01b6cf56c303bd3f581599c0d5c0b"),
Copy link

Choose a reason for hiding this comment

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

🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well?

Copy link

Choose a reason for hiding this comment

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

Any hint on an encoding?
Is this your 256 bits private key? 😄

@ARR4N ARR4N enabled auto-merge (squash) December 19, 2024 16:25
@ARR4N ARR4N merged commit 4575ced into main Dec 19, 2024
4 checks passed
@ARR4N ARR4N deleted the arr4n/header-json branch December 19, 2024 16:30
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.

3 participants