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 proto: fix kv value serialization #775

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Conversation

ryutamago
Copy link
Collaborator

@ryutamago ryutamago commented Jan 17, 2025

Context

Bug:
Running the example/counter.js sf fails on the second time, resulitng in derserialization error (Serde(AnyNotSupported))

Cause:
KvValue currently relies on #[bincode(with_serde)] for deserializing the serde_json::Value. This forcese bincode to invokes serde_json's deserialize method. For example, in serde_json, the Number type is deserialized using deserialized_any method.

However the deserializer, provided by bincode decoder, doesn't support deserializing "any" type, resulting in an error.

This PR fix this issue by manually implementing Decode/Encode method.

Task link

Description

  • implement Decode/Encode manually on KvValue
  • Removed the old (de)serialization method to string as it's not used anymore

Manually testing the PR

add unit tests for KvValue

cargo test --package jstz_proto --lib -- api::kv::tests --show-output 

manually tested by deploying sf counter.js and runnig it twice.

Before:

─(12:32:36 on leounoki-jstz-285/fix-kv-value ✹)──> cargo run --bin jstz run tezos://tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB/ --network dev --trace
    
Connected to trace smart function "tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB"
[🪵]: Counter: null
Status code: 200 OK
Headers: {}
┌─(~/tezos/jstz)───────────────────────────(leo@Leos-MacBook-Pro:s007)─┐
└─(12:32:47 on leounoki-jstz-285/fix-kv-value ✹)──> cargo run --bin jstz run tezos://tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB/ --network dev --trace
// fails with `AnyNotSupported` as `Number` cannot be deserialized by bincode

After:

─(12:32:36 on leounoki-jstz-285/fix-kv-value ✹)──> cargo run --bin jstz run tezos://tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB/ --network dev --trace
    
Connected to trace smart function "tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB"
[🪵]: Counter: null
Status code: 200 OK
Headers: {}
┌─(~/tezos/jstz)───────────────────────────(leo@Leos-MacBook-Pro:s007)─┐
└─(12:32:47 on leounoki-jstz-285/fix-kv-value ✹)──> cargo run --bin jstz run tezos://tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB/ --network dev --trace

Connected to trace smart function "tz1Lpp7y6gsXzPgoU6dd26uM1hrc5x2qprGB"
[🪵]: Counter: 0
Status code: 200 OK
Headers: {}

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 86.66667% with 8 lines in your changes missing coverage. Please review.

Project coverage is 49.33%. Comparing base (6e56c16) to head (7b7ca97).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/jstz_proto/src/api/kv.rs 86.66% 3 Missing and 5 partials ⚠️
Files with missing lines Coverage Δ
crates/jstz_proto/src/api/kv.rs 50.00% <86.66%> (+27.31%) ⬆️

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 6e56c16...7b7ca97. Read the comment docs.

@ryutamago ryutamago changed the title fix(jstz_proto): kv value encode/decode Jstz proto: fix kv value serialization Jan 17, 2025
@ryutamago ryutamago changed the base branch from refactor-value-trait to main January 17, 2025 15:04
@ryutamago ryutamago force-pushed the leounoki-jstz-285/fix-kv-value branch from 8116096 to f5b71a8 Compare January 17, 2025 15:20
@ryutamago ryutamago marked this pull request as ready for review January 17, 2025 15:24
@ryutamago ryutamago requested a review from zcabter January 17, 2025 15:24
@ryutamago ryutamago assigned ryutamago and zcabter and unassigned ryutamago Jan 17, 2025
@ryutamago ryutamago requested a review from johnyob January 17, 2025 15:30
@ryutamago ryutamago force-pushed the leounoki-jstz-285/fix-kv-value branch from f5b71a8 to 7b7ca97 Compare January 17, 2025 15:31
Copy link
Collaborator

@zcabter zcabter left a comment

Choose a reason for hiding this comment

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

Thanks for the fix

@zcabter zcabter merged commit 16791e5 into main Jan 17, 2025
5 checks passed
@zcabter zcabter deleted the leounoki-jstz-285/fix-kv-value branch January 17, 2025 16:45
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