Extra validation of data for encoding #86

davidyuk · 2021-11-10T13:26:28Z

No description provided.

dincho · 2021-11-11T16:51:56Z

src/DataFactory.js

@@ -85,6 +85,9 @@ class DataFactory {
        }

        if (type.name === 'list') {
+            if (!Array.isArray(value)) {


What's the reason to not validate it in the type class ? DataFactory should be general

Because later we are calling the map method of value, should we move it to FateList as well?

Fair point, I've overlooked that.

However there is non-sense tautology in the error message "list must be an array", same for tuple: "Tuple must be an array". Please try to find a better error message.

Something like "[Fate] List constructor expects an array of values, got X instead"

And slight note on formatting, please have a new line after each block of statements, if that not the last statement in another block. I know I've to commit the rules and it's not obvious from the codebase .. soon

[Fate] List constructor expects an array of values, got X instead

I'm not sure about the exact message

won't "Sophia" be more clear than "Fate" for third-party users?

the constructor seems to be too internal part to mention it

I would drop "of values" because this is the same for any array, technically we can generate the exact type instead (like array of numbers)

would be better to name Array from the capital letter to highlight that this is a JS type?

should we do JSON.stringify of X? should we handle the "Do not know how to serialize a BigInt" exception then?

Proposing to change it to "Sophia list must be an Array, got X instead"

My message was just an example, feel free to change it to whatever feel better. However, again on users .. this library has nothing to do with Sophia programming language, but FATE virtual machine :) So please don't use the term sophia, you don't have to use the term fate as well if you don't think it's a good fit

I don’t know really, AEVM is deprecated, so the only VM now is FATE. We also don’t know if there would be any other VM in future so let’s keep it simple? I’m more lean to drop off the calldata from the name as I plan to add full serializaion support anytime soon, however we might just split the serializer and encoder 🤔🤷‍♂️

also when looking into the readme that might be confusing. just want to raise that topic now :-)

Would you propose readme changes?

I shouldn't have interferred here 😅 - actually I currently don't have concrete proposals. I am fine with leaving like is for the moment in regards to the readme.

We also don’t know if there would be any other VM in future so let’s keep it simple?

👍

I’m more lean to drop off the calldata from the name as I plan to add full serializaion support anytime soon

might make sense in that case to do this better earlier than later. we should have some reliable release (& npm package) in the near future 🤔

I'm not sure we should rush about that, the only user of the lib is the SDK so we should be fine for quite some time.

The lib already have NPM packages and release process in place, once we all agree on it's stability we should tag 1.0.0

1.0.0-rc1 perhaps

dincho

please keep the DataFactory general - knowing nothing about types specifics, but only how to create types

dincho · 2021-11-11T16:53:14Z

src/types/FateInt.js

+    try {
+      this._value = BigInt(value)
+    } catch (error) {
+      if (/^Cannot convert .* to a BigInt$/.test(error.message)) {


what's the reason changing the error message ?!

Using calldata the developer may not use BigInts explicitly, so it may be unclear why an argument should be convertible to BigInt. "number" is more general than "BigInt" so the developer would guess that the problem is rather on his side than in the internals of calldata.

But I'm not 100% sure about it and ok to revert this if @marc0olo @mradkov agrees that this is not needed.

Mhhhh, when thinking about this I don't think we really need this. I mean every experienced dev should know how to handle this or whats the problem, right? Personally I favor the original error message.

That reason, don't hold for me. Please revert.

dincho · 2021-11-22T16:21:44Z

src/types/FateBytes.js

@@ -12,7 +12,13 @@ const toByteArray = (value) => {
        return HexStringToByteArray(value)
    }

-    return Int2ByteArray(BigInt(value))
+    try {


shouldn't this be reverted as well ? I didn't feel it was necessary to comment on each error message rewrite

The general idea is to show complete error message in case if we wasn't able to convert value to byte array.

Number is the less obvious representation of byte array, can we replace this validation with Array.isArray to show error like "is not an array" instead "can't convert to BigIng" in case we remove the complete error message?

We need an agreement on developer friendlies. SDK before was too developer friendly, I'm just trying to make the difference not so significant (on calldata side).

I’m more or less against changing of error messages. Additional validation should be fine. It’s also fine if SDK strive to be developer friendly, however this lib is more low level and doesn’t target non experienced devs. It should be focused on specification conformance, clean code, maintability (that includes minimal solid API) and performance

marc0olo · 2021-11-23T10:20:44Z

according to @davidyuk this is related to #79 (kind of the same issue) and could probably be merged now, too? :-)

davidyuk · 2021-11-23T18:45:59Z

src/types/FateBytes.js

@@ -12,7 +12,11 @@ const toByteArray = (value) => {
        return HexStringToByteArray(value)
    }

-    return Int2ByteArray(BigInt(value))


Actually, value converted to BigInt at the first line of Int2ByteArray

davidyuk requested a review from dincho November 10, 2021 13:26

davidyuk changed the title ~~Extra validation of encoded data~~ Extra validation of data for encoding Nov 10, 2021

davidyuk mentioned this pull request Nov 10, 2021

Replace http compiler with calldata-lib aeternity/aepp-sdk-js#1312

Closed

dincho reviewed Nov 11, 2021

View reviewed changes

dincho requested changes Nov 11, 2021

View reviewed changes

davidyuk force-pushed the extra-validation branch from 56c71b0 to 3348eb1 Compare November 22, 2021 15:41

dincho reviewed Nov 22, 2021

View reviewed changes

davidyuk force-pushed the extra-validation branch from 3348eb1 to b7e7953 Compare November 23, 2021 05:52

davidyuk added 2 commits November 23, 2021 21:12

Don't accept string as entire tuple or list

0b250b2

Custom error message instead "can't convert to BigInt"

41b478b

davidyuk force-pushed the extra-validation branch from b7e7953 to 41b478b Compare November 23, 2021 18:44

davidyuk commented Nov 23, 2021

View reviewed changes

dincho approved these changes Nov 24, 2021

View reviewed changes

dincho merged commit 39b4212 into master Nov 24, 2021

dincho deleted the extra-validation branch November 24, 2021 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra validation of data for encoding #86

Extra validation of data for encoding #86

davidyuk commented Nov 10, 2021

dincho Nov 11, 2021

davidyuk Nov 18, 2021

dincho Nov 19, 2021

davidyuk Nov 22, 2021

dincho Nov 22, 2021

dincho Nov 23, 2021 •

edited

Loading

dincho Nov 23, 2021

marc0olo Nov 23, 2021 •

edited

Loading

dincho Nov 23, 2021

dincho Nov 23, 2021

dincho left a comment

dincho Nov 11, 2021

davidyuk Nov 18, 2021

marc0olo Nov 18, 2021

dincho Nov 19, 2021

dincho Nov 22, 2021 •

edited

Loading

davidyuk Nov 23, 2021 •

edited

Loading

dincho Nov 23, 2021

marc0olo commented Nov 23, 2021

davidyuk Nov 23, 2021

Extra validation of data for encoding #86

Extra validation of data for encoding #86

Conversation

davidyuk commented Nov 10, 2021

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

dincho Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc0olo Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dincho left a 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

Choose a reason for hiding this comment

dincho Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

davidyuk Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc0olo commented Nov 23, 2021

Choose a reason for hiding this comment

dincho Nov 23, 2021 •

edited

Loading

marc0olo Nov 23, 2021 •

edited

Loading

dincho Nov 22, 2021 •

edited

Loading

davidyuk Nov 23, 2021 •

edited

Loading