aeternity / aepp-calldata-js

Aeternity data serialization library
ISC License
3 stars 4 forks source link

Support arbitrary-sized bytes in Ceres #258

Closed davidyuk closed 7 months ago

davidyuk commented 7 months ago

related to https://github.com/aeternity/aesophia/pull/456

This PR is supported by the Æternity Crypto Foundation

bin/aesophia_cli includes changed from https://github.com/aeternity/aesophia/pull/495

dincho commented 7 months ago

Testing the changes locally and trying to grasp in my head the new changes I realised the FateBytes already supports variable size data? Why did you added the new any symbol?

removing most of the changes still pass the tests:

iff --git a/src/types/FateBytes.js b/src/types/FateBytes.js
index c7eefac..12902db 100644
--- a/src/types/FateBytes.js
+++ b/src/types/FateBytes.js
@@ -35,9 +35,9 @@ class FateBytes extends FateData {
     constructor(value, size, name = 'bytes') {
         super(name)

-        this._value = toByteArray(value, size === 'any' ? undefined : size)
+        this._value = toByteArray(value, size)

-        if (size && size !== 'any' && this._value.byteLength !== size) {
+        if (size && this._value.byteLength !== size) {
             throw new FateTypeError(
                 name,
                 `Invalid length: got ${this._value.byteLength} bytes instead of ${size} bytes`
diff --git a/tests/Serializers/BytesSerializer.js b/tests/Serializers/BytesSerializer.js
index 0eaf120..6a504dd 100644
--- a/tests/Serializers/BytesSerializer.js
+++ b/tests/Serializers/BytesSerializer.js
@@ -17,7 +17,7 @@ test('Serialize', t => {
     )

     t.deepEqual(
-        s.serialize(new FateBytes(0xbeef, 'any')),
+        s.serialize(new FateBytes(0xbeef)),
         [159,1,9,190,239]
     )

diff --git a/tests/TypeResolver.js b/tests/TypeResolver.js
index 1ee4a29..6991de3 100644
--- a/tests/TypeResolver.js
+++ b/tests/TypeResolver.js
@@ -140,8 +140,8 @@ test('Resolve bytes', t => {
 test('Resolve bytes any size', t => {
     t.plan(1)
     t.deepEqual(
-        resolver.resolveType({bytes: 'any'}),
-        FateTypeBytes('any')
+        resolver.resolveType({bytes: 0}),
+        FateTypeBytes(0)
     )
 })
dincho commented 7 months ago

Perhaps, the 0 size used to denote variable sized data is not the best choice, I would accept improvements (Infinity, undefined) if you think it would be more readable.

dincho commented 7 months ago

new CLI version has been released: https://github.com/aeternity/aesophia_cli/releases/tag/v7.4.1

davidyuk commented 7 months ago

shouldn't we wait till 8.0.0-rc1? As I understand 7.4.1 won't include bytes() and AENSv2

davidyuk commented 7 months ago
resolver.resolveType({bytes: 'any'})

in this case any comes from contract ACI

dincho commented 7 months ago

shouldn't we wait till 8.0.0-rc1? As I understand 7.4.1 won't include bytes() and AENSv2

ah, yes I didn't noticed the version

dincho commented 7 months ago

@davidyuk could you please change the implementation to get rid of any as we discussed above ?

davidyuk commented 7 months ago

As I understand src/TypeResolver.js is used to get internal FATE type based on ACI. Internal FateTypeBytes accepts the actual size or undefined if it has no fixed size. Bytes with arbitrary size are defined in ACI as

"type": {
  "bytes": "any"
}

So, the only any remaining is ternary in TypeResolver.js which pass undefined size of FateTypeBytes in case of "any" in ACI. I don't think I can get rid of it 🙃

dincho commented 7 months ago

As I understand src/TypeResolver.js is used to get internal FATE type based on ACI. Internal FateTypeBytes accepts the actual size or undefined if it has no fixed size. Bytes with arbitrary size are defined in ACI as

"type": {
  "bytes": "any"
}

So, the only any remaining is ternary in TypeResolver.js which pass undefined size of FateTypeBytes in case of "any" in ACI. I don't think I can get rid of it 🙃

You're right, I overseen the location of any ;)