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

Internalize crc #3224

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Internalize crc #3224

merged 4 commits into from
Jan 10, 2024

Conversation

acolytec3
Copy link
Contributor

This duplicates the crc32 code from the crc dep into an internal utility.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6068530) 87.91% compared to head (34cda9c) 87.00%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client 84.66% <ø> (+0.02%) ⬆️
common ?
devp2p 82.15% <ø> (ø)
ethash ∅ <ø> (∅)
evm 76.96% <ø> (ø)
genesis 99.98% <ø> (ø)
rlp ?
statemanager 86.29% <ø> (ø)
trie 89.80% <ø> (ø)
tx 95.73% <ø> (ø)
util ?
vm 80.83% <ø> (ø)
wallet 91.00% <ø> (ø)

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

}

const crc = (current: Uint8Array, previous?: number) => {
let crc = previous === 0 ? 0 : ~~previous! ^ -1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one thing I don't get is how this works if previous is undefined. The original version of this code just asserts it's not undefined but I'm not sure . Will work on some tests.

Copy link
Member

Choose a reason for hiding this comment

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

~~undefined evaluates to 0 it seems. (! is a typescript operator (?))

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link to the original version to see how it was being done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/alexgorbatchev/crc/blob/master/src/calculators/crc32.ts It's this file, I copied basically unmodified other than for adding some type annotations that were missing.

@@ -1,6 +1,6 @@
{
"editor.codeActionsOnSave": {
"source.fixAll.eslint": true
"source.fixAll.eslint": "explicit"
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. It just keeps getting auto updated in my local vscode settings. Lemme remove it. Didn't mean to commit but it just keeps showing up.

}

const crc = (current: Uint8Array, previous?: number) => {
let crc = previous === 0 ? 0 : ~~previous! ^ -1
Copy link
Member

Choose a reason for hiding this comment

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

~~undefined evaluates to 0 it seems. (! is a typescript operator (?))

Copy link
Member

Choose a reason for hiding this comment

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

I am assuming this file is copied from the original package, I assume a license/source code pointer should be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, sorry, I had that in here but somehow deleted it. Will re-add.

Copy link
Member

Choose a reason for hiding this comment

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

Just saw this got merged wanted to add a super small nit: now the reference points to a GH file, but we should have used a permalink to that file instead (if it moves or changes it will not match what is there in the future))

Copy link
Contributor

@scorbajio scorbajio left a comment

Choose a reason for hiding this comment

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

Always nice to drop unnecessary dependencies 🙂. Might be worth checking the results of using a WASM version.

}

const crc = (current: Uint8Array, previous?: number) => {
let crc = previous === 0 ? 0 : ~~previous! ^ -1
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link to the original version to see how it was being done.

Copy link
Contributor

@scorbajio scorbajio left a comment

Choose a reason for hiding this comment

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

ACK

@scorbajio scorbajio merged commit 7bcb197 into master Jan 10, 2024
46 checks passed
@scorbajio scorbajio deleted the internalize-CRC branch January 10, 2024 18:24
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.

3 participants