-
Notifications
You must be signed in to change notification settings - Fork 991
feat: implement diff mode tracer #9442
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: implement diff mode tracer #9442
Conversation
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
...hyperledger/besu/ethereum/api/jsonrpc/internal/results/tracing/diff/StateTraceGenerator.java
Show resolved
Hide resolved
| * <li>A pre-state trace (complete state snapshot of relevant accounts) | ||
| * </ul> | ||
| */ | ||
| public class StateTraceGenerator { |
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.
Although it's tested indirectly with the integration tests would still be good to get some unit tests around this
| // Case 2: Account existed pre-transaction | ||
| final String originalValueHex = rootAccount.getStorageValue(key).toHexString(); | ||
| // Record differences, or everything if pre-state mode is enabled | ||
| if (!originalValueHex.equals(newValueHex) || isPreState) { |
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.
nit: Think it would be slightly better to do the comparison on the uint256 instead of comparing the hex string
| * Writes the "post" state for an account (diff mode). Includes only changed and non-zero | ||
| * fields. | ||
| */ | ||
| private void writePostNode( |
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.
Looks like there is a bit of duplication with writeNode.
| try { | ||
| gen.writeStringField("balance", b); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
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.
It probably makes sense to fail early if any exception occurs, but the same isn't being done for fields in writePostNode, should it be?
Signed-off-by: Gabriel-Trintinalia <[email protected]>
* Implement pre state diff mode tracer Signed-off-by: Gabriel-Trintinalia <[email protected]> * add tests Signed-off-by: Gabriel-Trintinalia <[email protected]> * add tests Signed-off-by: Gabriel-Trintinalia <[email protected]> * remove unused methods Signed-off-by: Gabriel-Trintinalia <[email protected]> * Accept PR suggestions Signed-off-by: Gabriel-Trintinalia <[email protected]> --------- Signed-off-by: Gabriel-Trintinalia <[email protected]> Signed-off-by: [email protected] <[email protected]>
PR description
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests