Pass registry to all codec types #1592
Conversation
amaury1093
left a comment
amaury1093
left a comment
There was a problem hiding this comment.
tbh looks good. Had a look at the most interesting parts (removal of getTypeRegistry/injector, api base, create type).
The rest is hard to review so I'll mostly trust the linting & testing
| it('has the correct encodedLength for constructor values (class BlockNumber)', (): void => { | ||
| expect( | ||
| new Compact(ClassOf('BlockNumber'), 0xfffffff9).encodedLength | ||
| new (Compact.with(ClassOf(registry, 'BlockNumber')))(registry, 0xfffffff9).encodedLength |
There was a problem hiding this comment.
Looking at these, we could put Compact, Uint, Vec, Tuple, Struct etc as abstract classes, and enforce the usage .with() which will create a real class
There was a problem hiding this comment.
Throwing ideas again, an alternative syntax would be new Compact.with(registry.BlockNumber), on in general new (api.)registry.BlockNumber('234')
There was a problem hiding this comment.
I would prefer them to be used that way, yes. Probably after #1518 (which has it's own issues, but opens the way for removing the U{8,16,32,64,128,256} classes (and allows stuff like U2048)
| const registry = new TypeRegistry(); | ||
|
|
||
| // eslint-disable-next-line no-new | ||
| new Metadata(registry, rpcMetadata); |
There was a problem hiding this comment.
Could we do const registry = new TypeRegistry(meta: string) (string being rpc). This way we could remove setMetadata from Metadata's constructor, and make it private.
There was a problem hiding this comment.
hmm, maybe not, because Metadata needs registry already
There was a problem hiding this comment.
So the above is only in tests - and it is a bit of magic, as per the comment suppression. I was thinking about doing that, but it actually only helps in tests - in the Api itself the registry is created right-up-front before we even have metadata, so we still need to inject or call setMetadata (Originally I have an explicit set, now metadata does it).
Your approach may be cleaner, at least for "it not just doing stuff" and will certainly clean up the tests.
There was a problem hiding this comment.
Still creates a small issue in the Decorated tests, but can call setMetadata there explicitly like we have in the Api then. Ok, will follow the meta?: string | Uint8Array and see where it gets us.
On second thought... this gets back into circular dep hell again. (Which is why the RegistryMetadata was introduced, it is a real mess without it with left-and-right breakages)
There was a problem hiding this comment.
Yeah, actually I don't think metadata should go inside registry. Ideally, I would see a class Context which has this interface:
constructor(registry: Registry, metadata?: Metadata);
registry: Registry;
metadata?: Metadata;
setMetadata: (meta: string|u8a);
In tests that don't need metadata, we just new Context. In tests with calls/events, we do an additional context.setMetadata()
Edit: So it's actually pretty similar to current architecture. Would just probably remove setMetadata from Metadata constructor, and put it more explicitly everywhere.
Co-Authored-By: Amaury Martiny <amaury.martiny@protonmail.com>
|
Well, it works. Not perfect, the CC here is f-ing out of control. Decent start. |
|
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. |
WIP, mucking about.