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

Add parity scale codec #547

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

Gauthamastro
Copy link

PR adds support of parity-scale-codec for rust-decimal.

References

  1. https://docs.substrate.io/reference/scale-codec/
  2. https://github.com/paritytech/parity-scale-codec

Copy link
Owner

@paupino paupino left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Sorry it's taken a while to look at this. Life - yada yada.

A couple of changes requested as well as the need to run makers format to pass compilation.

src/decimal.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@@ -124,6 +128,10 @@ pub struct UnpackedDecimal {
archive_attr(derive(Clone, Copy, Debug))
)]
#[cfg_attr(feature = "rkyv-safe", archive_attr(derive(CheckBytes)))]
#[cfg_attr(
feature = "scale-codec",
derive(Decode, Encode, TypeInfo, MaxEncodedLen),
Copy link
Owner

Choose a reason for hiding this comment

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

Side note (no change required): these derive attributes are causing a bit of code bloat. I want to rethink how we do this in v2. It may be by exposing a wrapper struct and aliasing it however all in all, this current approach isn't entirely scalable.

@Gauthamastro
Copy link
Author

@paupino I got busy with my work and lost this PR through the cracks. We use your library extensively, I would be great if we can merge this PR; I will fix your comments soon.

@Gauthamastro Gauthamastro requested a review from paupino June 22, 2023 02:18
Copy link
Owner

@paupino paupino left a comment

Choose a reason for hiding this comment

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

Sorry it's taken a little while to review this!

Added a few changes as suggestions in the PR to make it a little easier.
One thing I noticed: there is no test covering this or documentation for this feature.

As an example PR for adding a test and docs, please see this: https://github.com/paupino/rust-decimal/pull/583/files

Almost there!

@@ -78,6 +81,7 @@ rkyv = ["dep:rkyv"]
rkyv-safe = ["dep:bytecheck", "rkyv/validation"]
rocket-traits = ["dep:rocket"]
rust-fuzz = ["dep:arbitrary"]
scale-codec = ["parity-scale-codec-derive","parity-scale-codec","scale-info"]
Copy link
Owner

Choose a reason for hiding this comment

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

We're starting to use the updated format for feature dependencies:

Suggested change
scale-codec = ["parity-scale-codec-derive","parity-scale-codec","scale-info"]
scale-codec = ["dep:parity-scale-codec-derive","dep:parity-scale-codec","dep:scale-info"]

Cargo.toml Outdated
@@ -86,7 +90,7 @@ serde-str = ["serde-with-str"]
serde-with-arbitrary-precision = ["serde", "serde_json/arbitrary_precision", "serde_json/std"]
serde-with-float = ["serde"]
serde-with-str = ["serde"]
std = ["arrayvec/std", "borsh?/std", "bytecheck?/std", "byteorder?/std", "bytes?/std", "rand?/std", "rkyv?/std", "serde?/std", "serde_json?/std"]
std = ["arrayvec/std", "borsh?/std", "bytecheck?/std", "byteorder?/std", "bytes?/std", "rand?/std", "rkyv?/std", "serde?/std", "serde_json?/std", "parity-scale-codec?/std","scale-info?/std"]
Copy link
Owner

Choose a reason for hiding this comment

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

Just a simple reordering of inclusions:

Suggested change
std = ["arrayvec/std", "borsh?/std", "bytecheck?/std", "byteorder?/std", "bytes?/std", "rand?/std", "rkyv?/std", "serde?/std", "serde_json?/std", "parity-scale-codec?/std","scale-info?/std"]
std = ["arrayvec/std", "borsh?/std", "bytecheck?/std", "byteorder?/std", "bytes?/std", "parity-scale-codec?/std", "rand?/std", "rkyv?/std", "scale-info?/std", "serde?/std", "serde_json?/std"]

src/decimal.rs Outdated
@@ -120,6 +124,11 @@ pub struct UnpackedDecimal {
archive(compare(PartialEq)),
archive_attr(derive(Clone, Copy, Debug))
)]
#[cfg_attr(feature = "rkyv-safe", archive_attr(derive(CheckBytes)))]
Copy link
Owner

Choose a reason for hiding this comment

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

This is duplicated below

Suggested change
#[cfg_attr(feature = "rkyv-safe", archive_attr(derive(CheckBytes)))]

Comment on lines +128 to +131
#[cfg_attr(
feature = "scale-codec",
derive(Decode, Encode, TypeInfo, MaxEncodedLen),
)]
Copy link
Owner

Choose a reason for hiding this comment

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

This will fix the code formatting error in the build:

Suggested change
#[cfg_attr(
feature = "scale-codec",
derive(Decode, Encode, TypeInfo, MaxEncodedLen),
)]
#[cfg_attr(feature = "scale-codec", derive(Decode, Encode, TypeInfo, MaxEncodedLen))]

@@ -548,7 +557,7 @@ impl Decimal {
}

#[must_use]
pub(crate) const fn from_parts_raw(lo: u32, mid: u32, hi: u32, flags: u32) -> Decimal {
pub const fn from_parts_raw(lo: u32, mid: u32, hi: u32, flags: u32) -> Decimal {
Copy link
Owner

Choose a reason for hiding this comment

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

Reverting this line:

Suggested change
pub const fn from_parts_raw(lo: u32, mid: u32, hi: u32, flags: u32) -> Decimal {
pub(crate) const fn from_parts_raw(lo: u32, mid: u32, hi: u32, flags: u32) -> Decimal {

mkatychev and others added 20 commits July 16, 2023 23:39
* Added clippy configurations in .cargo/config.toml
* clippy --fix
* cargo clippy --fix -- -W clippy::uninlined_format_args
…aupino#619)

* Fixes issue whereby scale 29 is required however is optimized away
* Disable test for legacy ops
* Update to 1.1 borsh
* Fixes Borsh upgrade

---------

Co-authored-by: John Barker <dev@j16r.net>
Co-authored-by: Paul Mason <paul@paulmason.me>
…paupino#625)

* Fixes precision issue with to_f64 with some numbers without fractions
* Sign should not be applied to early for to_f64
Co-authored-by: Paul Mason <paul@paulmason.me>
schungx and others added 13 commits January 13, 2024 09:26
* Reimplement checked_powu.

* Add pow tests.

* Add short-cuts to powu.

* Fix x.powu(0)

* Refine powu implementation.

* Exclude a failing test that was caused by the deprecated legacy-ops feature

---------

Co-authored-by: Paul Mason <paul@paulmason.me>
…ino#636)

* Fixes error handling when second decimal place at tail position
* Added further bounds tests
* Version 1.34
* Corrected version in readme file
# Conflicts:
#	Cargo.toml
#	src/decimal.rs
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.

7 participants