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: add FuelError package and related utilities #1153

Merged

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Jul 29, 2023

This PR adds a new errors package based off of #1110.

Thanks @arboleya for your contributions!

An additional question remains: what should we do with the usage of @ethersproject/logger for throwing errors? Currently, throwing errors is done in two ways inside of the SDK, either via throw new Error... or logger.throw.... I've tried writing examples in the abi-coder package, but this ambiguity stopped me. @arboleya added examples in address package where he substituted logger with FuelError as a showcase, but we should still have a discussion because we're using the logger alongside our versions and currently FuelError doesn't have any version info in it.

This PR is mergable as-is, regardless of how we deal with the logger issue, as anything logger-related can be built on top of it. After we resolve the logger issue, errors throughout the SDK can be standardized to use only one approach. We can still reference this package in any subsequent PRs whenever we see someone of us throwing a new error directly and just say "Hey, can you think of a properly-named error code and use FuelError instead of Error?"

Note that updating the errors throughout the fuels-ts packages is outside of the scope of this PR, as that isn't a small endeavor because careful thought needs to be put into error code naming. Also, sometimes the actual code implementation where an error is thrown might not 100% reflect the error code being used, which might necessitate some refactoring to have it make more sense.

Closes #1110.

@nedsalk nedsalk linked an issue Jul 29, 2023 that may be closed by this pull request
@nedsalk nedsalk self-assigned this Jul 29, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 29, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
84.99% (+0.12% 🔼)
4857/5715
🟡 Branches
65.09% (+0.22% 🔼)
731/1123
🟡 Functions
74.17% (+0.28% 🔼)
824/1111
🟢 Lines
85.11% (+0.11% 🔼)
4642/5454
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / error-codes.ts
100% 100% 100% 100%
🟢
... / fuel-error.ts
100% 100% 100% 100%
🟢
... / expect-to-throw-fuel-error.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / address.ts
88.57% (-0.9% 🔻)
54.55% 82.35%
88.57% (-0.9% 🔻)
🟢
... / utils.ts
88.64% (-0.49% 🔻)
84.62% 90.91%
87.5% (-0.6% 🔻)

Test suite run success

1178 tests passing in 204 suites.

Report generated by 🧪jest coverage report action from b5b1c70

nedsalk and others added 12 commits July 29, 2023 16:16
* Adding extra Generic type for greater flexibility

* Removing Jest extension

* Adding utility helper

* Improving readability

* Re-configuring errors package; adding new `test-utils` entrypoint

* Begin to standardize `packages/address` errors

* Adding missing tests

* Fixing version range for workspace dependency

* Lintfix

* Updating lockfile

* Tyop

* Removing lost import

* Lintfix
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

I left a few more notes.

There's also a (unused import) lint issue in:

  • packages/address/src/utils.ts

jest.config.ts Outdated Show resolved Hide resolved
packages/address/src/address.ts Outdated Show resolved Hide resolved
packages/errors/src/index.ts Outdated Show resolved Hide resolved
@arboleya arboleya changed the title feat: add FuelError as a starting point towards error standardization feat: add FuelError package and related tooling Aug 1, 2023
@arboleya
Copy link
Member

arboleya commented Aug 1, 2023

@nedsalk We could have a read-only version property on FuelError. It can be read from @fuel-ts/versions.

I know you want to avoid standardizing error handling for more packages in this PR, but it's essential that we have subsequent (per-package?) PRs addressing this.

I created an issue to track this:

@arboleya arboleya changed the title feat: add FuelError package and related tooling feat: add FuelError package and related utilities Aug 1, 2023
Dhaiwat10
Dhaiwat10 previously approved these changes Aug 22, 2023
Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Clean 👏🏻

@Dhaiwat10
Copy link
Member

Please make sure to file a follow-up issue for the logger issue you mentioned in the PR description, and maybe an epic issue for the conversion of new Error to new FuelError throughout the repo across different packages.

Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Looking good, small change and can approve

apps/docs/package.json Outdated Show resolved Hide resolved
Co-authored-by: Daniel Bate <djbate23@gmail.com>
@nedsalk
Copy link
Contributor Author

nedsalk commented Aug 22, 2023

@Dhaiwat10

Please make sure to file a follow-up issue for the logger issue you mentioned in the PR description, and maybe an epic issue for the conversion of new Error to new FuelError throughout the repo across different packages.

I've opened up an issue for the logger: #1207.
@arboleya already created an epic for the whole repo: #1156.

@nedsalk nedsalk merged commit 1124b30 into master Aug 22, 2023
8 checks passed
@nedsalk nedsalk deleted the ns/feat/1110-standardize-error-handling-inside-of-the-sdk branch August 22, 2023 16:09
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.

Implement new Error handling package
4 participants