GenericCall & Event: prepare getting rid of injection/globals

[axelchalon]

Part of #580

The second parameter to the Method type constructor (method metadata) is now required. The method metadata must now be passed as second argument to the Extrinsic constructor (had to make the first argument required for this).

This lets us get rid of injectMethods and of storing the metadata globally in Method.ts. (Event will follow).

Ready for review but I have to check if this is still usable in apps; might make more updates here as I'm updating apps.

[jacogr]

Edgeware, Apps and the extension will be affected, at least from a findMethod perspective. Basically, since it decodes e.g. proposals, it needs to find the method.

With Method() does not having a default anymore (which breaks away from what is in parity-scale-codec as well), this could be problematic.

Not really on-board with this change, anywhere close to a new testnet - as it stands we are just trying to keep afloat and still don't have feature parity. To introduce even more breakages now, is really just adding oil to the fire.

If I have a string of bytes, as Call - how do I construct it without already knowing what it is? I only have the call indexes and the global metadata, nothing more on the method.

EDIT: Last point is invalid, there is Method.findMeta - which is a good-enough replacement.

[amaury1093]

The way we construct api.tx.*.* today, we need to pass metadata to Method. The PR as-is will break api, someone needs to inject metadata to Method.

A good test is to see if all e2e tests pass, they are failing on this PR.

[jacogr]

Let me just clarify - I'm not against the idea at all this opens up the door to some nice things, things. The findMeta is a good idea that solves the most pressing issues. I am however against this idea in the next 1 month.

(That is until we are stable and have feature parity between 2.x and 1.x at least, there are just a lot of unknowns as it stands and I don't particularly want to add some more)

[axelchalon]

👍 I'm adding 'on ice' to the title but feel free to close the PR if you prefer

The way we construct api.tx.. today, we need to pass metadata to Method. The PR as-is will break api, someone needs to inject metadata to Method.

The metadata is already passed to Method on master when constructing api.tx.*.*:

api/packages/type-extrinsics/src/fromMetadata/createUnchecked.ts

Lines 34 to 37 in 7e6e609

	return new Method({
	args,
	callIndex
	}, callMetadata);

[jacogr]

I think we can actually start pulling at least parts in - like the findMeta, ie. That is not breaking and we can already start pointing to using that. (So eg. Apps, Extension, Edgeware and I believe, SingleSource can already start using this interface)

[jacogr]

Just to clarify - the findMeta interface without getting rid of the older version. (And then start pointing to using that in examples and changelog). Same I guess with the Event equivalent.

[amaury1093]

If end-developers are using .findMeta, I would propose something like:

Method.findMeta(callIndex: Uint8Array, metadata: Metadata);
Method.findMeta(sectionName: string, methodName: string, metadata: Metadata);

// Then users can use like:
import {Method} from '...'
const api = new Api();
Method.findMeta([3,0], api.runtimeMetadata);
Method.findMeta('balances', 'balanceOf', api.runtimeMetadata);

The last argument is the whole metadata, so that users don't need to do Method.findMeta('balances', 'balanceOf', extrinsicsFromMetadata(api.runtimeMetadata));

[axelchalon]

Starting to update apps now and seeing what issues come up. Might update this PR along the way.

[axelchalon]

Ready for review/merge

[amaury1093]

Looks good overall to me! It's a nice start in the correct direction.

The tests should pass though, not sure what they're about.

[axelchalon]

The way I understood the closure idea was that the types constructors would be able to access some predefined globals, ultimately always guaranteed to be set, since the type constructors would only be instantiated from within api.createType, which would set the globals before calling the constructor, and unset them after (iirc the way React works internally?).

export default class Method extends Struct implements IMethod {
 constructor(value) {
   console.log(globalMetadata, globalTypeRegistry, /* ... */)
   // ...
 }
}

To ensure that the globals are always the same to an instance of the type constructor, we would need to enforce that the globals not be stored in the type class and not be used after the constructor is called (e.g. in a public method), and there's no way to enforce this afaik.

This being said, an advantage of this approach is that the type constructor doesn't need to pass the metadata etc. forward to other types it constructs (e.g. Extrinsic using Method and having to pass the received metadata forward to Method). The type constructor options are then type-specific options.

But since we can't enforce that this is used correctly, I guess passing the options w/ globals to the constructor remains the best option.

[axelchalon]

(Addresses #1224 in part)

• Not sure about the naming in the options, do we want meta (as in this PR) or metadata as key in the options?
• Will have to create an issue to rewrite the hasOrigin tests in Call.spec.ts using the runtime metadata, since Call doesn't accept the function metadata anymore
• A bit unsure if EventRecord still passes the metadata forward, haven't gotten my head around the latest changes yet (EventRecord being now an interface)

I'm getting some weird test errors, will have to investigate.

[jacogr]

@axelchalon Would still with meta for the method-specific version (like we have everywhere), and metadata for the full thing, i.e. everything an the kitchen-sink.

[axelchalon]

Getting weird test errors that seem to me unrelated to this PR, more related to metadata v6/v7.. a bit at a loss what to make of them or how to fix them

https://travis-ci.com/polkadot-js/api/builds/121655309#L3706

[jacogr]

So, this looks deceptively simple... between the registry, not having classes and trying to make things non-global, well, a bit over-the-top. (Just saying - underatand wtf you are dealing with here). Onto the stuff -

• TypeError: Right-hand side of 'instanceof' is not an object never seen this before - and it is the bulk of them. Will need to dig in specifically

• Unable to determine type from {"info":5,"type":"GenericAccountId"} - this mean the injector can not been called, i.e. the type is not there and not available registered. However... since Generic/Event.spec.ts should have called this (I'm not checking, just assumed it has) as a loss as to why...

• Unable to determine type from {"info":5,"name":"foo","type":"u32"} - weirdest of them all. It has a class of these, it should have been injected with the injector.

[axelchalon]

Well, this took me a while to track down... TypeRegistry is in urgent need of a rework (i.e. getting rid of global), there's a lot of weird shit going on under the hood (circular deps & nested calls) because of the way things are set up and I feel like this is a ticking timebomb of very hard-to-debug problems. For example I had the dynamic require statement only return part of the module's exports... and other weird stuff.

I still don't fully understand why things didn't work, but one part was something along the lines of this: the test uses a new TypeRegistry() and then registers a type using a string (e.g. registry.register({U32Renamed: 'u32'});). register then internally tries to fetch u32 from the global type registry. The global type registry doesn't exist yet, so we start the process to create it: first step is we dynamically require the index.types that will be later injected to the global type registry. index.types re-exports Call which imports @polkadot/api-metadata/extrinsics/fromMetadata which imports createUnchecked which imports import { createType, Metadata } from '@polkadot/types'. The index file of @polkadot/types runs the injector, which tries to get the global type registry, which doesn't exist yet, so it tries to create it, and re-requires index.types, etc.

In other words, when getDefaultRegistry wants to create the global registry, the dependencies make it call getDefaultRegistry again (and the global registry still doesn't exist, so it's probably created twice, once at each nesting level). Reading this I don't even understand why everything isn't already falling apart. Feels like we're walking a very fine line.

[jacogr]

The registry vs createType is an interesting thing - effectively there is duplication and it is very messy. (The fact that we support multiple registries, but only use one which is a global anyway, is even worse)

I actually just want to drop it as-is, just have not had a gap.

And that is actually part of the reason, I believe any "global types refactoring" is ill advised (nothing against this PR, but like we are doing here), without actually understanding what we are doing, get the base right. anything we do here is putting plasters on something which is not 100% in the way things are currently being used.

Since 90% of the types are now injected, the original intent of "fix Method" or "fix Event" is actually not the real issues we are facing.

[axelchalon]

Isn't the unification of constructor signatures (as done in this PR for Call, Event & Extrinsic) (#1224) still relevant for when we will want to provide a single access point to initialize types (api.type) ?

[jacogr]

Yes, but we are going the wrong way around here. We are unifying without even understanding how things are used everywhere, even worse is that the types will come from the metadata as well, putting even more pressure on the register/create funnel.

It is the base of everything, that only played a very small roll up till very recently and is now used everywhere. And the base now does things it is not supposed to, or rather was not quite intended for.

So any work on types (primitive or injected), I can almost guarantee will have rework due to (a) metadata reflection injection & (b) createType, ClassOf, registry stabilization

[axelchalon]

What would you recommend as a next step / next issue to tackle, to fix the base ?

[jacogr]

So if you are asking where to start, I would start here - #1263

What we should have, in terms of a pyramid of dependencies -

• 1. codec types, they only depend on each other
• 2. primitive types, they only depend on codec and each other
• 3. registry and creators, they only depend downwards
• 4. inherited types
• 5. api, only uses from below, mostly creators
• 6. any dapps build on the below

Effectively at this point the first 3 layers contain circular deps among them, which, as you've noticed, creates some really ugly issues. The above list is a pyramid, higher can depend down, but lower cannot depend up - which is currently the case.

After this, we need to sort out the registry/createType mess. It cross-depends on each other, it should be one clean lil bundle. (Not convinced we need 2, i.e. createType is what we tell people to use, no getRegistry.get)

[jacogr]

Closing, replaced by #1592

[polkadot-js-bot]

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.