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 Strings.toString for signed integers #3773

Merged
merged 19 commits into from
Dec 28, 2022
Merged

Add Strings.toString for signed integers #3773

merged 19 commits into from
Dec 28, 2022

Conversation

galadd
Copy link
Contributor

@galadd galadd commented Oct 19, 2022

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Oct 21, 2022

Hello, @gbolahananon

Considering that we already have SignedMath:abs(int256) and Strings.toString(uint256), I think the implementation could be greatly simplified.

function toString(int256 value) internal pure returns (string memory) {
    return string.packed(value < 0 ? "-" : "", toString(abs(value));
}

@galadd
Copy link
Contributor Author

galadd commented Oct 21, 2022

That makes sense. I just implemented that. You can check the latest update

* @dev Converts a `int256` to its ASCII `string` decimal representation.
*/
function toString(int256 value) internal pure returns (string memory) {
return string(abi.encodePacked(value < 0 ? "-" : "",toString(SignedMath.abs(value))));
Copy link
Collaborator

@Amxx Amxx Oct 22, 2022

Choose a reason for hiding this comment

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

Any reason to do string(abi.encodePacked(...)) and not string.concat(...)? I think the second one is clearer and cleaner

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 used abi.encodePacked because it works with previous version of solidity and if a beginner wants to use the library, they aren’t faced with much difficulty. But on second thought, it's better a beginner gets used to the new implementation.

I have made a new commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would require bumping the pragma from pragma solidity ^0.8.0; to pragma solidity ^0.8.12; ...

It not just a matter of beginner devs, but also of good support with big projects that are fixing a solidity version that is before 0.8.12.

Considering we can do without the pragma bump, you might be right that we should probable use string(abi.encodePacked(...)).

@galadd
Copy link
Contributor Author

galadd commented Oct 24, 2022

Alright then, I think it's all good to go 👍🏽

@galadd
Copy link
Contributor Author

galadd commented Oct 25, 2022

This PR has not been merged. Is there any change I need to effect?

@Amxx Amxx requested a review from frangio October 26, 2022 13:29
@Amxx
Copy link
Collaborator

Amxx commented Oct 26, 2022

We are still missing a Changelog entry for this PR.

@Amxx
Copy link
Collaborator

Amxx commented Oct 26, 2022

Note: I'd love #3666 to be merge first, so that the tests can use the new syntax (instead of having to update #3666 yet again)

@frangio frangio changed the title Feature/string operation for int256 #3758 Add Strings.toString for signed integers Dec 28, 2022
frangio
frangio previously approved these changes Dec 28, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM

test/utils/Strings.test.js Outdated Show resolved Hide resolved
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.

For the record, let's group tests by type so they don't mix with each other

describe('toString', function () {
  ...
  describe('int256', function () { ... });
  describe('uint256', function () { ... });
  ...
})

@frangio frangio enabled auto-merge (squash) December 28, 2022 22:38
@frangio frangio merged commit 8335676 into OpenZeppelin:master Dec 28, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Dec 28, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 OpenZeppelin Contracts Contributor:

GitPOAP: 2022 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

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.

None yet

4 participants