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

fix: use helper to deserialize mainnet TTD #5322

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Nov 5, 2023

Followup to #5290, using behavior from gakonst/ethers-rs#2617 instead of find/replace to deserialize the mainnet TTD.

@Rjected Rjected added C-bug An unexpected or incorrect behavior C-debt Refactor of code section that is hard to understand or maintain labels Nov 5, 2023
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

hmm, this is now baked into the jsonu256 helper type, but we only need this for genesis, right?

I think we should instead use an annotation there

@Rjected
Copy link
Member Author

Rjected commented Nov 5, 2023

hmm, this is now baked into the jsonu256 helper type, but we only need this for genesis, right?

I think we should instead use an annotation there

ah right, yeah, an annotation would be better so we can just do this hack for TTD rather than for everything

@Rjected Rjected requested a review from gakonst as a code owner November 5, 2023 22:16
@Rjected Rjected changed the title fix: use visit_f64 to deserialize mainnet TTD fix: use helper to deserialize mainnet TTD Nov 5, 2023
@mattsse mattsse added this pull request to the merge queue Nov 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2023
@mattsse
Copy link
Collaborator

mattsse commented Nov 6, 2023

@Rjected test fails in CI for some reason, couldn't reproduce locally in reth-rpc-types

@Rjected
Copy link
Member Author

Rjected commented Nov 6, 2023

hmm, I'm only able to repro with nextest run on the entire repo, but at least I can repro

@Rjected
Copy link
Member Author

Rjected commented Nov 6, 2023

hmm, apparently it's possible for is_f64, is_u64, is_i64 to all return false:

running 1 test
val: Number(58750000000000000000000), u64: false, f64: false, i64: false
test serde_helpers::json_u256::test::deserialize_ttd ... FAILED

Should be able to fix though

@Rjected
Copy link
Member Author

Rjected commented Nov 6, 2023

@mattsse we can actually use as_str from Number, and pass that into U256::from_str_radix

@Rjected
Copy link
Member Author

Rjected commented Nov 6, 2023

nevermind, that's only for arbitrary precision

@mattsse mattsse added this pull request to the merge queue Nov 6, 2023
Merged via the queue into main with commit e6ea251 Nov 6, 2023
25 checks passed
@mattsse mattsse deleted the dan/float-visitor-total-difficulty branch November 6, 2023 22:09
mattsse added a commit that referenced this pull request Nov 8, 2023
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug An unexpected or incorrect behavior C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants