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

fix: update errors endpoint to use latest error metadata for fetchErrorItem #1205

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

marshacb
Copy link
Collaborator

No description provided.

@marshacb marshacb marked this pull request as ready for review January 26, 2023 20:03
@marshacb marshacb requested a review from a team as a code owner January 26, 2023 20:03
@TarikGul TarikGul changed the title update errors endpoint to use latest error metadata for fetchErrorItem fix: update errors endpoint to use latest error metadata for fetchErrorItem Jan 30, 2023
@TarikGul
Copy link
Member

Whats the exact issue happening here with the types?

@marshacb
Copy link
Collaborator Author

Whats the exact issue happening here with the types?

There isn't an issue with these types remaining as is actually. It would just be a change to keep the return types of the errors endpoints more consistent with each other. It doesn't change any logic for the api so if preferred we can leave this unchanged.

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

Looks great!

  • Just to double check we still need the | Option<PalletErrorMetadataV14> { in line 43 (src/services/AbstractPalletsService.ts file) right ?
  • On that part of code why in line 51 we hardcode 'errors' ?
    return this.getProperty(meta as PalletMetadataV14, 'errors');
    
    We could again just use metadataFieldType right ? So maybe have
    return this.getProperty(meta as PalletMetadataV14, metadataFieldType);
    
  • One last question : why we did not use ErrorMetadataLatest since the beginning ?

@marshacb
Copy link
Collaborator Author

marshacb commented Jan 30, 2023

Looks great!

  • Just to double check we still need the | Option<PalletErrorMetadataV14> { in line 43 (src/services/AbstractPalletsService.ts file) right ?

  • On that part of code why in line 51 we hardcode 'errors' ?

    return this.getProperty(meta as PalletMetadataV14, 'errors');
    

    We could again just use metadataFieldType right ? So maybe have

    return this.getProperty(meta as PalletMetadataV14, metadataFieldType);
    
  • One last question : why we did not use ErrorMetadataLatest since the beginning ?

Yes, we still need the reference to Option<PalletErrorMetadataV14> in order to determine the pallets metadata type when we are looking for the errors of the pallet.

The errors in the else case has to be hardcoded because unlike the previous if check for storage, its value cannot be inferred based on its context. getProperty knows the value of metadataFieldType inside of the storage if check because it is being used within the if blocks context where it has already been determined to equal storage.

ErrorMetadataLatest wasn't used for the fetchErrorItem call because PalletErrorMetadataV14 was an acceptable type to return. This simply makes the return types for both endpoints consistent and will likely be similar in future endpoints such as events, constants and calls/dispatchables.

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

Thank you for the explanations! 💯

This pull request was closed.
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