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

common: move description string from being an object field to a comment #3500

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented Jul 12, 2024

This change is inspired by #3448 and refactors the following files by removing any description string fields and instead including them as comments to reduce code size:

  • eips.ts
  • hardforks.ts

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.

Ah, one step further please, so the v (+ the surrounding dict) should be killed completely! 🙂

@holgerd77
Copy link
Member

Can you also please post a set of pre and post work numbers here so that we always have these comparison values in the PRs themselves? (Simply the two files sizes in KB pre/post should be enough in this case I guess)

@scorbajio
Copy link
Contributor Author

scorbajio commented Jul 13, 2024

It seems like some tests from other packages that directly access the v field are failing, will clean those up, but the refactor seems to be successful, with the following results for the following test code:

import { Chain, Common, Hardfork } from '@ethereumjs/common'

const c = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Prague })
console.log(c.chainId())

Before: 187.4kb
After: 162.0kb
Delta: -24.4kb

I wasn't able to import the EIPs dictionary since it's not being exported in the index file, although since it's being accessed indirectly by other exports, results should still include the size reduction it's gone through (although not entirely sure on this, may need confirmation).

@scorbajio
Copy link
Contributor Author

scorbajio commented Jul 13, 2024

I wasn't able to locally recreate any of the failing vm-api or test-wallet tests, so I just triggered a rerun to double check the CI results.

Update 1: The wallet tests now pass, so those failures seem to have been intermittent, but the vm-api tests still fail on CI and pass locally.

Update 2: I tracked down all failing tests but one to a reference to the v field in the vm-api tests, however one final test is now failing due to what seems like a missing account, will need additional debugging to see where this could be coming from.

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.

Nice, cool, that we have a fix here!

Will merge.

@holgerd77 holgerd77 merged commit 4d8ad71 into master Jul 15, 2024
34 checks passed
@holgerd77 holgerd77 deleted the common-descriptions-refactor branch July 15, 2024 19:48
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