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 managed decimal support (as in the rust framework) #477

Merged
merged 21 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/smartcontracts/typesystem/managedDecimal.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { assert } from "chai";
import { ManagedDecimalType, ManagedDecimalValue } from "./managedDecimal";
import BigNumber from "bignumber.js";

describe("test managed decimal", () => {
it("should get correct metadata set", () => {
let type = new ManagedDecimalType("8");
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript allows us to do this trick:

type ManagedDecimalScale = number | "usize";

Can it be useful in our context?

const expectedMetadata = "8";

assert.equal(type.getMetadata(), expectedMetadata);
assert.isFalse(type.isVariable());
});

it("should get correct metadata set when variable", () => {
let type = new ManagedDecimalType("usize");
const expectedMetadata = "usize";

assert.equal(type.getMetadata(), expectedMetadata);
assert.isTrue(type.isVariable());
});

it("should return the expected values for scale and metadata", () => {
let firstValue = new ManagedDecimalValue(new BigNumber(1), 2, false);
let secondValue = new ManagedDecimalValue(new BigNumber(2), 2, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here / above / below - can also be const.

const expectedMetadata = "2";
const type = firstValue.getType() as ManagedDecimalType;

assert.equal(type.getMetadata(), expectedMetadata);
assert.isFalse(type.isVariable());
assert.equal(firstValue.getScale(), 2);
assert.equal(firstValue.toString(), "1.00");
assert.isFalse(firstValue.equals(secondValue));
});

it("should compare correctly two managed decimals even with different scale", () => {
let firstValue = new ManagedDecimalValue(new BigNumber(1.234), 3, false);
Copy link
Contributor

@andreibancioiu andreibancioiu Sep 9, 2024

Choose a reason for hiding this comment

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

For documentation purposes, when a BigNumber is created from non-integers, let's pass them as a string. Maybe in our examples, drop the BigNumber wrapping e.g.

new ManagedDecimalValue("1.234", 3, false);

Instead of:

new ManagedDecimalValue(new BigNumber(...), 3, false);

Since in the constructor of ManagedDecimalValue this is handled correctly anyway.

let secondValue = new ManagedDecimalValue(new BigNumber(12.34), 2, false);

assert.isFalse(firstValue.equals(secondValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a "positive" equality test / equal values, as well.

});

it("should set the correct scale when variable decimals", () => {
let value = new ManagedDecimalValue(new BigNumber(1.3), 2, true);
Copy link
Contributor

@andreibancioiu andreibancioiu Sep 9, 2024

Choose a reason for hiding this comment

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

Sorry for not thinking of this before - how about:

new ManagedDecimalValue("42.43", 8);
new ManagedDecimalValue("42.43", "usize");

Using the trick type ManagedDecimalScale = number | "usize".

const expectedMetadata = "usize";
const type = value.getType() as ManagedDecimalType;

assert.equal(type.getMetadata(), expectedMetadata);
assert.isTrue(type.isVariable());
assert.equal(value.toString(), "1.30");
assert.equal(value.getScale(), 2);
});
});
19 changes: 10 additions & 9 deletions src/smartcontracts/typesystem/managedDecimal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Type, TypedValue } from "./types";

export class ManagedDecimalType extends Type {
static ClassName = "ManagedDecimalType";
private readonly scale: any;

constructor(metadata: any) {
super("ManagedDecimal", undefined, undefined, metadata);
Expand Down Expand Up @@ -67,19 +66,21 @@ export class ManagedDecimalValue extends TypedValue {

export class ManagedDecimalSignedType extends Type {
static ClassName = "ManagedDecimalSignedType";
private readonly scale: number;

constructor(scale: number) {
super("ManagedDecimalSigned", undefined, undefined, scale);
this.scale = scale;
constructor(metadata: any) {
super("ManagedDecimalSigned", undefined, undefined, metadata);
}

getClassName(): string {
return ManagedDecimalType.ClassName;
}

getScale(): number {
return this.scale;
getMetadata(): string {
return this.metadata;
}

isVariable(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Useful both on type and value.

return this.metadata == "usize";
}
}

Expand All @@ -88,8 +89,8 @@ export class ManagedDecimalSignedValue extends TypedValue {
private readonly value: BigNumber;
private readonly scale: number;

constructor(value: BigNumber.Value, scale: number) {
super(new ManagedDecimalType(scale));
constructor(value: BigNumber.Value, scale: number, isVariableDecimals: boolean = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter name isVariableDecimals is not consistent with parameter name of ManagedDecimalValue constructor. Maybe isVariable for both of them?

super(new ManagedDecimalSignedType(isVariableDecimals ? "usize" : scale));
Copy link
Contributor

@andreibancioiu andreibancioiu Sep 9, 2024

Choose a reason for hiding this comment

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

Also in ManagedDecimalValue - is it worth to encapsulate this tricky logic somewhere? E.g. named constructor of the types? 🙈

this.value = new BigNumber(value);
this.scale = scale;
}
Expand Down
6 changes: 6 additions & 0 deletions src/smartcontracts/typesystem/types.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { I64Type, NumericalValue, U16Type, U32Type, U32Value } from "./numerical
import { StringType } from "./string";
import { TypeExpressionParser } from "./typeExpressionParser";
import { NullType, PrimitiveType, Type } from "./types";
import { ManagedDecimalSignedType, ManagedDecimalType } from "./managedDecimal";

describe("test types", () => {
let parser = new TypeExpressionParser();
Expand Down Expand Up @@ -63,6 +64,11 @@ describe("test types", () => {
parser.parse("Option<u32>").getFullyQualifiedName(),
"multiversx:types:Option<multiversx:types:u32>",
);
assert.equal(new ManagedDecimalType("8").getFullyQualifiedName(), "multiversx:types:ManagedDecimal*8*");
assert.equal(
new ManagedDecimalSignedType("8").getFullyQualifiedName(),
"multiversx:types:ManagedDecimalSigned*8*",
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also test for variable / usize.

});

it("types and values should have correct JavaScript class hierarchy", () => {
Expand Down
16 changes: 12 additions & 4 deletions src/smartcontracts/typesystem/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,26 @@ export class Type {
* Gets the fully qualified name of the type, to allow for better (efficient and non-ambiguous) type comparison within the custom typesystem.
*/
getFullyQualifiedName(): string {
return this.isGenericType() ? this.getFullNameForGeneric() : `multiversx:types:${this.getName()}`;
return this.isGenericType() || this.hasMetadata()
? this.getFullNameForGeneric()
: `multiversx:types:${this.getName()}`;
}

private getFullNameForGeneric(): string {
const hasTypeParameters = this.getTypeParameters.length > 0;
const hasTypeParameters = this.getTypeParameters().length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

this.isGenericType() could have been used, but this does the same thing.

const joinedTypeParameters = hasTypeParameters
? `${this.getTypeParameters()
.map((type) => type.getFullyQualifiedName())
.join(", ")}`
: "";
const baseName = `multiversx:types:${this.getName()}${joinedTypeParameters}`;
return this.metadata !== undefined ? `${baseName}*${this.metadata}*` : baseName;
let baseName = `multiversx:types:${this.getName()}`;
if (hasTypeParameters) {
baseName = `${baseName}<${joinedTypeParameters}>`;
}
if (this.metadata !== undefined) {
baseName = `${baseName}*${this.metadata}*`;
}
return baseName;
}

hasExactClass(className: string): boolean {
Expand Down
Loading