Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: coretime implementation #1558

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

filvecchiato
Copy link
Contributor

@filvecchiato filvecchiato commented Nov 29, 2024

New Feature: Implements the coretime endpoints for relay chain and for coretime chain.

It includes basic endpoints for single information record recovery, and more advanced endpoints with precomputed calculations and information.

@filvecchiato filvecchiato force-pushed the filippo_coretime_implementation branch from 744895d to 6de3395 Compare November 29, 2024 15:52
@filvecchiato filvecchiato changed the title Filippo coretime implementation Feat: coretime implementation Nov 29, 2024
@filvecchiato filvecchiato force-pushed the filippo_coretime_implementation branch from 4efc743 to f6e8a77 Compare December 9, 2024 15:15
@filvecchiato filvecchiato changed the title Feat: coretime implementation feat: coretime implementation Dec 9, 2024
@filvecchiato filvecchiato force-pushed the filippo_coretime_implementation branch from 9ef9d68 to e8a0808 Compare December 19, 2024 15:59
@filvecchiato filvecchiato force-pushed the filippo_coretime_implementation branch from 142039d to 4dcdf17 Compare December 20, 2024 11:12
@filvecchiato filvecchiato marked this pull request as ready for review December 20, 2024 11:13
@filvecchiato filvecchiato requested a review from a team as a code owner December 20, 2024 11:13
}

private checkCoretimeModule = (): void => {
if (this.api.query.onDemandAssignmentProvider && this.api.query.coretimeAssignmentProvider) {
Copy link
Member

Choose a reason for hiding this comment

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

So if I understand this correct - either broker needs to exist or onDemandAssignmentProvider and coretimeAssignmentProvider?

Would there ever be a case where they all need to exists at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding broker is a coretime chain pallet only while onDemandAssignemntProvider and coretimeAssignmentProvider are relay chain only pallets

CoretimeGenericController.sanitizedSend(res, await this.service.getCoretimeCores(hash));
};

private checkCoretimeModule = (): void => {
Copy link
Member

Choose a reason for hiding this comment

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

Same sentiment as the comment in CoretimeChain

return coreSchedules as unknown as Record<string, unknown>[];
};

private getAndDecodeCoreDescriptors = async (api: ApiDecoration<'promise'>): Promise<TCoreDescriptor[]> => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private getAndDecodeCoreDescriptors = async (api: ApiDecoration<'promise'>): Promise<TCoreDescriptor[]> => {
private getAndDecodeCoreDescriptors = async (api: ApiDecoration<'promise'>): Promise<TCoreDescriptor[]> => {
const coreDescriptors = await api.query.coretimeAssignmentProvider.coreDescriptors.entries();
return coreDescriptors.map((descriptor) => extractCoreDescriptorInfo(descriptor[0], descriptor[1]));
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

api.query.coretimeAssignmentProvider.coreDescriptors.entries() return a type: [StorageKey, Codec][]
for which i need to typecast to convert the codec into the actual type. I'm not sure if something could be off on my set up, it seems like I don't get the corresponding type correctly and thrown an error

return descriptors.map((descriptor) => extractCoreDescriptorInfo(descriptor[0], descriptor[1]));
};

private getAndDecodeParachainsLifecycle = async (api: ApiDecoration<'promise'>): Promise<TParaLifecycle[]> => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private getAndDecodeParachainsLifecycle = async (api: ApiDecoration<'promise'>): Promise<TParaLifecycle[]> => {
private getAndDecodeParachainsLifecycle = async (api: ApiDecoration<'promise'>): Promise<TParaLifecycle[]> => {
const parachains = await api.query.paras.paraLifecycles.entries();
return parachains.map((para) =>
extractParachainLifecycleInfo(
para[0],
para[1],
),
);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

api.query.paras.paraLifecycles.entries() return a type: [StorageKey, Codec][]
for which i need to typecast to convert the codec into the actual type. I'm not sure if something could be off on my set up, it seems like I don't get the corresponding type correctly and thrown an error

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Overall amazing PR! I have a few nits:

  1. The typecasting can be removed. AFAIU the types are correctly passed down from PJS so you should be good.

  2. For any imported types we can use import type.

Other than that the PR looks really great!

@filvecchiato
Copy link
Contributor Author

  1. The typecasting can be removed. AFAIU the types are correctly passed down from PJS so you should be good.

I removed most of typecasting (nice catch, not sure I was having issues before), I had to leave typecasting for coreDescriptors and paraLifecycles though, we can doublecheck if there is something wrong on my end though

Other than that the PR looks really great!

thanks for checking it out!

@filvecchiato filvecchiato linked an issue Dec 23, 2024 that may be closed by this pull request
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

Add support for Coretime
2 participants