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

Provider admin api with signing #996

Merged
merged 18 commits into from
Jan 24, 2024
Merged

Conversation

HughParry
Copy link
Contributor

No description provided.

Copy link

Pull Request Review - Summary

Hey there! 👋 Here's a summary of the previous tasks and their results for the pull request review:

Changes

  1. In funds.ts file, the transfer function was changed to transferAllowDeath at line 73.

Suggestions

  1. In funds.ts file, consider providing a comment explaining the reason for using transferAllowDeath instead of transfer at line 73.
  2. In start.ts file, the import statement for prosopoAdminRouter was added. Consider organizing the import statements alphabetically for better readability at line 16.

Bugs

  1. In funds.ts file, there is a potential bug in the sendFunds function. The resolve and reject functions are not called inside the signAndSend callback. This may cause the promise to never resolve or reject.
  2. In start.ts file, the handleErrors function is referenced but not defined. This may cause an error at runtime.

Improvements

  1. Refactoring suggestion in start.ts file:
import { prosopoAdminRouter, prosopoRouter } from '@prosopo/provider';

const router = express.Router();

router.use(prosopoRouter(env));
router.use(prosopoAdminRouter(env));

apiApp.use(router);

This refactoring separates the router setup into a separate router variable, making it more readable and modular.

Rating

  • Overall Rating: 7.5/10
  • Readability: 8/10
  • Performance: 7/10
  • Security: 7/10

The code overall is readable, but there are some areas that could be improved. Performance and security seem to be average.

Feel free to check the detailed results for more information. Let me know if you need any further assistance! 😄

@HughParry HughParry marked this pull request as ready for review January 18, 2024 14:40
@HughParry HughParry force-pushed the provider-admin-api-with-signing branch from 68b4b61 to 5dc66a1 Compare January 18, 2024 16:40
Copy link
Member

@forgetso forgetso left a comment

Choose a reason for hiding this comment

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

On track for world domination. Good work. One small Q. We can handle the commit stuff later when the populator is working.

@forgetso forgetso enabled auto-merge (squash) January 24, 2024 13:56
@forgetso forgetso merged commit d3178ea into main Jan 24, 2024
8 checks passed
@forgetso forgetso deleted the provider-admin-api-with-signing branch January 24, 2024 13:56
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.

2 participants