`Base` as an interface instead of a class #172

amaury1093 · 2018-09-27T23:12:06Z

Proposal: remove the Base class, use an interface Codec<T> instead, that all api-codec types needs to implement (+/- similar to a rust trait).

In particular, use as much as possible existing JavaScript classes (Array, Map, ...) and create child classes that implement Codec<T>.

Types:

Note: Tests don't change at all, cf *.spec.js, only syntax simplifications.

@jacogr If this is something that's interesting, I'll have a look more into it tomorrow.

jacogr · 2018-09-28T08:54:12Z

Related #161

jacogr · 2018-09-30T09:49:57Z

One additional thing, for UInt, probably want to extends BN (When going for BigInt, probably need to extends that). I'm currently doing a lot of stuff such as -

          new BlockNumber(
            sessionCurrentIndex.toBn()
              .sub(eraLastLengthChange.toBn())
              .mod(sessionsPerEra.toBn())
              .mul(sessionLength.toBn())
              .add(sessionBlockProgress.toBn())
          )

(in the observable API - they are all BlockNumber, but have a conversion for each and then a final wrap)

For the time-being, will probably just fake add/sub/mul (a couple of them), that way when we extend properly, there is no change, just thing being more effective)

amaury1093 · 2018-10-05T09:42:09Z

Things I'm not 100% satisfied:

fromJSON and fromU8a: most of the JS types (like BN, Uint8Array...) don't allow you to replace the value as is, so where before we had this.raw = ..., now I cannot say this = .... We really need to remove fromJSON and fromU8a for cleanliness, this PR is blocked by Intelligent constructor #217.
a.byteLength vs a.byteLength(): BN has byteLength() method, and Uint8Array has byteLength property. If we remove BN, we could go with get byteLength() {...} everywhere, for consistency.
Tuple should extend Vector instead of Struct.
Enum.spec.ts is the only .ts test file, because I need it to test TS enums.

Still TODO:

Option
EnumType
Address (extends AccountId | AccountIndex)

jacogr · 2018-10-05T16:07:36Z

packages/types/src/Moment.ts

-    this.raw = Moment.decode(input);
-
-    return this;
+    // FIXME this returns a new Object unfortunately, can't "replace" current value


Remove from, as per your other PR.

jacogr · 2018-10-05T16:08:22Z

packages/types/src/Moment.ts


 const BITLENGTH = 64;

 // A wrapper around seconds/timestamps. Internally the representation only has
 // second precicion (aligning with Rust), so any numbers passed an/out are always
 // per-second. For any encoding/decoding the 1000 multiplier would be applied to
 // get it in line with JavaScript formats
-export default class Moment extends Base<Date> {
+export default class Moment extends Date implements Codec<Moment> {


Is this correct, i.e. Codec<Moment> (e.g. Bool above extends built-in JS Boolean class, one of the 2 is wrong)

jacogr · 2018-10-05T16:10:17Z

packages/types/src/Text.ts

+    // Two solutions:
+    // - either use static
+    // - or completely remove from*, and force to use constructor
+    return new Text(u8aToUtf8(input.subarray(offset, offset + length.toNumber())));


Yeap, the other PR needs to go first. (This will just create too much messy-ness)

jacogr · 2018-10-05T16:11:30Z

packages/types/src/codec/Enum.spec.ts

@@ -4,6 +4,11 @@

 import Enum from './Enum';

+enum TestEnum {


We could actually go full-hog ts specs now. (Previously it was problematic since I fed it garbage data - either way, not needed right now)

jacogr · 2018-10-05T16:13:33Z

packages/types/src/codec/U8aFixed.ts

+    super(
+      value instanceof U8aFixed
+        ? value
+        : U8aFixed.trimLength(toU8a(value), bitLength / 8)


Just check current master as well, I had some hoops to jump through - well, if not corrects, tests do catch this one.

jacogr · 2018-10-05T16:14:47Z

packages/types/src/codec/UInt.ts

  private _bitLength: BitLength;
  private _isHexJson: boolean;

  constructor (value: AnyNumber = 0, bitLength: BitLength = 64, isHexJson: boolean = true) {
    super(
-      UInt.decode(value)
+      UInt.decode(value).toNumber()


Cannot use toNumber here - JS numbers are limited to 53 bits (on uints) - rather toString() it into a decimal representation.

jacogr · 2018-10-05T16:15:55Z

packages/types/src/types.d.ts

@@ -11,4 +11,16 @@ export type AnyNumber = UInt | BN | Uint8Array | number | string;

 export type AnyU8a = U8a | Uint8Array | Array<number> | string;

+export interface Codec<T> {
+  byteLength(): number;


We could, to get past the BN vs Array issue, call this encodedLength, which is actually exactly what it is. (As a property/getter)

amaury1093 · 2018-10-15T11:55:19Z

This is a clusterfuck of merge conflicts. Will open a new one once everything else is ready.

polkadot-js-bot · 2021-06-09T07:50:52Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

amaury1093 added the WIP Work in Progress label Sep 27, 2018

amaury1093 mentioned this pull request Oct 4, 2018

Small fixes in operation #200

Merged

amaury1093 added 13 commits October 4, 2018 14:52

Fix vector

6cd0e79

Remove length

3c0f383

Make Vector work

ce5a159

Make struct work

7041672

Make Uint work

03550b1

Make enum work

9864198

Fix U8a

30fd118

Compact length as BN

fdd51e9

Fix U8aFixed

edae6d0

Text extends String

2a3d8c5

Origin

1e29b0f

Fix Moment extends Date

0923c4c

Bool extends Boolean

91c3ec8

amaury1093 force-pushed the am-new-types branch from cb7b919 to 91c3ec8 Compare October 5, 2018 09:31

jacogr requested changes Oct 5, 2018

View reviewed changes

jacogr mentioned this pull request Oct 12, 2018

Return value from array on subscription #262

Merged

amaury1093 closed this Oct 15, 2018

jacogr deleted the am-new-types branch October 16, 2018 10:59

polkadot-js locked as resolved and limited conversation to collaborators Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Base` as an interface instead of a class #172

`Base` as an interface instead of a class #172

amaury1093 commented Sep 27, 2018 •

edited

jacogr commented Sep 28, 2018

jacogr commented Sep 30, 2018

amaury1093 commented Oct 5, 2018 •

edited

jacogr Oct 5, 2018

jacogr Oct 5, 2018

jacogr Oct 5, 2018

jacogr Oct 5, 2018

jacogr Oct 5, 2018

jacogr Oct 5, 2018

jacogr Oct 5, 2018

amaury1093 commented Oct 15, 2018

polkadot-js-bot commented Jun 9, 2021

Base as an interface instead of a class #172

Base as an interface instead of a class #172

Conversation

amaury1093 commented Sep 27, 2018 • edited

jacogr commented Sep 28, 2018

jacogr commented Sep 30, 2018

amaury1093 commented Oct 5, 2018 • edited

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

jacogr Oct 5, 2018

Choose a reason for hiding this comment

amaury1093 commented Oct 15, 2018

polkadot-js-bot commented Jun 9, 2021

`Base` as an interface instead of a class #172

`Base` as an interface instead of a class #172

amaury1093 commented Sep 27, 2018 •

edited

amaury1093 commented Oct 5, 2018 •

edited