Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
core/tracing: state journal wrapper #30441
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
core/tracing: state journal wrapper #30441
Changes from all commits
8659e68
b4e0174
f670a7f
cf873c3
365b715
aac4024
dbe5f83
b87c4fe
702a42f
c915bed
838fc25
1cc58cf
3c58155
501f302
1a64297
1862333
6650000
d9de74e
d2ba76f
a2ca5f8
85a85d0
2754b41
92337d8
36b4194
efed5a6
ea92ef4
fbd1d19
0f005af
5e4d6b8
4d2fb0e
b37f2ac
a0f7cd6
87582a4
6e4d14c
553f023
be93d72
1dda30d
4acea3b
018df6b
60b2222
6c56ea5
f4cf2a5
7fb2688
3228063
95b82cf
de48d55
9cae376
bf51dde
459c50f
831524a
6f5e74b
bca2e2c
8a2230e
59a5022
4ba05e9
2795c0e
51720dc
4787f31
8a44029
eaacae4
93432fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
V2
isn't helpful, we should useNonceChangeHokWithReason
. Function names should be explict in what they do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have chosen this naming scheme explicitly. The intent is communicating which version of the hook is the newest one. When we introduce a new hook version, the old one becomes deprecated and will eventually be unsupported. Also, if we were to introduce another revision of this hook, would it be called
NonceChangeHookWithReasonAndBellsAndWhistles
? The*Vx
naming scheme avoids this problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the old one is removed, you can remove the
WithReason
part, but in the meantime, you know at a glance why the two methods differ. I don't think there's a big risk of that function being rewritten many times and, therefore, having the names getting longer and longer. But sure, just giving my opinion on what could make the code readable, not going to hold the release for that one 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot rename the function because it is a stable, user-exposed API. If we could just change it, we wouldn't go through the trouble of having multiple versions of the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use-case is to have access to the headers of hashes that are accessed by the EVM. Alternative would be if we added a GetHeaderByHash method somewhere. But getting the hash from OnOpcode is also tricky since the hash will be put on the stack after OnOpcode is invoked.