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: trace_decoder::decoding #469

Merged
merged 17 commits into from
Aug 13, 2024

Conversation

0xaatif
Copy link
Contributor

@0xaatif 0xaatif commented Aug 7, 2024

#275

Change of pace to do some simple cleanup:

  • Most of the module consisted of static methods on ProcessedBlockTrace, I've made those functions now.
  • Removed all the custom error logic, and just use anyhow. The same details will be exposed to end users.
    • We may have to change error formatting elsewhere to get the full error chain, as the new approach makes use of std::error::Error::source, but no changes should be required for errors which [panic] or are returned from main.
  • Made the existing code less verbose in places

@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Aug 7, 2024
@0xaatif 0xaatif force-pushed the 0xaatif/refactor-trace-decoder-decoding branch from 6312836 to f65fbd0 Compare August 7, 2024 10:09
@Nashtare Nashtare added this to the System strengthening milestone Aug 9, 2024
Copy link

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

There are some nice changes and some potentially mildly controversial ones. See some comments below, mostly to do with general Rust coding conventions and such.

Note that the change in indentation makes it harder to see which are the genuine changes introduced in this PR. Therefore, some of the comments may refer to stuff that only incidentally shows up in the diff.

trace_decoder/src/decoding.rs Outdated Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
@muursh
Copy link
Member

muursh commented Aug 11, 2024

@Nashtare you mentioned some edge cases from John?

@Nashtare
Copy link
Collaborator

@muursh They mostly revolve around revert cases (either full txn revert or some chile context reversion). There seem to be some issues with txn reversions in mainnet blocks as well (cf #475, #421), but not sure yet whether this can be handled without changes on the Jerigon side or not, still looking at them.

@0xaatif
Copy link
Contributor Author

0xaatif commented Aug 12, 2024

@Nashtare you mentioned some edge cases from John?

I'm aiming for no behavioural changes with this PR - have I inadvertently broken something?

@0xaatif 0xaatif self-assigned this Aug 12, 2024
Copy link
Member

@atanmarko atanmarko left a comment

Choose a reason for hiding this comment

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

Only few non blocking nitpicks

trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
trace_decoder/src/decoding.rs Show resolved Hide resolved
@0xaatif 0xaatif enabled auto-merge (squash) August 13, 2024 11:55
@0xaatif 0xaatif merged commit 4aaab69 into develop Aug 13, 2024
15 checks passed
@0xaatif 0xaatif deleted the 0xaatif/refactor-trace-decoder-decoding branch August 13, 2024 14:38
atanmarko pushed a commit that referenced this pull request Aug 14, 2024
* mark: 0xaatif/refactor-trace-decoder-decoding

* refactor: remove inappropriate static methods

* refactor: remove inappropriate method

* refactor: remove TraceParsingErrorReason

* refactor: remove LocatedError

* refactor: remove EMPTY_ACCOUNT_BYTES_RLPED

* refactor: remove update_val_if_some

* refactor: impl Display for TrieType

* wibble

* refactor: inline TraceParsingResult

* wibble

* wibble: order

* refactor: remove unused variable

* wibble: inline TrieRoots etc

* wibble

* review: WithHash -> CustomFmt<T>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants