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.
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
Add toUint, toInt and hexToUint to Strings #5166
Changes from 31 commits
b2eedbe
efd2f30
bc42b25
07f4b44
40ba631
07ec518
95fb0db
f263819
f51fbe6
52a301b
027859e
a91a999
86abf5a
6dca3cb
a7a6e9e
ec9a659
568dc7b
0292c31
aea4a14
cf78a9f
26cec97
3a7f904
4d18729
c7a7c94
d6319e8
b3bf461
2ab63b7
231b93b
24f1490
43f0dc1
7b7c1fd
2abfa49
f433e6d
27c7c0d
75e1e4c
4f48757
1ec1e3f
53d72d7
c5790f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is it not memory safe if it's just reading? I think we can safely annotate it:
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.
this is not memory safe because it's possible that the block reads from memory outside the bounds allocated by solidity (it's not checking bounds)
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.
Ah got it. I consider it may be better to annotate as
memory-safe
and check bounds. Otherwise, optimizations get disabled, perhaps it's cheaper to check and annotate than not checking (when optimizations are applied)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.
The entier point of this function is to not check the bound. Checking the bound require loading the length, which is one expensive mload.
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.
I thought the entire point was to make it cheaper. I think disabling optimizations we're doing the opposite.
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.
Basically this function is already doing stuff as simple as possible. Not sure any further optimization is required from the compiler for this block.
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.
From here:
My point is that by having this assembly block that's not memory safe we might be hurting the performance of the whole contract.
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.
so: #5166 (comment)
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.
Although I agree on adding a
Bytes
library, I'm not sure this particular function should go in. Perhaps this whole discussion indicates that the function should remain private in its contract.I'd be in favor of not adding the Bytes library right now if we agree
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.
If it was just in this contract I'd agree.
But the truth is that this function is needed:
in the other operation that come in Bytes library and CAIP2/CAIP10 helpers #5252 (in Bytes.sol)So if we keep it private, we have to define it 3 times, in 3 different contracts/libraries.
EDIT: my bad its not needed in #5252, so that is just 2 implementations. Let go with that!