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

Introduce a required #[borsh(use_discriminant = true|false)] enum attribute when discriminants are specified #147

Closed
frol opened this issue Jun 7, 2023 · 4 comments · Fixed by #183
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@frol
Copy link
Collaborator

frol commented Jun 7, 2023

Context:

Before #138 (#137), borsh did not respect explicit enum discriminant values:

#[derive(BorshSerialize)]
enum MyEnum {
    A = 20,
}

assert_eq!(MyEnum::A as u8, 20);
assert_eq!(MyEnum::A.try_to_vec().unwrap(), vec![20]);

In #138, the default behavior changed and while it was marked as a breaking change by releasing 0.11.0 version, it was flagged that it may silently break someone's code, which is totally valid concern, thus, we decided to use a required #[borsh(use_discriminant = true|false)] enum attribute to resolve ambiguity, so after 0.11.1 release, the expected behavior would be that the following code will raise a compilation error:

#[derive(BorshSerialize)]
enum MyEnum {
    A = 20,
}

This will generate the code equivalent to the borsh before 0.11.0:

#[derive(BorshSerialize)]
#[borsh(use_discriminant = false)]
enum MyEnum {
    A = 20,
}

This will generate the code equivalent to the implementation in #138:

#[derive(BorshSerialize)]
#[borsh(use_discriminant = true)]
enum MyEnum {
    A = 20,
}

In the future, we will default to true and make this attribute optional, and eventually deprecate it.

@frol frol added enhancement New feature or request good first issue Good for newcomers labels Jun 7, 2023
@frol
Copy link
Collaborator Author

frol commented Jun 7, 2023

@iho I suggest you take this one next.

cc @mina86

@mina86
Copy link
Contributor

mina86 commented Jun 8, 2023

One thing worth noting here is that to make it as painless as possible for users, ideally code would detect if enum has discriminants specified. So #[derive(BorshSerialize)] enum Foo { A, B } would compile but #[derive(BorshSerialize)] enum Foo { A = 10, B } wouldn’t.

By the way, another use case for the annotation is enum Foo { A = 256 } which cannot be serialised when discriminants are used.

@iho
Copy link
Contributor

iho commented Jun 19, 2023

@mina86 I tried to do as you suggested by detecting explicit discriminate and emitting compiler errors. #148

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jul 31, 2023

closed by #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment