Skip to content

feat: augmented api using @polkadot/typegen#554

Merged
rflechtner merged 41 commits intodevelopfrom
rf-typegen-mark2
Jul 14, 2022
Merged

feat: augmented api using @polkadot/typegen#554
rflechtner merged 41 commits intodevelopfrom
rf-typegen-mark2

Conversation

@rflechtner
Copy link
Contributor

@rflechtner rflechtner commented Jun 13, 2022

fixes KILTProtocol/ticket#1061

After type collision issues have been resolved with @polkadot/api v7 and above, we can finally start using the typegen package to provide kilt-specific api augmentation.

How to test:

  • Augmented api should also be available when importing from sdk-js after installing the package in a project.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@rflechtner rflechtner requested a review from tjwelde June 16, 2022 14:23
@rflechtner rflechtner marked this pull request as ready for review June 16, 2022 14:24
@rflechtner rflechtner self-assigned this Jun 16, 2022
Copy link
Contributor

@arty-name arty-name left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility to provide sensible aliases at least for those types that we directly use in the SDK? AttestationAttestationsAttestationDetails looks funny even when you know the reason behind it.

@rflechtner
Copy link
Contributor Author

Is there a possibility to provide sensible aliases at least for those types that we directly use in the SDK? AttestationAttestationsAttestationDetails looks funny even when you know the reason behind it.

We could do type aliasing in some file for sure, but I decided against it not only because it means maintaining more code besides what is autogenerated but also because these are the name by which these types are 'known' in the different applications and interfaces:
image

function getChainKeyTypeForDidKeyType<T extends DidKey['type']>(
keyType: T
): ChainKeyTypeFor<T> {
return didKeyTypeToChainType[keyType] as ChainKeyTypeFor<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two functions seems to not be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maps could be accessed directly instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a type thing, we want it to return a ChainKeyTypeFor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

annoying

>((obj, [key, val]) => ({ ...obj, [val]: key }), {} as any)

type ChainKeyTypeFor<T extends DidKey['type']> = Capitalize<T>
type DidKeyTypeFor<T extends ChainKeyType> = Lowercase<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's super annoying. Is that necessary to be so explicit about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is, but typeof didKeyTypeToChainType[T] results in an inaccurate union type. yet another annoying TS limitation

Copy link
Contributor

@tjwelde tjwelde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rflechtner rflechtner merged commit 013633a into develop Jul 14, 2022
@rflechtner rflechtner deleted the rf-typegen-mark2 branch July 14, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants