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 enums type inference in NativeSerializer and fix Enum valueof() #300

Merged
merged 3 commits into from
Jul 14, 2023

Conversation

nvergez
Copy link
Contributor

@nvergez nvergez commented Jun 19, 2023

I have implemented type inference for enum types in the NativeSerializer.

  • If a number is provided, it will be processed as the discriminant.
  • If a string is provided, it will be processed as the name.
  • For enums with fields, a JavaScript object needs to be provided in the following format:
    { name: string; fields: any}
    The key of each property in the fields object must correspond to the name in the FieldDefinition.

I have also updated the valueof() method of EnumValue. When we receive an ABI file from a smart contract built with complex enums, the names of the fields are the same as those in the Rust code. This means they can be either numbers or strings.

This indicates that the following code cannot work correctly with a string as a name:

let result: any = { name: this.name, fields: [] };
this.fields.forEach((field) => (result.fields[field.name] = field.value.valueOf()));

I believe the fields property should be an object rather than an array, but this change would definitely lead to a breaking change. However, I think my fix doesn't break anything by using the index instead of the name itself.

Copy link
Contributor

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

@nvergez, thanks a lot for the contribution 🎉

Code looks great 🌟

This changes the behavior of enum.valueOf(). Indeed, the previous behavior wasn't correct (as you have said, it should have been an object, instead). I'll analyze this a bit more, then come back with a re-review.

In the meantime, do you think you can solve the Git conflicts?

@@ -149,7 +149,7 @@ export class EnumValue extends TypedValue {
valueOf() {
let result: any = { name: this.name, fields: [] };

this.fields.forEach((field) => (result.fields[field.name] = field.value.valueOf()));
this.fields.forEach((field, index) => (result.fields[index] = field.value.valueOf()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have to analyze this a bit more, then come back.

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned by you in the description, the existing behavior is not correct (at least, not practical) and we should change it to return fields as an object, in the future (major release).

The change in this PR is not breaking 👍

@andreibancioiu
Copy link
Contributor

@nvergez, do you think you can also add the following unit test in enum.spec.ts?

The first two assertions should normally hold with your changes, as well. Then, the third one would have to be adjusted, since your code uses indexes to populate result.fields.

Indeed, it seems that your code did not change the existing (incorrect) behavior of enum.valueOf(), and only extended it to work with properly-named enum fields, as well.

it.only("should get valueOf()", () => {
    // Define variants
    let greenVariant = new EnumVariantDefinition("Green", 0, [
        new FieldDefinition("0", "red component", new U8Type()),
        new FieldDefinition("1", "green component", new U8Type()),
        new FieldDefinition("2", "blue component", new U8Type()),
    ]);

    let orangeVariant = new EnumVariantDefinition("Orange", 1, [
        new FieldDefinition("0", "hex code", new StringType())
    ]);

    let yellowVariant = new EnumVariantDefinition("Yellow", 2, [
        new FieldDefinition("red", "red component", new U8Type()),
        new FieldDefinition("green", "green component", new U8Type()),
        new FieldDefinition("blue", "blue component", new U8Type()),
    ]);

    // Define enum type
    let enumType = new EnumType("Colour", [
        greenVariant,
        orangeVariant,
        yellowVariant
    ]);

    // Create enum values
    let green = new EnumValue(enumType, greenVariant, [
        new Field(new U8Value(0), "0"),
        new Field(new U8Value(255), "1"),
        new Field(new U8Value(0), "2")
    ]);

    let orange = new EnumValue(enumType, orangeVariant, [
        new Field(new StringValue("#FFA500"), "0")
    ]);

    let yellow = new EnumValue(enumType, yellowVariant, [
        new Field(new U8Value(255), "red"),
        new Field(new U8Value(255), "green"),
        new Field(new U8Value(0), "blue")
    ]);

    // Test valueOf()
    assert.deepEqual(green.valueOf(), {
        "name": "Green",
        "fields": [
            new BigNumber(0),
            new BigNumber(255),
            new BigNumber(0)
        ]
    });

    assert.deepEqual(orange.valueOf(), {
        "name": "Orange",
        "fields": [
            "#FFA500"
        ]
    });

    // Incorrect (proper field names aren't handled correctly).
    // Normally, fixed by your PR.
    assert.deepEqual(yellow.valueOf(), {
        "name": "Yellow",
        "fields": [
        ]
    });
});

In v13, we should definitely have fields as an object, thank you 🙏

@nvergez
Copy link
Contributor Author

nvergez commented Jul 13, 2023

In the meantime, do you think you can solve the Git conflicts?

Resolving the conflict will cause a test to fail in this PR because it will add the test from main to nativeSerializer.spec.ts but not the related fix in nativeSerializer.ts.

I'm unsure whether you would prefer to merge main into this branch, or just resolve the conflict and deal with a failing test.

@andreibancioiu
Copy link
Contributor

@nvergez, normally, the main branch should be merged into this one, and the conflicts should be solved. Re-basing should work, as well.

What test would fail?

Copy link
Contributor

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@@ -149,7 +149,7 @@ export class EnumValue extends TypedValue {
valueOf() {
let result: any = { name: this.name, fields: [] };

this.fields.forEach((field) => (result.fields[field.name] = field.value.valueOf()));
this.fields.forEach((field, index) => (result.fields[index] = field.value.valueOf()));
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned by you in the description, the existing behavior is not correct (at least, not practical) and we should change it to return fields as an object, in the future (major release).

The change in this PR is not breaking 👍

@andreibancioiu
Copy link
Contributor

@andreibancioiu andreibancioiu merged commit 135b511 into multiversx:main Jul 14, 2023
@andreibancioiu
Copy link
Contributor

@nvergez, released here:

https://github.com/multiversx/mx-sdk-js-core/releases/tag/v12.6.0

Thanks a lot for the contribution!

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