-
Notifications
You must be signed in to change notification settings - Fork 37
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
Non-breaking change: add support for "counted-variadic" #323
Changes from 7 commits
2a26b8a
ee375cc
9d7b4fd
e1c4ae7
407bce7
6e2e695
84e2782
dc18d3a
a79a6e8
56edcd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { ARGUMENTS_SEPARATOR } from "../constants"; | ||
import { BinaryCodec } from "./codec"; | ||
import { EndpointParameterDefinition, Type, TypedValue } from "./typesystem"; | ||
import { EndpointParameterDefinition, Type, TypedValue, U32Type, U32Value } from "./typesystem"; | ||
import { OptionalType, OptionalValue } from "./typesystem/algebraic"; | ||
import { CompositeType, CompositeValue } from "./typesystem/composite"; | ||
import { VariadicType, VariadicValue } from "./typesystem/variadic"; | ||
|
@@ -74,13 +74,22 @@ export class ArgSerializer { | |
let typedValue = readValue(type.getFirstTypeParameter()); | ||
return new OptionalValue(type, typedValue); | ||
} else if (type.hasExactClass(VariadicType.ClassName)) { | ||
let variadicType = <VariadicType>type; | ||
let typedValues = []; | ||
|
||
while (!hasReachedTheEnd()) { | ||
typedValues.push(readValue(type.getFirstTypeParameter())); | ||
if (variadicType.isCounted) { | ||
const count: number = readValue(new U32Type()).valueOf().toNumber(); | ||
|
||
for (let i = 0; i < count; i++) { | ||
typedValues.push(readValue(type.getFirstTypeParameter())); | ||
} | ||
} else { | ||
while (!hasReachedTheEnd()) { | ||
typedValues.push(readValue(type.getFirstTypeParameter())); | ||
} | ||
} | ||
|
||
return new VariadicValue(type, typedValues); | ||
return new VariadicValue(variadicType, typedValues); | ||
} else if (type.hasExactClass(CompositeType.ClassName)) { | ||
let typedValues = []; | ||
|
||
|
@@ -158,6 +167,13 @@ export class ArgSerializer { | |
} | ||
} else if (value.hasExactClass(VariadicValue.ClassName)) { | ||
let valueAsVariadic = <VariadicValue>value; | ||
let variadicType = <VariadicType>valueAsVariadic.getType(); | ||
|
||
if (variadicType.isCounted) { | ||
const countValue = new U32Value(valueAsVariadic.getItems().length); | ||
buffers.push(self.codec.encodeTopLevel(countValue)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, fixed. Extracted handling into separate (though "nested") functions. Also re-organized branches for less indenting 👍 |
||
} | ||
|
||
for (const item of valueAsVariadic.getItems()) { | ||
handleValue(item); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import BigNumber from "bignumber.js"; | ||
import { assert } from "chai"; | ||
import { Address } from "../address"; | ||
import { ErrInvalidArgument } from "../errors"; | ||
import { NativeSerializer } from "./nativeSerializer"; | ||
import { AbiRegistry, AddressType, AddressValue, BigUIntType, BooleanType, BooleanValue, CompositeType, CompositeValue, EndpointDefinition, EndpointModifiers, EndpointParameterDefinition, EnumType, ListType, NullType, OptionalType, OptionalValue, OptionType, OptionValue, TupleType, U32Type, U64Type, U64Value, U8Type, U8Value, VariadicType, VariadicValue } from "./typesystem"; | ||
import { AbiRegistry, AddressType, AddressValue, BigUIntType, BooleanType, BooleanValue, CompositeType, CompositeValue, EndpointDefinition, EndpointModifiers, EndpointParameterDefinition, ListType, NullType, OptionalType, OptionalValue, OptionType, OptionValue, TupleType, TypePlaceholder, U32Type, U32Value, U64Type, U64Value, U8Type, U8Value, VariadicType, VariadicValue } from "./typesystem"; | ||
import { BytesType, BytesValue } from "./typesystem/bytes"; | ||
|
||
describe("test native serializer", () => { | ||
|
@@ -45,6 +46,87 @@ describe("test native serializer", () => { | |
assert.deepEqual(typedValues[6].valueOf(), null); | ||
}); | ||
|
||
it("should perform type inference (variadic arguments)", async () => { | ||
const endpointModifiers = new EndpointModifiers("", []); | ||
const inputParameters = [new EndpointParameterDefinition("", "", new VariadicType(new U32Type(), false))]; | ||
const endpoint = new EndpointDefinition("foo", inputParameters, [], endpointModifiers); | ||
const typedValues = NativeSerializer.nativeToTypedValues([8, 9, 10], endpoint); | ||
|
||
assert.deepEqual(typedValues[0].getType(), new VariadicType(new U32Type(), false)); | ||
assert.deepEqual(typedValues[0].valueOf(), [new BigNumber(8), new BigNumber(9), new BigNumber(10)]); | ||
}); | ||
|
||
it("should perform type inference (counted-variadic arguments)", async () => { | ||
const endpointModifiers = new EndpointModifiers("", []); | ||
const inputParameters = [ | ||
new EndpointParameterDefinition("", "", new VariadicType(new U32Type(), true)), | ||
new EndpointParameterDefinition("", "", new VariadicType(new BytesType(), true)) | ||
]; | ||
const endpoint = new EndpointDefinition("foo", inputParameters, [], endpointModifiers); | ||
|
||
// Implicit counted-variadic (not supported). | ||
assert.throws(() => NativeSerializer.nativeToTypedValues([8, 9, 10, "a", "b", "c"], endpoint), ErrInvalidArgument); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cannot be supported in the current implementation of the |
||
|
||
// Explicit, non-empty counted-variadic. | ||
let typedValues = NativeSerializer.nativeToTypedValues([ | ||
VariadicValue.fromItemsCounted(new U32Value(8), new U32Value(9), new U32Value(10)), | ||
VariadicValue.fromItemsCounted(BytesValue.fromUTF8("a"), BytesValue.fromUTF8("b"), BytesValue.fromUTF8("c")) | ||
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible to have multiple counted variadic arguments (users are required to pass them in the explicitly typed form). |
||
], endpoint); | ||
|
||
assert.lengthOf(typedValues, 2); | ||
assert.deepEqual(typedValues[0].getType(), new VariadicType(new U32Type(), true)); | ||
assert.deepEqual(typedValues[0].valueOf(), [new BigNumber(8), new BigNumber(9), new BigNumber(10)]); | ||
assert.deepEqual(typedValues[1].getType(), new VariadicType(new BytesType(), true)); | ||
assert.deepEqual(typedValues[1].valueOf(), [Buffer.from("a"), Buffer.from("b"), Buffer.from("c")]); | ||
|
||
// Explicit, empty counted-variadic. | ||
typedValues = NativeSerializer.nativeToTypedValues([ | ||
VariadicValue.fromItemsCounted(), | ||
VariadicValue.fromItemsCounted() | ||
], endpoint); | ||
|
||
assert.lengthOf(typedValues, 2); | ||
assert.deepEqual(typedValues[0].getType(), new VariadicType(new TypePlaceholder(), true)); | ||
assert.deepEqual(typedValues[0].valueOf(), []); | ||
assert.deepEqual(typedValues[1].getType(), new VariadicType(new TypePlaceholder(), true)); | ||
assert.deepEqual(typedValues[1].valueOf(), []); | ||
}); | ||
|
||
it("should perform type inference (counted-variadic and regular variadic arguments)", async () => { | ||
const endpointModifiers = new EndpointModifiers("", []); | ||
const inputParameters = [ | ||
new EndpointParameterDefinition("", "", new VariadicType(new U32Type(), true)), | ||
new EndpointParameterDefinition("", "", new VariadicType(new BytesType(), false)) | ||
]; | ||
const endpoint = new EndpointDefinition("foo", inputParameters, [], endpointModifiers); | ||
|
||
// Implicit counted-variadic (not supported). | ||
assert.throws(() => NativeSerializer.nativeToTypedValues([8, 9, 10], endpoint), ErrInvalidArgument); | ||
|
||
// Explicit counted-variadic, empty implicit regular variadic. | ||
let typedValues = NativeSerializer.nativeToTypedValues([ | ||
VariadicValue.fromItemsCounted(new U32Value(8), new U32Value(9), new U32Value(10)) | ||
], endpoint); | ||
|
||
assert.lengthOf(typedValues, 2); | ||
assert.deepEqual(typedValues[0].getType(), new VariadicType(new U32Type(), true)); | ||
assert.deepEqual(typedValues[0].valueOf(), [new BigNumber(8), new BigNumber(9), new BigNumber(10)]); | ||
assert.deepEqual(typedValues[1].getType(), new VariadicType(new BytesType(), false)); | ||
assert.deepEqual(typedValues[1].valueOf(), []); | ||
|
||
// Explicit counted-variadic, non-empty implicit regular variadic. | ||
typedValues = NativeSerializer.nativeToTypedValues([ | ||
VariadicValue.fromItemsCounted(new U32Value(8), new U32Value(9), new U32Value(10)), | ||
"a", "b", "c" | ||
], endpoint); | ||
|
||
assert.lengthOf(typedValues, 2); | ||
assert.deepEqual(typedValues[0].getType(), new VariadicType(new U32Type(), true)); | ||
assert.deepEqual(typedValues[0].valueOf(), [new BigNumber(8), new BigNumber(9), new BigNumber(10)]); | ||
assert.deepEqual(typedValues[1].getType(), new VariadicType(new BytesType(), false)); | ||
assert.deepEqual(typedValues[1].valueOf(), [Buffer.from("a"), Buffer.from("b"), Buffer.from("c")]); | ||
}); | ||
|
||
it("should should handle optionals in a strict manner (but it does not)", async () => { | ||
const endpoint = AbiRegistry.create({ | ||
"endpoints": [ | ||
|
@@ -279,7 +361,7 @@ describe("test native serializer", () => { | |
assert.deepEqual(typedValues[0].valueOf(), new BigNumber(42)); | ||
assert.deepEqual(typedValues[1].getType(), new VariadicType(new BytesType())); | ||
assert.deepEqual(typedValues[1].valueOf(), []); | ||
}); | ||
}); | ||
|
||
it("should perform type inference (enums)", async () => { | ||
const abiRegistry = AbiRegistry.create({ | ||
|
@@ -354,8 +436,8 @@ describe("test native serializer", () => { | |
assert.deepEqual(typedValues[1].getType(), enumType); | ||
assert.deepEqual(typedValues[1].valueOf(), { name: "Nothing", fields: [] }); | ||
assert.deepEqual(typedValues[2].getType(), enumType); | ||
assert.deepEqual(typedValues[2].valueOf(), { name: 'Something', fields: [ new Address('erd1dc3yzxxeq69wvf583gw0h67td226gu2ahpk3k50qdgzzym8npltq7ndgha') ] }); | ||
assert.deepEqual(typedValues[2].valueOf(), { name: 'Something', fields: [new Address('erd1dc3yzxxeq69wvf583gw0h67td226gu2ahpk3k50qdgzzym8npltq7ndgha')] }); | ||
assert.deepEqual(typedValues[3].getType(), enumType); | ||
assert.deepEqual(typedValues[3].valueOf(), { name: 'Else', fields: [ new BigNumber(42), new BigNumber(43) ] }); | ||
assert.deepEqual(typedValues[3].valueOf(), { name: 'Else', fields: [new BigNumber(42), new BigNumber(43)] }); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,14 @@ export namespace NativeSerializer { | |
*/ | ||
export function nativeToTypedValues(args: any[], endpoint: EndpointDefinition): TypedValue[] { | ||
args = args || []; | ||
args = handleVariadicArgsAndRePack(args, endpoint); | ||
|
||
checkArgumentsCardinality(args, endpoint); | ||
|
||
if (hasNonCountedVariadicParameter(endpoint)) { | ||
args = repackNonCountedVariadicParameters(args, endpoint); | ||
} else { | ||
// Repacking makes sense (it's possible) only for regular, non-counted variadic parameters. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No repacking: there are no regular (non-counted) variadic parameters. |
||
} | ||
|
||
let parameters = endpoint.input; | ||
let values: TypedValue[] = []; | ||
|
@@ -33,16 +40,31 @@ export namespace NativeSerializer { | |
return values; | ||
} | ||
|
||
function handleVariadicArgsAndRePack(args: any[], endpoint: EndpointDefinition) { | ||
let parameters = endpoint.input; | ||
|
||
let { min, max, variadic } = getArgumentsCardinality(parameters); | ||
function checkArgumentsCardinality(args: any[], endpoint: EndpointDefinition) { | ||
// With respect to the notes of "repackNonCountedVariadicParameters", "getArgumentsCardinality" will not be needed anymore. | ||
// Currently, it is used only for a arguments count check, which will become redundant. | ||
const { min, max } = getArgumentsCardinality(endpoint.input); | ||
|
||
if (!(min <= args.length && args.length <= max)) { | ||
throw new ErrInvalidArgument(`Wrong number of arguments for endpoint ${endpoint.name}: expected between ${min} and ${max} arguments, have ${args.length}`); | ||
} | ||
} | ||
|
||
if (variadic) { | ||
function hasNonCountedVariadicParameter(endpoint: EndpointDefinition): boolean { | ||
const lastParameter = endpoint.input[endpoint.input.length - 1]; | ||
return lastParameter?.type instanceof VariadicType && !lastParameter.type.isCounted; | ||
} | ||
|
||
// In a future version of the type inference system, re-packing logic will be removed. | ||
// The client code will be responsible for passing the correctly packed arguments (variadic arguments explicitly packed as arrays). | ||
// For developers, calling `foo(["erd1", 42, [1, 2, 3]])` will be less ambiguous than `foo(["erd1", 42, 1, 2, 3])`. | ||
// Furthermore, multiple counted-variadic arguments cannot be expressed in the current variant. | ||
// E.g. now, it's unreasonable to decide that `foo([1, 2, 3, "a", "b", "c"])` calls `foo(counted-variadic<int>, counted-variadic<string>)`. | ||
function repackNonCountedVariadicParameters(args: any[], endpoint: EndpointDefinition) { | ||
let parameters = endpoint.input; | ||
|
||
// TODO: Remove after first review. Temporarily left this way to make the review easier. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
if (true) { | ||
const lastEndpointParamIndex = parameters.length - 1; | ||
const argAtIndex = args[lastEndpointParamIndex]; | ||
|
||
|
@@ -135,7 +157,11 @@ export namespace NativeSerializer { | |
return new OptionalValue(type, converted); | ||
} | ||
|
||
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()"`); | ||
} | ||
|
||
if (native == null) { | ||
native = []; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-counted variadic arguments (regular variadic arguments) can only be at the tail of the arguments.