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

Implement TFChainError in chain client #3207

Merged
merged 13 commits into from
Aug 7, 2024

Conversation

Mahmoud-Emad
Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad commented Aug 4, 2024

Changes

  • Created a new Error wrapper to throw errors based on their type.
  • Determined the type of error and threw it accordingly.
  • Updated the error interface to differentiate between Module, Token, and other error types.
  • Remove the TFChain error from the types package.

Related Issues

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

- Created a new Error wrapper to throw errors based on their type.
- Determined the type of error and threw it accordingly.
- Updated the error interface to differentiate between Module, Token, and other error types.
@0oM4R
Copy link
Contributor

0oM4R commented Aug 4, 2024

I see we are going to move the tfchain error from the types package to the tfchain package, is there a reason for that, as we discussed before we need to only refactor the class, and for the other issue we could add a method to that class to resolve the error.

- Moved TFChainError into the types package.
- Updated imports to reflect the new location of TFChainError.
@Mahmoud-Emad
Copy link
Contributor Author

I see we are going to move the tfchain error from the types package to the tfchain package, is there a reason for that, as we discussed before we need to only refactor the class, and for the other issue we could add a method to that class to resolve the error.

As discussed with @AhmedHanafy725 , we'll create an interface with specific fields to capture error details. We'll populate these fields based on the error type. Placing the changes in the types package makes sense yeah. Please review the changes.

*/
throw(): TFChainError {
const args = this.extrinsic.method.args;
const method = this.extrinsic.method.method;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use the extrinsic data, it may invalid error data #2896, it happened before and we decided to get the exact error method #2995

we can't rely on the extrinsic

- Updated the if check to check on the 'KeyError'
- Updated the class to get the args from the extrinsic
- Updated the class to get the metho from the extrinsic
- Updated the class to get the section from the extrinsic
- Handle the error message and added the section, method, and args into it
Copy link
Contributor

@0oM4R 0oM4R 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 great job ya Omda

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion

i notice that when throwing generic error we have something like this
Screenshot from 2024-08-06 12-39-25

what i suggest :
1- set keyError default value to Generic Error
2- hide the undefined props,

we can do that by remove the class properties and if it is passed use Object.DefineProperty

class TFChainError extends Error implements ITFChainError {
  constructor(options: ITFChainError) {
    super(options.message);
    this.name = "TFChainError";
    options.keyError && Object.defineProperty(this, "keyError", options.keyError);
    options.section && Object.defineProperty(this, "section", options.section);
    options.args && Object.defineProperty(this, "args", options.args);
    options.method && Object.defineProperty(this, "method", options.method);
    options.docs && Object.defineProperty(this, "docs", options.docs);
  }
}

this will hide the undefined props:
image

but this have one consequenc: we won't have autcomplete and will be diffecult to access the props if it is defined, so the solution is to define an interface with the same name of the class with the optional props

interface TFChainError {
  keyError?: string;
  section?: string;
  method?: string;
  args?: AnyTuple | string[];
  docs?: string[];
}

now we will be able to access them normally
Screenshot from 2024-08-06 13-17-46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my honest :), it's not good to follow, why? because we have a class that implements an interface, you have to follow the interface, there are no blockers with the undefined value, make it simple ya Omar xD :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean the 'ITFChainError'?
We can remove the optional values from the interface and follow it, anyways my suggestion is for better look only.

- Remove the console.log
- Remove the the undefined values from the interface
- Defined a new interface with the same name of the 'TFChainError' class to offer autcomplete
- Added defineProperty to define only the defined values
- Updated the import of the 'TFChainError' class
- Updated the TFChain README file with some hints about 'TFChainError'
@Mahmoud-Emad Mahmoud-Emad force-pushed the development_tfchain_error branch from 7a624b5 to f478614 Compare August 7, 2024 12:50
@Mahmoud-Emad Mahmoud-Emad merged commit 3f66011 into development Aug 7, 2024
10 checks passed
@Mahmoud-Emad Mahmoud-Emad deleted the development_tfchain_error branch August 7, 2024 13:06
@xmonader xmonader added this to the 2.6.0 milestone Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants