ivy-wallet
ivy-wallet copied to clipboard
[Data Layer] Migrate the code to use the new the `TransactionRepository`
Please confirm the following
- [X] I checked the current issues for duplicate issues.
What would you like to improve?
- Implement
TransactionRepositoryfor handling data operations - Migrate from using
TransactionDaoandWriteTransactionDaodirectly to leveraging the repository
Because...
- To transition away from the outdated data management practices
- To ensure consistency in the architectural framework
- To improve safety and simplify the process of making future enhancements
Description
No response
Success Criteria
- Adequate unit testing coverage
- Uninterrupted functionality of pre-existing features
- The application's continuous stability and performance with the newly adopted data layer
Thank you @ILIYANGERMANOV for raising Issue #2963! 🚀 What's next? Read our Contribution Guidelines 📚.
Tagging @ILIYANGERMANOV for review & approval 👀
I'm on it
Thank you for your interest @michalguspiel! 🎉 Issue #2963 is assigned to you. You can work on it! ✅
If you don't want to work on it now, please un-assign yourself so other contributors can take it.
Also, make sure to read our Contribution Guidelines.
@ILIYANGERMANOV
I understand that
Implement TransactionRepository for handling data operations part of this task is already done.
Thus this task requires to leverage TransactionRepository in the domain layer instead of directly calling Data Access Objects?
@ILIYANGERMANOV
I understand that
Implement TransactionRepository for handling data operationspart of this task is already done.Thus this task requires to leverage
TransactionRepositoryin the domain layer instead of directly calling Data Access Objects?
Yup, correct. It's migration DAOs usages to the repository ones. This also includes using the new domain model
Okay so basically plan for this is to add internal visibility modifier to both TransactionDao and WriteTransactionDao
and fix the errors.
What do you mean by new domain model?
Okay so basically plan for this is to add
internalvisibility modifier to bothTransactionDaoandWriteTransactionDaoand fix the errors.What do you mean by new domain model?
This. When you fix the errors you'll need to pass Transaction instance and not TransactionEntity. But it's exactly what you said
@ILIYANGERMANOV
Starting to work with this, I'm realizing how huge of a task it is. It's going to take weeks if not months, considering the amount of my free time I have for this. By the time I'm done with this, I'll need to resolve tons of conflicts which will extend this work even further.
When migrating to the new TransactionRepository it doesn't quite make sense to map the results in the Presentation layer back to the legacy Domain model. Because of that a lot of logic needs to be refactored.
Have you any idea how to split this task to smaller chunks?
For instance: instead of
Migrate from using TransactionDao and WriteTransactionDao directly to leveraging the repository
We could do:
Migrate from using TransactionDao directly to leveraging the TransactionRepository
Migrate from using Write TransactionDao to leveraging the TransactionRepository
WDYT
@michalguspiel yes your suggestion is good 👍 You can split it to even smaller, migrate one or a few usages -> submit a PR, merge it. This way it would be easier to review
What do we do about TransactionHistoryItem? I got to the point where passing history: List<TransactionHistoryItem> to transactions composable creates a big problem for refactoring.
TransactionHistoryItem is deprecated but TransactionHistoryDateDivider is not. Legacy Transaction and TransactionHistoryDateDivider both inherit from TransactionHistoryItem. How do we want to fix this in regard to new Transaction model?
I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.
Any suggestions?
I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.
@michalguspiel just duplicate it - that's a good strategy!