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 From [u8; n] impl for uint types #859

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

pgherveou
Copy link
Contributor

Changes:

  • removes the From [u8; n] conversions implementations.
    Since uint types can be both encoded with little-endian or big-endian format, the From implementation can easily be misused.
  • to_big_endian and to_little_endian are renamed write_as_*
  • to_little_endian and to_big_endian are now functions that returns the bytes array

@pgherveou pgherveou changed the title Remove From impl for uint types Remove From [u8; n] impl for uint types Sep 9, 2024
@pgherveou pgherveou force-pushed the pg/remove-from-impl-for-uint-types branch from 5024f87 to 6a65ee6 Compare September 9, 2024 15:07
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Thanks. The From implementation already bit me when converting from little endian.

I am not sure of we should also bump the version in this PR or if this is done at release time.

uint/benches/bigint.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Theißen <[email protected]>
@pgherveou
Copy link
Contributor Author

Thanks. The From implementation already bit me when converting from little endian.

I am not sure of we should also bump the version in this PR or if this is done at release time.

Looks like bumping is done in releases, I can update the changelogs once this is approved by owners

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

FWIW the alloy_primitives U256 doesn't support this conversion neither (although I don't know the exact reason).

@athei
Copy link
Member

athei commented Sep 10, 2024

FWIW the alloy_primitives U256 doesn't support this conversion neither (although I don't know the exact reason).

Implementing From where the conversion isn't obvious is against the API guidelines. See the last point here. They even mention bytes to number conversion as an explicit example where to not implement it.

@ordian
Copy link
Member

ordian commented Sep 10, 2024

Looks like bumping is done in releases, I can update the changelogs once this is approved by owners

Updating the changelogs should ideally happen in the PR itself. If you want to release a new version straight away, you can also bump the versions here. Note that this breaking change will propagate to other crates like primitive-types.
It's also a good practice to write how to migrate to a new version in the changelog (this method removed, use that instead).

Releasing a new version then should be easy:
https://github.com/paritytech/parity-common/blob/master/CONTRIBUTING.md#releasing-a-new-version

@ordian
Copy link
Member

ordian commented Sep 10, 2024

FWIW the alloy_primitives U256 doesn't support this conversion neither (although I don't know the exact reason).

Implementing From where the conversion isn't obvious is against the API guidelines. See the last point here. They even mention bytes to number conversion as an explicit example where to not implement it.

Agreed. This code in uint is very old and predates these guidelines probably.

ethbloom/CHANGELOG.md Outdated Show resolved Hide resolved
uint/CHANGELOG.md Outdated Show resolved Hide resolved
ethereum-types/CHANGELOG.md Outdated Show resolved Hide resolved
primitive-types/CHANGELOG.md Outdated Show resolved Hide resolved
primitive-types/impls/serde/CHANGELOG.md Outdated Show resolved Hide resolved
@ordian ordian merged commit 0db43ee into master Sep 10, 2024
6 checks passed
@ordian ordian deleted the pg/remove-from-impl-for-uint-types branch September 10, 2024 10:17
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.

4 participants