Start supporting double map by xlc · Pull Request #843 · polkadot-js/api

xlc · 2019-04-15T02:55:43Z

We are upgrading our modules to use double map so there will be a set of contributions from us to make double map actually works with polkadot.js.

This doesn't actually support double map, just fixed some crashing error I encountered while working on apps.

jacogr · 2019-04-16T10:19:52Z

Thank you!

xlc · 2019-04-16T10:36:00Z

The metadata versions is starting getting messy and adding a new feature is not easy because it needs to downgrade from latest version back to v0.

Any plan to refactor so that it is performing upgrade instead of downgrade?

~~There is v4 (paritytech/substrate#2268) and maybe v5 (paritytech/substrate#2261) upcoming so it is better to do this refactor before they get merged in.~~ (Read the code bit more, maybe they don't need metadata version bump)

jacogr · 2019-04-17T12:13:28Z

Needs to be done, yes. Agreed that it is sprawling a bit atm. Have not been able to get there myself.

@amaurymartiny could you take a peek at this PR please - cannot give it the right kind of attention with limited access.

amaury1093

Yeah, I agree there's some cleaning to be done here. It's planned, I'll be able to personally start cleaning stuff starting May.

amaury1093 · 2019-04-18T15:51:51Z

packages/types/src/Metadata/v1/Storage.ts

 }

-export class MetadataStorageType extends EnumType<PlainType | MapType> {
+export class MetadataStorageType extends EnumType<PlainType | MapType | DoubleMapType> {


You can remove this class, and import StorageFunctionType from v0/modules

amaury1093 · 2019-04-18T15:52:09Z

packages/types/src/Metadata/v2/Storage.ts

 }

-export class MetadataStorageType extends EnumType<PlainType | MapType> {
+export class MetadataStorageType extends EnumType<PlainType | MapType | DoubleMapType> {


You can remove this class, and import StorageFunctionType from v0/modules

amaury1093 · 2019-04-18T15:54:18Z

packages/types/src/Metadata/v0/Modules.ts

 }

-export class StorageFunctionType extends EnumType<PlainType | MapType> {
+export class StorageFunctionType extends EnumType<PlainType | MapType | DoubleMapType> {


Just to understand your intent here: we agree that v0 doesn't have DoubleMapType, but you're still putting it here to be able to factorize code, as this change doesn't break compatibility, correct?

If yes, then see my 2 comments above where we can factorize code a bit more.

amaury1093 · 2019-04-18T15:56:15Z

packages/types/src/Metadata/v2/Storage.ts

 import Text from '../../primitive/Text';
 import Type from '../../primitive/Type';
 import { MetadataStorageModifier } from '../v1/Storage';
+import { DoubleMapType } from '../v0/Modules';


tiny nit: we tend to order things alphabetically by default, when there's no other natural ordering

Regarding the ordering and spacing rules, can we have them in tslint?

That's a good idea. But I think since TypeScript is officially moving towards eslint, we'd like to use eslint everywhere in the mid-term, this looks for for our case.

But now in this "transition" period before eslint is officially supported, i propose not to overbloat tslint.json, and to manually enforce this few rules.

In any case, you can create an issue on https://github.com/polkadot-js/dev

amaury1093 · 2019-04-26T09:44:47Z

Hey @xlc I refactored types/Metadata a bit, so that it's less entangled, but conflicts with this implementation. imo there's only small stuff like https://github.com/polkadot-js/api/pull/843/files#diff-023179d9a94bd59761a7319a645b1dd0R112 to be added in this PR (+ eventually the actual key hashing, but can be done later if needed)

AlexZhenWang · 2019-05-02T03:58:10Z

Hi @amaurymartiny. Thank you for taking the time to review the code. I will change the code based on the suggestions and rebase this branch on the master to fix the conflict.

amaury1093 · 2019-05-02T08:42:13Z

packages/types/src/Metadata/v0/Storage.ts

 }

-export class StorageFunctionType extends EnumType<PlainType | MapType> {
+export class StorageFunctionType extends EnumType<PlainType | MapType | DoubleMapType> {


There's no DoubleMap in v0, everything that's related to DoubleMap in this file should be deleted

I got ‘FATAL: Unable to initialize the API: Object.values(...)[index] is not a constructor’ error at EnumType.ts:109:14 without having DoubleMapType here. I have to add DoubleMapType for fixing this issue.
In function
private static createValue (def: TypesDef, index: number = 0, value?: any): Decoded { return { index, value: new (Object.values(def)[index])(value) }; }
, index is 2 but def only have MapType and PlainType. Call stack says, it's because of const extrinsics = extrinsicsFromMeta(this.runtimeMetadata.asV0);

Ah, I suspected this would happen...

I think there's a small refactoring to do, have a look at point 1 in #858 (comment):

create v0/ and v4/ folders in type-extrinsic

move current fromMetadata to fromMetadataV0

fromMetadata should call fromMetadataV0 if it's v0-3

in v4/, create a fromMetadataV4, which correctly creates extrinsics from v4

Have a look at type-storage, type-extrinsics should follow the same pattern, and this would allow adding hacks like adding DoubleMap in v0

Edit: if this seems confusing to you, ping me here and I'll do this small refactoring tomorrow 1st thing, so that you can rebase on my PR and then we can merge this PR

Thank you @amaurymartiny. I got the idea. I will do the refactoring following your guide.

@AlexWZ0129 I've done similar stuffs, you can reuse code from my refactor branch.

@ianhe8x Okay. Thank you, Ian!

amaury1093 · 2019-05-02T08:42:45Z

packages/types/src/Metadata/v2/Storage.ts

 import Text from '../../primitive/Text';
 import Type from '../../primitive/Type';
 import { PlainType, StorageFunctionModifier } from '../v1/Storage';
+import { DoubleMapType } from '../v4/Storage';


Same as above, remove DoubleMap from v2

packages/types/src/Metadata/v3/Storage.ts

amaury1093 · 2019-05-02T08:50:43Z

packages/types/src/Metadata/v3/Storage.ts

+      key1: Type,
+      key2: Type,
+      value: Type,
+      key2Hasher: StorageHasher


Unfortunately key2Hasher is still a Text, revert. Please see https://github.com/paritytech/substrate/blob/master/srml/metadata/src/lib.rs

Thank you for the review. When using Text, if the type of the key has trait prefix like ’T::AssetId’, I got an error ‘Unknown types found, no types for T::AccountId’. Using Type fix the issue. I guess it’s because Type does some format optimise when decoding (such as removing all the trait prefixes Type._removeTraits()).

Changed type of key2Hasher to Text. Thanks for the suggestion.

amaury1093 · 2019-05-02T08:51:44Z

packages/types/src/Metadata/v4/Storage.ts

+      key1: Type,
+      key2: Type,
+      value: Type,
+      key2Hasher: StorageHasher


same as above

packages/types/src/Metadata/v4/Storage.ts

amaury1093

lgtm 👍

AlexZhenWang · 2019-05-13T09:52:27Z

lgtm 👍

Thanks @amaurymartiny !

* double map fix * update metadata to prevent error using double map

polkadot-js-bot · 2021-06-08T15:51:18Z

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.

xlc mentioned this pull request Apr 15, 2019

Refactor all the asXxx method to actually assert the underlying type is matching #844

Closed

AlexZhenWang force-pushed the double-map branch 3 times, most recently from f2f4d39 to 3e48232 Compare April 16, 2019 02:19

ianhe8x mentioned this pull request Apr 18, 2019

refactor type system (a rough idea) #847

Closed

amaury1093 reviewed Apr 18, 2019

View reviewed changes

AlexZhenWang force-pushed the double-map branch from 3e48232 to 6312887 Compare May 2, 2019 04:41

amaury1093 suggested changes May 2, 2019

View reviewed changes

amaury1093 added the WIP Work in Progress label May 3, 2019

AlexZhenWang mentioned this pull request May 9, 2019

Update extrinsics FromMetadata. Drop storages with doublemap type when converting v3 to v2 #888

Merged

xlc and others added 2 commits May 13, 2019 21:09

double map fix

252f495

update metadata to prevent error using double map

5c9b09c

AlexZhenWang force-pushed the double-map branch from cdef94e to 5c9b09c Compare May 13, 2019 09:19

amaury1093 approved these changes May 13, 2019

View reviewed changes

amaury1093 removed the WIP Work in Progress label May 13, 2019

amaury1093 merged commit 46fee31 into polkadot-js:master May 13, 2019

xlc deleted the double-map branch May 13, 2019 22:48

ianhe8x pushed a commit to plugblockchain/api.js that referenced this pull request Jun 12, 2019

Start supporting double map (polkadot-js#843)

a46cb94

* double map fix * update metadata to prevent error using double map

ianhe8x pushed a commit to plugblockchain/api.js that referenced this pull request Jun 12, 2019

Start supporting double map (polkadot-js#843)

f55732a

* double map fix * update metadata to prevent error using double map

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

Conversation

xlc commented Apr 15, 2019

Uh oh!

jacogr commented Apr 16, 2019

Uh oh!

xlc commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacogr commented Apr 17, 2019

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaury1093 Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaury1093 commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexZhenWang commented May 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexZhenWang May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaury1093 May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexZhenWang May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

AlexZhenWang commented May 13, 2019

Uh oh!

polkadot-js-bot commented Jun 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xlc commented Apr 16, 2019 •

edited

Loading

amaury1093 Apr 18, 2019 •

edited

Loading

amaury1093 commented Apr 26, 2019 •

edited

Loading

AlexZhenWang May 2, 2019 •

edited

Loading

amaury1093 May 2, 2019 •

edited

Loading

AlexZhenWang May 3, 2019 •

edited

Loading