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

[common] Add Canonical Serialization Spec #622

Merged
merged 4 commits into from Aug 27, 2019

Conversation

@davidiw
Copy link

commented Aug 19, 2019

Same as title. This defines the spec and gives concrete examples for
RawTransaction and its contents.

Motivation

Inform those that write clients how to implement the specification for their own domain.

Test Plan

Contains examples and those examples are embedded within new unit tests.

Related PRs

#606

@davidiw davidiw force-pushed the davidiw:serialization branch 2 times, most recently from 5aaf596 to 25cb24c Aug 19, 2019
@davidiw davidiw requested review from bmaurer, huitseeker, kphfb and weilei0107 Aug 19, 2019
@davidiw

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

@davidiw davidiw force-pushed the davidiw:serialization branch from 25cb24c to 1a341a7 Aug 19, 2019
@davidiw davidiw force-pushed the davidiw:serialization branch 3 times, most recently from e4b5264 to d6d482b Aug 20, 2019
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

See my comments on #606 (review) which actually apply here.

@davidiw davidiw force-pushed the davidiw:serialization branch 7 times, most recently from b124642 to 93f7b0b Aug 21, 2019
@davidiw davidiw force-pushed the davidiw:serialization branch from 682407f to 728761a Aug 23, 2019
@davidiw davidiw force-pushed the davidiw:serialization branch from 143bb53 to 3610196 Aug 24, 2019
Copy link
Contributor

left a comment

Looks good overall. Just a couple minor comments. I'd like another set of eyes on it since it's such a large diff though. Would also like Rain/Brandon to look at the macro usage. But overall, I accept

@davidiw davidiw requested review from bmwill, sunshowers and metajack Aug 26, 2019
@davidiw

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

@bmwill, @sunshowers, @metajack

I added you as reviewers for my usage of rust macros. Feedback is welcome :).

Copy link
Contributor

left a comment

Thanks! I left a few comments, how much investment in macros you're willing to go for would help fine-tune their relevance.

@davidiw davidiw force-pushed the davidiw:serialization branch 3 times, most recently from 544fa6e to 8e2be1d Aug 26, 2019
@davidiw davidiw force-pushed the davidiw:serialization branch 2 times, most recently from d511a46 to 6da3ae0 Aug 26, 2019
Copy link
Contributor

left a comment

I think it's best to keep the macros to the impl rather than to the trait.

macro_rules! impl_canonical_serializer {
($(($function:ident, $type:ty),)+ $(tuple: ($tuple_function:ident, $($tuple_type:ident)+),)+) => (
/// Trait for serializers that implement LCS
pub trait CanonicalSerializer {

This comment has been minimized.

Copy link
@huitseeker

huitseeker Aug 27, 2019

Contributor

Same remark as for the macro on pub trait CanonicalDeSerializer.

@libra libra deleted a comment from huitseeker Aug 27, 2019
David Wolinsky added 4 commits Aug 19, 2019
Same as title. This defines the spec and gives concrete examples (also
implemented in Rust) for RawTransaction and its contents.
Given the importance of canonical serialization within the code base, we
are restructuring it to make it to simplify access and understanding.

This diff moves each major component into its own separate module (file)
and re-exports so as to not break any dependencies.

In addition, major types are now fully exposed and implemented, namely
signed integers.
Macros reduce the duplicate code.

Some challenges arise though:
- macros cannot exist within a trait or an impl therefore the macro has
to wrap around it so we avoid macros for traits
- macros do not support concatenation of inputs, so the entire function
name must be specified
- macros involving indexes of tuples are just plain complicated and
probably less readable for our purposes
- tuple of size 1 is colliding with all types -- needs more
investigation
Proptest allows for broader testing than fixed sets of test vectors.
This diff migrates to proptest and also covers all of the types within
LCS which did not have coverage before.
@davidiw davidiw force-pushed the davidiw:serialization branch from 6da3ae0 to a101ebf Aug 27, 2019
Copy link
Contributor

left a comment

Kudos on the excellent, rigorous work on a relatively ungrateful task! Thanks @davidiw

@davidiw davidiw merged commit d54980c into libra:master Aug 27, 2019
1 check passed
1 check passed
commit-workflow Workflow: commit-workflow
Details
@davidiw davidiw deleted the davidiw:serialization branch Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.