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

Remove dependency on solidity-stringutils #91

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ericglau
Copy link
Member

  • Remove dependency on solidity-stringutils. Instead, uses a combination of OpenZeppelin's Strings library, Forge's string cheatcodes, and an internal library for additional string finder functions.
  • Sets the assembly block in Core._deployFromBytecode as /// @solidity memory-safe-assembly. This annotation is used instead of assembly ("memory-safe") so that it remains compatible with versions of Solidity used in both OpenZeppelin Contracts v4 and v5.

Fixes #89

@ericglau ericglau requested a review from a team January 14, 2025 23:03
* Returns whether the subject string contains the search string.
*/
function contains(string memory subject, string memory search) internal returns (bool) {
Vm vm = Vm(Utils.CHEATCODE_ADDRESS);
Copy link

@Amxx Amxx Jan 24, 2025

Choose a reason for hiding this comment

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

I thing that can be defined only once as a library constant:

library StringFinder {
    Vm private constant VM = Vm(Utils.CHEATCODE_ADDRESS);
    
    function contains(string memory subject, string memory search) internal returns (bool) {
        return VM.contains(subject, search);
    }
    
    function startsWith(string memory subject, string memory search) internal pure returns (bool) {
        return VM.indexOf(subject, search) == 0;
    }
    
    // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't work on some older versions of Solidity 0.8.x which should be supported for previous versions of OpenZeppelin Contracts.

Comment on lines +24 to +26
Vm vm = Vm(Utils.CHEATCODE_ADDRESS);
uint256 index = vm.indexOf(subject, search);
return index == 0;
Copy link

@Amxx Amxx Jan 24, 2025

Choose a reason for hiding this comment

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

You could do without the Vm:

return return bytes(subject).length >= bytes(search).length
    && string(bytes(subject).slice(0, bytes(search).length)).equal(search);

Copy link

Choose a reason for hiding this comment

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

Note that it would be pure, without would allow all the previously view function to remain view

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use Bytes.slice here because we want to support OpenZeppelin Contracts 4.x and 5.x, and Bytes isn't available in some older versions. We also can't just copy in the Bytes.slice code because older versions of Solidity don't support mcopy.

Comment on lines 33 to 38
Vm vm = Vm(Utils.CHEATCODE_ADDRESS);
if (!vm.contains(subject, search)) {
return false;
}
string[] memory tokens = vm.split(subject, search);
return bytes(tokens[tokens.length - 1]).length == 0;
Copy link

Choose a reason for hiding this comment

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

Same:

return bytes(subject).length >= bytes(search).length
 && string(bytes(subject).slice(bytes(subject).length - bytes(search).length)).equal(search);

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reasoning as my other comment

Copy link
Member Author

@ericglau ericglau Jan 24, 2025

Choose a reason for hiding this comment

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

Removed the use of vm.contains, similar to the suggestion in count(). This allows this to be pure, and reverted some other functions' mutability to view.

Comment on lines 46 to 48
if (!vm.contains(subject, search)) {
return 0;
}
Copy link

Choose a reason for hiding this comment

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

is the contains really needed ?

I'm expecting vm.split(subject, search) to return a string[1] that contains everything when search is not present in subject.

So tokens.length is 1, and tokens.length - 1 is 0

Copy link

Choose a reason for hiding this comment

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

Note that neither this is not a correct implementation of count. It will not return the correct value when the instances overlap. For example, count("aaa", "aa") should return 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed vm.split. Changed function documentation to say that this is for non-overlapping occurrences, as we currently don't have a use case for counting overlaps.

Copy link

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

I'm thinking that there may be stuff from solidity-stringutils that we want to put in our String.sol

@ericglau ericglau requested a review from Amxx January 24, 2025 22:32
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.

solidity-stringutils dependency can cause problems with compilation
2 participants