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.

Sign up for GitHub

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

Jump to bottom

More exact balance change reasons in tracing #3040

Merged
merged 7 commits into from
Apr 2, 2025
Merged

More exact balance change reasons in tracing #3040

merged 7 commits into from
Apr 2, 2025

Conversation

ganeshvanahalli
Copy link
Contributor

@ganeshvanahalli ganeshvanahalli commented Mar 21, 2025 •
edited
Loading

This PR uses more exact balance change reasons in tracing.

Pulls geth PR- OffchainLabs/go-ethereum#433
Resolves NIT-3174

[ This PR fails CI because geth pin on nitro master is a couple of commits behind compared to geth's master- thus pulling some changes that nitro master is not yet ready ]

@ganeshvanahalli ganeshvanahalli mentioned this pull request Mar 21, 2025
Merged
diegoximenes
diegoximenes reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@diegoximenes diegoximenes left a comment

Choose a reason for hiding this comment

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

Do we consider this kind of change a breaking change?

@eljobe
Copy link
Member

eljobe commented Apr 1, 2025

@ganeshvanahalli, once the corresponding change is merged on go-ethereum, please update the go-ethereum pin, and address review feedback from @diegoximenes

@ganeshvanahalli
Copy link
Contributor Author

Do we consider this kind of change a breaking change?

Not sure, because we still have the scenario for transfer as util.TracingDuringEVM but the balance change reasons are more accurate than before which was "during evm execution". @tsahee thoughts on this?

@joshuacolvin0 joshuacolvin0 enabled auto-merge April 1, 2025 23:13
joshuacolvin0
joshuacolvin0 approved these changes Apr 1, 2025
View reviewed changes
Copy link
Member

@joshuacolvin0 joshuacolvin0 left a comment

Choose a reason for hiding this comment

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

LGTM

@joshuacolvin0
Copy link
Member

Do we consider this kind of change a breaking change?

Not sure, because we still have the scenario for transfer as util.TracingDuringEVM but the balance change reasons are more accurate than before which was "during evm execution". @tsahee thoughts on this?

Not a breaking change. Only changes the output of trace command, and as Ganesh states, doesn't change the meaning, just more specific

@joshuacolvin0 joshuacolvin0 merged commit 81bbff0 into master Apr 2, 2025
15 of 16 checks passed
@joshuacolvin0 joshuacolvin0 deleted the exact-balancechangereason-tracing branch April 2, 2025 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegoximenes diegoximenes diegoximenes approved these changes

@joshuacolvin0 joshuacolvin0 joshuacolvin0 approved these changes

@gligneul gligneul Awaiting requested review from gligneul

@tsahee tsahee Awaiting requested review from tsahee

Assignees

@ganeshvanahalli ganeshvanahalli

Labels
design-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ganeshvanahalli @eljobe @joshuacolvin0 @diegoximenes