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

[storage] Allow one variant enum to derive SpreadLayout #942

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

tash-2s
Copy link
Contributor

@tash-2s tash-2s commented Sep 27, 2021

I have an enum that has only one variant, and I want to store the enum value in the storage. I might add new variants later in development, so the enum is not useless.

The current implementation doesn't allow to derive the SpreadLayout trait for one variant enums, and I fixed it on this PR.

Sample code

#![cfg_attr(not(feature = "std"), no_std)]

use ink_lang as ink;

#[derive(
    Copy,
    Clone,
    scale::Encode,
    scale::Decode,
    ink_storage::traits::SpreadLayout,
    ink_storage::traits::PackedLayout,
)]
#[cfg_attr(
    feature = "std",
    derive(scale_info::TypeInfo, ink_storage::traits::StorageLayout)
)]
pub enum MyEnum {
    A,
}

#[ink::contract]
mod enum_test {
    use super::MyEnum;

    #[ink(storage)]
    pub struct EnumTest {
        value: MyEnum,
    }

    impl EnumTest {
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: MyEnum::A }
        }

        #[ink(message)]
        pub fn get(&self) -> MyEnum {
            self.value
        }
    }
}

Without this change, I get this error.

$ cargo +nightly contract build
 [1/5] Building cargo project
    Updating crates.io index
   Compiling enum_test v0.1.0 (/private/var/folders/zn/l2f569z56vnghtt524x1mv6w0000gn/T/cargo-contract_FM50JF)
error: proc-macro derive panicked
  --> /snip/enum_test/lib.rs:10:5
   |
10 |     ink_storage::traits::SpreadLayout,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: can only operate on enums

I have an enum that has only one variant, and I want to store the enum value in the storage. I might add new variants later in development, so the enum is not useless.

The current implementation doesn't allow to derive the `SpreadLayout` trait for one variant enums, and I fixed it on this PR.

# Sample code

```rust
#![cfg_attr(not(feature = "std"), no_std)]

use ink_lang as ink;

#[derive(
    Copy,
    Clone,
    scale::Encode,
    scale::Decode,
    ink_storage::traits::SpreadLayout,
    ink_storage::traits::PackedLayout,
)]
#[cfg_attr(
    feature = "std",
    derive(scale_info::TypeInfo, ink_storage::traits::StorageLayout)
)]
pub enum MyEnum {
    A,
}

#[ink::contract]
mod enum_test {
    use super::MyEnum;

    #[ink(storage)]
    pub struct EnumTest {
        value: MyEnum,
    }

    impl EnumTest {
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: MyEnum::A }
        }

        #[ink(message)]
        pub fn get(&self) -> MyEnum {
            self.value
        }
    }
}
```

Without this change, I get this error.

```
$ cargo +nightly contract build
 [1/5] Building cargo project
    Updating crates.io index
   Compiling enum_test v0.1.0 (/private/var/folders/zn/l2f569z56vnghtt524x1mv6w0000gn/T/cargo-contract_FM50JF)
error: proc-macro derive panicked
  --> /snip/enum_test/lib.rs:10:5
   |
10 |     ink_storage::traits::SpreadLayout,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: can only operate on enums
```
Copy link
Collaborator

@Robbepop Robbepop 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 the PR. All in all LGTM.

crates/storage/derive/src/spread_layout.rs Outdated Show resolved Hide resolved
tash-2s and others added 2 commits September 27, 2021 16:31
Co-authored-by: Robin Freyler <robbepop@web.de>
@codecov-commenter
Copy link

Codecov Report

Merging #942 (78b31e4) into master (beaf989) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   82.88%   82.86%   -0.03%     
==========================================
  Files         177      177              
  Lines        8058     8059       +1     
==========================================
- Hits         6679     6678       -1     
- Misses       1379     1381       +2     
Impacted Files Coverage Δ
crates/storage/derive/src/spread_layout.rs 98.50% <100.00%> (+0.02%) ⬆️
...ates/storage/src/collections/hashmap/fuzz_tests.rs 97.87% <0.00%> (-2.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beaf989...78b31e4. Read the comment docs.

@tash-2s
Copy link
Contributor Author

tash-2s commented Sep 27, 2021

Thank you for your review. CI passed.

Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you a lot for writing the test case!
This PR really is an improvement. :)

@Robbepop Robbepop merged commit fd94d38 into use-ink:master Sep 27, 2021
@tash-2s tash-2s deleted the one-variant-enum branch September 27, 2021 08:53
tash-2s added a commit to tash-2s/open-emoji-battler that referenced this pull request Oct 2, 2021
ascjones pushed a commit that referenced this pull request Oct 11, 2021
* [storage] Allow one variant enum to derive SpreadLayout

I have an enum that has only one variant, and I want to store the enum value in the storage. I might add new variants later in development, so the enum is not useless.

The current implementation doesn't allow to derive the `SpreadLayout` trait for one variant enums, and I fixed it on this PR.

# Sample code

```rust
#![cfg_attr(not(feature = "std"), no_std)]

use ink_lang as ink;

#[derive(
    Copy,
    Clone,
    scale::Encode,
    scale::Decode,
    ink_storage::traits::SpreadLayout,
    ink_storage::traits::PackedLayout,
)]
#[cfg_attr(
    feature = "std",
    derive(scale_info::TypeInfo, ink_storage::traits::StorageLayout)
)]
pub enum MyEnum {
    A,
}

#[ink::contract]
mod enum_test {
    use super::MyEnum;

    #[ink(storage)]
    pub struct EnumTest {
        value: MyEnum,
    }

    impl EnumTest {
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: MyEnum::A }
        }

        #[ink(message)]
        pub fn get(&self) -> MyEnum {
            self.value
        }
    }
}
```

Without this change, I get this error.

```
$ cargo +nightly contract build
 [1/5] Building cargo project
    Updating crates.io index
   Compiling enum_test v0.1.0 (/private/var/folders/zn/l2f569z56vnghtt524x1mv6w0000gn/T/cargo-contract_FM50JF)
error: proc-macro derive panicked
  --> /snip/enum_test/lib.rs:10:5
   |
10 |     ink_storage::traits::SpreadLayout,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: can only operate on enums
```

* Update crates/storage/derive/src/spread_layout.rs

Co-authored-by: Robin Freyler <robbepop@web.de>

* cargo fmt

Co-authored-by: Robin Freyler <robbepop@web.de>
ascjones added a commit that referenced this pull request Oct 11, 2021
* Bump scale-info requirement to 0.9

* Update manual TypeInfo impls

* Use decode feature of scale-info

* scale-info 1.0

* Fix tests, 0 indexing on type registry

* Update some missing examples

* TMP: use ink rc5 for compat with cargo-contract

* Update RELEASES.md

* Bump versions back to 1.0

* Introduce versioning of ink metadata format

* Update RELEASES.md

Co-authored-by: Michael Müller <michi@parity.io>

* Update crates/metadata/src/lib.rs

Co-authored-by: Michael Müller <michi@parity.io>

* Clippy: allow large enum variant

* [storage] Allow one variant enum to derive SpreadLayout (#942)

* [storage] Allow one variant enum to derive SpreadLayout

I have an enum that has only one variant, and I want to store the enum value in the storage. I might add new variants later in development, so the enum is not useless.

The current implementation doesn't allow to derive the `SpreadLayout` trait for one variant enums, and I fixed it on this PR.

# Sample code

```rust
#![cfg_attr(not(feature = "std"), no_std)]

use ink_lang as ink;

#[derive(
    Copy,
    Clone,
    scale::Encode,
    scale::Decode,
    ink_storage::traits::SpreadLayout,
    ink_storage::traits::PackedLayout,
)]
#[cfg_attr(
    feature = "std",
    derive(scale_info::TypeInfo, ink_storage::traits::StorageLayout)
)]
pub enum MyEnum {
    A,
}

#[ink::contract]
mod enum_test {
    use super::MyEnum;

    #[ink(storage)]
    pub struct EnumTest {
        value: MyEnum,
    }

    impl EnumTest {
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: MyEnum::A }
        }

        #[ink(message)]
        pub fn get(&self) -> MyEnum {
            self.value
        }
    }
}
```

Without this change, I get this error.

```
$ cargo +nightly contract build
 [1/5] Building cargo project
    Updating crates.io index
   Compiling enum_test v0.1.0 (/private/var/folders/zn/l2f569z56vnghtt524x1mv6w0000gn/T/cargo-contract_FM50JF)
error: proc-macro derive panicked
  --> /snip/enum_test/lib.rs:10:5
   |
10 |     ink_storage::traits::SpreadLayout,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: message: can only operate on enums
```

* Update crates/storage/derive/src/spread_layout.rs

Co-authored-by: Robin Freyler <robbepop@web.de>

* cargo fmt

Co-authored-by: Robin Freyler <robbepop@web.de>

* CI: remove gcda files from cache (#943)

* Update pretty_assertions requirement from 0.7.1 to 1.0.0 (#944)

Updates the requirements on [pretty_assertions](/~https://github.com/colin-kiegel/rust-pretty-assertions) to permit the latest version.
- [Release notes](/~https://github.com/colin-kiegel/rust-pretty-assertions/releases)
- [Changelog](/~https://github.com/colin-kiegel/rust-pretty-assertions/blob/main/CHANGELOG.md)
- [Commits](rust-pretty-assertions/rust-pretty-assertions@v0.7.1...v1.0.0)

---
updated-dependencies:
- dependency-name: pretty_assertions
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add 'versioning' to spellcheck dict

Co-authored-by: Michael Müller <michi@parity.io>
Co-authored-by: tash-2s <81064017+tash-2s@users.noreply.github.com>
Co-authored-by: Robin Freyler <robbepop@web.de>
Co-authored-by: Denis Pisarev <denis.pisarev@parity.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
forbesus added a commit to forbesus/OpenEmojiBattler that referenced this pull request Feb 18, 2024
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.

3 participants