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

Jstz core: Introduce BinEncodable trait for consistent serialization #773

Conversation

ryutamago
Copy link
Collaborator

@ryutamago ryutamago commented Jan 16, 2025

Context

The original Value design had the following issues:

  • The Value trait required types to implement encode()
  • But deserialization happened through standalone deserialize() function
  • This meant a type could implement Value::encode() using one serialization format
    while the standalone deserialize() used a different format
  • This could lead to silent failures where encoded data cannot be properly decoded

This PR ensures consistent serialization format through BinEncodable trait, removing potential for format mismatches. Additionally, it simplifies the storage/transaction APIs as the Value trait is bounded by the BinEncodable trait

related comment:
#767 (comment)

Task

Description

This PR introduces a dedicated BinEncodable trait to handle serialization consistently:

  1. Creates a new BinEncodable trait with both encode/decode methods
  2. Makes Value trait use BinEncodable as a bound
  3. Provides automatic BinEncodable implementation for types with bincode's Encode + Decode traits
  4. Removes standalone serialize/deserialize functions as well as the default encode config
  5. Simplified the storage and transaction apis
  6. Add docs to Value trait

Manually testing the PR

Unit tests for encoding:

cargo test --package jstz_core --lib -- encoding::tests --show-output

Deployed and ran sf manually.

@zcabter zcabter force-pushed the ryan-jstz-281/use-bincode-for-value-storage branch from 6dc9b92 to b13ddf3 Compare January 16, 2025 16:48
@zcabter zcabter force-pushed the ryan-jstz-281/use-bincode-for-value-storage branch from b13ddf3 to 2738895 Compare January 16, 2025 18:27
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 68.75000% with 30 lines in your changes missing coverage. Please review.

Project coverage is 48.49%. Comparing base (635c6ba) to head (53b96fc).

Files with missing lines Patch % Lines
crates/jstz_core/src/kv/transaction.rs 61.90% 7 Missing and 1 partial ⚠️
crates/jstz_node/src/services/accounts.rs 0.00% 8 Missing ⚠️
crates/jstz_core/src/bin_encodable.rs 83.33% 4 Missing and 2 partials ⚠️
crates/jstz_node/src/services/operations.rs 0.00% 4 Missing ⚠️
crates/jstz_core/src/kv/mod.rs 75.00% 0 Missing and 2 partials ⚠️
crates/jstz_core/src/kv/outbox.rs 0.00% 0 Missing and 1 partial ⚠️
crates/jstz_kernel/src/inbox.rs 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
crates/jstz_core/src/kv/value.rs 57.14% <ø> (+2.59%) ⬆️
crates/jstz_crypto/src/public_key_hash.rs 79.19% <100.00%> (ø)
crates/jstz_kernel/src/lib.rs 88.23% <100.00%> (ø)
crates/jstz_mock/src/host.rs 97.43% <100.00%> (+0.37%) ⬆️
crates/jstz_proto/src/executor/fa_deposit.rs 94.72% <ø> (ø)
crates/jstz_proto/src/executor/fa_withdraw.rs 94.57% <ø> (ø)
crates/jstz_proto/src/executor/smart_function.rs 85.81% <100.00%> (ø)
crates/jstz_proto/src/operation.rs 60.00% <100.00%> (-2.88%) ⬇️
crates/jstz_core/src/kv/outbox.rs 90.23% <0.00%> (ø)
crates/jstz_kernel/src/inbox.rs 74.32% <0.00%> (+0.45%) ⬆️
... and 5 more

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@ryutamago ryutamago changed the title Encapsulate encode/decode Jstz core: Introduce BinEncodable trait for consistent serialization Jan 16, 2025
@ryutamago ryutamago force-pushed the refactor-value-trait branch 3 times, most recently from ad5d8f2 to 1c68b7b Compare January 16, 2025 21:31
@ryutamago ryutamago force-pushed the refactor-value-trait branch from 1c68b7b to 53b96fc Compare January 17, 2025 10:43
@ryutamago ryutamago marked this pull request as ready for review January 17, 2025 10:44
@ryutamago ryutamago requested a review from zcabter January 17, 2025 11:42
@ryutamago ryutamago assigned ryutamago and zcabter and unassigned ryutamago Jan 17, 2025
@zcabter zcabter force-pushed the ryan-jstz-281/use-bincode-for-value-storage branch from 2738895 to 43d81da Compare January 17, 2025 14:47
@ryutamago
Copy link
Collaborator Author

closing this as it's cherry picked into #767

@ryutamago ryutamago closed this Jan 17, 2025
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.

2 participants