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

getScalarCodecEnum does not encode an enum to its values in bytes #2319

Closed
lithdew opened this issue Mar 14, 2024 · 5 comments · Fixed by #2430
Closed

getScalarCodecEnum does not encode an enum to its values in bytes #2319

lithdew opened this issue Mar 14, 2024 · 5 comments · Fixed by #2430
Assignees
Labels
bug Something isn't working

Comments

@lithdew
Copy link

lithdew commented Mar 14, 2024

getScalarCodecEnum does not encode an enum to its values bytes.

This issue is a follow-up from #2296.

import { getScalarEnumCodec } from "@solana/web3.js";

export enum Currency {
    sol = 3,
    like = 5,
    usdc = 9,
}

const codec = getScalarEnumCodec(Currency);
console.log(codec.encode('sol')); // Should be 0x03, got 0x00
console.log(codec.encode('like')); // Should be 0x05, got 0x01
console.log(codec.encode('usdc')); // Should be 0x09, got 0x02
@lithdew lithdew added the bug Something isn't working label Mar 14, 2024
@lorisleiva lorisleiva self-assigned this Mar 14, 2024
@lorisleiva
Copy link
Collaborator

So I was writing a regression test and I'm no longer certain that the current behaviour is a bug.

Consider the following example:

enum ExplicitNumbers {
    Zero,
    Three = 3,
    Four,
    Size = 'six',
    Nine = 9,
    FourtyTwo = 42,
}

Whilst the values of the enum will be kept as-is, the keys of the enum is what gets stored and what allows you to have these complex mapping that otherwise wouldn't work — e.g. "six" cannot be encoded as a u8.

Even without such string literals, JavaScript values are used for more than just storing bytes and therefore, they likely will need to differ from their byte representation.

The issue is that Rust enums act differently and what comes after the equal sign is not a value like in JavaScript but a discriminant whose only purpose is to be used when storing the enum as bytes. So whilst they share the same syntax, their behaviour is different which is why some people might expect the behaviour you're suggesting here.

I'm not too sure how to solve this because I do think what you're suggesting should be possible but I don't think JavaScript enums lend themselves well to this use-case.

One solution could be to offer another helper function like getScalarEnumWithExplicitDiscriminantsCodec (or some much better name) that would a) throw if the provided enum contains non-numerical values and b) use the enum values as discriminants.

Wdyt?

@lorisleiva
Copy link
Collaborator

Actually, maybe it's just the case of adding an option like useValuesAsDiscriminants in the existing getScalarEnumCodec...

@lorisleiva
Copy link
Collaborator

TIL this is valid TypeScript... 🫠

enum MyEnum {
    Zero = 42,
    One = 42,
    Two = 42
}

console.log(MyEnum);
// {
//   "42": "Two",
//   "Zero": 42,
//   "One": 42,
//   "Two": 42
// } 

@lithdew
Copy link
Author

lithdew commented Apr 3, 2024

TIL this is valid TypeScript... 🫠

enum MyEnum {
    Zero = 42,
    One = 42,
    Two = 42
}

console.log(MyEnum);
// {
//   "42": "Two",
//   "Zero": 42,
//   "One": 42,
//   "Two": 42
// } 

Yeah ... it is why I tend to avoid using enums and wanted to instead opt in for, i.e., an object for defining enum codecs. There's a talk (linked below) that goes in-depth into why they're bad.

https://www.youtube.com/watch?v=0fTdCSH_QEU

lorisleiva added a commit that referenced this issue Apr 4, 2024
…iminators option (#2430)

This PR significantly solidifies the implementation of the `getEnumCodec` function by supporting and testing all its edge cases:

- Simple numerical enums — `enum { A, B }`.
- Explicit numerical enums — `enum { A, B = 5 }`.
- Explicit numerical enums with duplicated values — `enum { A = 5, B = 5 }`.
- Lexical enums — `enum { A = "A", B = "B" }`.
- Hybrid enums — `enum { A = "A", B = 5 }`.

Prior to this PR, only simple numerical enums and lexical enums were supported.

Additionally, this PR adds a new `useValuesAsDiscriminators` option to the `getEnumCodec` function which, when dealing with explicit numerical enums, stores the value of the enum variant instead of its index.

```ts
enum Numbers {
    One,
    Five = 5,
    Six,
    Nine = 9,
}

// Without useValuesAsDiscriminators.
const codecWithout = getEnumCodec(Numbers);
codecWithout.encode(Direction.One); // 0x00
codecWithout.encode(Direction.Five); // 0x01
codecWithout.encode(Direction.Six); // 0x02
codecWithout.encode(Direction.Nine); // 0x03

// With useValuesAsDiscriminators.
const codecWith = getEnumCodec(Numbers, { useValuesAsDiscriminators: true });
codecWith.encode(Direction.One); // 0x00
codecWith.encode(Direction.Five); // 0x05
codecWith.encode(Direction.Six); // 0x06
codecWith.encode(Direction.Nine); // 0x09
```

Note that when using the `useValuesAsDiscriminators` option on an enum that contains a lexical value, an error will be thrown.

```ts
enum Lexical {
    One,
    Two = 'two',
}
getEnumCodec(Lexical, { useValuesAsDiscriminators: true }); // Throws an error.
```

Fixes #2319
Copy link
Contributor

Because there has been no activity on this issue for 7 days since it was closed, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
2 participants