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.

Sign up for GitHub

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

Jump to bottom

Bytes to bytesNN conversion #11047

Merged
merged 6 commits into from
Apr 26, 2021
Merged

Bytes to bytesNN conversion #11047

merged 6 commits into from
Apr 26, 2021

Conversation

mijovic
Copy link
Contributor

@mijovic mijovic commented Mar 4, 2021 •
edited
Loading

Fixes #9170

TODO:

  • Add more tests
  • Changelog
  • Documentation
  • Cleanup dirty bytes

@mijovic mijovic requested review from chriseth, ekpyron and hrkrshnn March 4, 2021 09:28
chriseth
chriseth reviewed Mar 4, 2021
View reviewed changes
libsolidity/ast/Types.cpp Outdated Show resolved Hide resolved
@chriseth
Copy link
Contributor

chriseth commented Mar 4, 2021

Please also fix https://github.com/ethereum/solidity/pull/9167/files

@mijovic mijovic force-pushed the bytesToBytesNNConversion branch 2 times, most recently from 6e8ab8e to de8e09f Compare March 4, 2021 10:54
@mijovic mijovic force-pushed the bytesToBytesNNConversion branch 8 times, most recently from 889bae1 to 79dc623 Compare March 18, 2021 16:13
@mijovic
Copy link
Contributor Author

mijovic commented Mar 18, 2021

This is now ready for review

chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
Changelog.md Outdated
@@ -1,7 +1,7 @@
### 0.8.3 (unreleased)

Language Features:

* Allowing conversion from ``bytes`` to ``bytes1``/.../``bytes32``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Has to be moved.

chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
docs/types/conversion.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
docs/types/conversion.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
docs/types/conversion.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
libsolidity/ast/Types.cpp Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Mar 24, 2021
View reviewed changes
libsolutil/ErrorCodes.h Outdated Show resolved Hide resolved
@mijovic mijovic force-pushed the bytesToBytesNNConversion branch 3 times, most recently from db4ef20 to ff0c772 Compare April 21, 2021 21:37
@mijovic
Copy link
Contributor Author

mijovic commented Apr 21, 2021

PR is updated to truncate instead of panic.

@mijovic mijovic force-pushed the bytesToBytesNNConversion branch 2 times, most recently from 4feaade to 776ed2e Compare April 21, 2021 21:48
chriseth
chriseth reviewed Apr 22, 2021
View reviewed changes
Changelog.md Outdated
@@ -1,6 +1,7 @@
### 0.8.5 (unreleased)

Language Features:
* Allowing conversion from ``bytes`` to ``bytes1``/.../``bytes32``.
Copy link
Contributor

Choose a reason for hiding this comment

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

and byte array slices?

chriseth
chriseth reviewed Apr 22, 2021
View reviewed changes
docs/types/conversion.rst
@@ -99,6 +99,27 @@ rules explicit::
uint8 d = uint8(uint16(a)); // d will be 0x34
uint8 e = uint8(bytes1(a)); // e will be 0x12

``bytes`` arrays and ``bytes`` calldata slices can be converted explicitly to fixed bytes types (``bytes1``/.../``bytes32``).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we usually list the allowed explicit conversions in the sections about the individual types. Can you check and maybe add something there as well?

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 couldn't find anything for bytes but will check once more

@chriseth
Copy link
Contributor

Looks good apart from the documentation bits!

@mijovic mijovic force-pushed the bytesToBytesNNConversion branch from 776ed2e to fc82f3a Compare April 22, 2021 16:51
@mijovic mijovic force-pushed the bytesToBytesNNConversion branch from 11d3291 to 42bd89e Compare April 23, 2021 07:47
axic
axic reviewed Apr 23, 2021
View reviewed changes
libsolidity/analysis/TypeChecker.cpp
solAssert(
argArrayType->location() != DataLocation::Storage ||
(
if (auto resultArrayType = dynamic_cast<ArrayType const*>(resultType))
Copy link
Member

Choose a reason for hiding this comment

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

Holy... this assertion is complicated.

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 didn't change it, but yes, it is too complicated. I can maybe refactor it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pulling the variable declaration inside does not make it simpler :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, I thought it was an if (x; y)

axic
axic reviewed Apr 23, 2021
View reviewed changes
libsolidity/ast/Types.cpp
if (_convertTo.category() != category())
return false;
return isByteArray() && !isString() && _convertTo.category() == Type::Category::FixedBytes;
Copy link
Member

Choose a reason for hiding this comment

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

Is isByteArray() true for bytes1[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it is regular array type of bytes1

axic
axic reviewed Apr 23, 2021
View reviewed changes
libsolidity/codegen/CompilerUtils.cpp
typeOnStack.isByteArray() && !typeOnStack.isString(),
"Array types other than bytes not convertible to bytesNN."
);
solAssert(typeOnStack.isDynamicallySized(), "");
Copy link
Member

Choose a reason for hiding this comment

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

These three settings suggest to me we may want to have something like isDynamicBytes() at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, we need to check if it occurs in multiple places and add such helper

@mijovic mijovic force-pushed the bytesToBytesNNConversion branch from c54ec13 to 337adee Compare April 23, 2021 11:30
@chriseth chriseth merged commit 659da4b into develop Apr 26, 2021
@chriseth chriseth deleted the bytesToBytesNNConversion branch April 26, 2021 09:51
@mijovic mijovic mentioned this pull request May 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@axic axic axic left review comments

@chriseth chriseth chriseth approved these changes

@ekpyron ekpyron Awaiting requested review from ekpyron

@hrkrshnn hrkrshnn Awaiting requested review from hrkrshnn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow conversion from bytes to bytesNN
4 participants
@mijovic @chriseth @axic @cameel