Skip to content

feat: error handler middleware #234

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

Merged
merged 12 commits into from
Apr 7, 2025
Merged

Conversation

r4mmer
Copy link
Member

@r4mmer r4mmer commented Apr 3, 2025

Motivation

middy does not have a default error handler, so unhandled errors would just return 200 with an empty body.

Acceptance Criteria

  • Create error handler middleware
  • Use error handler on all apis

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged
  • Make sure either the unit tests and/or the QA tests are capable of testing the new features
  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@r4mmer r4mmer self-assigned this Apr 3, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

packages/wallet-service/src/api/middlewares/errorHandler.ts:26

  • Consider adding unit tests for the errorHandler middleware to verify that it correctly returns a generic error message in production and a detailed error message otherwise.
if (process.env.NODE_ENV === 'production') {

// Initialize response with default values if it hasn't been done yet.
request.response = request.response ?? {...defaultResponse};
// Force the status code to 500 since this is an unhandled error
request.response.statusCode = 500;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about using the STATUS_CODE_TABLE from api/utils?

Copy link
Member

Choose a reason for hiding this comment

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

Above as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

@@ -1169,3 +1171,20 @@ describe('getWalletBalancesForTx', () => {
});
});
});

test('getTokenListFromInputsAndOutputs', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this test being added in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

To increase coverage since the global coverage was failing

@@ -103,6 +103,7 @@ export const sendMessageToClient = async (
}
};

/* istanbul ignore next */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

This is apparently being used in wsAdminDisconnect

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

pedroferreira1
pedroferreira1 previously approved these changes Apr 7, 2025
@pedroferreira1 pedroferreira1 moved this from Todo to In Review (Done) in Hathor Network Apr 7, 2025
@github-project-automation github-project-automation bot moved this from In Review (Done) to In Review (WIP) in Hathor Network Apr 7, 2025
@r4mmer r4mmer merged commit 7e6fb31 into master Apr 7, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from In Review (WIP) to Waiting to be deployed in Hathor Network Apr 7, 2025
@r4mmer r4mmer deleted the feat/error-handler-middleware branch April 7, 2025 17:04
r4mmer added a commit that referenced this pull request Apr 7, 2025
…-lib-v2.1.1

* origin/master:
  feat: error handler middleware (#234)
@r4mmer r4mmer moved this from Waiting to be deployed to Done in Hathor Network Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants