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

[fix] #1897: Removed usize/isize from serialization #1989

Conversation

SamHSmith
Copy link
Contributor

Description of the Change

In all the locations where a struct derives Serialize or Deserialize usize has been replaced
with either u64 or u32. No instances of isize existed. In the cases where code using the
data required a usize u32 was chosen as the replacement. There is no point in avoiding usize
in the on disk formats if 32-bit platforms can't read and use the values written.

Issue

Resolves #1897

Benefits

No chance of crashing due to serializing on 64-bit then deserializing on 32-bit or vice versa.

Possible Drawbacks

This change limits the size of certain configuration values to 4 billion. Probably a non issue.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1989 (7aa9552) into iroha2-dev (eaa2b7e) will decrease coverage by 0.23%.
The diff coverage is 85.18%.

❗ Current head 7aa9552 differs from pull request most recent head 788d8c3. Consider uploading reports for the commit 788d8c3 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev    #1989      +/-   ##
==============================================
- Coverage       78.00%   77.76%   -0.24%     
==============================================
  Files             176      174       -2     
  Lines           23951    23828     -123     
==============================================
- Hits            18684    18531     -153     
- Misses           5267     5297      +30     
Impacted Files Coverage Δ
cli/src/config.rs 100.00% <ø> (ø)
cli/src/torii/config.rs 100.00% <ø> (ø)
core/src/sumeragi/config.rs 79.45% <ø> (ø)
data_model/src/lib.rs 66.48% <ø> (ø)
logger/src/config.rs 90.00% <ø> (ø)
data_model/src/pagination.rs 77.94% <61.90%> (-2.06%) ⬇️
actor/src/lib.rs 89.93% <100.00%> (-0.47%) ⬇️
cli/src/torii/routing.rs 82.75% <100.00%> (ø)
core/src/block_sync.rs 90.36% <100.00%> (ø)
core/src/kura.rs 94.07% <100.00%> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa2b7e...788d8c3. Read the comment docs.

appetrosyan
appetrosyan previously approved these changes Mar 22, 2022
@SamHSmith SamHSmith changed the title [fix] #1897: Changed usize to u64/u32 on serde derived structs [fix] #1897: Removed usize/isize from serialization Mar 24, 2022
@SamHSmith SamHSmith force-pushed the remove_usize_from_serde_structs branch from ea99006 to ec59b12 Compare March 24, 2022 07:58
@appetrosyan appetrosyan requested review from mversic and Arjentix March 24, 2022 13:43
@SamHSmith SamHSmith force-pushed the remove_usize_from_serde_structs branch 2 times, most recently from b750861 to 2ec9034 Compare March 24, 2022 19:51
Arjentix
Arjentix previously approved these changes Mar 24, 2022
Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

Good job!

@SamHSmith SamHSmith force-pushed the remove_usize_from_serde_structs branch from 2ec9034 to d249dd2 Compare March 25, 2022 07:35
@appetrosyan appetrosyan requested a review from Arjentix March 25, 2022 07:36
appetrosyan
appetrosyan previously approved these changes Mar 25, 2022
@appetrosyan appetrosyan self-requested a review March 25, 2022 07:46
@SamHSmith SamHSmith force-pushed the remove_usize_from_serde_structs branch 4 times, most recently from 7df5d81 to fd84821 Compare March 25, 2022 10:02
appetrosyan
appetrosyan previously approved these changes Mar 25, 2022
Arjentix
Arjentix previously approved these changes Mar 25, 2022
@appetrosyan appetrosyan enabled auto-merge (rebase) March 25, 2022 11:43
@appetrosyan appetrosyan disabled auto-merge March 25, 2022 11:43
@SamHSmith SamHSmith dismissed stale reviews from Arjentix and appetrosyan via 7aa9552 March 26, 2022 12:12
Arjentix
Arjentix previously approved these changes Mar 28, 2022
@Arjentix Arjentix merged commit 85d881c into hyperledger-iroha:iroha2-dev Mar 28, 2022
@SamHSmith SamHSmith deleted the remove_usize_from_serde_structs branch March 29, 2022 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants