Replace Some and None variants with the exact value and null accordingly about aepp-calldata-js HOT 28 CLOSED

However I wonder can we do a general revamp of the interface?

I think it would be too expensive to integrate calldata library into SDK without breaking changes, so propose to introduce all reasonable changes that we have in mind

in the past undefined was used for None in the js-sdk as far as I know

More correct to return null in this case (as far we already making breaking changes), would be good to accept both null and undefined as None(some type)

from aepp-calldata-js.

I should stop replying to this one but, the library is a 1to1 encoder to Sophia data types, there is no "null" there, Some and None are variants of optional, once the Sophia adds a null type I'll be more than happy to implement it.

from aepp-calldata-js.

OK @davidyuk, looks like it's something quite important to you, I'll reconsider it.

from aepp-calldata-js.

difficult topic for me as I don't have a strong opinion here. would be nice to get this sorted out. thanks @dincho, really appreciated! :-)

from aepp-calldata-js.

I think if we aim to keep supporting both ways we probably should make this configurable in the lib. or how is this supposed to work otherwise?

from aepp-calldata-js.

I know that we want to have a standalone lib in general. we can leave like is and cover the desired functionality in SDK. personally I am just wondering and thinking about what other tools & SDKs might make use of this library except our official javascript sdk.

A JS compiler would use this as well. Also a JS blockchain explorer would benefit by decoding call results. This is just in the top of my head, but I'm sure it would find some more uses.

Onse the lib gets i's full serialization support, it could be used in the blockchain explorer to show a FATE VM assembly as well, that of course needs an additional lib to traslate the binary to FASM, but still you get the idea.

In future if we have FATE VM implemented in JS, this lib would be used there as well.

In the call we agreed to support both undefined/null/T/canonical structure and decode to undefined. (option variant only)

from aepp-calldata-js.

@dincho what do you think about this?

from aepp-calldata-js.

In general I don't really like the variants public API, so I'm lean to simplify it. However I wonder can we do a general revamp of the interface?

from aepp-calldata-js.

in the past undefined was used for None in the js-sdk as far as I know

from aepp-calldata-js.

At first I somehow missed the point of this request/issue, however after I clearly understand the request now - I strongly disagree with it the proposal.

I would not argue on it but refer to the datatype description from https://en.wikipedia.org/wiki/Tagged_union

Tagged unions are most important in functional languages such as ML and Haskell, where they are called datatypes (see algebraic data type) and the compiler is able to verify that all cases of a tagged union are always handled, avoiding many types of errors.

Also https://rescript-lang.org/docs/manual/latest/variant#design-decisions

Variants, in their many forms (polymorphic variant, open variant, GADT, etc.), are likely the feature of a type system such as ReScript's. The aforementioned option variant, for example, obliterates the need for nullable types, a major source of bugs in other languages. Philosophically speaking, a problem is composed of many possible branches/conditions. Mishandling these conditions is the majority of what we call bugs. A type system doesn't magically eliminate bugs; it points out the unhandled conditions and asks you to cover them*. The ability to model "this or that" correctly is crucial.

While because of the lack of pattern matching in Javascript it is significantly verbose and "harder" to handle this it's not enough argument to introduce nullable data. In general my opinion is that null is one of the worst mistake in programming languages which is nicely handled by tagged unions/variants and with optional in Sophia in particular and libs should not magically defeat that. Given that one can get nulls in JS for various reasons is also significant decision factor.

Moreover I'm always for explicitly over implicitly thus handling special cases would be out of scope of this library.

from aepp-calldata-js.

@mradkov @marc0olo @thepiwo what do you think about it?

from aepp-calldata-js.

If we ever implicitly agreed on this premise: We need to keep Sophia types and its JS representations semantically as close as possible for delivering developers something that naturally makes sense.

..then the conclusion cannot can not be we reject using null (not undefined, as that would be given for some accidentally mistyped var name, too, for example) for None or Some because of academic reasons. At least until somebody proposes semantically matching alternatives that are also native to JS, using null would be the way to go for now.

from aepp-calldata-js.

You're free to implement that mapping in the SDK then, just like it is right now this library prefers to work in a more reasonably way.

from aepp-calldata-js.

(not undefined, as that would be given for some accidentally mistyped var name, too, for example)

exactly the same applies to null as well

from aepp-calldata-js.

You're free to implement that mapping in the SDK then

Splitting the implementation would make it more difficult to maintain. I don't want to do this recursive replacing by ACI on SDK side just because we can't agree here. Different interfaces in SDK and calldata would complicate switching between them (is calldata intended to be used separately?).

not undefined, as that would be given for some accidentally mistyped var name, too, for example

Actually, modern environments would throw a ReferenceError (something is not defined) in that case. I don't think that

let foo; // undefined
contract.methods.bar(foo);

should be forbidden. Also, in TS optional function parameters are defined as <type> | undefined, so null even won't work there. 🤔

@dincho have you tried to find some third-party None/Some implementation for JS? This would make calldata compatible with them, otherwise, it may be like

const t = libraryA.foo();
libraryB.bar(t === libraryA.None ? libraryB.None : t)
// instead of just
libraryB.bar(libraryA.foo())

or even recursive replacement may be needed if there are complex structure 😄

I agree that nulls are error-prone in JS, but TS compiler (with strictNullChecks flag) will ask the developer to check if the optional value is present or not, the same can do an IDE with TS support in JS project. This would be more obvious and natural for the usual JS developer: "Object is possibly 'null/undefined'" than "Object is possibly doesn't have 'Some' property"(?).

from aepp-calldata-js.

is calldata intended to be used separately?

I don't really think so. it could be used separately but probably nobody will do it.

fact is we need to decide as quickly as possible how to proceed here in general. seems like currently it's not easy to get aligned on that.

I would like to have some feedback of @thepiwo and @mradkov about this again.

personally I am a fan of typescript but I am more interested in moving the way the majority thinks we should go. this shouldn't block us from moving forward.

from aepp-calldata-js.

for call-data lib I don't mind as long as in sdk we map it logically without need for additional libs
I also like the typescript approach with value | undefined

from aepp-calldata-js.

@davidyuk what's the exact issue to do it on SDK side? I think we should move forward asap here 😅

from aepp-calldata-js.

(is calldata intended to be used separately?).

actually yes in my master plan, this library could evolve to a general encoder/serializer and to be eventually used in a JS compiler, client-side only decoder etc.

I definitely want to keep this library standalone, Vanilla JS in browser.

@davidyuk what's the exact issue to do it on SDK side? I think we should move forward asap here 😅

Isn't it already implemented like that in the SDK, I pointed to a source code above sorry if I misunderstood that

@dincho have you tried to find some third-party None/Some implementation for JS? This would make calldata compatible with them, otherwise, it may be like

I don't like the idea to introduce additional lib for types interop, interfaces should be POJO of a given standalone lib or library provided datatype, that's general library (good) practice IMO

from aepp-calldata-js.

FYI: I've clarified the library purpose and goals in 4a8baba

from aepp-calldata-js.

Also https://rescript-lang.org/docs/manual/latest/variant#design-decisions

While Rescript doesn't implement nullable types as a source of bugs they don't force JS to adopt that, e.g. the transpiled code uses undefined instead of None: example.

If you have chosen Rescript instead of JavaScript then we would be able to transpile it to JS without this issue. But calldata is written in JavaScript so it should be consistent with decisions made in this language (even if they are wrong).

what's the exact issue to do it on SDK side?

It should do most of the work that already calldata does to wrap and unwrap optional values in nested structures, more maintainable would be to do these changes on calldata side.

from aepp-calldata-js.

However, the lib will keep accepting {None: []} and {Some: []} (the generic variant format) as well.

from aepp-calldata-js.

the lib will keep accepting {None: []} and {Some: []}

Will it decode as { None: [] } or null then? I'm proposing my implementation in #85

from aepp-calldata-js.

Will it decode as { None: [] } or null then? I'm proposing my implementation in #85

Null perhaps ? Does it break the current implementation somehow ?

from aepp-calldata-js.

we are breaking lots of the current implementation anyway now, so we can also change it. Let just support one way in the lib for encoding/decoding for each type, otherwise it will just be confusing.

from aepp-calldata-js.

I see that we want to solve this topic and somehow it seems feedback is required but I am struggling to understand what I can do in this case or how I can support here.

the discussion in this PR #85 (comment) seems to be related.

However, the lib will keep accepting {None: []} and {Some: []} (the generic variant format) as well.

@dincho so you would like to keep accepting both ways in the public interface - do I understand that correctly?

sorry @davidyuk, I am somehow still struggling with this topic in general 😅

from aepp-calldata-js.

@dincho so you would like to keep accepting both ways in the public interface - do I understand that correctly?

exactly, no less, no more

from aepp-calldata-js.

so we have now multiple options:

• only support one way (@thepiwo)
  • which one doesn't matter
• support both ways (@dincho)
  • initially only one way that is not favored by SDK devs
• implementation in #85 (@davidyuk and @mradkov)
  • not sure how this right now aligns with Dinchos request

my humble opinion right now is that I just want to take a decision here. maybe we should set up a call to sort this out. from what I heard from @davidyuk it would be quite an overhead (not sure how much right now) to tackle the SDK favored approach directly in the SDK instead of the lib and it would also increase maintenance costs in case we face changes in that area (probably unlikely, but you never know 😅)

I know that we want to have a standalone lib in general. we can leave like is and cover the desired functionality in SDK. personally I am just wondering and thinking about what other tools & SDKs might make use of this library except our official javascript sdk.

anyway, we need to take a decision here and I think will leave that up to you @dincho as you maintain this library. in the call today we agreed that we don't want to fork this library and maintain a separate version of it. fact is that the current state of the sdk that aims to include this calldata relies on the implementation included in #85.

from aepp-calldata-js.