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

Util: bytes cleanup + do not import from ethereum-cryptography/utils #2845

Merged
merged 36 commits into from
Jul 3, 2023

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Jun 30, 2023

Closes #2843

This PR:

TODOs

  • All tasks in the linked issue
  • Add lint rule preventing imports from ethereum-cryptography/utils
  • Verify in entire repo there are no more occurences of prefixedHexStringToBytes and bytesToPrefixedHexString
  • Upgrade performance of hexToBytes (do not slice + pad, this copies the string) - will go in a different PR, this commit b079375 breaks tests. See Update bytes performance #2848

@jochem-brouwer jochem-brouwer changed the title monorepo: do not import from ethereum-cryptography/util [no ci] Util: bytes cleanup + do not import from ethereum-cryptography/utils Jun 30, 2023
@jochem-brouwer jochem-brouwer added the type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. label Jun 30, 2023
@jochem-brouwer
Copy link
Member Author

Hmm great, merge conflicts.

Anyways, will stop for today. There are 2 test suits failing: wallet (seems to hang?) and client.

To debug keep this in mind:

The new bytesToHex is what was (and is re-exported) bytesToUnprefixedHex.

The new hexToBytes is what was (and is re-exported) unprefixedHexToBytes

@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

Merging #2845 (e7592ed) into master (499c496) will increase coverage by 0.04%.
The diff coverage is 89.33%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.16% <95.00%> (-0.02%) ⬇️
blockchain 92.52% <92.10%> (+0.01%) ⬆️
client 87.08% <86.50%> (+<0.01%) ⬆️
common 97.06% <100.00%> (-0.01%) ⬇️
devp2p 86.95% <85.29%> (+0.01%) ⬆️
ethash ?
evm 66.99% <83.78%> (-0.01%) ⬇️
rlp ∅ <ø> (∅)
statemanager 86.41% <96.22%> (-0.05%) ⬇️
trie 90.36% <87.50%> (+0.34%) ⬆️
tx 95.96% <100.00%> (-0.01%) ⬇️
util 86.54% <93.75%> (+0.46%) ⬆️
vm 77.25% <78.26%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Jul 2, 2023

Remove toUtf8 in 21d26fe

@jochem-brouwer jochem-brouwer removed the type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. label Jul 2, 2023
@jochem-brouwer
Copy link
Member Author

Performance will go in a different PR because my changes in b079375 broke tests

packages/client/genesis.json Outdated Show resolved Hide resolved
packages/client/jwtsecret Outdated Show resolved Hide resolved
packages/util/src/bytes.ts Outdated Show resolved Hide resolved
@holgerd77 holgerd77 marked this pull request as ready for review July 3, 2023 09:30
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

OMG is this an insane PR.

Have nevertheless forced me to go at least once through the whole thing.

Beautiful.

🤩

LGTM.

@holgerd77 holgerd77 merged commit 1d70d7c into master Jul 3, 2023
@holgerd77 holgerd77 deleted the util-bytes-cleanup branch July 3, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Util/Monorepo: Clean-Up bytes module Usage / Remove ethereum-cryptography helper usage
3 participants