Non-breaking change: add support for "counted-variadic" #323

andreibancioiu · 2023-08-29T13:46:43Z

Should fix: #316.

ArgsSerializer and the type inference system have been adjusted to handle counted-variadic arguments, as well. Though, note that (due to constraints brought by the current implementation of the type inference system) counted variadic arguments must be explicitly typed when calling contract methods. E.g.

contract.methods.doSomething([42, VariadicValue.fromItemsCounted(...), "foobar"]);

Or:

contract.methods.doSomething([42, new VariadicValue(...), "foobar"]);

In order to create a counted-variadic type, pass true as the second parameter for the VariadicType constructor. E.g.

const type = new VariadicType(..., true);

andreibancioiu · 2023-08-30T21:27:28Z

src/smartcontracts/nativeSerializer.ts

+    function repackNonCountedVariadicParameters(args: any[], endpoint: EndpointDefinition) {
+        let parameters = endpoint.input;
+
+        // TODO: Remove after first review. Temporarily left this way to make the review easier.


⚠️ Remove if (true) after first review! ⚠️

andreibancioiu · 2023-08-30T21:28:10Z

src/smartcontracts/typesystem/variadic.ts

+        return this.createFromItems(items, false);
+    }
+
+    static fromItemsCounted(...items: TypedValue[]): VariadicValue {


New utility function.

andreibancioiu · 2023-08-30T21:28:53Z

src/smartcontracts/typesystem/typeMapper.spec.ts

@@ -76,11 +74,11 @@ describe("test mapper", () => {
        assert.deepEqual(mappedType, new ArrayVecType(size, typeParameter));
    }

-    function testMapping(expression: string, constructor: TypeConstructor, typeParameters: Type[] = []) {
+    function testMapping(expression: string, expectedType: Type) {


Just a refactoring, simplification in this tests file.

andreibancioiu · 2023-08-31T06:49:14Z

src/smartcontracts/argSerializer.ts

+                        typedValues.push(readValue(type.getFirstTypeParameter()));
+                    }
+                } else {
+                    while (!hasReachedTheEnd()) {


Non-counted variadic arguments (regular variadic arguments) can only be at the tail of the arguments.

andreibancioiu · 2023-08-31T07:09:02Z

src/smartcontracts/argSerializer.ts

+
+                if (variadicType.isCounted) {
+                    const countValue = new U32Value(valueAsVariadic.getItems().length);
+                    buffers.push(self.codec.encodeTopLevel(countValue));


@count@var1@var2@var3.

Maybe the variadic arg handling can be pulled into a function so we have less complexity in the if/else branches.

Q: do you like the else if {} approach better? Since every branch returns, we could also have less indenting doing:

if (condition) { ... return result } if (condition2) { ... return result2; }

Indeed, fixed. Extracted handling into separate (though "nested") functions. Also re-organized branches for less indenting 👍

andreibancioiu · 2023-08-31T07:13:00Z

src/smartcontracts/nativeSerializer.spec.ts

+        const endpoint = new EndpointDefinition("foo", inputParameters, [], endpointModifiers);
+
+        // Implicit counted-variadic (not supported).
+        assert.throws(() => NativeSerializer.nativeToTypedValues([8, 9, 10, "a", "b", "c"], endpoint), ErrInvalidArgument);


Cannot be supported in the current implementation of the NativeSerializer, due to args-packing ambiguity.

andreibancioiu · 2023-08-31T07:13:55Z

src/smartcontracts/nativeSerializer.spec.ts

+            VariadicValue.fromItemsCounted(new U32Value(8), new U32Value(9), new U32Value(10)),
+            VariadicValue.fromItemsCounted(BytesValue.fromUTF8("a"), BytesValue.fromUTF8("b"), BytesValue.fromUTF8("c"))


It's possible to have multiple counted variadic arguments (users are required to pass them in the explicitly typed form).

andreibancioiu · 2023-08-31T07:15:10Z

src/smartcontracts/nativeSerializer.ts

+        if (hasNonCountedVariadicParameter(endpoint)) {
+            args = repackNonCountedVariadicParameters(args, endpoint);
+        } else {
+            // Repacking makes sense (it's possible) only for regular, non-counted variadic parameters.


No repacking: there are no regular (non-counted) variadic parameters.

andreibancioiu · 2023-08-31T07:18:26Z

src/smartcontracts/nativeSerializer.ts

-    function toVariadicValue(native: any, type: Type, errorContext: ArgumentErrorContext): TypedValue {
+    function toVariadicValue(native: any, type: VariadicType, errorContext: ArgumentErrorContext): TypedValue {
+        if (type.isCounted) {
+            throw new ErrInvalidArgument(`Counted variadic arguments must be explicitly typed. E.g. use "VariadicValue.fromItemsCounted()" or "new VariadicValue()"`);


In other words, the type inference system won't perform any type inference for counted-variadic.

andreibancioiu · 2023-08-31T07:59:45Z

src/smartcontracts/typesystem/variadic.ts

@@ -2,6 +2,20 @@ import { Type, TypeCardinality, TypedValue, TypePlaceholder } from "./types";

 export class VariadicType extends Type {
    static ClassName = "VariadicType";
+    public readonly isCounted: boolean;
+
+    constructor(typeParameter: Type, isCounted: boolean = false) {


Theoretically, not a breaking change.

andreibancioiu · 2023-08-31T08:50:13Z

Related (examples): multiversx/mx-sdk-js-examples#27.

andreibancioiu added 2 commits August 29, 2023 16:39

Preparatory refactoring on tests.

2a26b8a

Sketch counted variadic types.

ee375cc

andreibancioiu self-assigned this Aug 29, 2023

andreibancioiu added 5 commits August 29, 2023 17:09

Add new unit test.

9d7b4fd

Add extra tests.

e1c4ae7

Handle multiple counted variadic args. Work in progress.

407bce7

Merge branch 'main' into counted-variadic

6e2e695

Handle counted vs. regular variadic. Handle multiple counted variadic.

84e2782

andreibancioiu commented Aug 30, 2023

View reviewed changes

Fix after self-review.

dc18d3a

andreibancioiu marked this pull request as ready for review August 31, 2023 06:47

andreibancioiu commented Aug 31, 2023

View reviewed changes

Fix after self review.

a79a6e8

andreibancioiu commented Aug 31, 2023

View reviewed changes

andreibancioiu changed the title ~~Add support for "counted-variadic"~~ Non-breaking change: add support for "counted-variadic" Aug 31, 2023

dragos-rebegea previously approved these changes Aug 31, 2023

View reviewed changes

Fix after review.

56edcd6

andreibancioiu dismissed dragos-rebegea’s stale review via 56edcd6 August 31, 2023 11:48

andreibancioiu requested review from ccorcoveanu and dragos-rebegea August 31, 2023 11:48

dragos-rebegea approved these changes Aug 31, 2023

View reviewed changes

ccorcoveanu approved these changes Aug 31, 2023

View reviewed changes

andreibancioiu merged commit d8b4893 into main Aug 31, 2023
1 check passed

andreibancioiu deleted the counted-variadic branch August 31, 2023 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-breaking change: add support for "counted-variadic" #323

Non-breaking change: add support for "counted-variadic" #323

andreibancioiu commented Aug 29, 2023 •

edited

Loading

andreibancioiu Aug 30, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023

andreibancioiu Aug 30, 2023

andreibancioiu Aug 30, 2023

andreibancioiu Aug 31, 2023

andreibancioiu Aug 31, 2023 •

edited

Loading

ccorcoveanu Aug 31, 2023

andreibancioiu Aug 31, 2023

andreibancioiu Aug 31, 2023

andreibancioiu Aug 31, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023

andreibancioiu Aug 31, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023

andreibancioiu commented Aug 31, 2023

		VariadicValue.fromItemsCounted(new U32Value(8), new U32Value(9), new U32Value(10)),
		VariadicValue.fromItemsCounted(BytesValue.fromUTF8("a"), BytesValue.fromUTF8("b"), BytesValue.fromUTF8("c"))

Non-breaking change: add support for "counted-variadic" #323

Non-breaking change: add support for "counted-variadic" #323

Conversation

andreibancioiu commented Aug 29, 2023 • edited Loading

andreibancioiu Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu commented Aug 31, 2023

andreibancioiu commented Aug 29, 2023 •

edited

Loading

andreibancioiu Aug 30, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023 •

edited

Loading

andreibancioiu Aug 31, 2023 •

edited

Loading