-
Notifications
You must be signed in to change notification settings - Fork 107
feat(LibBytes): add address<->bytes32 conversion helpers (EXSC-618) [LibBytes v1.1.0] #2055
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
Open
0xDEnYO
wants to merge
3
commits into
main
Choose a base branch
from
feature/exsc-618-sc-libbytes-v110-addressbytes32-conversion-helpers
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toHexStringchange silently altersSquidFacetbytecode with no version bump.LibBytesfunctions areinternal pureand get inlined into consumers.SquidFacet.sol:187is the sole caller oftoHexString, so swapping therequire-string forrevert HexLengthInsufficient()changes SquidFacet's compiled bytecode and its revert data — yet SquidFacet stays at@custom:version 1.0.0and isn't listed among the "call sites intentionally not migrated". The PR body implies zero facet behavior changes, but this one does change.Per
[CONV:VERSIONING], "new revert on existing function" is a MAJOR-type change, and this is a revert-encoding change on a shipped function. In practice the branch is an unreachable invariant (after the loopvalueis always 0 for a correctly-sized input), so behavioral/security risk is effectively nil — but it should be surfaced rather than folded silently into a "no facets touched" PR.Suggestion: either split the
toHexStringrefactor into its own PR acknowledging the SquidFacet impact, or note in the PR body that SquidFacet's bytecode changes and confirm with audit/deploy that an unbumped version is acceptable here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not bumping SquidFacet's version here — but you're right that the transitive bytecode change should be surfaced, so I've added a note to the PR description.
Reasoning:
[CONV:VERSIONING]says a library MAJOR bump should be evaluated against consumers, not that any library change bumps them. ThetoHexStringedit isn't a "new revert on existing function" — the revert already existed (require(value == 0, …)); only the revert encoding changed (Error(string)→HexLengthInsufficient()). And that branch is unreachable for a correctly-sizedlength(after the loopvalueis always 0, and SquidFacet always passes a correct length), so there's zero reachable behavior change at the ABI level → PATCH-class, which doesn't cascade. So SquidFacet stays 1.0.0 and LibBytes' MINOR bump (the three new functions dominate) is correct.The only real action item was your "surface it rather than fold it silently" point — done: the description now flags that SquidFacet's compiled bytecode/revert-encoding changes (dead-code only) so whoever next deploys it expects the 1.0.0 bytecode to differ.