feat: Remove filesystem usage from WalletService.ts #203
Conversation
- Remove all filesystem dependencies from wallet module - Fix import paths in WalletService.ts and tests - Update WalletService test DTOs with required properties - Disable AuthService integration tests until refactoring - All wallet tests passing without filesystem dependencies
WalkthroughTests for wallet authentication were skipped and decoupled from AuthService. WalletService import/export paths were adjusted to the application-layer. WalletVerificationResponseDto gained optional fields: accountExists and verifiedAt. WalletService imports were updated for new directory structure without changing behavior. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/wallet/__tests__/WalletAuthIntegration.test.ts (1)
52-53: UndefinedauthServicereferences inside skipped suite may break type-checking.Even in
describe.skip, TS may error. Either stubauthServicelocally or comment out the calls (as done in tests/wallet/...).- const token = await authService.authenticate(validWalletAddress); + // const token = await authService.authenticate(validWalletAddress); + const token = "mock-token"; @@ - await expect( - authService.authenticate(invalidWalletAddress) - ).rejects.toThrow("Invalid wallet address"); + // await expect(authService.authenticate(invalidWalletAddress)) + // .rejects.toThrow("Invalid wallet address"); @@ - await expect( - authService.authenticate(validWalletAddress) - ).rejects.toThrow("User not found"); + // await expect(authService.authenticate(validWalletAddress)) + // .rejects.toThrow("User not found"); @@ - const result = await authService.register( - registrationData.name, - registrationData.lastName, - registrationData.email, - registrationData.password, - registrationData.wallet - ); + // const result = await authService.register( + // registrationData.name, + // registrationData.lastName, + // registrationData.email, + // registrationData.password, + // registrationData.wallet + // );Also applies to: 67-69, 84-86, 138-145, 215-215
🧹 Nitpick comments (4)
src/modules/wallet/application/services/WalletService.ts (1)
12-16: Prefer DI-friendly constructor to avoid test-only private field patching.Direct instantiation hinders testing and forces (any) mutations in tests. Consider optional DI with sensible defaults.
- constructor() { - this.walletRepository = new HorizonWalletRepository(); - this.verifyWalletUseCase = new VerifyWalletUseCase(this.walletRepository); - this.validateWalletFormatUseCase = new ValidateWalletFormatUseCase(); - } + constructor( + walletRepository: HorizonWalletRepository = new HorizonWalletRepository(), + verifyWalletUseCase: VerifyWalletUseCase = new VerifyWalletUseCase(walletRepository), + validateWalletFormatUseCase: ValidateWalletFormatUseCase = new ValidateWalletFormatUseCase() + ) { + this.walletRepository = walletRepository; + this.verifyWalletUseCase = verifyWalletUseCase; + this.validateWalletFormatUseCase = validateWalletFormatUseCase; + }src/modules/wallet/__tests__/services/WalletService.test.ts (1)
20-27: Avoid mutating private fields; construct WalletService with injected mocks.Current approach relies on type casts and private field overrides. After making WalletService DI-friendly, pass mocks via constructor.
Also applies to: 29-36
src/modules/wallet/__tests__/WalletAuthIntegration.test.ts (1)
1-2: Skipping is fine temporarily — add traceability.Keep the TODO referencing the ticket that will re-enable these tests; consider a deadline/date to avoid test rot.
Also applies to: 22-24
tests/wallet/WalletAuthIntegration.test.ts (1)
1-1: OK to skip; mirror TODO with issue link.Matches src test intent; maintains suite without execution.
Also applies to: 22-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/modules/wallet/__tests__/WalletAuthIntegration.test.ts(6 hunks)src/modules/wallet/__tests__/services/WalletService.test.ts(8 hunks)src/modules/wallet/application/services/WalletService.ts(1 hunks)src/modules/wallet/index.ts(1 hunks)tests/wallet/WalletAuthIntegration.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/wallet/WalletAuthIntegration.test.ts (1)
src/modules/wallet/application/services/WalletService.ts (1)
WalletService(7-50)
src/modules/wallet/__tests__/services/WalletService.test.ts (1)
src/modules/wallet/application/services/WalletService.ts (1)
WalletService(7-50)
🔇 Additional comments (4)
src/modules/wallet/index.ts (1)
18-18: Export path relocation looks correct.Barrel now points to application layer; no API change for consumers importing from the module root.
src/modules/wallet/application/services/WalletService.ts (1)
1-5: Path updates only — OK.Imports shifted two levels up; no behavior change.
src/modules/wallet/__tests__/services/WalletService.test.ts (1)
40-49: Mocks and assertions read well.Happy-path and error-path expectations are clear; DTO shape (accountExists, verifiedAt) consistently applied.
Also applies to: 51-53, 76-79, 93-102, 117-126, 141-152, 169-175, 185-191, 198-205
tests/wallet/WalletAuthIntegration.test.ts (1)
147-159: Placeholders keep intent; keep assertions minimal and consistent.Good use of placeholders; once AuthService is back, replace with real flows.
Also applies to: 205-213, 229-267
1 participant
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])WalletService.ts(nofs, no local dirs) #161📌 Type of Change
📝 Changes description
Removed filesystem usage from WalletService.ts and the entire wallet module.
I also resolved all the import path issues in the wallet module
Then I temporarily disabled WalletAuthIntegration.test.ts tests since they depend on a non-existent
AuthService📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
🌌 Comments
Thank you for contributing to Volunchain, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
Refactor
Tests