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

Use Vec<u8> as middle man to deserialize various types #483

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

contrun
Copy link
Collaborator

@contrun contrun commented Jan 15, 2025

While developing #474, we wanted an easy way to convert the old ChannelActorState struct to a new ChannelActorState struct (if the newer struct only adds a few fields with default value or removes a few obsolete fields). We tried serde_json which does not work because json can't serialize binary data natively.

We then tried cbor. To our surprise, current code can't even deserialize the serialized result of a ChannelActorState. This is shown in the commit 6f9d1bffdb5a7b41cbc127835c194cb44c37dcb4. The test test_serde_channel_actor_state_ciborium will fail with error Semantic(None, "invalid type: byte array, expected a borrowed byte array") (see github actions log).

After some investigation, I found we need to deserialize the data to Vec<u8> instead of &[u8]. I don't know why bincode deserializtion succeeded. But there are many similar issues on github (e.g. jonasbb/serde_with#372 (comment) and serde-rs/json#530 ). With the change in commit 99298012ef362ccfd0c9fa8832196cb09daad29e, I can successfully deserialize data with cbor.

I have tried the patch in this PR to successfully convert from one ChannelActorState to another ChannelActorState with some additional Option fields (code: chengyukang:refactor-migration-with-crate...contrun:fiber:refactor-migration-with-crate). I didn't add the simpler approach to #474 because we need to back port this PR to fiber v0.2.0, v0.2.1 and v0.3.0-rc1. There is also one field in v0.3.0-rc1 local_tlc_info which has no default value.

@contrun contrun requested a review from chenyukang January 15, 2025 08:19
@quake quake merged commit d59762b into nervosnetwork:develop Jan 15, 2025
18 checks passed
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