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

hex: Add from_hex<T> for user types #649

Merged
merged 1 commit into from
Jun 2, 2022
Merged

hex: Add from_hex<T> for user types #649

merged 1 commit into from
Jun 2, 2022

Conversation

chfast
Copy link
Member

@chfast chfast commented May 26, 2022

Depends on #648.

@chfast chfast requested review from axic, gumb0 and yperbasis May 26, 2022 07:25
@chfast chfast force-pushed the hex_to_T branch 2 times, most recently from 98570a9 to a66d6d5 Compare May 26, 2022 07:36
@chfast chfast mentioned this pull request May 26, 2022

/// Decodes hex-encoded string into custom type T with .bytes array of uint8_t.
///
/// The decoded bytes are right aligned in case the input is smaller than the result type.
Copy link
Member

@axic axic May 28, 2022

Choose a reason for hiding this comment

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

Why not left aligned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because they imitate BE numbers.

Copy link
Member

Choose a reason for hiding this comment

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

So by right aligned do you mean padded with zeroes on the left?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but left/right does not have clear meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

When the input is smaller than the result type, the result is padded with zeros on the left (the result bytes of lowest indices are filled with zeros).

?

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a flag for alignment?

Coming from Solidity would prefer left aligned if we are writing into a "bytes" type. But this opinion is not too strong if you only have use cases for right aligned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, an option can be added if needed, but so far this is only used for literals and loading storage values from JSON.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add // TODO: support left alignment too with a flag.

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #649 (568f9df) into master (568f9df) will not change coverage.
The diff coverage is n/a.

❗ Current head 568f9df differs from pull request most recent head a65e4a9. Consider uploading reports for the commit a65e4a9 to get more accurate results

@@           Coverage Diff           @@
##           master     #649   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files          25       25           
  Lines        3626     3626           
  Branches      377      377           
=======================================
  Hits         3372     3372           
  Misses        144      144           
  Partials      110      110           

@chfast chfast force-pushed the hex_to_T branch 3 times, most recently from 9471c85 to 95805a2 Compare June 1, 2022 21:08
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Perhaps add // TODO: support left alignment too with a flag.

Base automatically changed from hex_refactor to master June 2, 2022 10:59
@chfast chfast merged commit 9dcb0a1 into master Jun 2, 2022
@chfast chfast deleted the hex_to_T branch June 2, 2022 11:42
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.

3 participants