-
Notifications
You must be signed in to change notification settings - Fork 25
ZKP-43 Implement serde_deprecated flag for concordium-base repo #843
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
base: main
Are you sure you want to change the base?
Conversation
ae9942b to
966b6cd
Compare
966b6cd to
07508c6
Compare
| use crate::common::cbor::{cbor_decode, cbor_encode, value}; | ||
|
|
||
| /// Serialize a `HashMap<String, value::Value>` as hex-encoded CBOR. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to put the map_hex_cbor_values module behind the feature flag instead of allowing dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seems that this module should just be removed. Since it is not used. But would be nice with a separate PR for fixing this. So in this PR I suggest leaving the warning there. And then raise a PR to fix the warnings.
| /// | ||
| /// * A problem decoding to a group element | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(dead_code)] |
| /// | ||
| /// * A problem decompressing to a scalar or group element, | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(dead_code)] |
Same here, remove the error module in a separate PR
| } | ||
|
|
||
| /// Serialize `Bytes` as a hex string. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to put this behind a feature flag. We should very rarely need to use #[allow(dead_code)]
| } | ||
|
|
||
| /// Deserialize `Bytes` from a hex string. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, use the feature flag to guard the function
| use itertools::izip; | ||
| use std::rc::Rc; | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(dead_code)] |
Lets keep it here for now and ignore the warning
| }, | ||
| }; | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[allow(dead_code)] |
Same
allanbrondum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good
a42a040 to
e5928f6
Compare
Purpose
Sub task of ZKP-41. To eventually deprecate use of
serdeon non primitive data types.Changes
Guard non primitive
serde::Serialize, serde::Deserializebehind a flag. List of data types can be found hereChecklist
hard-to-understand areas.