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(store,world,world-modules): use BYTE_TO_BITS constant where applicable [N-09] #2015

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Dec 5, 2023

No description provided.

@yonadaa yonadaa requested review from alvrs and holic as code owners December 5, 2023 12:16
Copy link

changeset-bot bot commented Dec 5, 2023

⚠️ No Changeset found

Latest commit: 7820ce2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@yonadaa yonadaa changed the title refactor(store,world,world-modules): use BYTES_TO_BITS constant where applicable [N-09] refactor(store,world,world-modules): use BYTE_TO_BITS constant where applicable [N-09] Dec 5, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

Unfortunately the changes increase gas cost in a way that makes me doubt if it's worth it. I think the gas cost increase only comes from the places where we replace an inline 8 in code, not in places where we only use it to define another constant that already exists. Maybe a good compromise would be to only change it in those constant definitions where it doesn't impact gas cost, and just add a comment in the other places?

@yonadaa
Copy link
Contributor Author

yonadaa commented Jan 5, 2024

@alvrs I agree with the direction of adding comments, but the increases actually come from constant definitions(!), not inline code. I've added comments to these files instead, and the gas reports are now equivalent to main.

Specifically, the increase comes from these constants in the Resource ID files:

ResourceId.sol

/// @dev Number of bits reserved for the type in the ResourceId.
uint256 constant TYPE_BITS = 2 * 8;
/// @dev Number of bits reserved for the name in the ResourceId.
uint256 constant NAME_BITS = 32 * 8 - TYPE_BITS;

WorldResourceId.sol

uint256 constant NAMESPACE_BITS = 14 * 8;
uint256 constant NAME_BITS = 16 * 8;

@yonadaa yonadaa requested a review from alvrs January 5, 2024 10:57
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

the named constant's purpose is mostly to make the calculations easier to parse for humans; in places where using the constant would increase gas, using a verbose comment instead is an acceptable alternative imo (no need to use the exact same BYTE_TO_BITS token here since this isn't a value that's ever going to change so we don't need to be able to ie search for it)

@holic holic merged commit b1f5816 into main Jan 12, 2024
11 checks passed
@holic holic deleted the yonadaaa/byte-to-bits-magic-number branch January 12, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants