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

Add toUint, toInt and hexToUint to Strings #5166

Merged
merged 39 commits into from
Oct 14, 2024

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Aug 28, 2024

As discussed. Usefull for parsing addresses from CAIP10 identifier (strings)

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@Amxx Amxx requested review from ernestognw and cairoeth August 28, 2024 09:34
Copy link

changeset-bot bot commented Aug 28, 2024

🦋 Changeset detected

Latest commit: c5790f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

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

contracts/utils/Strings.sol Outdated Show resolved Hide resolved
.changeset/eighty-hounds-promise.md Outdated Show resolved Hide resolved
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
@ernestognw ernestognw added this to the 5.2 milestone Sep 3, 2024
cairoeth
cairoeth previously approved these changes Sep 5, 2024
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
function hexToUint(string memory input) internal pure returns (uint256) {
bytes memory buffer = bytes(input);

// skip 0x prefix if present. Length check doesn't appear to be critical
Copy link
Contributor

Choose a reason for hiding this comment

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

second sentence feels out of place

Suggested change
// skip 0x prefix if present. Length check doesn't appear to be critical
// skip 0x prefix if present

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for the record, this is what the comment was about:

The string length may be less than 2. String could be empty, of just "1". In some cases, doing a input[1] or even a input[0] would revert (out of bound acces). Doing bytes2(input) does not revert if the string is to short.

So we check the length to verify that it ok to read the prefix ? It turn out no.

  • If the string is empty, then regardless of the result of that lookup (that could read dirty bytes), the loop will not run (because length == 0), and the result will be 0 -> that is ok
  • If the string has length 1 then we have two options
    • the check identifies the prefix
      • that means the string is "0", and there is a dirty x after.
      • In that case we have an offset of 2, and the length is 1, so the for loop does not run and the function returns 0 -> that is ok
    • the check does not find the prefix
      • the only "digit" is read by the loop, and the result should be just fine
  • If the string has length >= 2, then the prefix lookup is in the bounds

That is the long explanation (I'm happy its visible in the PR 😃) to something that is not really trivial, and can be missed, but missing it is not a risk.

We may get questions about it though ...

cairoeth
cairoeth previously approved these changes Sep 9, 2024
@cairoeth cairoeth dismissed their stale review September 9, 2024 15:46

TODO: add functions to specify start and end of string to parse

@Amxx Amxx force-pushed the feature/parse-strings branch from ef973fc to f433e6d Compare October 11, 2024 21:49
@Amxx
Copy link
Collaborator Author

Amxx commented Oct 11, 2024

I made unsafeReadBytesOffset private again, and duplicated it in Governor.sol and Strings.sol.

@ernestognw ernestognw changed the title Add Bytes library and toUint, toInt and hexToUint to Strings Add and toUint, toInt and hexToUint to Strings Oct 11, 2024
@ernestognw ernestognw changed the title Add and toUint, toInt and hexToUint to Strings Add toUint, toInt and hexToUint to Strings Oct 11, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

I'd like to reconsider the naming of parseHex as raised by @arr00

The function name is ambiguous because it's not clear whether it parses the string as a literal hex (i.e. hex"1234") or as a decimal string (i.e. 1234). I'd suggest renaming to parseHexUint

contracts/utils/README.adoc Outdated Show resolved Hide resolved
contracts/governance/Governor.sol Outdated Show resolved Hide resolved
contracts/governance/Governor.sol Outdated Show resolved Hide resolved
Amxx and others added 2 commits October 12, 2024 00:47
ernestognw
ernestognw previously approved these changes Oct 11, 2024
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

lfg

@ernestognw ernestognw dismissed their stale review October 11, 2024 22:49

oh nvm we haven't agreed the parseHex naming u.u

ernestognw
ernestognw previously approved these changes Oct 12, 2024
@Amxx Amxx merged commit bd58895 into OpenZeppelin:master Oct 14, 2024
18 checks passed
@Amxx Amxx deleted the feature/parse-strings branch October 14, 2024 15:15
ernestognw added a commit that referenced this pull request Oct 14, 2024
Co-authored-by: cairo <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
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.

6 participants